impl_trait_overcaptures: Don't worry about uncaptured contravariant lifetimes if they outlive a captured lifetime by compiler-errors · Pull Request #129028 · rust-lang/rust (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

compiler-errors

@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

Aug 12, 2024

lcnr

lcnr

lcnr

@rustbot 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

@rustbot 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

lcnr

@rustbot 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

@compiler-errors

@compiler-errors

@bors 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

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Sep 5, 2024

@matthiaskrgr

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:

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 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.

This was referenced

Sep 5, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Sep 5, 2024

@bors

…iaskrgr

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Sep 5, 2024

@bors

…iaskrgr

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Sep 5, 2024

@rust-timer

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:

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 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.