More precise suggestion for non_upper_case_globals

[phansch]

This makes the suggestion span for the non_upper_case_globals lint a bit
more precise. Before it was highlighting the whole line. With this change
it will highlight only the variable name itself.

Currently the way the span is calculated doesn't look very nice.
Especially the name.as_str().chars().as_str() which I used to turn an
InternedString into a &str. Unfortunately I couldn't find a better way.

r? @Manishearth

Fixes #48103

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[zackmdavis]

While we're here, it might also make sense to make this a structured suggestion, as @killercup suggests on the issue?

That is, I think changing (line 357+)

cx.span_lint(NON_UPPER_CASE_GLOBALS,
                              span,
                              &format!("{} `{}` should have an upper case name such as `{}`", sort, name, uc));

to something like

let mut err = cx.struct_span_lint(NON_UPPER_CASE_GLOBALS, span, &format!("{} `{}` should have an upper case name");
err.span_suggestion(span, "use an uppercase name", uc);
err.emit();

should give a suggestion like

warning: static variable `hello` should have an upper case name
 --> src/main.rs:3:1
  |
3 | static hello: &str = "Hello";
  |        ^^^^^ use an uppercase name: `HELLO`
  |

[Manishearth]

Hm, it seems like we can't get the span of the name directly. That's unfortunate -- we can't rely on span_to_snippet because of macros and such; so this seems pretty fragile. I'm not sure what we can do here.

In general manually building spans is a bad idea.

[oli-obk]

While I agree that its unfortunate that we can't get the span of the name directly, There is precedent:

https://github.com/rust-lang/rust/blob/master/src/librustc_lint/builtin.rs#L1197-L1205

[oli-obk]

If I'm reading the code correctly, const c: usize = 1 would hilight the c in const, not the one on the name.

[phansch]

I also agree that using span_to_snippet is not very robust, it also caused some issues with attribute linting and macros in clippy. I saw some other lints in rustc that seem to highlight the variable name correctly, but they rely on hir::Pat instead of hir::Item as far as I can tell.

[pietroalbini]

What's the status on this PR?

[bors]

☔ The latest upstream changes (presumably #48449) made this pull request unmergeable. Please resolve the merge conflicts.

[pietroalbini]

Ping from the release team!

@phansch thank you for this PR! Recently, #48449 was merged, changing the way the output is checked for ui tests. You should rebase this PR on the master branch, and update the expected stderr for the lint-group-style test.

@Manishearth or @rust-lang/compiler, can someone recommend the author the best way to generate a span in this case?

[Manishearth]

I don't think there is one. We could try and output the warning during parsing as a builtin lint.

[estebank]

In order to avoid the issue mentioned below, you should probably find the end of the first occurrence of whitespace, and only then find the name.

[estebank]

If adding a span to AST, then the above comment is moot.

[nrc]

Hm, it seems like we can't get the span of the name directly.

Can you change the AST to include the span of the ident (there used to be SpannedIdent, but I think it might have changed to something else).

In general manually building spans is a bad idea.

This 100 times! I did it a lot in save-analysis and it has been a source of lots of pain.

[Manishearth]

I'm wary of bloating the AST for a single lint -- are you okay with that tradeoff?

We throw away the AST at some point so it's not too bad I think.

[nrc]

are you okay with that tradeoff?

Yeah, definitely. I'd like the AST to have every span in it eventually and this should be useful for save-analysis/IDEs too.

[pietroalbini]

@phansch ping from triage! Multiple members of the compiler team replied to this thread, can you finish this PR so it can be merged?

[pietroalbini]

Thank you so much for this PR @phansch! Unfortunately, we haven't heard from you in more than two weeks, so I'm closing this to keep the queue clean.

If you have more time to work on this in the future, please reopen the PR so we can merge this!