Suggestion Span for non_upper_case_globals could be more precise

[phansch]

Playground link: https://play.rust-lang.org/?gist=054aaeb143021ea121fc0c091903697b&version=nightly

It currently prints:

warning: static variable `hello` should have an upper case name such as `HELLO`
 --> src/main.rs:3:1
  |
3 | static hello: &str = "Hello";
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(non_upper_case_globals)] on by default

warning: constant `world` should have an upper case name such as `WORLD`
 --> src/main.rs:4:1
  |
4 | const world: &str = ", world!";
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

It would be nicer if it only underlines the variable name itself:

warning: static variable `hello` should have an upper case name such as `HELLO`
 --> src/main.rs:3:1
  |
3 | static hello: &str = "Hello";
  |        ^^^^^
  |
  = note: #[warn(non_upper_case_globals)] on by default

warning: constant `world` should have an upper case name such as `WORLD`
 --> src/main.rs:4:1
  |
4 | const world: &str = ", world!";
  |       ^^^^^

I have some basic lint writing experience through clippy, so I would like to give this a go.

[Manishearth]

Sounds good! Let me know if you need help.

(Self-assigning since I can't assign it to you, but this means I'm mentoring this bug)

[killercup]

I'm pretty sure we can also this as a suggestion to make it auto-fixable. Not sure if we have a good SCREAMING_SNAKE_CASE transformer in rustc, but I guess it's fine to only suggest this for trivial changes at first.

[phansch]

Just a quick update because it's been a few days. I was able to fix this locally already, but got a faster laptop this week where the rust repo setup is broken somehow. I will try to fix it today, otherwise it will be next week 👍

[euclio]

I'm interested in reviving this. I see in the linked PR that there's some discussion about adding the span to the HIR. I'm happy to try this, but I'm unsure where it would be added?

[oli-obk]

So my reading of the PR points to

rust/src/librustc/hir/mod.rs

Line 2178 in a49316d

pub name: Name,

That Name could be an Ident (it loses its Identness in

rust/src/librustc/hir/lowering.rs

Line 3456 in a49316d

name,

)

Three years ago, this already was an Ident, but was changed to a Name: a4af958#diff-12e06f1e9ca371a11bdc4615f50a4071R1193

This move was tracked in #6993 so there might be some hygiene or similar topics related to this.

cc @petrochenkov @nrc

.... and now I'm wondering why this is a LateLintPass... can we just make it an EarlyLintPass (maybe even one of the pre-macro-expansion lint passes?)

[euclio]

I don't think it can be made an EarlyLintPass as is because of

rust/src/librustc_lint/nonstandard_style.rs

Lines 403 to 415 in 0c1dc62

	fn check_pat(&mut self, cx: &LateContext, p: &hir::Pat) {
	// Lint for constants that look like binding identifiers (#7526)
	if let PatKind::Path(hir::QPath::Resolved(None, ref path)) = p.node {
	if let Def::Const(..) = path.def {
	if path.segments.len() == 1 {
	NonUpperCaseGlobals::check_upper_case(cx,
	"constant in pattern",
	path.segments[0].ident.name,
	path.span);
	}
	}
	}
	}

. I wonder if this should be extracted out into another lint, though?

EDIT: Oh, I didn't realize you can define multiple passes for the same lint! Neat.

EDIT 2: Well, maybe not. Registering multiple passes causes an ICE.

[euclio]

Made a bit more progress on this. I have a commit that replaces Name with Ident in hir::Item and passes all tests. However, once I modify the lint to use the Ident's span, the lint is getting triggered by constants inside #[test] expansions, like this:

warning: constant `foo` should have an upper case name
  --> $DIR/test-inner-fn.rs:14:4
   |
LL | fn foo() {
   |    ^^^ help: convert the identifier to upper case: `FOO`
   |
   = note: #[warn(non_upper_case_globals)] on by default

warning: constant `bar` should have an upper case name
  --> $DIR/test-inner-fn.rs:16:8
   |
LL |     fn bar() {}
   |        ^^^ help: convert the identifier to upper case: `BAR`

error: cannot test inner items
  --> $DIR/test-inner-fn.rs:15:5
   |
LL |     #[test] //~ ERROR cannot test inner items [unnameable_test_items]
   |     ^^^^^^^
   |
   = note: requested on the command line with `-D unnameable-test-items`

warning: constant `foo` should have an upper case name
  --> $DIR/test-inner-fn.rs:22:8
   |
LL |     fn foo() {
   |        ^^^ help: convert the identifier to upper case: `FOO`

warning: constant `bar` should have an upper case name
  --> $DIR/test-inner-fn.rs:24:12
   |
LL |         fn bar() {}
   |            ^^^ help: convert the identifier to upper case: `BAR`

error: cannot test inner items
  --> $DIR/test-inner-fn.rs:23:9
   |
LL |         #[test] //~ ERROR cannot test inner items [unnameable_test_items]
   |         ^^^^^^^

error: aborting due to 2 previous errors

I suspect that it has something to do with this line:

rust/src/libsyntax_ext/test.rs

Line 137 in d48ab69

let mut test_const = cx.item(sp, item.ident.gensym(),

, because gensym reuses the span of the function identifier for the constant. Therefore, when the lint gets triggered, the span doesn't trip the macro check, and a warning is reported. However, I'm not sure of the best way to fix it. I suppose I could throw an #[allow(non_upper_case_globals)] on the generated constant but that doesn't feel like the right solution; I think the macro check should silence the warning. Is there a way to create that Ident with a span that indicates it's part of a macro expansion or another solution? Is my reasoning correct?

[oli-obk]

I still think that the correct fix is to use an early lint, and just split out the lowercase const pattern lint to stay as a late lint pass.

I'd separate the datastructures completely to get around the duplication checks that you've been hitting.

Most likely there's a very good reason not to have idents in HIR

[euclio]

The duplication check is by name, so to separate the lints I think I'd have to rename the const pattern lint. Is that possible to do given Rust's stability guarantees?

FWIW, seems like there's some desire for that given #25207, #39371.

[oli-obk]

Yea that's definitely possible. Reading those issues I agree that there is desire to do so. we should do it in steps though. so first create a new lint just for the pattern const part, and once that is through, move the other lint to an early pass

[petrochenkov]

FWIW, #56225 already replaces Names with Idents in HIR items.