Skip to content

Also emit suggestions for usages in the non_upper_case_globals lint #142645

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 25, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions compiler/rustc_lint/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,20 @@ pub trait LintContext {
});
}

/// Emit a lint at `span` from a lazily-constructed lint struct (some type that implements
/// `LintDiagnostic`, typically generated by `#[derive(LintDiagnostic)]`).
fn emit_span_lint_lazy<S: Into<MultiSpan>, L: for<'a> LintDiagnostic<'a, ()>>(
&self,
lint: &'static Lint,
span: S,
decorator: impl FnOnce() -> L,
) {
self.opt_span_lint(lint, Some(span), |lint| {
let decorator = decorator();
decorator.decorate_lint(lint);
});
}

/// Emit a lint at the appropriate level, with an associated span.
///
/// [`lint_level`]: rustc_middle::lint::lint_level#decorate-signature
Expand Down
19 changes: 18 additions & 1 deletion compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1348,6 +1348,8 @@ pub(crate) struct NonUpperCaseGlobal<'a> {
pub name: &'a str,
#[subdiagnostic]
pub sub: NonUpperCaseGlobalSub,
#[subdiagnostic]
pub usages: Vec<NonUpperCaseGlobalSubTool>,
}

#[derive(Subdiagnostic)]
Expand All @@ -1357,14 +1359,29 @@ pub(crate) enum NonUpperCaseGlobalSub {
#[primary_span]
span: Span,
},
#[suggestion(lint_suggestion, code = "{replace}", applicability = "maybe-incorrect")]
#[suggestion(lint_suggestion, code = "{replace}")]
Suggestion {
#[primary_span]
span: Span,
#[applicability]
applicability: Applicability,
replace: String,
},
}

#[derive(Subdiagnostic)]
#[suggestion(
lint_suggestion,
code = "{replace}",
applicability = "machine-applicable",
style = "tool-only"
)]
pub(crate) struct NonUpperCaseGlobalSubTool {
#[primary_span]
pub(crate) span: Span,
pub(crate) replace: String,
}

// noop_method_call.rs
#[derive(LintDiagnostic)]
#[diag(lint_noop_method_call)]
Expand Down
107 changes: 93 additions & 14 deletions compiler/rustc_lint/src/nonstandard_style.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
use rustc_abi::ExternAbi;
use rustc_attr_data_structures::{AttributeKind, ReprAttr};
use rustc_attr_parsing::AttributeParser;
use rustc_errors::Applicability;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::intravisit::FnKind;
use rustc_hir::def_id::DefId;
use rustc_hir::intravisit::{FnKind, Visitor};
use rustc_hir::{AttrArgs, AttrItem, Attribute, GenericParamKind, PatExprKind, PatKind};
use rustc_middle::hir::nested_filter::All;
use rustc_middle::ty;
use rustc_session::config::CrateType;
use rustc_session::{declare_lint, declare_lint_pass};
Expand All @@ -13,7 +16,7 @@ use {rustc_ast as ast, rustc_hir as hir};

use crate::lints::{
NonCamelCaseType, NonCamelCaseTypeSub, NonSnakeCaseDiag, NonSnakeCaseDiagSub,
NonUpperCaseGlobal, NonUpperCaseGlobalSub,
NonUpperCaseGlobal, NonUpperCaseGlobalSub, NonUpperCaseGlobalSubTool,
};
use crate::{EarlyContext, EarlyLintPass, LateContext, LateLintPass, LintContext};

