`impl_trait_overcaptures`: Don't worry about uncaptured contravariant lifetimes if they outlive a captured lifetime #129028

compiler-errors · 2024-08-12T22:19:19Z

NOTE: Review only the first commit carefully. The second one is just moving stuff around, so you can turn whitespace off for that one.

This PR relaxes the impl_trait_overcaptures lint to not fire in cases like:

struct Ctxt<'tcx>(&'tcx ());

impl<'tcx> Ctxt<'tcx> {
    fn compute(&self) -> impl Sized + '_ { }
}

Specifically, the lint will not fire if all overcaptured regions (i.e. those which will be captured in edition 2024, which are not captured today) satisfy:

The region is contravariant (or bivariant) in the function signature
The region outlives some other region which is captured by the opaque

The idea behind this

Why is this OK? My reasoning is that since the region is contravariant in the function signature, we know that it can be shortened arbitrarily at the call site. And specifically, we know it can be shortened to be equal to one of the regions that it outlives -- that's why we need to prove that it outlives some other region that is captured.

We could technically relax this further, but there would be (IMO somewhat easy) cases to make this a false negative in real code. For example, if the region is invariant, then we can hit issues like:

struct Ctxt<'tcx>(&'tcx mut &'tcx mut ());

impl<'tcx> Ctxt<'tcx> {
    fn compute(&self) -> impl Sized + use<'_, 'tcx> { }
    // We use `use<'_, 'tcx>` to show what happens in edition 2024
}

fn test<'a, 'b>(x: &'a Ctxt<'b>, y: &'a Ctxt<'a>) {
    let results = [x.compute(), y.compute()];
    //~^ ERROR lifetime may not live long enough
    // Since both opaques now capture `'tcx`, this enforces that `'a == 'b`.
}

Is this actually totally fine?

There's one case where users might still hit issues, and it's if we turbofish lifetimes directly:

struct Ctxt<'tcx>(&'tcx ());

impl<'tcx> Ctxt<'tcx> {
    fn compute(&self) -> impl Sized + use<'_, 'tcx> { }
}

fn test<'a, 'b>(x: &'a Ctxt<'b>, y: &'a Ctxt<'a>) {
    let results = [Ctxt::<'b>::compute(x), Ctxt::<'a>::compute(y)];
    //~^ ERROR lifetime may not live long enough
    // Since both opaques now capture `'tcx`, this enforces that `'a == 'b`.
    // Note that we don't shorten `'b` to `'a` since we turbofished it.
}

Well... we should still warn?

I kinda don't care about this case, though I guess we could possibly downgrade the lint to something like IMPL_TRAIT_OVERCAPTURES_STRICT instead of suppressing it altogether. Thoughts? If we were to do this, then I'd probably also opt to include the invariant case in IMPL_TRAIT_OVERCAPTURES_STRICT and move it out of IMPL_TRAIT_OVERCAPTURES.

rustbot · 2024-08-12T22:19:27Z

r? @Nadrieril

rustbot has assigned @Nadrieril.
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

lcnr · 2024-08-13T08:30:44Z

r? @lcnr

lcnr

ignore the review comments for now. I am still a bit confused by the general approach, gonna try to walk through it in a separate comment

compiler/rustc_lint/src/impl_trait_overcaptures.rs

lcnr

The idea is: this lint only works on edition < 2021 because it uses the fact that uncaptured lifetimes are bivariant in the opaque type.

So walking the whole signature, including the opaque type correctly gets the variance of all used lifetimes. The lint currently triggers for all currently uncaptured lifetimes which are not outlived by a captured lifetime.

The idea is now that we also don't lint for contravariant lifetimes of the function. In inputs, these are covariant for the caller, so the caller can arbitrarily shorten their lifetimes before passing them into the function. While for contravariant lifetimes in the return type¹, the caller can simply rely on subtyping of the return type to lengthen them back.

Now, the reason these contravariant lifetimes have to outlive any of the captured region is that otherwise shortening them may result in new errors because it reduces the lifetime of the opaque type itself¹.

If so, is that in issue even if we have an explicit outlives bound on the opaque?

@compiler-errors did I understand this correctly?

tests please ↩ ↩²

lcnr · 2024-08-13T08:47:35Z

I personally don't care for the cases where users explicitly specify the lifetimes used in their function. I can't imagine that there will be many (if any) cases where it cases issues.

compiler-errors · 2024-08-13T14:09:48Z

The idea is: this lint only works on edition < 2021 because it uses the fact that uncaptured lifetimes are bivariant in the opaque type.

