-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Slightly improve mismatched GAT where clause error #99353
Slightly improve mismatched GAT where clause error #99353
Conversation
r? @cjgillot (rust-highfive has picked a reviewer for you, use r? to override) |
134a487
to
3d3bf10
Compare
@@ -1,20 +1,22 @@ | |||
error: `impl` associated type signature for `A` doesn't match `trait` associated type signature | |||
--> $DIR/impl_bounds.rs:15:5 | |||
error[E0310]: the parameter type `T` may not live long enough |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird, lol, but seems more like a side-effect than due to these changes. I bet if we do the same setup with methods it would report the same error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we complain here that the bounds have changed? There should be something saying that the Self: 'static
bound does not match the Self: 'a
bound from the trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It think the problem here is that we're not just comparing the bounds syntactically, but instead using the lifetime machinery.
So "given T: 'a, does T: 'static" (or vice versa) ends up erroring out in a harder to understand way than just a trait bounds mismatch... lifetime bounds mismatch sometimes can't even point to a bound at all when it's, for example, implied by the self type of the impl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit suprised. In the case of a method, we get the expected "impl has stricter requirements than trait" error here too: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=8dada6c436de5aadf8d5a4d2f18cfd7d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I found it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 2bbcdc7, then I stacked another commit on top that combines the obligation cause codes so something like this is harder to miss in the future (and adjusts the wording a bit).
3d3bf10
to
9734d79
Compare
9734d79
to
3bbe95c
Compare
@bors r+ |
Rollup of 7 pull requests Successful merges: - rust-lang#98211 (Implement `fs::get_path` for FreeBSD.) - rust-lang#99353 (Slightly improve mismatched GAT where clause error) - rust-lang#99593 (Suggest removing the tuple struct field for the unwrapped value) - rust-lang#99615 (Remove some explicit `self.infcx` for `FnCtxt`, which already derefs into `InferCtxt`) - rust-lang#99711 (Remove reachable coverage without counters) - rust-lang#99718 (Avoid `&str`/`Symbol` to `String` conversions) - rust-lang#99720 (Sync rustc_codegen_cranelift) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 7 pull requests Successful merges: - rust-lang#98211 (Implement `fs::get_path` for FreeBSD.) - rust-lang#99353 (Slightly improve mismatched GAT where clause error) - rust-lang#99593 (Suggest removing the tuple struct field for the unwrapped value) - rust-lang#99615 (Remove some explicit `self.infcx` for `FnCtxt`, which already derefs into `InferCtxt`) - rust-lang#99711 (Remove reachable coverage without counters) - rust-lang#99718 (Avoid `&str`/`Symbol` to `String` conversions) - rust-lang#99720 (Sync rustc_codegen_cranelift) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This makes the error reporting a bit more standardized between
where
on GATs and functions.cc #99206 (@BoxyUwU), don't want to mark this as as "fixed" because they're still not perfect, but this is still an improvement IMO so I want to land it incrementally.
regarding "consider adding where clause to trait definition", we don't actually do that for methods as far as i can tell? i could file an issue to look into that maybe.