Expand Down Expand Up @@ -489,22 +492,82 @@ declare_lint! {
declare_lint_pass!(NonUpperCaseGlobals => [NON_UPPER_CASE_GLOBALS]);

impl NonUpperCaseGlobals {
fn check_upper_case(cx: &LateContext<'_>, sort: &str, ident: &Ident) {
fn check_upper_case(cx: &LateContext<'_>, sort: &str, did: Option<LocalDefId>, ident: &Ident) {
let name = ident.name.as_str();
if name.chars().any(|c| c.is_lowercase()) {
let uc = NonSnakeCase::to_snake_case(name).to_uppercase();

// If the item is exported, suggesting changing it's name would be breaking-change
// and could break users without a "nice" applicable fix, so let's avoid it.
let can_change_usages = if let Some(did) = did {
!cx.tcx.effective_visibilities(()).is_exported(did)
} else {
false
};

// We cannot provide meaningful suggestions
// if the characters are in the category of "Lowercase Letter".
let sub = if *name != uc {
Comment on lines +502 to 510
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move let can_change_usages and let sub into the callback of emit_span_lint_lazy? Should fix perf

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could but I don't actually think there is any regression here, the only regressed crate is clap_derive-4.5.3/opt/full, no other benchmark, including any of the others clap_derive, or the previously regressed libc crate seems to affected and looking at the details the regression is in the backend, not the frontend. I think it's a spurious regression. What do you think?

NonUpperCaseGlobalSub::Suggestion { span: ident.span, replace: uc }
NonUpperCaseGlobalSub::Suggestion {
span: ident.span,
replace: uc.clone(),
applicability: if can_change_usages {
Applicability::MachineApplicable
} else {
Applicability::MaybeIncorrect
},
}
} else {
NonUpperCaseGlobalSub::Label { span: ident.span }
};
cx.emit_span_lint(
NON_UPPER_CASE_GLOBALS,
ident.span,
NonUpperCaseGlobal { sort, name, sub },
);

struct UsageCollector<'a, 'tcx> {
cx: &'tcx LateContext<'a>,
did: DefId,
collected: Vec<Span>,
}

impl<'v, 'tcx> Visitor<'v> for UsageCollector<'v, 'tcx> {
type NestedFilter = All;

fn maybe_tcx(&mut self) -> Self::MaybeTyCtxt {
self.cx.tcx
}

fn visit_path(
&mut self,
path: &rustc_hir::Path<'v>,
_id: rustc_hir::HirId,
) -> Self::Result {
if let Some(final_seg) = path.segments.last()
&& final_seg.res.opt_def_id() == Some(self.did)
{
self.collected.push(final_seg.ident.span);
}
}
}

cx.emit_span_lint_lazy(NON_UPPER_CASE_GLOBALS, ident.span, || {
// Compute usages lazily as it can expansive and useless when the lint is allowed.
// cf. https://github.com/rust-lang/rust/pull/142645#issuecomment-2993024625
let usages = if can_change_usages
&& *name != uc
&& let Some(did) = did
{
let mut usage_collector =
UsageCollector { cx, did: did.to_def_id(), collected: Vec::new() };
cx.tcx.hir_walk_toplevel_module(&mut usage_collector);
usage_collector
.collected
.into_iter()
.map(|span| NonUpperCaseGlobalSubTool { span, replace: uc.clone() })
.collect()
} else {
vec![]
};

NonUpperCaseGlobal { sort, name, sub, usages }
});
}
}
}
Expand All @@ -516,26 +579,36 @@ impl<'tcx> LateLintPass<'tcx> for NonUpperCaseGlobals {
hir::ItemKind::Static(_, ident, ..)
if !ast::attr::contains_name(attrs, sym::no_mangle) =>
{
NonUpperCaseGlobals::check_upper_case(cx, "static variable", &ident);
NonUpperCaseGlobals::check_upper_case(
cx,
"static variable",
Some(it.owner_id.def_id),
&ident,
);
}
hir::ItemKind::Const(ident, ..) => {
NonUpperCaseGlobals::check_upper_case(cx, "constant", &ident);
NonUpperCaseGlobals::check_upper_case(
cx,
"constant",
Some(it.owner_id.def_id),
&ident,
);
}
_ => {}
}
}

fn check_trait_item(&mut self, cx: &LateContext<'_>, ti: &hir::TraitItem<'_>) {
if let hir::TraitItemKind::Const(..) = ti.kind {
NonUpperCaseGlobals::check_upper_case(cx, "associated constant", &ti.ident);
NonUpperCaseGlobals::check_upper_case(cx, "associated constant", None, &ti.ident);
}
}

fn check_impl_item(&mut self, cx: &LateContext<'_>, ii: &hir::ImplItem<'_>) {
if let hir::ImplItemKind::Const(..) = ii.kind
&& !assoc_item_in_trait_impl(cx, ii)
{
NonUpperCaseGlobals::check_upper_case(cx, "associated constant", &ii.ident);
NonUpperCaseGlobals::check_upper_case(cx, "associated constant", None, &ii.ident);
}
}

