Better diagnostics with mismatched types due to implicit static lifetime #87244
Conversation
This comment has been hidden.
This comment has been hidden.
jackh726
force-pushed the
jackh726:issue-71883
branch
2 times, most recently
from
0542c18
to
69531a4
This comment has been hidden.
This comment has been hidden.
|
The new output is great! I have some nitpicks that I'd like to address before we land this.
If we can add a new error code that'd be great, but it is perfectly fine to land things like this without an error code, at least at the beginning, as long as the error itself contains as much information as an error code index entry would. |
...rc/infer/error_reporting/nice_region_error/mismatched_static_lifetime.rs
Show resolved
Hide resolved
...eric-associated-types/issue-78113-lifetime-mismatch-dyn-trait-box.stderr
Outdated
Show resolved
Hide resolved
...rc/infer/error_reporting/nice_region_error/mismatched_static_lifetime.rs
Outdated
Show resolved
Hide resolved
...rc/infer/error_reporting/nice_region_error/mismatched_static_lifetime.rs
Outdated
Show resolved
Hide resolved
| LL | pub struct Wrapper<T: Trait>(T); | ||
| | ^^^^^ |
estebank
Jul 19, 2021
Contributor
In this case the 'static lifetime obligation is introduced by the type parameter (T: Trait is implicitly 'sized). It'd be nice to proactively handle this, detect this case and mention that.
In this case the 'static lifetime obligation is introduced by the type parameter (T: Trait is implicitly 'sized). It'd be nice to proactively handle this, detect this case and mention that.
jackh726
Jul 19, 2021
Author
Contributor
Actually, this is because the lifetime on Ref in impl Trait for Ref {} falls back to 'static in resolution.
The lifetime for Ref in pub fn repro(_: Wrapper<Ref>) on the other hand defaults to an anonymous lifetimes.
A diagnostic here still might be useful to say something like "this lifetime is being assumed as 'static'"
Actually, this is because the lifetime on Ref in impl Trait for Ref {} falls back to 'static in resolution.
The lifetime for Ref in pub fn repro(_: Wrapper<Ref>) on the other hand defaults to an anonymous lifetimes.
A diagnostic here still might be useful to say something like "this lifetime is being assumed as 'static'"
jackh726
Jul 19, 2021
Author
Contributor
I think an extra diagnostic here might make sense for another PR?
I think an extra diagnostic here might make sense for another PR?
I would prefer if it was in the same PR, but I'm ready to merge this after addressing my nitpicks if you don't have the time to try and handle that case. |
...rc/infer/error_reporting/nice_region_error/mismatched_static_lifetime.rs
Show resolved
Hide resolved
...rc/infer/error_reporting/nice_region_error/mismatched_static_lifetime.rs
Outdated
Show resolved
Hide resolved
|
Ok @estebank I modified the output just a bit, in order to make it a bit better in cases where there is an explicit static, multiple implicit statics, or when the impl isn't local. I also added a small note to the assumed static in cases of missing lifetimes. This isn't in the implicit static error message, but should maybe be enough for someone to figure out what's going on. I'd rather land this and maybe work more on that another time. |
| @@ -336,6 +336,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> { | |||
| insertion_sp, | |||
| suggestion, | |||
| ); | |||
| err.note("assuming a `'static` lifetime..."); | |||
estebank
Jul 20, 2021
Contributor
Not a fan of this new note, but I see your point.
Not a fan of this new note, but I see your point.
|
@bors r+ |
|
|
|
|
|
This was flagged as causing a moderate regression with respect to instruction counts in the compiler: perf.rlo link Most of the regressions are under the 1% threshold, but there's a lot of them that exceed the 0.2% threshold we use for non-noisy benchmarks. Was this PR, which I understand adds an extra region error pass, expected to cause that much additional overhead? Skimming, I see a place where a |
|
@pnkfelix nope that's unexpected; the pass should should only run after a region error has been generated. The only thing that could have possibly had an effect, I think, is https://github.com/rust-lang/rust/pull/87244/files#diff-34d5893dd95fe09f9a0fd3341efacd1c21853bb34ba29d9d79bb9af26bb8a0a0R1906-R1911. Which is...not much of a change. |
Fixes #78113
I think this is my first diagnostics PR...definitely happy to hear thoughts on the direction/implementation here.
I was originally just trying to solve the error above, where the lifetime on a GAT was causing a cryptic "mismatched types" error. But as I was writing this, I realized that this (unintentionally) also applied to a different case:
wf-in-foreign-fn-decls-issue-80468.rs. I'm not sure if this diagnostic should get a new error code, or even reuse an existing one. And, there might be some ways to make this even more generalized. Also, the error is a bit more lengthy and verbose than probably needed. So thoughts there are welcome too.This PR essentially ended up adding a new nice region error pass that triggers if a type doesn't match the self type of an impl which is selected because of a predicate because of an implicit static bound on that self type.
r? @estebank