Add method find_ancestor_not_from_macro and find_ancestor_not_from_extern_macro to supersede find_oldest_ancestor_in_same_ctxt

[xizheyin]

As I was using it, I realized that the function is supposed to walk up to expand the chain? This seems to be the opposite of what I understood.

r? @jieyouxu

[rustbot]

jieyouxu is currently at their maximum review capacity.
They may take a while to respond.

[jieyouxu]

Hm, I'm not actually 100% this helper is doing what I originally wanted it to do... This will try to keep traversing up the macro invocation chain that produced the provided final span at hand to find the oldest ancestor which has... the same syntax context as the provided final span (???) I suppose this might work if the passed in span you know is "local" and the user actually has control over 🤔

I feel like this is wrong, or least quite confusing... I wonder if you could go the other direction and use instead https://doc.rust-lang.org/nightly/nightly-rustc/rustc_span/struct.Span.html#method.find_ancestor_in_same_ctxt or https://doc.rust-lang.org/nightly/nightly-rustc/rustc_span/struct.Span.html#method.find_ancestor_inside_same_ctxt and get rid of this helper.

[jieyouxu]

@rustbot author (re. discussion)

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[xizheyin]

I found some problems with the comments of these APIs, I subconsciously thought that finding an ancestor would be traversing the macro expansion chain from bottom to top, but they all use “walk down”. This seems to create ambiguity. I'll look into these APIs later when I have time and find the best way to do it.

[xizheyin]

I found that find_oldest_ancestor_in_same_ctxt does have a logical problem, but by some mischance it works successfully in some places.
The places where it is needed are basically related to macros, and my understanding is that the proper ancestor should be found without any other span help, e.g. outer in span.find_ancestor_inside(outer).

I added two methods that allow for more fine-grained control. This change should help fix all the diagnostics shown in the macros in the standard library(#142403).

@rustbot ready

[jieyouxu]

Hm, is it possible to add specific test cases demonstrating the behaviors of

• find_ancestor_not_from_macro
• find_ancestor_not_from_extern_macro
• find_oldest_ancestor_in_same_ctxt

Otherwise, we'd have no idea what effect changing the impls of these helpers might be.

[xizheyin]

I don't know how to add the right test cases, but I can find the test case corresponding to the diagnostic I modified in this PR, which should be very helpful to understand its behavior. I'm going to insert the debug macro in the method of concern, and then print the debug message via what compile-flag? Do you have a better idea?

[xizheyin]

For find_ancestor_not_from_macro, it will look along the call chain from the span inside the macro until it finds the first span that is not inside the macro. As in this example, the span is in L16 instead of L11.

rust/tests/ui/borrowck/span-semicolon-issue-139049.rs

Lines 11 to 17 in d242a8b

	macro_rules! perform { ($e:expr) => { D(&$e).end() } }
	//~^ ERROR does not live long enough
	//~| ERROR does not live long enough
	fn main() {
	{ let l = (); perform!(l) };
	//~^ SUGGESTION ;

rust/tests/ui/borrowck/span-semicolon-issue-139049.stderr

Lines 18 to 21 in d242a8b

	help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
	|
	LL | { let l = (); perform!(l); };
	| +

---

For find_ancestor_not_from_macro, it will look along the call chain from the initial span until it finds the first span that is not inside the external macro.
In this example, the initial span is located inside the format macro, and we need user-written code (not concerned here with whether it's located inside the macro), so we use find_ancestor_not_from_macro. It return the span inside entire_fn which is user-written code.

rust/tests/ui/typeck/issue-110017-format-into-help-deletes-macro.rs

Lines 32 to 39 in d242a8b

	macro_rules! entire_fn {
	() => {
	pub fn baz(x: &str) -> Result<(), Box<dyn std::error::Error>> {
	Err(format!("error: {x}"))
	//~^ ERROR mismatched types
	}
	}
	}

rust/tests/ui/typeck/issue-110017-format-into-help-deletes-macro.stderr

Lines 43 to 56 in d242a8b

	error[E0308]: mismatched types
	--> $DIR/issue-110017-format-into-help-deletes-macro.rs:51:5
	|
	LL | nontrivial!(x)
	| ^^^^^^^^^^^^^^ expected `Box<dyn Error>`, found `String`
	|
	= note: expected struct `Box<dyn std::error::Error>`
	found struct `String`
	= note: this error originates in the macro `format` which comes from the expansion of the macro `nontrivial` (in Nightly builds, run with -Z macro-backtrace for more info)
	help: call `Into::into` on this expression to convert `String` into `Box<dyn std::error::Error>`
	|
	LL | Err(format!("error: {}", $x).into())
	| +++++++

---

For the find_oldest_ancestor_in_same_ctxt implementation, we can see from the source code that it looks for the oldest ancestor in the same syntactic context.

rust/compiler/rustc_span/src/lib.rs

Lines 795 to 803 in d242a8b

	pub fn find_oldest_ancestor_in_same_ctxt(self) -> Span {
	let mut cur = self;
	while cur.eq_ctxt(self)
	&& let Some(parent_callsite) = cur.parent_callsite()
	{
	cur = parent_callsite;
	}
	cur
	}

For both examples, it worked successfully by mistake.

1. for the first example, the initial span (i.e. self) is located at L11, but since at the very beginning cur.eq_ctxt(self) is true, it goes into a loop once, so self becomes its parent, i.e. L16. in the second loop, L11.eq(L16) is false, so it ends up returning L16.
2. for the second example, the original span is located inside the format macro (there is only one layer of calls), and similarly, after the first loop, the new self is located in L20, and the original span is located in the standard library, so cur.eq_ctxt(self) is false, and once again the correct value is returned by mistake.
   So we can see that the method actually returns the first ancestor span that is not in the same context.

This problem was discovered when I was solving #144266. Because it had multiple layers of macro calls, so the returned span of find_oldest_ancestor_in_same_ctxt was still in the debug_assert_eq macro.

@rustbot ready

1. https://github.com/rust-lang/rust/blob/master/tests/ui/borrowck/span-semicolon-issue-139049.rs
2. https://github.com/rust-lang/rust/blob/d242a8bd5a73f633ba1ec5aacf19acf35a3c747d/tests/ui/typeck/issue-110017-format-into-help-deletes-macro.rs

[jieyouxu]

Okay, I think there might still be cases where these are insufficient, but at least find_oldest_ancestor_in_same_ctxt seems definitely wrong.

[jieyouxu]

Thanks
@bors r+ rollup

[bors]

📌 Commit 66d2d16 has been approved by jieyouxu

It is now in the queue for this repository.

[Zalathar]

This seems to have failed in rollup: #144619 (comment)

@bors r-

[xizheyin]

i should rebase the branch

[xizheyin]

waiting for CI green

[xizheyin]

rebased.

[xizheyin]

ready:>

[jieyouxu]

@bors r+

[bors]

📌 Commit 81176b1 has been approved by jieyouxu

It is now in the queue for this repository.