extract pattern lint from non_upper_case_globals
#56478
Conversation
rust-highfive
added
the
S-waiting-on-review
|
Since this is merely moving around / extracting linting behavior rather than adding new behavior I don't think T-lang needs to be involved here. (cc @cramertj -- do you agree?) |
|
@Centril Yup! This seems fine to me. |
euclio
force-pushed
the
split-out-constant-pattern-lint
branch
from
6f0f7f8 to
ab74712
Compare
| &format!("constant pattern `{}` should be upper case", name), | ||
| ) | ||
| .span_label(span, "looks like a binding") | ||
| .span_suggestion_with_applicability( |
There was a problem hiding this comment.
I don't think this suggestion is useful without also changing the import or enum's name. I'd rather have this suggestion removed entirely than have it.
| @@ -165,7 +166,8 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) { | |||
| "nonstandard_style", | |||
| NON_CAMEL_CASE_TYPES, | |||
| NON_SNAKE_CASE, | |||
| NON_UPPER_CASE_GLOBALS); | |||
| NON_UPPER_CASE_GLOBALS, | |||
| MISLEADING_CONSTANT_PATTERNS); | |||
There was a problem hiding this comment.
cc @Manishearth
- what do you think of the name? It follows our rule of needing to be readable as "allow misleading constant patterns"
- Should this maybe just live in clippy, since it's not really a bug, but more a stylistic confusing problem
There was a problem hiding this comment.
In thinking about (2) a bit more, I do wonder if this lint really carries its weight. The rationale for the lint in #7526 is to avoid misleading patterns where constants look like bindings. However, to even get in this situation you'd have to explicitly #[allow(non_upper_case_globals)] on your const or static in the first place. For cases where you have to use a non-upper case global, then this lint really just serves as an extra hurdle that you're going to allow anyways.
I think it might suffice to include a blurb about the misleading patterns in the rationale for non_upper_case_globals: "patterns containing consts or statics that are not upper cased look like bindings, and can cause difficult-to-spot bugs". Maybe as a note?
There was a problem hiding this comment.
The issue is not declaring and using such a constant within one crate, but exporting such constants in a crate and then pattern matching on the name e.g. by accident, when you actually wanted a binding.
There was a problem hiding this comment.
Ah, that makes sense. I'll wait for @Manishearth to weigh in before making any more changes.
|
☔ The latest upstream changes (presumably #57108) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm going to close this as I'm no longer interested in landing it. If someone else would like it, I'm happy to revive it. |
This PR extracts a new lint
misleading_constant_patternsfrom the existingnon_upper_case_globalslint. The new lint is also part of thenonstandard_stylelint group.The motivation for this change is twofold: first, we'd like to move the identifier-checking
nonstandard_stylelints to be early lint passes, but this lint must be a late pass. Second, from issues like #25207 and #39371, it's clear that there is some desire for this part of the lint to be configured independently. I also believe that this lint is fundamentally checking something different from thenon_upper_case_globalslint, so this lint may want to evolve separately.cc #48103
r? @oli-obk