Don't target synthetic APIT params in lifetime suggestion

[ozankenangungor]

Fixes #158954

This fixes an invalid lifetime suggestion involving argument-position impl Trait.

rustc could insert the new lifetime before the impl Trait argument, producing invalid code like 'a, impl Clone. Now it introduces the lifetime on the function generics instead.

This only changes the diagnostic suggestion.

[rustbot]

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 75 candidates
• Random selection from 20 candidates

[Enselic]

I have done a fairly detailed review of this fix, and as far as I can tell, this is a legitimate fix. Since the fix looks good to me, I will r+ it, but I will wait a day or two to allow others to raise objections. Since this is just a diagnostics change, it's relatively low-risk though.

r? me

The old logic tried to find "the first generic parameter with an explicit name", and assumed that would be the right place to insert 'a, at. However, as pointed out in the issue, APIT is lowered in such a way that impl Trait is considered a generic parameter with an explicit name, but its Span is not inside <...>, but its original place. So 'a, ends up inserted in the wrong spot.

The fix instead uses a helper that essentially instead checks "is there any <...> at all?", by checking if the span of the first generic arg is inside the Generics Span.

This helper is used in the same way in two more places. So adding one more such place seems very uncontroversial. Here is one such other place:

rust/compiler/rustc_hir_analysis/src/collect/resolve_bound_vars.rs

Lines 1324 to 1329 in ae45457

	let new_param_sugg =
	if let Some(span) = generics.span_for_lifetime_suggestion() {
	(span, "'a, ".to_owned())
	} else {
	(generics.span, "<'a>".to_owned())
	};

I'd also like to make a couple of bonus points:

• It looks like the other uses of the same helper can be de-duplicated, but that's optional and can be done in a follow-up PR, if the diff of such a refactoring ends up nice in the first place.
• When adding a new test to test a fix, it's good to add the test in a separate commit so that it becomes blessed without the fix, and then make a second commit that introduces the fix and re-blesses the output. That way it becomes super clear what impact the fix has on the diagnostics.
• The test uses // instead of /// for docs, but some other tests do the same, and if we really care we should add a tidy check for it and fix other tests.

View changes since this review

[Enselic]

@bors r+ rollup

(See my previous comment.)

[rust-bors]

📌 Commit 3d96096 has been approved by Enselic

It is now in the queue for this repository.