Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Sep 14, 2024

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:

trait Foo<T>: Bar<Assoc = T> {}

trait Bar {
    type Assoc;
}

The type dyn for<'a> Foo<&'a T> (basically) elaborates to dyn for<'a> Foo<&'a T> + for<'a> Bar<Assoc = &'a T>1. However, the Bar 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 else dyn 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

  1. We don't actually elaborate it like that in rustc; we only keep the principal trait ref Foo<&'a T> and the projection part of Bar<Assoc = ...>, but it's useful to be a bit verbose here for the purpose of explaining the issue.

  2. Well, we could also make dyn for<'a> Foo<&'a T> not implement for<'a> Bar<Assoc = &'a T>, but this is inconsistent with the case where the user writes Assoc = ... in the type itself, and it overly complicates the implementation of trait objects' built-in impls.

@compiler-errors
Copy link
Member Author

@bors try

@rustbot
Copy link
Collaborator

rustbot commented Sep 14, 2024

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 14, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 14, 2024

HIR ty lowering was modified

cc @fmease

@compiler-errors
Copy link
Member Author

r? types

@rustbot rustbot added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Sep 14, 2024
@rustbot rustbot assigned spastorino and unassigned nnethercote Sep 14, 2024
@compiler-errors
Copy link
Member Author

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

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 14, 2024
… 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
@bors
Copy link
Contributor

bors commented Sep 14, 2024

⌛ Trying commit f16d211 with merge f827120...

@compiler-errors compiler-errors removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 14, 2024
@bors
Copy link
Contributor

bors commented Sep 14, 2024

☀️ Try build successful - checks-actions
Build commit: f827120 (f827120c9528a1a0e4c9881169eacd52170f9a08)

@compiler-errors
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-130367 created and queued.
🤖 Automatically detected try build f827120
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 14, 2024
[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.
@jackh726
Copy link
Member

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.

@compiler-errors
Copy link
Member Author

compiler-errors commented Sep 14, 2024

@jackh726:

I'm curious why the constrained case (https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9a297b502ceb3bd1e07ceae618f9a31e) does fail today.

I think you wrote that example wrong. we want T to be equal to &'?0 str here, but due to the way you added that new X variable to B, it's being set to the dyn type. That's why it's resulting in a type mismatch rather than a lifetime error. I believe a more faithful way of representing this is:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9483d15a409bdeac141dd529b4465da2

Specifically, I've renamed the T variable of the Erase struct to DYN to make it clear that it isn't the same as the T variable in traits A and B.

The deeper problem here is likely instead around implied bounds for higher-ranked lifetimes resulting in implied 'static.

I don't think this has to do with implied bounds. It really does only have to do with elaboration AFAICT.

@compiler-errors
Copy link
Member Author

compiler-errors commented Sep 14, 2024

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 B, we'd need to introduce a new type parameter T to B:

trait A<T>: B<T, T = T> {}

trait B<T> {
    type T;
}

That necessitates modifying foo like:

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 /* what do we put here? */.

@craterbot
Copy link
Collaborator

🚧 Experiment pr-130367 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-crater Status: Waiting on a crater run to be completed. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unconstrained lifetime in associated type using trait object leads to unsound lifetime extension and ICE
7 participants