From 59750dceb8bb7a743efc4b4a84c3ea79e31791a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Wed, 24 Feb 2021 23:20:49 +0100 Subject: [PATCH 1/3] upper_case_acronyms: add optional aggressive mode and relax default Moves the lint back from pedantic to style group. The lint default now only warns on names that are completely capitalized, like "WORD" and only if the name is longer than 2 chars (so that names where each of the letter represents a word are still distinguishable). For example: FP (false positive) would still be "valid" and not warned about (but EOF would warn). A "upper_case_acronyms_aggressive: true/false" config option was added that restores the original lint behaviour to warn on any kind of camel case name that had more than one capital letter following another capital letter. --- clippy_lints/src/lib.rs | 5 ++-- clippy_lints/src/upper_case_acronyms.rs | 40 ++++++++++++++++++++----- clippy_lints/src/utils/conf.rs | 2 ++ 3 files changed, 37 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 5add8d44b429..15262cfbf54a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1216,7 +1216,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: let enum_variant_name_threshold = conf.enum_variant_name_threshold; store.register_early_pass(move || box enum_variants::EnumVariantNames::new(enum_variant_name_threshold)); store.register_early_pass(|| box tabs_in_doc_comments::TabsInDocComments); - store.register_early_pass(|| box upper_case_acronyms::UpperCaseAcronyms); + let upper_case_acronyms_aggressive = conf.upper_case_acronyms_aggressive; + store.register_early_pass(move || box upper_case_acronyms::UpperCaseAcronyms::new(upper_case_acronyms_aggressive)); store.register_late_pass(|| box default::Default::default()); store.register_late_pass(|| box unused_self::UnusedSelf); store.register_late_pass(|| box mutable_debug_assertion::DebugAssertWithMutCall); @@ -1416,7 +1417,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&unnecessary_wraps::UNNECESSARY_WRAPS), LintId::of(&unnested_or_patterns::UNNESTED_OR_PATTERNS), LintId::of(&unused_self::UNUSED_SELF), - LintId::of(&upper_case_acronyms::UPPER_CASE_ACRONYMS), LintId::of(&wildcard_imports::ENUM_GLOB_USE), LintId::of(&wildcard_imports::WILDCARD_IMPORTS), LintId::of(&zero_sized_map_values::ZERO_SIZED_MAP_VALUES), @@ -1835,6 +1835,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&types::FN_TO_NUMERIC_CAST_WITH_TRUNCATION), LintId::of(&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME), LintId::of(&unused_unit::UNUSED_UNIT), + LintId::of(&upper_case_acronyms::UPPER_CASE_ACRONYMS), LintId::of(&write::PRINTLN_EMPTY_STRING), LintId::of(&write::PRINT_LITERAL), LintId::of(&write::PRINT_WITH_NEWLINE), diff --git a/clippy_lints/src/upper_case_acronyms.rs b/clippy_lints/src/upper_case_acronyms.rs index b593fd0a6996..6995fbce72cc 100644 --- a/clippy_lints/src/upper_case_acronyms.rs +++ b/clippy_lints/src/upper_case_acronyms.rs @@ -5,16 +5,20 @@ use rustc_ast::ast::{Item, ItemKind, Variant}; use rustc_errors::Applicability; use rustc_lint::{EarlyContext, EarlyLintPass, LintContext}; use rustc_middle::lint::in_external_macro; -use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::symbol::Ident; declare_clippy_lint! { - /// **What it does:** Checks for camel case name containing a capitalized acronym. + /// **What it does:** Checks for fully capitalized names and optionally names containing a capitalized acronym. /// /// **Why is this bad?** In CamelCase, acronyms count as one word. /// See [naming conventions](https://rust-lang.github.io/api-guidelines/naming.html#casing-conforms-to-rfc-430-c-case) /// for more. /// + /// By default, the lint only triggers on fully-capitalized names. + /// You can use the `upper_case_acronyms_aggressive: true` config option to enable linting + /// on all camel case names + /// /// **Known problems:** When two acronyms are contiguous, the lint can't tell where /// the first acronym ends and the second starts, so it suggests to lowercase all of /// the letters in the second acronym. @@ -29,11 +33,24 @@ declare_clippy_lint! { /// struct HttpResponse; /// ``` pub UPPER_CASE_ACRONYMS, - pedantic, + style, "capitalized acronyms are against the naming convention" } -declare_lint_pass!(UpperCaseAcronyms => [UPPER_CASE_ACRONYMS]); +#[derive(Default)] +pub struct UpperCaseAcronyms { + upper_case_acronyms_aggressive: bool, +} + +impl UpperCaseAcronyms { + pub fn new(aggressive: bool) -> Self { + Self { + upper_case_acronyms_aggressive: aggressive, + } + } +} + +impl_lint_pass!(UpperCaseAcronyms => [UPPER_CASE_ACRONYMS]); fn correct_ident(ident: &str) -> String { let ident = ident.chars().rev().collect::(); @@ -56,11 +73,18 @@ fn correct_ident(ident: &str) -> String { ident } -fn check_ident(cx: &EarlyContext<'_>, ident: &Ident) { +fn check_ident(cx: &EarlyContext<'_>, ident: &Ident, be_aggressive: bool) { let span = ident.span; let ident = &ident.as_str(); let corrected = correct_ident(ident); - if ident != &corrected { + // warn if we have pure-uppercase idents + // assume that two-letter words are some kind of valid abbreviation like FP for false positive + // (and don't warn) + if (ident.chars().all(|c| c.is_ascii_uppercase()) && ident.len() > 2) + // otherwise, warn if we have SOmeTHING lIKE THIs but only warn with the aggressive + // upper_case_acronyms_aggressive config option enabled + || (be_aggressive && ident != &corrected) + { span_lint_and_sugg( cx, UPPER_CASE_ACRONYMS, @@ -82,12 +106,12 @@ impl EarlyLintPass for UpperCaseAcronyms { ItemKind::TyAlias(..) | ItemKind::Enum(..) | ItemKind::Struct(..) | ItemKind::Trait(..) ); then { - check_ident(cx, &it.ident); + check_ident(cx, &it.ident, self.upper_case_acronyms_aggressive); } } } fn check_variant(&mut self, cx: &EarlyContext<'_>, v: &Variant) { - check_ident(cx, &v.ident); + check_ident(cx, &v.ident, self.upper_case_acronyms_aggressive); } } diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 7d7b35c21680..34b9ecf2830f 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -173,6 +173,8 @@ define_Conf! { (disallowed_methods, "disallowed_methods": Vec, Vec::::new()), /// Lint: UNREADABLE_LITERAL. Should the fraction of a decimal be linted to include separators. (unreadable_literal_lint_fractions, "unreadable_literal_lint_fractions": bool, true), + /// Lint: UPPER_CASE_ACRONYMS. Enabler verbose mode triggers if there is more than one uppercase char next to each other + (upper_case_acronyms_aggressive, "upper_case_acronyms_aggressive": bool, false), /// Lint: _CARGO_COMMON_METADATA. For internal testing only, ignores the current `publish` settings in the Cargo manifest. (cargo_ignore_publish, "cargo_ignore_publish": bool, false), } From 913c71018ca0adfd8c68677d6c4cf5a8acb44de8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Wed, 24 Feb 2021 23:41:32 +0100 Subject: [PATCH 2/3] upper_case_acronyms: add io-toml tests and bless previous tests --- clippy_lints/src/upper_case_acronyms.rs | 4 +- .../toml_unknown_key/conf_unknown_key.stderr | 2 +- .../clippy.toml | 1 + .../upper_case_acronyms.rs | 22 ++++++ .../upper_case_acronyms.stderr | 70 +++++++++++++++++++ tests/ui/upper_case_acronyms.rs | 7 +- tests/ui/upper_case_acronyms.stderr | 24 +------ 7 files changed, 103 insertions(+), 27 deletions(-) create mode 100644 tests/ui-toml/upper_case_acronyms_aggressive/clippy.toml create mode 100644 tests/ui-toml/upper_case_acronyms_aggressive/upper_case_acronyms.rs create mode 100644 tests/ui-toml/upper_case_acronyms_aggressive/upper_case_acronyms.stderr diff --git a/clippy_lints/src/upper_case_acronyms.rs b/clippy_lints/src/upper_case_acronyms.rs index 6995fbce72cc..0470e1dbbb81 100644 --- a/clippy_lints/src/upper_case_acronyms.rs +++ b/clippy_lints/src/upper_case_acronyms.rs @@ -16,7 +16,7 @@ declare_clippy_lint! { /// for more. /// /// By default, the lint only triggers on fully-capitalized names. - /// You can use the `upper_case_acronyms_aggressive: true` config option to enable linting + /// You can use the `upper-case-acronyms-aggressive: true` config option to enable linting /// on all camel case names /// /// **Known problems:** When two acronyms are contiguous, the lint can't tell where @@ -82,7 +82,7 @@ fn check_ident(cx: &EarlyContext<'_>, ident: &Ident, be_aggressive: bool) { // (and don't warn) if (ident.chars().all(|c| c.is_ascii_uppercase()) && ident.len() > 2) // otherwise, warn if we have SOmeTHING lIKE THIs but only warn with the aggressive - // upper_case_acronyms_aggressive config option enabled + // upper-case-acronyms-aggressive config option enabled || (be_aggressive && ident != &corrected) { span_lint_and_sugg( diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index 7ccd0b54845d..d83080b69f5e 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -1,4 +1,4 @@ -error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `msrv`, `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `pass-by-value-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `vec-box-size-threshold`, `max-trait-bounds`, `max-struct-bools`, `max-fn-params-bools`, `warn-on-all-wildcard-imports`, `disallowed-methods`, `unreadable-literal-lint-fractions`, `cargo-ignore-publish`, `third-party` at line 5 column 1 +error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `msrv`, `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `pass-by-value-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `vec-box-size-threshold`, `max-trait-bounds`, `max-struct-bools`, `max-fn-params-bools`, `warn-on-all-wildcard-imports`, `disallowed-methods`, `unreadable-literal-lint-fractions`, `upper-case-acronyms-aggressive`, `cargo-ignore-publish`, `third-party` at line 5 column 1 error: aborting due to previous error diff --git a/tests/ui-toml/upper_case_acronyms_aggressive/clippy.toml b/tests/ui-toml/upper_case_acronyms_aggressive/clippy.toml new file mode 100644 index 000000000000..cc94ec53e135 --- /dev/null +++ b/tests/ui-toml/upper_case_acronyms_aggressive/clippy.toml @@ -0,0 +1 @@ +upper-case-acronyms-aggressive = true diff --git a/tests/ui-toml/upper_case_acronyms_aggressive/upper_case_acronyms.rs b/tests/ui-toml/upper_case_acronyms_aggressive/upper_case_acronyms.rs new file mode 100644 index 000000000000..fdf8905f812f --- /dev/null +++ b/tests/ui-toml/upper_case_acronyms_aggressive/upper_case_acronyms.rs @@ -0,0 +1,22 @@ +#![warn(clippy::upper_case_acronyms)] + +struct HTTPResponse; // not linted by default, but with cfg option + +struct CString; // not linted + +enum Flags { + NS, // not linted + CWR, + ECE, + URG, + ACK, + PSH, + RST, + SYN, + FIN, +} + +struct GCCLLVMSomething; // linted with cfg option, beware that lint suggests `GccllvmSomething` instead of + // `GccLlvmSomething` + +fn main() {} diff --git a/tests/ui-toml/upper_case_acronyms_aggressive/upper_case_acronyms.stderr b/tests/ui-toml/upper_case_acronyms_aggressive/upper_case_acronyms.stderr new file mode 100644 index 000000000000..1cc59dc45f2a --- /dev/null +++ b/tests/ui-toml/upper_case_acronyms_aggressive/upper_case_acronyms.stderr @@ -0,0 +1,70 @@ +error: name `HTTPResponse` contains a capitalized acronym + --> $DIR/upper_case_acronyms.rs:3:8 + | +LL | struct HTTPResponse; // not linted by default, but with cfg option + | ^^^^^^^^^^^^ help: consider making the acronym lowercase, except the initial letter: `HttpResponse` + | + = note: `-D clippy::upper-case-acronyms` implied by `-D warnings` + +error: name `NS` contains a capitalized acronym + --> $DIR/upper_case_acronyms.rs:8:5 + | +LL | NS, // not linted + | ^^ help: consider making the acronym lowercase, except the initial letter (notice the capitalization): `Ns` + +error: name `CWR` contains a capitalized acronym + --> $DIR/upper_case_acronyms.rs:9:5 + | +LL | CWR, + | ^^^ help: consider making the acronym lowercase, except the initial letter: `Cwr` + +error: name `ECE` contains a capitalized acronym + --> $DIR/upper_case_acronyms.rs:10:5 + | +LL | ECE, + | ^^^ help: consider making the acronym lowercase, except the initial letter: `Ece` + +error: name `URG` contains a capitalized acronym + --> $DIR/upper_case_acronyms.rs:11:5 + | +LL | URG, + | ^^^ help: consider making the acronym lowercase, except the initial letter: `Urg` + +error: name `ACK` contains a capitalized acronym + --> $DIR/upper_case_acronyms.rs:12:5 + | +LL | ACK, + | ^^^ help: consider making the acronym lowercase, except the initial letter (notice the capitalization): `Ack` + +error: name `PSH` contains a capitalized acronym + --> $DIR/upper_case_acronyms.rs:13:5 + | +LL | PSH, + | ^^^ help: consider making the acronym lowercase, except the initial letter: `Psh` + +error: name `RST` contains a capitalized acronym + --> $DIR/upper_case_acronyms.rs:14:5 + | +LL | RST, + | ^^^ help: consider making the acronym lowercase, except the initial letter: `Rst` + +error: name `SYN` contains a capitalized acronym + --> $DIR/upper_case_acronyms.rs:15:5 + | +LL | SYN, + | ^^^ help: consider making the acronym lowercase, except the initial letter: `Syn` + +error: name `FIN` contains a capitalized acronym + --> $DIR/upper_case_acronyms.rs:16:5 + | +LL | FIN, + | ^^^ help: consider making the acronym lowercase, except the initial letter: `Fin` + +error: name `GCCLLVMSomething` contains a capitalized acronym + --> $DIR/upper_case_acronyms.rs:19:8 + | +LL | struct GCCLLVMSomething; // linted with cfg option, beware that lint suggests `GccllvmSomething` instead of + | ^^^^^^^^^^^^^^^^ help: consider making the acronym lowercase, except the initial letter: `GccllvmSomething` + +error: aborting due to 11 previous errors + diff --git a/tests/ui/upper_case_acronyms.rs b/tests/ui/upper_case_acronyms.rs index af0b57763486..fdf8905f812f 100644 --- a/tests/ui/upper_case_acronyms.rs +++ b/tests/ui/upper_case_acronyms.rs @@ -1,11 +1,11 @@ #![warn(clippy::upper_case_acronyms)] -struct HTTPResponse; // linted +struct HTTPResponse; // not linted by default, but with cfg option struct CString; // not linted enum Flags { - NS, // linted + NS, // not linted CWR, ECE, URG, @@ -16,6 +16,7 @@ enum Flags { FIN, } -struct GCCLLVMSomething; // linted, beware that lint suggests `GccllvmSomething` instead of `GccLlvmSomething` +struct GCCLLVMSomething; // linted with cfg option, beware that lint suggests `GccllvmSomething` instead of + // `GccLlvmSomething` fn main() {} diff --git a/tests/ui/upper_case_acronyms.stderr b/tests/ui/upper_case_acronyms.stderr index 2065fe10bb15..bbe38991e527 100644 --- a/tests/ui/upper_case_acronyms.stderr +++ b/tests/ui/upper_case_acronyms.stderr @@ -1,22 +1,10 @@ -error: name `HTTPResponse` contains a capitalized acronym - --> $DIR/upper_case_acronyms.rs:3:8 - | -LL | struct HTTPResponse; // linted - | ^^^^^^^^^^^^ help: consider making the acronym lowercase, except the initial letter: `HttpResponse` - | - = note: `-D clippy::upper-case-acronyms` implied by `-D warnings` - -error: name `NS` contains a capitalized acronym - --> $DIR/upper_case_acronyms.rs:8:5 - | -LL | NS, // linted - | ^^ help: consider making the acronym lowercase, except the initial letter (notice the capitalization): `Ns` - error: name `CWR` contains a capitalized acronym --> $DIR/upper_case_acronyms.rs:9:5 | LL | CWR, | ^^^ help: consider making the acronym lowercase, except the initial letter: `Cwr` + | + = note: `-D clippy::upper-case-acronyms` implied by `-D warnings` error: name `ECE` contains a capitalized acronym --> $DIR/upper_case_acronyms.rs:10:5 @@ -60,11 +48,5 @@ error: name `FIN` contains a capitalized acronym LL | FIN, | ^^^ help: consider making the acronym lowercase, except the initial letter: `Fin` -error: name `GCCLLVMSomething` contains a capitalized acronym - --> $DIR/upper_case_acronyms.rs:19:8 - | -LL | struct GCCLLVMSomething; // linted, beware that lint suggests `GccllvmSomething` instead of `GccLlvmSomething` - | ^^^^^^^^^^^^^^^^ help: consider making the acronym lowercase, except the initial letter: `GccllvmSomething` - -error: aborting due to 11 previous errors +error: aborting due to 8 previous errors From 2a6b06108d88830d3a6d5a50f733b8f7cf2799ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Thu, 25 Feb 2021 00:10:06 +0100 Subject: [PATCH 3/3] run cargo dev update_lints fix sentence / address review comments --- clippy_lints/src/lib.rs | 1 + clippy_lints/src/utils/conf.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 15262cfbf54a..176eeadcc630 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1716,6 +1716,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&unused_unit::UNUSED_UNIT), LintId::of(&unwrap::PANICKING_UNWRAP), LintId::of(&unwrap::UNNECESSARY_UNWRAP), + LintId::of(&upper_case_acronyms::UPPER_CASE_ACRONYMS), LintId::of(&useless_conversion::USELESS_CONVERSION), LintId::of(&vec::USELESS_VEC), LintId::of(&vec_init_then_push::VEC_INIT_THEN_PUSH), diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 34b9ecf2830f..bc241adc7d5f 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -173,7 +173,7 @@ define_Conf! { (disallowed_methods, "disallowed_methods": Vec, Vec::::new()), /// Lint: UNREADABLE_LITERAL. Should the fraction of a decimal be linted to include separators. (unreadable_literal_lint_fractions, "unreadable_literal_lint_fractions": bool, true), - /// Lint: UPPER_CASE_ACRONYMS. Enabler verbose mode triggers if there is more than one uppercase char next to each other + /// Lint: UPPER_CASE_ACRONYMS. Enables verbose mode. Triggers if there is more than one uppercase char next to each other (upper_case_acronyms_aggressive, "upper_case_acronyms_aggressive": bool, false), /// Lint: _CARGO_COMMON_METADATA. For internal testing only, ignores the current `publish` settings in the Cargo manifest. (cargo_ignore_publish, "cargo_ignore_publish": bool, false),