This lint only matters on edition < 2021, for the record. It's an edition migration lint after all, so in edition 2024 everything is captured already.

So walking the whole signature, including the opaque type correctly gets the variance of all used lifetimes. The lint currently triggers for all currently uncaptured lifetimes which are not outlived by a captured lifetime.

The lint on nightly triggers for all uncaptured lifetimes, regardless if they are longer than a captured lifetime.

The idea is now that we also don't lint for contravariant lifetimes of the function. In inputs, these are covariant for the caller, so the caller can arbitrarily shorten their lifetimes before passing them into the function. While for contravariant lifetimes in the return type1, the caller can simply rely on subtyping of the return type to lengthen them back.

The idea now is that we don't lint for contravariant lifetimes that are also longer (i.e. they outlive) than a captured lifetime. But yes, you're exactly right.

Now, the reason these contravariant lifetimes have to outlive any of the captured region is that otherwise shortening them may result in new errors because it reduces the lifetime of the opaque type itself1.

Very much correct. We know that any type-outlives obligation that is being put on the opaque already has to prove that all the captured lifetimes outlive the goal lifetime, so if we put another lifetime into there that already outlives one of those captured lifetimes, then it's still trivially provable.

If so, is that in issue even if we have an explicit outlives bound on the opaque?

Well, probably not; if we have an explicit outlives bound on the opaque then capturing another lifetime shouldn't matter for the purposes of liveness computation. I guess we could always suggest overcapturing a lifetime if there's an explicit outlives bound, but things will still fail in exactly the cases that #116733 did not fix.

compiler/rustc_lint/src/impl_trait_overcaptures.rs

lcnr · 2024-08-14T06:39:13Z

@rustbot author

compiler-errors · 2024-08-19T17:20:29Z

@rustbot ready

lcnr · 2024-09-03T12:00:58Z

