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

GAT type Assoc<T: ?Sized> implicitly requires Self to be 'static #131008

Open
Veetaha opened this issue Sep 29, 2024 · 4 comments
Open

GAT type Assoc<T: ?Sized> implicitly requires Self to be 'static #131008

Veetaha opened this issue Sep 29, 2024 · 4 comments
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions A-trait-objects Area: trait objects, vtable layout D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Veetaha
Copy link
Contributor

Veetaha commented Sep 29, 2024

I tried this code:

trait Layout {}
trait Desc {
    type Children<A: ?Sized>;
    fn stage(_children: &Self::Children<dyn Layout>);
}

fn stage<D: Desc>(children: D::Children<dyn Layout>) {
    D::stage(&children);
}

This doesn't compile with two errors, and I can't explain why:

error[E0310]: the parameter type `D` may not live long enough
  --> crates/sandbox/src/main.rs:68:5
   |
68 |     D::stage(&children);
   |     ^^^^^^^^^^^^^^^^^^^
   |     |
   |     the parameter type `D` must be valid for the static lifetime...
   |     ...so that the type `D` will meet its required lifetime bounds
   |
help: consider adding an explicit lifetime bound
   |
67 | fn stage<D: Desc + 'static>(children: D::Children<dyn Layout>) {
   |                  +++++++++

error[E0597]: `children` does not live long enough
  --> crates/sandbox/src/main.rs:68:14
   |
67 | fn stage<D: Desc>(children: D::Children<dyn Layout>) {
   |                   -------- binding `children` declared here
68 |     D::stage(&children);
   |     ---------^^^^^^^^^-
   |     |        |
   |     |        borrowed value does not live long enough
   |     argument requires that `children` is borrowed for `'static`
69 | }
   | - `children` dropped here while still borrowed

Why is the D required to be 'static here?
Why does the &children need to have a 'static lifetime here as well?


Here are variations that do compile, but I also can't explain why they compile:

Adding `dyn Layout + 'static` in the trait definition

trait Layout {}
trait Desc {
    type Children<A: ?Sized>;
    fn stage(_children: &Self::Children<dyn Layout + 'static>);
}

fn stage<D: Desc>(children: D::Children<dyn Layout>) {
    D::stage(&children);
}

Adding `dyn Layout + '_` in the trait definition

trait Layout {}
trait Desc {
    type Children<A: ?Sized>;
    fn stage(_children: &Self::Children<dyn Layout + '_>);
}

fn stage<D: Desc>(children: D::Children<dyn Layout>) {
    D::stage(&children);
}

This variation doesn't compile, but it removes the `D` must be valid for the static lifetime error and I also don't understand why that is:

trait Layout {}
trait Desc {
    type Children<A: ?Sized>: 'static;
    fn stage(_children: &Self::Children<dyn Layout>);
}

fn stage<D: Desc>(children: D::Children<dyn Layout>) {
    D::stage(&children);
}

There is definitely something implicit going on here which I don't know. Some helpful people suggested this may be related to #87479, but I don't see how.

@nikomatsakis do you have an idea if this is related? Is this some compiler bug or smth not documented?

Meta

rustc --version --verbose:

rustc 1.81.0 (eeb90cda1 2024-09-04)
binary: rustc
commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c
commit-date: 2024-09-04
host: x86_64-unknown-linux-gnu
release: 1.81.0
LLVM version: 18.1.7
@Veetaha Veetaha added the C-bug Category: This is a bug. label Sep 29, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 29, 2024
@ismailarilik
Copy link
Contributor

@lukas-code
Copy link
Member

lukas-code commented Sep 29, 2024

This is not a bug, but the error message here is terrible.

Due to how trait object lifetime elision works,