Expand All @@ -551,6 +624,7 @@ impl<'tcx> LateLintPass<'tcx> for NonUpperCaseGlobals {
NonUpperCaseGlobals::check_upper_case(
cx,
"constant in pattern",
None,
&segment.ident,
);
}
Expand All @@ -560,7 +634,12 @@ impl<'tcx> LateLintPass<'tcx> for NonUpperCaseGlobals {

fn check_generic_param(&mut self, cx: &LateContext<'_>, param: &hir::GenericParam<'_>) {
if let GenericParamKind::Const { .. } = param.kind {
NonUpperCaseGlobals::check_upper_case(cx, "const parameter", &param.name.ident());
NonUpperCaseGlobals::check_upper_case(
cx,
"const parameter",
Some(param.def_id),
&param.name.ident(),
);
}
}
}
Expand Down
44 changes: 44 additions & 0 deletions tests/ui/lint/lint-non-uppercase-usages.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Checks that the `non_upper_case_globals` emits suggestions for usages as well
// <https://github.com/rust-lang/rust/issues/124061>

//@ check-pass
//@ run-rustfix

#![allow(dead_code)]

use std::cell::Cell;

const MY_STATIC: u32 = 0;
//~^ WARN constant `my_static` should have an upper case name
//~| SUGGESTION MY_STATIC

const LOL: u32 = MY_STATIC + 0;
//~^ SUGGESTION MY_STATIC

mod my_mod {
const INSIDE_MOD: u32 = super::MY_STATIC + 0;
//~^ SUGGESTION MY_STATIC
}

thread_local! {
static FOO_FOO: Cell<usize> = unreachable!();
//~^ WARN constant `fooFOO` should have an upper case name
//~| SUGGESTION FOO_FOO
}

fn foo<const FOO: u32>() {
//~^ WARN const parameter `foo` should have an upper case name
//~| SUGGESTION FOO
let _a = FOO + 1;
//~^ SUGGESTION FOO
}

fn main() {
let _a = crate::MY_STATIC;
//~^ SUGGESTION MY_STATIC

FOO_FOO.set(9);
//~^ SUGGESTION FOO_FOO
println!("{}", FOO_FOO.get());
//~^ SUGGESTION FOO_FOO
}
44 changes: 44 additions & 0 deletions tests/ui/lint/lint-non-uppercase-usages.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Checks that the `non_upper_case_globals` emits suggestions for usages as well
// <https://github.com/rust-lang/rust/issues/124061>

//@ check-pass
//@ run-rustfix

#![allow(dead_code)]

use std::cell::Cell;

const my_static: u32 = 0;
//~^ WARN constant `my_static` should have an upper case name
//~| SUGGESTION MY_STATIC

const LOL: u32 = my_static + 0;
//~^ SUGGESTION MY_STATIC

mod my_mod {
const INSIDE_MOD: u32 = super::my_static + 0;
//~^ SUGGESTION MY_STATIC
}

thread_local! {
static fooFOO: Cell<usize> = unreachable!();
//~^ WARN constant `fooFOO` should have an upper case name
//~| SUGGESTION FOO_FOO
}

fn foo<const foo: u32>() {
//~^ WARN const parameter `foo` should have an upper case name
//~| SUGGESTION FOO
let _a = foo + 1;
//~^ SUGGESTION FOO
}

fn main() {
let _a = crate::my_static;
//~^ SUGGESTION MY_STATIC

fooFOO.set(9);
//~^ SUGGESTION FOO_FOO
println!("{}", fooFOO.get());
//~^ SUGGESTION FOO_FOO
}
39 changes: 39 additions & 0 deletions tests/ui/lint/lint-non-uppercase-usages.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
warning: constant `my_static` should have an upper case name
--> $DIR/lint-non-uppercase-usages.rs:11:7
|
LL | const my_static: u32 = 0;
| ^^^^^^^^^
|
= note: `#[warn(non_upper_case_globals)]` on by default
help: convert the identifier to upper case
|
LL - const my_static: u32 = 0;
LL + const MY_STATIC: u32 = 0;
|

warning: constant `fooFOO` should have an upper case name
--> $DIR/lint-non-uppercase-usages.rs:24:12
|
LL | static fooFOO: Cell<usize> = unreachable!();
| ^^^^^^
|
help: convert the identifier to upper case
|
LL - static fooFOO: Cell<usize> = unreachable!();
LL + static FOO_FOO: Cell<usize> = unreachable!();
|

warning: const parameter `foo` should have an upper case name
--> $DIR/lint-non-uppercase-usages.rs:29:14
|
LL | fn foo<const foo: u32>() {
| ^^^
|
help: convert the identifier to upper case (notice the capitalization difference)
|
LL - fn foo<const foo: u32>() {
LL + fn foo<const FOO: u32>() {
|

warning: 3 warnings emitted

Loading