Trying to suggest additional lifetime parameter #102323

Stoozy · 2022-09-26T17:13:22Z

@cjgillot This is what I have so far for #100615

rust-highfive · 2022-09-26T17:13:25Z

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.

estebank · 2022-09-26T18:35:07Z

Can you see if you can call this function from here

rust/compiler/rustc_resolve/src/late/diagnostics.rs

Line 2071 in 89e4e1f

fn suggest_introducing_lifetime(

cjgillot · 2022-09-26T20:19:52Z

compiler/rustc_resolve/src/late/lifetimes.rs

-                            sym::anonymous_lifetime_in_impl_trait,
-                            lifetime_ref.span,
-                            "anonymous lifetimes in `impl Trait` are unstable",
-                        ).emit();


You can keep this a feature error. You can keep this line, but instead of calling emit immediately, store it in a mut variable diag, decorate it using span_... methods, and call emit when you're done.

cjgillot · 2022-09-26T20:20:22Z

compiler/rustc_resolve/src/late/lifetimes.rs

-                        ).emit();
-                        return;
+
+                        match self.tcx.hir().get_generics(lifetime_ref.hir_id.owner.to_def_id().as_local().unwrap()) {


Suggested change

match self.tcx.hir().get_generics(lifetime_ref.hir_id.owner.to_def_id().as_local().unwrap()) {

match self.tcx.hir().get_generics(lifetime_ref.hir_id.owner.def_id) {

cjgillot · 2022-09-26T20:26:16Z

compiler/rustc_resolve/src/late/lifetimes.rs

-                        return;
+
+                        match self.tcx.hir().get_generics(lifetime_ref.hir_id.owner.to_def_id().as_local().unwrap()) {
+                            Some(generics) => {


Now you have generics you want 2 things to know where to suggest the lifetime parameter param_span, and in what form param_sugg.
Once you have that, you call

diag.multipart_suggestion( "consider introducing a named lifetime parameter", vec![ (lifetime_ref.span, "'a".to_owned()), // this is at the use site (param_span, param_sugg), // this is in generics ], )

it seems if I put "'a" for the message, it replaces the ampersand symbol from the original posters input. Should I keep this?

You can use lifetime_ref.span.shrink_to_hi() instead then.

cjgillot · 2022-09-26T20:27:35Z

@estebank That method is on the AST-based resolver, while the diagnostic is on the HIR-based resolver. There is no simple way to reuse that code.

cjgillot · 2022-09-27T18:42:32Z

@Stoozy you can run ./x.py fmt to format the code according to style guide.
Once you have something you are happy with, you can run ./x.py test --bless src/test/ui/<path to test file.rs> to record the change to error output, and commit the test file.

cjgillot · 2022-09-27T18:47:57Z

compiler/rustc_resolve/src/late/lifetimes.rs

+                                        diag.multipart_suggestion("consider introducing a named lifetime parameter",
+                                                                  vec![
+                                                                    (lifetime_ref.span.shrink_to_hi(), "'a ".to_owned()),
+                                                                    (generics.span, "<'a>".to_owned())


<'a> is only correct if there are no explicit generics. When there are explicit generics, you need to insert a 'a, just before the first explicit generic parameter.

cjgillot · 2022-09-27T19:09:13Z

compiler/rustc_resolve/src/late/lifetimes.rs

+
+                                        diag.multipart_suggestion("consider introducing a named lifetime parameter",
+                                                                  vec![
+                                                                    (lifetime_ref.span.shrink_to_hi(), "'a ".to_owned()),


This suggestion is only correct if you have a &, but it's a good start 😄. You can also have an elided lifetime with the '_ syntax, or in a path Ref<u8> instead of Ref<'_, u8> for instance.

cjgillot · 2022-09-28T19:38:20Z

compiler/rustc_resolve/src/late/lifetimes.rs

+
+                                    if !generics.span.contains(generics.params[i].span)  {
+
+                                        let mut diag =  rustc_session::parse::feature_err(


This code is common to both branches. Could you move it above the match?

cjgillot · 2022-09-28T19:38:49Z

compiler/rustc_resolve/src/late/lifetimes.rs

+
+                        match self.tcx.hir().get_generics(lifetime_ref.hir_id.owner.def_id) {
+                            Some(generics) => {
+                                for i in 0..generics.params.len()  {


You only use a single parameter. Could you separate the choice of param from the suggestion / move the suggestion code out of the loop?

cjgillot · 2022-09-28T19:39:32Z

compiler/rustc_resolve/src/late/lifetimes.rs

+
+                                                                    }
+                                                                  ], rustc_errors::Applicability::MaybeIncorrect);
+                                        diag.emit();


This is also common to both branches. Could you move this below the match?

compiler/rustc_resolve/src/late/lifetimes.rs

estebank · 2022-09-30T16:08:06Z

src/test/ui/suggestions/impl-trait-missing-lifetime-gated.stderr

+help: consider introducing a named lifetime parameter
+   |
+LL | fn g<'a>(x: impl Iterator<Item = &'_'a  ()>) -> Option<&'_ ()> { x.next() }
+   |     ++++                            ++


Looking at the resulting code here, the .shrink_to_hi() of the span without checking whether the span covers only & or '_ causes invalid code to be produced. It also seems like we might also want to replace the anon lifetime in the return type?

Replacing the anon lifetime in the return type seems like an outsized change. I'd rather stabilize this feature sooner, to get rid of the question.

Is there anything else I should do?

The question is: do you want to take a bit more time in this PR to fix the '_'a situation?

cjgillot

This is starting to look good, thanks for your patience @Stoozy. Could you clean-up formatting a bit? Rustfmt unfortunately skips let chains.

cjgillot · 2022-10-01T07:32:06Z

compiler/rustc_resolve/src/late/lifetimes.rs

+                                rustc_errors::Applicability::MaybeIncorrect);
+
+                            },
+                            None => { continue; }


Suggested change

None => { continue; }

None => {}

We still want to emit an error if there are no generics.

cjgillot · 2022-10-01T07:33:12Z

compiler/rustc_resolve/src/late/lifetimes.rs

@@ -1318,13 +1318,49 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
                        && let hir::IsAsync::NotAsync = self.tcx.asyncness(lifetime_ref.hir_id.owner.def_id)
                        && !self.tcx.features().anonymous_lifetime_in_impl_trait
                    {
-                        rustc_session::parse::feature_err(
+
+


Stray newlines.

cjgillot · 2022-10-01T07:34:40Z

compiler/rustc_resolve/src/late/lifetimes.rs

+                                                Some((self.tcx.sess.source_map().span_through_char(generics.span, '<').shrink_to_hi(), "'a, ".to_owned()))
+                                            },
+                                            None => Some((generics.span, "<'a>".to_owned()))
+                                        }


The loop body is independent of the loop itself. Could you remove the loop and just keep the assignment?

cjgillot · 2022-10-01T07:35:32Z

compiler/rustc_resolve/src/late/lifetimes.rs

                        return;
+


Stray newline.

cjgillot · 2022-10-01T07:36:53Z

src/test/ui/suggestions/impl-trait-missing-lifetime-gated.stderr

+help: consider introducing a named lifetime parameter
+   |
+LL | fn g<'a>(x: impl Iterator<Item = &'_'a  ()>) -> Option<&'_ ()> { x.next() }
+   |     ++++                            ++


The question is: do you want to take a bit more time in this PR to fix the '_'a situation?

Stoozy · 2022-10-01T17:59:28Z

I don't mind fixing the '_'a issue, I just need the steps.

cjgillot · 2022-10-09T16:25:59Z

@Stoozy let's do that in a follow-up PR.
@bors r+ rollup

bors · 2022-10-09T16:26:01Z

📌 Commit 2657f9d has been approved by cjgillot

It is now in the queue for this repository.

Trying to suggest additional lifetime parameter `@cjgillot` This is what I have so far for rust-lang#100615

Rollup of 6 pull requests Successful merges: - rust-lang#102275 (Stabilize `half_open_range_patterns`) - rust-lang#102323 (Trying to suggest additional lifetime parameter) - rust-lang#102345 (Recover from impl Trait in type param bound) - rust-lang#102845 (Elaborate trait ref to compute object safety.) - rust-lang#102860 (Add missing documentation for FileNameDisplayPreference variants) - rust-lang#102862 (From<Alignment> for usize & NonZeroUsize) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Trying to suggest additional lifetime parameter

887515a

rust-highfive assigned oli-obk Sep 26, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 26, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 26, 2022

estebank closed this Sep 26, 2022

estebank reopened this Sep 26, 2022

cjgillot reviewed Sep 26, 2022

View reviewed changes

cjgillot self-assigned this Sep 26, 2022

This comment has been minimized.

Sign in to view

Using multipart suggestion

365457b

This comment has been minimized.

Sign in to view

oli-obk removed their assignment Sep 27, 2022

Properly formatting the multipart suggestion

24c8e27

cjgillot reviewed Sep 27, 2022

View reviewed changes

Proper span for new generic param suggestion

e7cb6ad

cjgillot reviewed Sep 28, 2022

View reviewed changes

Emitting error regardless of new param suggestion

ef930a2

Stoozy force-pushed the master branch from 3569935 to ef930a2 Compare September 29, 2022 08:07

estebank reviewed Sep 30, 2022

View reviewed changes

cjgillot reviewed Oct 1, 2022

View reviewed changes

Removed unnecessary for loop

2657f9d

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2022

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 9, 2022

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 10, 2022

Rollup merge of rust-lang#102323 - Stoozy:master, r=cjgillot

1ba8c95

Trying to suggest additional lifetime parameter `@cjgillot` This is what I have so far for rust-lang#100615

This was referenced Oct 10, 2022

Rollup of 7 pull requests #102865

Closed

Rollup of 6 pull requests #102867

Merged

bors merged commit 0db05f1 into rust-lang:master Oct 10, 2022

rustbot added this to the 1.66.0 milestone Oct 10, 2022

BGR360 mentioned this pull request Nov 7, 2022

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

Closed

c410-f3r mentioned this pull request Jan 27, 2023

Stabilize anonymous_lifetime_in_impl_trait #107378

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trying to suggest additional lifetime parameter #102323

Trying to suggest additional lifetime parameter #102323

Stoozy commented Sep 26, 2022

rust-highfive commented Sep 26, 2022

estebank commented Sep 26, 2022

cjgillot Sep 26, 2022

cjgillot Sep 26, 2022

cjgillot Sep 26, 2022

Stoozy Sep 27, 2022

estebank Sep 27, 2022

cjgillot commented Sep 26, 2022

This comment has been minimized.

This comment has been minimized.

cjgillot commented Sep 27, 2022

cjgillot Sep 27, 2022

cjgillot Sep 27, 2022

cjgillot Sep 28, 2022

cjgillot Sep 28, 2022

cjgillot Sep 28, 2022

estebank Sep 30, 2022

cjgillot Sep 30, 2022

Stoozy Oct 1, 2022

cjgillot Oct 1, 2022

cjgillot left a comment

cjgillot Oct 1, 2022

cjgillot Oct 1, 2022

cjgillot Oct 1, 2022

cjgillot Oct 1, 2022

cjgillot Oct 1, 2022

Stoozy commented Oct 1, 2022

cjgillot commented Oct 9, 2022

bors commented Oct 9, 2022

	match self.tcx.hir().get_generics(lifetime_ref.hir_id.owner.to_def_id().as_local().unwrap()) {
	match self.tcx.hir().get_generics(lifetime_ref.hir_id.owner.def_id) {


		if !generics.span.contains(generics.params[i].span) {

		let mut diag = rustc_session::parse::feature_err(

Trying to suggest additional lifetime parameter #102323

Trying to suggest additional lifetime parameter #102323

Conversation

Stoozy commented Sep 26, 2022

rust-highfive commented Sep 26, 2022

estebank commented Sep 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot commented Sep 26, 2022

This comment has been minimized.

This comment has been minimized.

cjgillot commented Sep 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stoozy commented Oct 1, 2022

cjgillot commented Oct 9, 2022

bors commented Oct 9, 2022