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

Anon lifetime in impl Trait no longer suggests adding a lifetime parameter #100615

Closed
estebank opened this issue Aug 16, 2022 · 14 comments
Closed
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: lifetime related D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@estebank
Copy link
Contributor

estebank commented Aug 16, 2022

Given fn foo(_: impl Iterator<Item = &u32>) {}.

In 1.63:

error[E0106]: missing lifetime specifier
 --> src/lib.rs:1:32
  |
1 | fn foo(_: impl Iterator<Item = &u32>) {}
  |                                ^ expected named lifetime parameter
  |
help: consider introducing a named lifetime parameter
  |
1 | fn foo<'a>(_: impl Iterator<Item = &'a u32>) {}
  |       ++++                          ++

In 1.64:

error[E0658]: anonymous lifetimes in `impl Trait` are unstable
 --> src/lib.rs:1:32
  |
1 | fn foo(_: impl Iterator<Item = &u32>) {}
  |                                ^
@estebank estebank added A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: lifetime related T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. labels Aug 16, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 16, 2022
@TaKO8Ki TaKO8Ki self-assigned this Aug 16, 2022
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Aug 17, 2022
@pnkfelix pnkfelix self-assigned this Sep 1, 2022
@pnkfelix
Copy link
Member

pnkfelix commented Sep 1, 2022

self-assigning to ensure we record a cargo-bisect to the injecting PR

@apiraino
Copy link
Contributor

apiraino commented Sep 1, 2022

I went ahead and attempted a bisection. The breadcrumbs seem to lead to rollup of #99231 with possibly #97720 being the patchset where this issue originates cc @cjgillot ?

searched nightlies: from nightly-2022-07-13 to nightly-2022-07-15
regressed nightly: nightly-2022-07-15
searched commit range: 87588a2...c2f428d
regressed commit: f1a8854

bisected with cargo-bisect-rustc v0.6.3

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start 2022-07-13 --end 2022-07-15 --preserve 

@spastorino
Copy link
Member

I'm not sure what would be wrong after @cjgillot change. I think this is exactly the support Camille has added. If you use #![feature(anonymous_lifetime_in_impl_trait)] the example compiles and if you don't the error points to the anonymous lifetime and tells you that you would need the feature flag to support such case.

@cjgillot
Copy link
Contributor

cjgillot commented Sep 2, 2022

Added an explanation in #85417 (comment).
TLDR: I proposed to allow this syntax, we made a feature gate instead, and I forgot to put back the suggestion.

@apiraino
Copy link
Contributor

Visited during last weeks' T-compiler meeting on Zulip, relaying Felix's comment here for visibility:

@cjgillot are you planning to put back the suggestion? Alternatively, do you think that might be a nice small task for a new contributor to pick up?

@Mark-Simulacrum Mark-Simulacrum added this to the 1.64.0 milestone Sep 16, 2022
@wesleywiser wesleywiser added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Sep 22, 2022
@cjgillot
Copy link
Contributor

This should be an easy enough task for a new contributor. Summary instructions:

  • the feature-gate error is emitted from rustc_resolve::late::lifetimes::resolve_lifetime_ref;
  • the generics of the enclosing item can be accessed by tcx.hir().get_generics(lifetime_ref.hir_id.owner);
  • if that generics struct is Some, this means the item can have generics, not that it actually has;
  • the contained value is a hir::Generics, which should be analysed to know how to suggest: span contains the full <...> span, and params shows how many of them each with their own span;
  • I recommend suggesting the additional lifetime parameter as the first one, to avoid questions about the order of lifetime/type/const parameters.

@cjgillot cjgillot added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-help-wanted Call for participation: Help is requested to fix this issue. labels Sep 22, 2022
@Stoozy
Copy link
Contributor

Stoozy commented Sep 26, 2022

Hello, I would like to work on this. So far I've been able to get this kind of output

error[E0106]: missing lifetime specifier
 --> test.rs:1:32
  |
1 | fn foo(_: impl Iterator<Item = & u32>) {}
  |                                ^ expected named lifetime parameter
  |
help: consider introducing a named lifetime parameter
 --> test.rs:1:7
  |
1 | fn foo(_: impl Iterator<Item = & u32>) {}
  |       ^

with the input found in the original message.

I am looping over all generic params and checking if the full <...> span doesn't contain the span associated with the parameter, at which point I try to suggest the introduction of the named lifetime parameter.

I'm looking for a few pointers on:

  • If my logic actually makes sense
  • what other suggestions rustc_resolve::late::lifetimes::resolve_lifetime_ref should handle.
  • how to properly emit the errors/suggestions (I see that late/diagnostics.rs also does similar suggestions but I'm unsure if/how I should use it)

@cjgillot
Copy link
Contributor

@Stoozy could you open a PR with what you have and tag me? Discussing on code will be more concrete and easier.

@Rageking8
Copy link
Contributor

@Stoozy Another friendly tip: when you want to undertake an issue, you should claim the issue first to prevent duplicated effort (Write the command shown below in the target issue as a comment to assign urself). Thanks.

Screenshot_2022-09-26-17-40-49-17_320a9a695de7cdce83ed5281148d6f19.jpg

@estebank
Copy link
Contributor Author

@Stoozy can you call LateResolutionVisitor::suggest_introducing_lifetime (or split part of its body to make it appropriate for this case too) and reuse that logic?

@Stoozy
Copy link
Contributor

Stoozy commented Sep 26, 2022

@rustbot claim

JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 10, 2022
Trying to suggest additional lifetime parameter

`@cjgillot` This is what I have so far for rust-lang#100615
JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 10, 2022
Trying to suggest additional lifetime parameter

``@cjgillot`` This is what I have so far for rust-lang#100615
@BGR360
Copy link
Contributor

BGR360 commented Nov 7, 2022

Closed by #102323? Or is there more to do?

@cjgillot
Copy link
Contributor

cjgillot commented Nov 7, 2022

Yes, closed by #102323, and #104048 will handle some suggestion corner cases.

@cjgillot cjgillot closed this as completed Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: lifetime related D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. 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