-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check elaborated projections from dyn don't mention unconstrained late bound lifetimes #130367
base: master
Are you sure you want to change the base?
Check elaborated projections from dyn don't mention unconstrained late bound lifetimes #130367
Conversation
@bors try |
r? @nnethercote rustbot has assigned @nnethercote. Use |
HIR ty lowering was modified cc @fmease |
r? types |
If this ends up having too much fallout, I guess we could easily make this an FCW. But this seems pretty soundness-critical, given the number of creative ways that people have found in #130347 to make this unsound :D |
… r=<try> Check elaborated projections from dyn don't mention unconstrained late bound lifetimes Check that the projections that are *not* explicitly written but which we deduce from elaborating the principal of a `dyn` *also* do not reference unconstrained late-bound lifetimes, just like the ones that the user writes by hand. Fixes rust-lang#130347
☀️ Try build successful - checks-actions |
@craterbot check |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
[CRATER] Crater rollup This is a crater rollup of: * rust-lang#124141 * rust-lang#130285 * rust-lang#130367 **What is a crater rollup?** It's simply a (manually set-up) crater job that is run on all of the containing PRs together, and then we can set the crates list for each of these jobs to *just* the list of failures after it's done. It should cut out on the bulk of "normal" crates that do nothing and simply just waste time to build without being affected by the union of all of these changes. After this is finished, I will adjust all of the jobs to use only the list of failed crates. That should significantly speed up these jobs from taking like ~6 days to taking ~2. See the last time I did this: rust-lang#129660. Given that rust-lang#130285 is running in build-and-test mode, let's run all of them in build-and-test mode.
I'm curious why the constrained case (https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9a297b502ceb3bd1e07ceae618f9a31e) does fail today. If it was only elaboration that played a role here, I would expect the above example to both compile (but also be unsound). The deeper problem here is likely instead around implied bounds for higher-ranked lifetimes resulting in implied 'static. |
I think you wrote that example wrong. we want Specifically, I've renamed the
I don't think this has to do with implied bounds. It really does only have to do with elaboration AFAICT. |
Perhaps a more illustrative example is taking this example: trait A<T>: B<T = T> {}
trait B {
type T;
}
fn foo<'b>(a: &'b str) -> <dyn for<'a> A<&'a str> as B>::T {
a
}
fn main() {
let x = foo(String::from("hello").as_str());
dbg!(x);
} In order to properly constrain trait A<T>: B<T, T = T> {}
trait B<T> {
type T;
} That necessitates modifying fn foo<'b>(a: &'b str) -> <dyn for<'a> A<&'a str> as B</* what do we put here? */>>::T {
a
} But wait... what do we need to put in
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
Check that the projections that are not explicitly written but which we deduce from elaborating the principal of a
dyn
also do not reference unconstrained late-bound lifetimes, just like the ones that the user writes by hand.That is to say, given:
The type
dyn for<'a> Foo<&'a T>
(basically) elaborates todyn for<'a> Foo<&'a T> + for<'a> Bar<Assoc = &'a T>
1. However, theBar
projection predicate is not well-formed, since'a
must show up in the trait's arguments to be referenced in the term of a projection. We must error in this situation2, or elsedyn for<'a> Foo<&'a T>
is unsound.We already detect this for user-written projections during HIR->rustc_middle conversion, so this largely replicates that logic using the helper functions that were already conveniently defined.
I'm cratering this first to see the fallout; if it's minimal or zero, then let's land it as-is. If not, the way that this is implemented is very conducive to an FCW.
Fixes #130347
Footnotes
We don't actually elaborate it like that in rustc; we only keep the principal trait ref
Foo<&'a T>
and the projection part ofBar<Assoc = ...>
, but it's useful to be a bit verbose here for the purpose of explaining the issue. ↩Well, we could also make
dyn for<'a> Foo<&'a T>
not implementfor<'a> Bar<Assoc = &'a T>
, but this is inconsistent with the case where the user writesAssoc = ...
in the type itself, and it overly complicates the implementation of trait objects' built-in impls. ↩