Bad suggestion dyn pub on proc macro (#61963 regressed)

[SNCPlay42]

#72306 causes the compiler to emit (and blessed) this bad suggestion in the test for #61963 (output):

error: trait objects without an explicit `dyn` are deprecated
  --> $DIR/issue-61963.rs:18:1
   |
LL | pub struct Foo {
   | ^^^ help: use `dyn`: `dyn pub`

This is the behaviour the test is supposed to guard against (almost - the original bad suggestion was to insert dyn before the proc macro attribute), so the issue is regressed.

The issue was originally fixed nearly a year ago, so this is a regression from stable.

[LeSeulArtichaut]

cc @Aaron1011 @davidtwco

[Aaron1011]

I don't think this is actually an issue anymore. The original issue was caused by the compiler losing span information, causing tokens emitted by a proc-macro to end up with a span (the proc-macro attribute) that the author did not intend.

However, the test is explicitly setting the span of the token in question:

rust/src/test/ui/suggestions/auxiliary/issue-61963-1.rs

Lines 13 to 15 in 0cd7ff7

	// Construct a dummy span - `#0 bytes(0..0)` - which is present in the input because
	// of the specially crafted generated tokens in the `attribute-crate` proc-macro.
	let dummy_span = input.clone().into_iter().nth(0).unwrap().span();

However, the first token in the stream no longer has a dummy span, so this proc-macro ends up explicitly spanning the trait object token with the span of the pub token.

I don't think this is a bug - when a proc-macro explicitly sets the span of a token, I don't think the compiler should be trying to second-guess that decision. There are legitimate use cases that involve causing an error to be displayed to be displayed in a unusual location - e.g. pointing to a field definition for a trait bound that the field type fails to meet.

We might want to alter the test documentation to better explain its current behavior (while still guarding for regressions to the original behavior).

[SNCPlay42]

So this is different from the original situation in #61963? There was discussion there about whether the problem was a bug in the proc macros or the compiler.

There are legitimate use cases that involve causing an error to be displayed to be displayed in a unusual location - e.g. pointing to a field definition for a trait bound that the field type fails to meet.

This I understand, but should the compiler then emit suggestions that produce invalid syntax? They at least shouldn't be reported as MachineApplicable. This causes cargo fix to try to automatically apply the fix, and then fail to compile the result, which it says "likely indicates a bug in either rustc or cargo itself".

[Aaron1011]

So this is different from the original situation in #61963? There was discussion there about whether the problem was a bug in the proc macros or the compiler.

Yes - there is no longer any implicit respanning occuring, which was the root of the original issue.

This I understand, but should the compiler then emit suggestions that produce invalid syntax?

I think this is unsolvable in the general case - the span could point to a token Foo instead of pub, which could either be a trait Foo or a struct Foo.

However, I think it would be fine to add in some special cases (e.g. only suggest dyn <ident>, not dyn <keyword> if people are hitting this in real code.

I think the blame for the bad suggestion lies with the proc macro doing the explicit re-spanning.

[Dylan-DPC-zz]

Marking it as p-medium based on the discussion