Suggest adding a new lifetime parameter when two elided lifetimes should match up for traits and impls. #94464
Conversation
…uld match up for traits and impls. Issue rust-lang#94462
rustbot
added
the
T-compiler
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @oli-obk (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
| .. | ||
| }) | ||
| | hir::Node::TraitItem(&hir::TraitItem { ref generics, .. }) | ||
| | hir::Node::ImplItem(&hir::ImplItem { ref generics, .. }) => generics, |
There was a problem hiding this comment.
We should handle the impl case specifically with extra wording in the suggestion to let people know that they'll have to modify the trait as well. This is likely not a problem in practice, but I'd like us to be thorough and avoid confusing people.
It might be enough to just have a boolean flag that you use to modify the suggestion description with extra wording.
There was a problem hiding this comment.
Good call. I'll make that change.
There was a problem hiding this comment.
How about this for impl?
help: consider introducing a named lifetime parameter and update trait if needed
|
32 | fn foo<'a>(&'a self, x: &i32, z: &'a i32) -> Option<&i32> {
| ++++ ++ ++
error: aborting due to previous error
For more information about this error, try `rustc --explain E0623`.
There was a problem hiding this comment.
Ideally we would have something like "consider introducing a named lifetime parameter and update trait TraitName as necessary", by getting the TraitName from the parent DefId, but that can be left as follow up work.
| help: consider introducing a named lifetime parameter | ||
| | | ||
| LL | fn a<'a>(self: Pin<&'a Foo>, f: &'a Foo) -> &Foo { f } | ||
| | ++++ ++ ++ |
There was a problem hiding this comment.
I'm somewhat surprised by the suggestions I'm seeing that we don't also suggest adding 'a to the return type. Maybe an existing bug? No need to handle it in this case directly, but would we should make a note of it.
There was a problem hiding this comment.
Do you want me to add a comment about that note?
I'm not 100% sure it's a bug. Seems like the code that added the hint did not take into consideration return types. To be fair though, AFAICT the diagnosis in this module is implemented very differently than what is in LifetimeContext in add_missing_lifetime_specifiers_label (which does add lifetime hints for the return type). LifetimeContext has a lot more "information", which it gathers during compilation, about the error so it can construct a better hint.
There was a problem hiding this comment.
Let's just create a targeted ticket for this instead of leaving it in the code. It'd be p-low, as we already have follow up suggestions to get you to working code, although it isn't great when there's another lifetime involved
|
@bors r+ |
|
📌 Commit 1b08cba has been approved by |
bors
added
S-waiting-on-bors
…askrgr Rollup of 9 pull requests Successful merges: - rust-lang#94464 (Suggest adding a new lifetime parameter when two elided lifetimes should match up for traits and impls.) - rust-lang#94476 (7 - Make more use of `let_chains`) - rust-lang#94478 (Fix panic when handling intra doc links generated from macro) - rust-lang#94482 (compiler: fix some typos) - rust-lang#94490 (Update books) - rust-lang#94496 (tests: accept llvm intrinsic in align-checking test) - rust-lang#94498 (9 - Make more use of `let_chains`) - rust-lang#94503 (Provide C FFI types via core::ffi, not just in std) - rust-lang#94513 (update Miri) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
|
Closes #94462 |
Suggest adding a new lifetime parameter when two elided lifetimes should match up for functions in traits and impls.
Issue #94462