Incorrect help message for type declared on struct using raw identifier

[olegnn]

This code

struct r#struct<r#fn>;

fn main() {}

(Playground)

Produces incorrect help message:

   Compiling playground v0.0.1 (/playground)
warning: type `struct` should have an upper camel case name
 --> src/main.rs:1:8
  |
1 | struct r#struct<r#fn>;
  |        ^^^^^^^^ help: convert the identifier to upper camel case: `Struct`
  |
  = note: `#[warn(non_camel_case_types)]` on by default

warning: type parameter `fn` should have an upper camel case name
 --> src/main.rs:1:17
  |
1 | struct r#struct<r#fn>;
  |                 ^^^^ help: convert the identifier to upper camel case: `Fn`

error[E0392]: parameter `fn` is never used
 --> src/main.rs:1:17
  |
1 | struct r#struct<r#fn>;
  |                 ^^^^ unused parameter
  |
  = help: consider removing `fn`, referring to it in a field, or using a marker such as `std::marker::PhantomData`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0392`.
error: could not compile `playground`.

To learn more, run the command again with --verbose.

[Centril]

Related: #69052 and #69054.

[petrochenkov]

I wonder whether cases like this need to be "fixed" at all.
There going to be an endless stream of these issues, basically every diagnostic that reports an arbitrary identifier.

I'm not sure whether #68963 alone has the right cost/benefit ratio for merging, let alone changing this everywhere.

[petrochenkov]

Suggestion №1: Forget about raw identifier after the original tokens are lost (~lexing/parsing/expansion) and never report them.

Suggestion №2: Use an approximate identifier printing routine that may print edition-specific keywords inaccurately, but at least doesn't require keeping precise spans.
Note that even #68963 doesn't do edition hygiene correctly, because the spans it uses may be stripped from "transparent" macro layers (e.g. ident = ident.modern()) which are not needed for name resolution, but must be preserved for edition hygiene.

[olegnn]

@petrochenkov I didn't get the problem - why we can't format all identifiers according to the edition used by current project? It's the only thing which matters, we don't need to know from and how we got this identifier. Even if we import something from, let's say old reqwest with edition=2015, which had async module, if we're using edition=2018, we should format it r#async, and if we're using 2015, we can use it as a pure ident async.

[petrochenkov]

why we can't format all identifiers according to the edition used by current project?

We can, that would be a very good approximation for the suggestion №2, since we are mostly reporting error for identifiers coming directly from the current crate.

That would be Ident::with_dummy_span(name) from the original version of #68963.
The only thing is that I'd like to see a method with some other name than Ident::with_dummy_span(name), because it's entirely unclear why you need to create an Ident, if you can just print the name, unless you know about this subtlety about raw identifiers.

[estebank]

The PR looked trivial enough by using Idents instead of Symbols to be worth it as a best-effort approach to correct code.

The reason I feel these in particular are important is because normally we suggest using raw idents when the parser can identify them, but in these cases of final and fn the parser does not recover gracefully guiding the user in the right direction.

[olegnn]

@petrochenkov what is the best place to define this function if we assume to use it not only for resolve diagnostics but also for any kind of help messages and notes, which include identifiers?

[petrochenkov]

Hmm, rustc_span should be the common denominator.
It can be a method on Symbol.
It should include something like "guess" in its name since it's an approximation.

[petrochenkov]

@olegnn
Ok, I perhaps went a bit overboard with the "guess" naming, given that ident.to_string() already does some guessing (#69387).

I added the method with a detailed comment in #69491.

[zaneduffield]

@petrochenkov I was going to have a go at fixing this since #68963 became inactive, but I think you were right before when you said

There going to be an endless stream of these issues

and one can expect the user to understand that they need the raw identifiers when copying the compiler's suggestions (they just tried to build with one).

In any case, if the user copies the example from the diagnostic that omits the r# from the identifier then the compiler is going to suggest using it on the next build.