Fix argument removal suggestion around macros

[lukas-code]

Fixes #112437.
Fixes #113866.
Helps with #114255.

The issue was that span.find_ancestor_inside(outer) could previously return a span with a different expansion context from outer.

This happens for example for the built-in macro panic!, which expands to another macro call of panic_2021! or panic_2015!. Because the call site of panic_20xx! has not associated source code, its span currently points to the call site of panic! instead.

Something similar also happens items that get desugared in AST->HIR lowering. For example, for loops get two spans: One "inner" span that has the .desugaring_kind() kind set to DesugaringKind::ForLoop and one "outer" span that does not. Similar to the macro situation, both of these spans point to the same source code, but have different expansion contexts.

This causes problems, because joining two spans with different expansion contexts will usually¹ not actually join them together to avoid creating "spaghetti" spans that go from the macro definition to the macro call. For example, in the following snippet full_span might not actually contain the adjusted_start and adjusted_end. This caused the broken suggestion / debug ICE in the linked issues.

let adjusted_start = start.find_ancestor_inside(shared_ancestor);
let adjusted_end = end.find_ancestor_inside(shared_ancestor);
let full_span = adjusted_start.to(adjusted_end)

To fix the issue, this PR introduces a new method, find_ancestor_inside_same_ctxt, which combines the functionality of find_ancestor_inside and find_ancestor_in_same_ctxt: It finds an ancestor span that is contained within the parent and has the same syntax context, and is therefore safe to extend. This new method should probably be used everywhere, where the returned span is extended, but for now it is just used for the argument removal suggestion.

Additionally, this PR fixes a second issue where the function call itself is inside a macro but the arguments come from outside the macro. The test is added in the first commit to include stderr diff, so this is best reviewed commit by commit.

Footnotes

1. If one expansion context is the root context and the other is not. ↩

[rustbot]

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

[cjgillot]

We should be able to suggest empty() here. Should normalize_span return an Result<Span, Span> , Ok if successfully normalized, Err otherwise, instead of merging both cases?

[lukas-code]

AFAIK there is no way to go from the span of a macro argument

foo!(~, 1);
        ^ here

to the span of the metavariable

empty(1, $y);
         ^^ here

But it should be possible to suggest removing the other argument that is inside the macro, I'll look into that.

EDIT: What's actually happening here is the span of the argument gets "extended" to the empty span before the comma, which results in an empty span similar to the one from the bug report. This empty span then gets extended to just before the ), so we end up with:

empty(1, $y);
      │└─┬┘
      │  │
      │  help: remove the extra argument
      unexpected argument of type `{integer}`

Note that the span doesn't include the 1, this just isn't very visible in the stderr output.

[lukas-code]

Suggesting to remove the argument in the macro may be incorrect for this test if the macro definition is correct and the invocation is wrong: https://github.com/rust-lang/rust/blob/d1e79b1c74c0b68ef200b317de5aedb8790dc004/tests/ui/macros/issue-26094.rs#L1-L12

This especially applies to macros defined by dependencies.

Since the error already says unexpected argument of type `whatever` and that should be enough to figure out a solution, I think it's probably better to just leave the suggestion off.

[cjgillot]

This happens for example for the built-in macro panic!, which expands to another macro call of panic_2021! or panic_2015!. The call site of panic_20xx! points to the def site of panic!, but since panic! is built-in, its "def site" points at the same source code as its call site.

That's a surprising behaviour. panic! is not defined anywhere, so does not have a def site, should one be crafted instead of using the call site?

I'm not sure the change to find_ancestor_inside is the right solution here.

@petrochenkov has more knowledge than I do on macros.

[WaffleLapkin]

r? compiler

[jackh726]

Macro expansion isn't my strong suit. @cjgillot are you comfortable enough to take review here, or should I reroll?

[Noratrieb]

r? petrochenkov

[petrochenkov]

That's a surprising behaviour. panic! is not defined anywhere, so does not have a def site, should one be crafted instead of using the call site?

panic is not much different from a regular proc macro, if a proc macro messes up hygiene data in any way the compiler still should not ICE on that.

[petrochenkov]

@bors r+

[bors]

📌 Commit 985036b has been approved by petrochenkov

It is now in the queue for this repository.

[bors]

⌛ Testing commit 985036b with merge c94cb83...

[bors]

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing c94cb83 to master...

[bors]

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing c94cb83 to master...

[rust-timer]

Finished benchmarking commit (c94cb83): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.6%	[0.6%, 0.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 635.223s -> 634.344s (-0.14%)
Artifact size: 346.78 MiB -> 346.74 MiB (-0.01%)