compiler/rustc_lint/src/impl_trait_overcaptures.rs

    // Then visit the signature to walk through all the binders (incl. the late-bound
    // vars on the function itself, which we need to count too).
    sig.visit_with(&mut VisitOpaqueTypes {
        tcx,
        parent_def_id,
        in_scope_parameters,
        seen: Default::default(),
+        // Lazily compute these two, since they're likely a bit expensive.
+        variances: LazyCell::new(|| {


did you actually test whether computing them eagerly is expensive?

Kinda don't think it's necessary to test, and I feel like I'd rather not generate these for every item for every edition anyways

lcnr

r=me on the impl, does this need any team signoff?

lcnr · 2024-09-03T12:04:58Z

@rustbot author

compiler-errors · 2024-09-03T12:05:16Z

No I don't think so. I've discussed this optimization with @traviscross too and he seemed enthusiastic.

lcnr · 2024-09-03T12:06:34Z

then r=me after rebase

compiler-errors · 2024-09-03T12:07:52Z

Er, not optimization; refinement of the lint. cool, can do. Ty for review.

…a captured lifetime

compiler-errors · 2024-09-05T13:50:02Z

@bors r=lcnr

bors · 2024-09-05T13:50:04Z

📌 Commit c1d0410 has been approved by lcnr

It is now in the queue for this repository.

`impl_trait_overcaptures`: Don't worry about uncaptured contravariant lifetimes if they outlive a captured lifetime **NOTE:** Review only the first commit carefully. The second one is just moving stuff around, so you can turn whitespace off for that one. This PR relaxes the `impl_trait_overcaptures` lint to not fire in cases like: ```rust struct Ctxt<'tcx>(&'tcx ()); impl<'tcx> Ctxt<'tcx> { fn compute(&self) -> impl Sized + '_ { } } ``` Specifically, the lint will not fire if **all** overcaptured regions (i.e. those which will be captured in edition 2024, which are not captured today) **satisfy**: * The region is contravariant (or bivariant) in the function signature * The region outlives some other region which is captured by the opaque ### The idea behind this Why is this OK? My reasoning is that since the region is contravariant in the function signature, we know that it can be shortened arbitrarily at the call site. And specifically, we know it can be shortened to be equal to one of the regions that it outlives -- that's why we need to prove that it outlives some other region that *is* captured. We could technically relax this further, but there would be (IMO somewhat easy) cases to make this a false negative in real code. For example, if the region is invariant, then we can hit issues like: ```rust struct Ctxt<'tcx>(&'tcx mut &'tcx mut ()); impl<'tcx> Ctxt<'tcx> { fn compute(&self) -> impl Sized + use<'_, 'tcx> { } // We use `use<'_, 'tcx>` to show what happens in edition 2024 } fn test<'a, 'b>(x: &'a Ctxt<'b>, y: &'a Ctxt<'a>) { let results = [x.compute(), y.compute()]; //~^ ERROR lifetime may not live long enough // Since both opaques now capture `'tcx`, this enforces that `'a == 'b`. } ``` ### Is this actually totally fine? There's one case where users might still hit issues, and it's if we turbofish lifetimes directly: ```rust struct Ctxt<'tcx>(&'tcx ()); impl<'tcx> Ctxt<'tcx> { fn compute(&self) -> impl Sized + use<'_, 'tcx> { } } fn test<'a, 'b>(x: &'a Ctxt<'b>, y: &'a Ctxt<'a>) { let results = [Ctxt::<'b>::compute(x), Ctxt::<'a>::compute(y)]; //~^ ERROR lifetime may not live long enough // Since both opaques now capture `'tcx`, this enforces that `'a == 'b`. // Note that we don't shorten `'b` to `'a` since we turbofished it. } ``` ### Well... we should still warn? I kinda don't care about this case, though I guess we could possibly downgrade the lint to something like `IMPL_TRAIT_OVERCAPTURES_STRICT` instead of suppressing it altogether. Thoughts? If we were to do this, then I'd probably also opt to include the invariant case in `IMPL_TRAIT_OVERCAPTURES_STRICT` and move it out of `IMPL_TRAIT_OVERCAPTURES`.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#128820 (fix: get llvm type of global val) - rust-lang#129028 (`impl_trait_overcaptures`: Don't worry about uncaptured contravariant lifetimes if they outlive a captured lifetime) - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance) - rust-lang#129706 (Rename dump of coroutine by-move-body to be more consistent, fix ICE in dump_mir) - rust-lang#129720 (Simplify DestProp memory management) - rust-lang#129796 (Unify scraped examples with other code examples) - rust-lang#129938 (Elaborate on deriving vs implementing `Copy`) - rust-lang#129973 (run_make_support: rename `Command::stdin` to `stdin_buf` and add `std{in,out,err}` config helpers) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#129028 - compiler-errors:contra, r=lcnr `impl_trait_overcaptures`: Don't worry about uncaptured contravariant lifetimes if they outlive a captured lifetime **NOTE:** Review only the first commit carefully. The second one is just moving stuff around, so you can turn whitespace off for that one. This PR relaxes the `impl_trait_overcaptures` lint to not fire in cases like: ```rust struct Ctxt<'tcx>(&'tcx ()); impl<'tcx> Ctxt<'tcx> { fn compute(&self) -> impl Sized + '_ { } } ``` Specifically, the lint will not fire if **all** overcaptured regions (i.e. those which will be captured in edition 2024, which are not captured today) **satisfy**: * The region is contravariant (or bivariant) in the function signature * The region outlives some other region which is captured by the opaque ### The idea behind this Why is this OK? My reasoning is that since the region is contravariant in the function signature, we know that it can be shortened arbitrarily at the call site. And specifically, we know it can be shortened to be equal to one of the regions that it outlives -- that's why we need to prove that it outlives some other region that *is* captured. We could technically relax this further, but there would be (IMO somewhat easy) cases to make this a false negative in real code. For example, if the region is invariant, then we can hit issues like: ```rust struct Ctxt<'tcx>(&'tcx mut &'tcx mut ()); impl<'tcx> Ctxt<'tcx> { fn compute(&self) -> impl Sized + use<'_, 'tcx> { } // We use `use<'_, 'tcx>` to show what happens in edition 2024 } fn test<'a, 'b>(x: &'a Ctxt<'b>, y: &'a Ctxt<'a>) { let results = [x.compute(), y.compute()]; //~^ ERROR lifetime may not live long enough // Since both opaques now capture `'tcx`, this enforces that `'a == 'b`. } ``` ### Is this actually totally fine? There's one case where users might still hit issues, and it's if we turbofish lifetimes directly: ```rust struct Ctxt<'tcx>(&'tcx ()); impl<'tcx> Ctxt<'tcx> { fn compute(&self) -> impl Sized + use<'_, 'tcx> { } } fn test<'a, 'b>(x: &'a Ctxt<'b>, y: &'a Ctxt<'a>) { let results = [Ctxt::<'b>::compute(x), Ctxt::<'a>::compute(y)]; //~^ ERROR lifetime may not live long enough // Since both opaques now capture `'tcx`, this enforces that `'a == 'b`. // Note that we don't shorten `'b` to `'a` since we turbofished it. } ``` ### Well... we should still warn? I kinda don't care about this case, though I guess we could possibly downgrade the lint to something like `IMPL_TRAIT_OVERCAPTURES_STRICT` instead of suppressing it altogether. Thoughts? If we were to do this, then I'd probably also opt to include the invariant case in `IMPL_TRAIT_OVERCAPTURES_STRICT` and move it out of `IMPL_TRAIT_OVERCAPTURES`.

rustbot assigned Nadrieril Aug 12, 2024

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 Aug 12, 2024

rustbot assigned lcnr and unassigned Nadrieril Aug 13, 2024

lcnr reviewed Aug 13, 2024

View reviewed changes

lcnr reviewed Aug 14, 2024

View reviewed changes

compiler/rustc_lint/src/impl_trait_overcaptures.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2024

compiler-errors force-pushed the contra branch from a73852b to a08aa18 Compare August 19, 2024 17:20

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 19, 2024

lcnr reviewed Sep 3, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 3, 2024

compiler-errors force-pushed the contra branch from a08aa18 to f29b818 Compare September 5, 2024 08:21

This comment has been minimized.

Sign in to view

Don't worry about uncaptured contravariant lifetimes if they outlive …

f8f4d50

…a captured lifetime

compiler-errors added 2 commits September 5, 2024 06:34

Do less work on the good path

7064135

Review comments

c1d0410

compiler-errors force-pushed the contra branch from f29b818 to c1d0410 Compare September 5, 2024 10:35

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 5, 2024

This was referenced Sep 5, 2024

Rollup of 9 pull requests #129993

Closed

Rollup of 8 pull requests #129994

Merged

bors merged commit b0cc78c into rust-lang:master Sep 5, 2024
6 checks passed

rustbot added this to the 1.83.0 milestone Sep 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`impl_trait_overcaptures`: Don't worry about uncaptured contravariant lifetimes if they outlive a captured lifetime #129028

`impl_trait_overcaptures`: Don't worry about uncaptured contravariant lifetimes if they outlive a captured lifetime #129028

compiler-errors commented Aug 12, 2024 •

edited

Loading

rustbot commented Aug 12, 2024

lcnr commented Aug 13, 2024

lcnr left a comment

lcnr left a comment •

edited

Loading

lcnr commented Aug 13, 2024

compiler-errors commented Aug 13, 2024 •

edited

Loading

lcnr commented Aug 14, 2024

compiler-errors commented Aug 19, 2024

lcnr Sep 3, 2024

compiler-errors Sep 3, 2024

lcnr left a comment

lcnr commented Sep 3, 2024

compiler-errors commented Sep 3, 2024

lcnr commented Sep 3, 2024

compiler-errors commented Sep 3, 2024

This comment has been minimized.

compiler-errors commented Sep 5, 2024

bors commented Sep 5, 2024

impl_trait_overcaptures: Don't worry about uncaptured contravariant lifetimes if they outlive a captured lifetime #129028

impl_trait_overcaptures: Don't worry about uncaptured contravariant lifetimes if they outlive a captured lifetime #129028

Conversation

compiler-errors commented Aug 12, 2024 • edited Loading

The idea behind this

Is this actually totally fine?

Well... we should still warn?

rustbot commented Aug 12, 2024

lcnr commented Aug 13, 2024

lcnr left a comment

Choose a reason for hiding this comment

lcnr left a comment • edited Loading

Choose a reason for hiding this comment

Footnotes

lcnr commented Aug 13, 2024

compiler-errors commented Aug 13, 2024 • edited Loading

lcnr commented Aug 14, 2024

compiler-errors commented Aug 19, 2024

lcnr Sep 3, 2024

Choose a reason for hiding this comment

compiler-errors Sep 3, 2024

Choose a reason for hiding this comment

lcnr left a comment

Choose a reason for hiding this comment

lcnr commented Sep 3, 2024

compiler-errors commented Sep 3, 2024

lcnr commented Sep 3, 2024

compiler-errors commented Sep 3, 2024

This comment has been minimized.

compiler-errors commented Sep 5, 2024

bors commented Sep 5, 2024

`impl_trait_overcaptures`: Don't worry about uncaptured contravariant lifetimes if they outlive a captured lifetime #129028

`impl_trait_overcaptures`: Don't worry about uncaptured contravariant lifetimes if they outlive a captured lifetime #129028

compiler-errors commented Aug 12, 2024 •

edited

Loading

lcnr left a comment •

edited

Loading

compiler-errors commented Aug 13, 2024 •

edited

Loading