  • &Self::Children<dyn Layout> actually means &'a Self::Children<dyn Layout + 'a> for one specific lifetime 'a
  • &Self::Children<dyn Layout + '_> actually means &'a Self::Children<dyn Layout + 'b> for two distinct lifetimes 'a and 'b
  • D::Children<dyn Layout> actually means D::Children<dyn Layout + 'static>

So with all inferred lifetimes annotated explicitly, your program becomes this:

trait Layout {}
trait Desc {
    type Children<A: ?Sized>;
    fn stage<'a>(_children: &'a Self::Children<dyn Layout + 'a>);
}

fn stage<D: Desc>(children: D::Children<dyn Layout + 'static>) {
    // here we unify the argument `&'b D::Children<dyn Layout + 'static>`
    // with the function parameter `&'a D::Children<dyn Layout + 'a>`
    // and end up with `'b == 'a == 'static`.
    D::stage(&/*'b */ children);
}

So the borrowchecker thinks that the borrow must be valid for 'static -- this causes the (correct) errors that children and D do not live long enough.

(In this example it is also not possible to shorten the trait object lifetime from D::Children<dyn Layout + 'static> to D::Children<dyn Layout + 'b>, because GAT arguments are invariant.)

The diagnostic here should probably mention default trait object lifetimes and offer the two workarounds that you already managed to come up with.

@rustbot label -C-bug +A-diagnostics +A-borrow-checker +A-trait-objects +D-terse -needs-triage

@rustbot rustbot added A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints A-trait-objects Area: trait objects, vtable layout D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. and removed C-bug Category: This is a bug. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 29, 2024
@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 29, 2024

So the borrowchecker thinks that the borrow must be valid for 'static -- this causes the (correct) errors that children and D do not live long enough.

@lukas-code, thank you for the explanation! Although I don't understand why D is required to be 'static? There is no trait Desc: 'static requirement in the trait, and the fact that the compiler unifies lifetimes to &'static D::Children<dyn Layout + 'static> doesn't explain why D has to be static to me. Sure, the D::Children<...> needs to be 'static (because it's under a 'static reference), but why D itself? Maybe it's documented somewhere and I can't find it?

@lukas-code
Copy link
Member

lukas-code commented Sep 29, 2024

Sure, the D::Children<...> needs to be 'static (because it's under a 'static reference)

Yeah and D::Children<dyn Layout + 'static> could be equal to D, for example when a dependant of your crate defines:

struct Wrapper<T>(T);
impl<T> Desc for Wrapper<T> {
    type Children<A: ?Sized> = Wrapper<T>;
}

In that case when the dependant calls your function stage, you would create a &'static D, so D has to outlive 'static.

@fmease fmease added A-lifetimes Area: Lifetimes / regions T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 29, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 4, 2024
…static, r=<try>

`best_blame_constraint`: Blame better constraints when failing to outlive `'static`

This fixes rust-lang#132749 by changing which constraint is blamed for region errors when failing to prove a region outlives `'static`. The comments give a better explanation, but very roughly, all the `'static: R` edges in the region graph made `best_blame_constraint` consider every constraint (other than those from ascriptions, returns, and yields) uninteresting to point to, so it chose semi-randomly based on an ordering of how interesting each variant of `ConstraintCategory` is expected to be. This PR (admittedly hackily) makes it ignore all those edges, so that the existing logic works the same as it would when failing to outlive any other region.

Looking at UI test diffs, most of them that changed I think changed for the better. Unfortunately, since a lot of borrowck's diagnostics depend on exactly which constraint is blamed, some things broke. A list of what I'm not happy with:
- For `CopyBound` constraints, e.g. [`tests/ui/nll/do-not-ignore-lifetime-bounds-in-copy.stderr`](https://github.com/rust-lang/rust/compare/master...dianne:rust:better-blame-constraints-for-static?expand=1#diff-e220f1e433c5e48d9afd431787f6ff27fc66b653762ee8a0283e370c2d88e7d0), I think it's helpful to surface that the `Copy` impl introduces the bound. If this is an issue, maybe it's worth prioritizing it or adding a variant of `ExtraConstraintInfo` for it.
- Likewise for `UseAsConst` and `UseAsStatic`; I've already added a special case for those. Without a special case, this was blaming `CallArgument` in [`tests/ui/consts/const-mut-refs/mut_ref_in_final.stderr`](https://github.com/dianne/rust/blob/189b2f892e6d0809a77fc92fe1108a07d8de9be0/tests/ui/consts/const-mut-refs/mut_ref_in_final.stderr#L38), which seemed pretty egregious. I'm assuming we want to blame `UseAsConst`/`UseAsStatic` when possible, rather than add something to `ExtraConstraintInfo`, since it's pretty unambiguously to-blame for "outlives `'static`" constraints. The special-casing admittedly also changes the output for [`tests/ui/inline-const/const-match-pat-lifetime-err.rs`](https://github.com/rust-lang/rust/compare/master...dianne:rust:better-blame-constraints-for-static?expand=1#diff-e4d2c147aa96dd8dd963ec3d98ead9a8096c9de809d19ab379be3c53951ca1ca), but I think the new output there is fine; it's more direct about how `'a` and `'static` are getting related.
- The subdiagnostic [`BorrowExplanation::add_object_lifetime_default_note`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_borrowck/diagnostics/explain_borrow/enum.BorrowExplanation.html#method.add_object_lifetime_default_note) depends on `Cast` being reported as the constraint category, so I've added a special case for it to keep it disappearing from [`tests/ui/traits/trait-object-lifetime-default-note.stderr`](https://github.com/dianne/rust/blob/better-blame-constraints-for-static/tests/ui/traits/trait-object-lifetime-default-note.stderr)[^1] . As I've outlined in a `FIXME` comment though, I think that subdiagnostic needs changing. There's multiple cases throughout `tests/ui` where it would be helpful, but doesn't fire, because a different constraint is picked. rust-lang#131008 would also benefit from that note, but there's not a coercion there at all. I tried making it fire in more cases, but fundamentally, since it doesn't *actually* check if an object lifetime default was used in the HIR, it produced some extraneous notes[^2]. I tried seeing if it'd be easy enough to fix that first, but it seems nontrivial[^3] enough to warrant a separate PR.

A final note: this may have perf consequences, since `best_blame_constraint` gets called on happy code too. If it's an issue, I can try to make it faster, or only do the expensive stuff when an error's been hit, or something. If nothing else, this makes the fallback logic (still relevant for invariant lifetimes[^4]) a bit faster.

[^1]: Incidentally, without the special-casing, this would pick `CallArgument`, which I think produces a nicer message, apart from the missing note.

[^2]: There's even some in current UI test output: [`tests/ui/borrowck/two-phase-surprise-no-conflict.stderr`](https://github.com/rust-lang/rust/blob/733616f7236b4be140ce851a30b3bb06532b9364/tests/ui/borrowck/two-phase-surprise-no-conflict.stderr#L68). The object lifetime is [explicitly provided](https://github.com/rust-lang/rust/blob/733616f7236b4be140ce851a30b3bb06532b9364/tests/ui/borrowck/two-phase-surprise-no-conflict.rs#L94), and despite the note, it's not `'static`.

[^3]: In case anyone reading this has advice: ultimately I think there has to be a choice between extraneous notes and missing notes unless "this object type had a missing lifetime in the HIR" is in crate metadata for some reason, since rustdoc doesn't elaborate object lifetime defaults, and the note is potentially helpful for library-users who may be wondering where a `'static` lifetime came from. That said, it's a bit confusing to point out lifetime defaults when they're not relevant[^5], so it's at least worth a best effort to look at user annotations. However, the HIR missing an object lifetime could be anywhere: a cast, an ascription, in the return type, in an input, in the signature for a function that's called, in an ADT field, in a type alias... I've considered looking at the entire output of `RegionInferenceContext::find_constraint_path_between_regions` and doing a best-effort association between typeck results (ideally MIR typeck for the region information) and whatever HIR seems relevant. But that seems like a lot of work for that note.

[^4]: Cycles in the region graph due to invariance also confuse `better_blame_constraint`'s attempt to pick a constraint where the regions aren't unified. I'm not sure if there's a better heuristic to use there, though; I played around a bit with it, but my experiments only made diagnostic output worse.

[^5]: Unless maybe there's a way of rewording the note so that it always makes sense to output when object lifetimes are involved in region errors?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions A-trait-objects Area: trait objects, vtable layout D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants