From 2e2021bbda7ce7b7095b79f07fe6a408a6cd8f07 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Thu, 6 May 2021 12:41:58 -0500 Subject: [PATCH 01/10] Add avoid_breaking_exported_api config option --- README.md | 1 + clippy_lints/src/utils/conf.rs | 2 ++ tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr | 2 +- 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index c6b67bb62..6c556f579 100644 --- a/README.md +++ b/README.md @@ -147,6 +147,7 @@ Some lints can be configured in a TOML file named `clippy.toml` or `.clippy.toml value` mapping eg. ```toml +avoid-breaking-exported-api = false blacklisted-names = ["toto", "tata", "titi"] cognitive-complexity-threshold = 30 ``` diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index fd2dddb3b..d65270a94 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -122,6 +122,8 @@ macro_rules! define_Conf { // N.B., this macro is parsed by util/lintlib.py define_Conf! { + /// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION. Suppress lints whenever the suggested change would cause breakage for other crates. + (avoid_breaking_exported_api: bool = true), /// Lint: CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE. The minimum rust version that the project supports (msrv: Option = None), /// Lint: BLACKLISTED_NAME. The list of blacklisted names to lint about. NB: `bar` is not here since it has legitimate uses 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 d83080b69..a7be00426 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`, `upper-case-acronyms-aggressive`, `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 `avoid-breaking-exported-api`, `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 From d7f47f280ec267c0583f7d38fc149a2351b923e6 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Wed, 26 May 2021 16:39:39 -0500 Subject: [PATCH 02/10] Use break api config for wrong_pub_self_convention --- clippy_lints/src/deprecated_lints.rs | 9 ++++ clippy_lints/src/lib.rs | 9 ++-- clippy_lints/src/methods/mod.rs | 40 ++++------------ .../src/methods/wrong_self_convention.rs | 13 ++--- tests/ui/def_id_nocore.rs | 2 +- tests/ui/module_name_repetitions.rs | 8 ---- tests/ui/wrong_self_convention.rs | 1 - tests/ui/wrong_self_convention.stderr | 48 +++++++++---------- tests/ui/wrong_self_convention2.rs | 1 - tests/ui/wrong_self_convention2.stderr | 4 +- 10 files changed, 55 insertions(+), 80 deletions(-) diff --git a/clippy_lints/src/deprecated_lints.rs b/clippy_lints/src/deprecated_lints.rs index 50ffc2e3f..dd780ff87 100644 --- a/clippy_lints/src/deprecated_lints.rs +++ b/clippy_lints/src/deprecated_lints.rs @@ -141,3 +141,12 @@ declare_deprecated_lint! { pub FILTER_MAP, "this lint has been replaced by `manual_filter_map`, a more specific lint" } + +declare_deprecated_lint! { + /// **What it does:** Nothing. This lint has been deprecated. + /// + /// **Deprecation reason:** The `avoid_breaking_exported_api` config option was added, which + /// enables the `wrong_self_conversion` lint for public items. + pub WRONG_PUB_SELF_CONVENTION, + "set the `avoid_breaking_exported_api` config option to `false` to enable the `wrong_self_convention` lint for public items" +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index cf5402120..6dd2486af 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -505,6 +505,10 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: "clippy::filter_map", "this lint has been replaced by `manual_filter_map`, a more specific lint", ); + store.register_removed( + "clippy::wrong_pub_self_convention", + "set the `avoid_breaking_exported_api` config option to `false` to enable the `wrong_self_convention` lint for public items", + ); // end deprecated lints, do not remove this comment, it’s used in `update_lints` // begin register lints, do not remove this comment, it’s used in `update_lints` @@ -802,7 +806,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: methods::UNNECESSARY_LAZY_EVALUATIONS, methods::UNWRAP_USED, methods::USELESS_ASREF, - methods::WRONG_PUB_SELF_CONVENTION, methods::WRONG_SELF_CONVENTION, methods::ZST_OFFSET, minmax::MIN_MAX, @@ -1026,7 +1029,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(methods::FILETYPE_IS_FILE), LintId::of(methods::GET_UNWRAP), LintId::of(methods::UNWRAP_USED), - LintId::of(methods::WRONG_PUB_SELF_CONVENTION), LintId::of(misc::FLOAT_CMP_CONST), LintId::of(misc_early::UNNEEDED_FIELD_PATTERN), LintId::of(missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS), @@ -1862,7 +1864,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: }) }); - store.register_late_pass(move || box methods::Methods::new(msrv)); + let avoid_breaking_exported_api = conf.avoid_breaking_exported_api; + store.register_late_pass(move || box methods::Methods::new(avoid_breaking_exported_api, msrv)); store.register_late_pass(move || box matches::Matches::new(msrv)); store.register_early_pass(move || box manual_non_exhaustive::ManualNonExhaustive::new(msrv)); store.register_late_pass(move || box manual_strip::ManualStrip::new(msrv)); diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index cabfe8023..0b998dbf8 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -282,30 +282,6 @@ declare_clippy_lint! { "defining a method named with an established prefix (like \"into_\") that takes `self` with the wrong convention" } -declare_clippy_lint! { - /// **What it does:** This is the same as - /// [`wrong_self_convention`](#wrong_self_convention), but for public items. - /// - /// **Why is this bad?** See [`wrong_self_convention`](#wrong_self_convention). - /// - /// **Known problems:** Actually *renaming* the function may break clients if - /// the function is part of the public interface. In that case, be mindful of - /// the stability guarantees you've given your users. - /// - /// **Example:** - /// ```rust - /// # struct X; - /// impl<'a> X { - /// pub fn as_str(self) -> &'a str { - /// "foo" - /// } - /// } - /// ``` - pub WRONG_PUB_SELF_CONVENTION, - restriction, - "defining a public method named with an established prefix (like \"into_\") that takes `self` with the wrong convention" -} - declare_clippy_lint! { /// **What it does:** Checks for usage of `ok().expect(..)`. /// @@ -1658,13 +1634,17 @@ declare_clippy_lint! { } pub struct Methods { + avoid_breaking_exported_api: bool, msrv: Option, } impl Methods { #[must_use] - pub fn new(msrv: Option) -> Self { - Self { msrv } + pub fn new(avoid_breaking_exported_api: bool, msrv: Option) -> Self { + Self { + avoid_breaking_exported_api, + msrv, + } } } @@ -1673,7 +1653,6 @@ impl_lint_pass!(Methods => [ EXPECT_USED, SHOULD_IMPLEMENT_TRAIT, WRONG_SELF_CONVENTION, - WRONG_PUB_SELF_CONVENTION, OK_EXPECT, MAP_UNWRAP_OR, RESULT_MAP_OR_INTO_OPTION, @@ -1838,11 +1817,13 @@ impl<'tcx> LateLintPass<'tcx> for Methods { } } - if sig.decl.implicit_self.has_implicit_self() { + if sig.decl.implicit_self.has_implicit_self() + && !(self.avoid_breaking_exported_api + && cx.access_levels.is_exported(impl_item.hir_id())) + { wrong_self_convention::check( cx, &name, - item.vis.node.is_pub(), self_ty, first_arg_ty, first_arg.pat.span, @@ -1915,7 +1896,6 @@ impl<'tcx> LateLintPass<'tcx> for Methods { wrong_self_convention::check( cx, &item.ident.name.as_str(), - false, self_ty, first_arg_ty, first_arg_span, diff --git a/clippy_lints/src/methods/wrong_self_convention.rs b/clippy_lints/src/methods/wrong_self_convention.rs index 1773c26c2..a2e09e5ec 100644 --- a/clippy_lints/src/methods/wrong_self_convention.rs +++ b/clippy_lints/src/methods/wrong_self_convention.rs @@ -6,7 +6,6 @@ use rustc_middle::ty::TyS; use rustc_span::source_map::Span; use std::fmt; -use super::WRONG_PUB_SELF_CONVENTION; use super::WRONG_SELF_CONVENTION; #[rustfmt::skip] @@ -21,9 +20,9 @@ const CONVENTIONS: [(&[Convention], &[SelfKind]); 9] = [ // Conversion using `to_` can use borrowed (non-Copy types) or owned (Copy types). // Source: https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv - (&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(false), + (&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(false), Convention::IsTraitItem(false), Convention::ImplementsTrait(false)], &[SelfKind::Ref]), - (&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(true), + (&[Convention::StartsWith("to_"), Convention::NotEndsWith("_mut"), Convention::IsSelfTypeCopy(true), Convention::IsTraitItem(false), Convention::ImplementsTrait(false)], &[SelfKind::Value]), ]; @@ -85,18 +84,12 @@ impl fmt::Display for Convention { pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, item_name: &str, - is_pub: bool, self_ty: &'tcx TyS<'tcx>, first_arg_ty: &'tcx TyS<'tcx>, first_arg_span: Span, implements_trait: bool, is_trait_item: bool, ) { - let lint = if is_pub { - WRONG_PUB_SELF_CONVENTION - } else { - WRONG_SELF_CONVENTION - }; if let Some((conventions, self_kinds)) = &CONVENTIONS.iter().find(|(convs, _)| { convs .iter() @@ -142,7 +135,7 @@ pub(super) fn check<'tcx>( span_lint_and_help( cx, - lint, + WRONG_SELF_CONVENTION, first_arg_span, &format!( "{} usually take {}", diff --git a/tests/ui/def_id_nocore.rs b/tests/ui/def_id_nocore.rs index 2a948d60b..cba7666c2 100644 --- a/tests/ui/def_id_nocore.rs +++ b/tests/ui/def_id_nocore.rs @@ -20,7 +20,7 @@ fn start(_argc: isize, _argv: *const *const u8) -> isize { 0 } -pub struct A; +struct A; impl A { pub fn as_ref(self) -> &'static str { diff --git a/tests/ui/module_name_repetitions.rs b/tests/ui/module_name_repetitions.rs index 669bf01a8..f5908cb57 100644 --- a/tests/ui/module_name_repetitions.rs +++ b/tests/ui/module_name_repetitions.rs @@ -15,12 +15,4 @@ mod foo { pub struct Foobar; } -#[cfg(test)] -mod test { - #[test] - fn it_works() { - assert_eq!(2 + 2, 4); - } -} - fn main() {} diff --git a/tests/ui/wrong_self_convention.rs b/tests/ui/wrong_self_convention.rs index cdfbdb8b0..151dd0c27 100644 --- a/tests/ui/wrong_self_convention.rs +++ b/tests/ui/wrong_self_convention.rs @@ -1,6 +1,5 @@ // edition:2018 #![warn(clippy::wrong_self_convention)] -#![warn(clippy::wrong_pub_self_convention)] #![allow(dead_code)] fn main() {} diff --git a/tests/ui/wrong_self_convention.stderr b/tests/ui/wrong_self_convention.stderr index 29f5ba826..ce23317ab 100644 --- a/tests/ui/wrong_self_convention.stderr +++ b/tests/ui/wrong_self_convention.stderr @@ -1,5 +1,5 @@ error: methods called `from_*` usually take no `self` - --> $DIR/wrong_self_convention.rs:18:17 + --> $DIR/wrong_self_convention.rs:17:17 | LL | fn from_i32(self) {} | ^^^^ @@ -8,7 +8,7 @@ LL | fn from_i32(self) {} = help: consider choosing a less ambiguous name error: methods called `from_*` usually take no `self` - --> $DIR/wrong_self_convention.rs:24:21 + --> $DIR/wrong_self_convention.rs:23:21 | LL | pub fn from_i64(self) {} | ^^^^ @@ -16,7 +16,7 @@ LL | pub fn from_i64(self) {} = help: consider choosing a less ambiguous name error: methods called `as_*` usually take `self` by reference or `self` by mutable reference - --> $DIR/wrong_self_convention.rs:36:15 + --> $DIR/wrong_self_convention.rs:35:15 | LL | fn as_i32(self) {} | ^^^^ @@ -24,7 +24,7 @@ LL | fn as_i32(self) {} = help: consider choosing a less ambiguous name error: methods called `into_*` usually take `self` by value - --> $DIR/wrong_self_convention.rs:38:17 + --> $DIR/wrong_self_convention.rs:37:17 | LL | fn into_i32(&self) {} | ^^^^^ @@ -32,7 +32,7 @@ LL | fn into_i32(&self) {} = help: consider choosing a less ambiguous name error: methods called `is_*` usually take `self` by reference or no `self` - --> $DIR/wrong_self_convention.rs:40:15 + --> $DIR/wrong_self_convention.rs:39:15 | LL | fn is_i32(self) {} | ^^^^ @@ -40,7 +40,7 @@ LL | fn is_i32(self) {} = help: consider choosing a less ambiguous name error: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference - --> $DIR/wrong_self_convention.rs:42:15 + --> $DIR/wrong_self_convention.rs:41:15 | LL | fn to_i32(self) {} | ^^^^ @@ -48,7 +48,7 @@ LL | fn to_i32(self) {} = help: consider choosing a less ambiguous name error: methods called `from_*` usually take no `self` - --> $DIR/wrong_self_convention.rs:44:17 + --> $DIR/wrong_self_convention.rs:43:17 | LL | fn from_i32(self) {} | ^^^^ @@ -56,7 +56,7 @@ LL | fn from_i32(self) {} = help: consider choosing a less ambiguous name error: methods called `as_*` usually take `self` by reference or `self` by mutable reference - --> $DIR/wrong_self_convention.rs:46:19 + --> $DIR/wrong_self_convention.rs:45:19 | LL | pub fn as_i64(self) {} | ^^^^ @@ -64,7 +64,7 @@ LL | pub fn as_i64(self) {} = help: consider choosing a less ambiguous name error: methods called `into_*` usually take `self` by value - --> $DIR/wrong_self_convention.rs:47:21 + --> $DIR/wrong_self_convention.rs:46:21 | LL | pub fn into_i64(&self) {} | ^^^^^ @@ -72,7 +72,7 @@ LL | pub fn into_i64(&self) {} = help: consider choosing a less ambiguous name error: methods called `is_*` usually take `self` by reference or no `self` - --> $DIR/wrong_self_convention.rs:48:19 + --> $DIR/wrong_self_convention.rs:47:19 | LL | pub fn is_i64(self) {} | ^^^^ @@ -80,7 +80,7 @@ LL | pub fn is_i64(self) {} = help: consider choosing a less ambiguous name error: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference - --> $DIR/wrong_self_convention.rs:49:19 + --> $DIR/wrong_self_convention.rs:48:19 | LL | pub fn to_i64(self) {} | ^^^^ @@ -88,7 +88,7 @@ LL | pub fn to_i64(self) {} = help: consider choosing a less ambiguous name error: methods called `from_*` usually take no `self` - --> $DIR/wrong_self_convention.rs:50:21 + --> $DIR/wrong_self_convention.rs:49:21 | LL | pub fn from_i64(self) {} | ^^^^ @@ -96,7 +96,7 @@ LL | pub fn from_i64(self) {} = help: consider choosing a less ambiguous name error: methods called `as_*` usually take `self` by reference or `self` by mutable reference - --> $DIR/wrong_self_convention.rs:95:19 + --> $DIR/wrong_self_convention.rs:94:19 | LL | fn as_i32(self) {} | ^^^^ @@ -104,7 +104,7 @@ LL | fn as_i32(self) {} = help: consider choosing a less ambiguous name error: methods called `into_*` usually take `self` by value - --> $DIR/wrong_self_convention.rs:98:25 + --> $DIR/wrong_self_convention.rs:97:25 | LL | fn into_i32_ref(&self) {} | ^^^^^ @@ -112,7 +112,7 @@ LL | fn into_i32_ref(&self) {} = help: consider choosing a less ambiguous name error: methods called `is_*` usually take `self` by reference or no `self` - --> $DIR/wrong_self_convention.rs:100:19 + --> $DIR/wrong_self_convention.rs:99:19 | LL | fn is_i32(self) {} | ^^^^ @@ -120,7 +120,7 @@ LL | fn is_i32(self) {} = help: consider choosing a less ambiguous name error: methods called `from_*` usually take no `self` - --> $DIR/wrong_self_convention.rs:104:21 + --> $DIR/wrong_self_convention.rs:103:21 | LL | fn from_i32(self) {} | ^^^^ @@ -128,7 +128,7 @@ LL | fn from_i32(self) {} = help: consider choosing a less ambiguous name error: methods called `as_*` usually take `self` by reference or `self` by mutable reference - --> $DIR/wrong_self_convention.rs:119:19 + --> $DIR/wrong_self_convention.rs:118:19 | LL | fn as_i32(self); | ^^^^ @@ -136,7 +136,7 @@ LL | fn as_i32(self); = help: consider choosing a less ambiguous name error: methods called `into_*` usually take `self` by value - --> $DIR/wrong_self_convention.rs:122:25 + --> $DIR/wrong_self_convention.rs:121:25 | LL | fn into_i32_ref(&self); | ^^^^^ @@ -144,7 +144,7 @@ LL | fn into_i32_ref(&self); = help: consider choosing a less ambiguous name error: methods called `is_*` usually take `self` by reference or no `self` - --> $DIR/wrong_self_convention.rs:124:19 + --> $DIR/wrong_self_convention.rs:123:19 | LL | fn is_i32(self); | ^^^^ @@ -152,7 +152,7 @@ LL | fn is_i32(self); = help: consider choosing a less ambiguous name error: methods called `from_*` usually take no `self` - --> $DIR/wrong_self_convention.rs:128:21 + --> $DIR/wrong_self_convention.rs:127:21 | LL | fn from_i32(self); | ^^^^ @@ -160,7 +160,7 @@ LL | fn from_i32(self); = help: consider choosing a less ambiguous name error: methods called `into_*` usually take `self` by value - --> $DIR/wrong_self_convention.rs:146:25 + --> $DIR/wrong_self_convention.rs:145:25 | LL | fn into_i32_ref(&self); | ^^^^^ @@ -168,7 +168,7 @@ LL | fn into_i32_ref(&self); = help: consider choosing a less ambiguous name error: methods called `from_*` usually take no `self` - --> $DIR/wrong_self_convention.rs:152:21 + --> $DIR/wrong_self_convention.rs:151:21 | LL | fn from_i32(self); | ^^^^ @@ -176,7 +176,7 @@ LL | fn from_i32(self); = help: consider choosing a less ambiguous name error: methods with the following characteristics: (`to_*` and `self` type is `Copy`) usually take `self` by value - --> $DIR/wrong_self_convention.rs:176:22 + --> $DIR/wrong_self_convention.rs:175:22 | LL | fn to_u64_v2(&self) -> u64 { | ^^^^^ @@ -184,7 +184,7 @@ LL | fn to_u64_v2(&self) -> u64 { = help: consider choosing a less ambiguous name error: methods with the following characteristics: (`to_*` and `self` type is not `Copy`) usually take `self` by reference - --> $DIR/wrong_self_convention.rs:185:19 + --> $DIR/wrong_self_convention.rs:184:19 | LL | fn to_u64(self) -> u64 { | ^^^^ diff --git a/tests/ui/wrong_self_convention2.rs b/tests/ui/wrong_self_convention2.rs index 3a72174d0..501bc1e6a 100644 --- a/tests/ui/wrong_self_convention2.rs +++ b/tests/ui/wrong_self_convention2.rs @@ -1,6 +1,5 @@ // edition:2018 #![warn(clippy::wrong_self_convention)] -#![warn(clippy::wrong_pub_self_convention)] #![allow(dead_code)] fn main() {} diff --git a/tests/ui/wrong_self_convention2.stderr b/tests/ui/wrong_self_convention2.stderr index d2d74ce09..0e0d066d6 100644 --- a/tests/ui/wrong_self_convention2.stderr +++ b/tests/ui/wrong_self_convention2.stderr @@ -1,5 +1,5 @@ error: methods called `from_*` usually take no `self` - --> $DIR/wrong_self_convention2.rs:56:29 + --> $DIR/wrong_self_convention2.rs:55:29 | LL | pub fn from_be_self(self) -> Self { | ^^^^ @@ -8,7 +8,7 @@ LL | pub fn from_be_self(self) -> Self { = help: consider choosing a less ambiguous name error: methods called `from_*` usually take no `self` - --> $DIR/wrong_self_convention2.rs:65:25 + --> $DIR/wrong_self_convention2.rs:64:25 | LL | fn from_be_self(self) -> Self; | ^^^^ From ee79077d8008006f25fb591946bdc17e99c0b91a Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Thu, 6 May 2021 12:45:21 -0500 Subject: [PATCH 03/10] Use break api config for pass by value or ref --- clippy_lints/src/lib.rs | 1 + clippy_lints/src/pass_by_ref_or_value.rs | 13 +++++++++++-- tests/ui/trivially_copy_pass_by_ref.stderr | 8 +------- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 6dd2486af..30db45a34 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1947,6 +1947,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: let pass_by_ref_or_value = pass_by_ref_or_value::PassByRefOrValue::new( conf.trivial_copy_size_limit, conf.pass_by_value_size_limit, + conf.avoid_breaking_exported_api, &sess.target, ); store.register_late_pass(move || box pass_by_ref_or_value); diff --git a/clippy_lints/src/pass_by_ref_or_value.rs b/clippy_lints/src/pass_by_ref_or_value.rs index 6b64846c2..f6a704785 100644 --- a/clippy_lints/src/pass_by_ref_or_value.rs +++ b/clippy_lints/src/pass_by_ref_or_value.rs @@ -102,10 +102,16 @@ declare_clippy_lint! { pub struct PassByRefOrValue { ref_min_size: u64, value_max_size: u64, + avoid_breaking_exported_api: bool, } impl<'tcx> PassByRefOrValue { - pub fn new(ref_min_size: Option, value_max_size: u64, target: &Target) -> Self { + pub fn new( + ref_min_size: Option, + value_max_size: u64, + avoid_breaking_exported_api: bool, + target: &Target, + ) -> Self { let ref_min_size = ref_min_size.unwrap_or_else(|| { let bit_width = u64::from(target.pointer_width); // Cap the calculated bit width at 32-bits to reduce @@ -120,10 +126,14 @@ impl<'tcx> PassByRefOrValue { Self { ref_min_size, value_max_size, + avoid_breaking_exported_api, } } fn check_poly_fn(&mut self, cx: &LateContext<'tcx>, hir_id: HirId, decl: &FnDecl<'_>, span: Option) { + if self.avoid_breaking_exported_api && cx.access_levels.is_exported(hir_id) { + return; + } let fn_def_id = cx.tcx.hir().local_def_id(hir_id); let fn_sig = cx.tcx.fn_sig(fn_def_id); @@ -184,7 +194,6 @@ impl<'tcx> PassByRefOrValue { } if_chain! { - if !cx.access_levels.is_exported(hir_id); if is_copy(cx, ty); if !is_self_ty(input); if let Some(size) = cx.layout_of(ty).ok().map(|l| l.size.bytes()); diff --git a/tests/ui/trivially_copy_pass_by_ref.stderr b/tests/ui/trivially_copy_pass_by_ref.stderr index ccc3cdb2b..2b0005bbf 100644 --- a/tests/ui/trivially_copy_pass_by_ref.stderr +++ b/tests/ui/trivially_copy_pass_by_ref.stderr @@ -88,12 +88,6 @@ error: this argument (N byte) is passed by reference, but would be more efficien LL | fn trait_method(&self, _foo: &Foo); | ^^^^ help: consider passing by value instead: `Foo` -error: this argument (N byte) is passed by reference, but would be more efficient if passed by value (limit: N byte) - --> $DIR/trivially_copy_pass_by_ref.rs:80:37 - | -LL | fn trait_method2(&self, _color: &Color); - | ^^^^^^ help: consider passing by value instead: `Color` - error: this argument (N byte) is passed by reference, but would be more efficient if passed by value (limit: N byte) --> $DIR/trivially_copy_pass_by_ref.rs:108:21 | @@ -106,5 +100,5 @@ error: this argument (N byte) is passed by reference, but would be more efficien LL | fn foo(x: &i32) { | ^^^^ help: consider passing by value instead: `i32` -error: aborting due to 17 previous errors +error: aborting due to 16 previous errors From 3d77a2b8617f914b1e5503dd93f67a8334693ce0 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Thu, 6 May 2021 14:02:24 -0500 Subject: [PATCH 04/10] Use break api config for enum_variant_names --- clippy_lints/src/deprecated_lints.rs | 10 ++++ clippy_lints/src/enum_variants.rs | 80 ++++++++++------------------ clippy_lints/src/lib.rs | 8 +-- tests/ui/enum_variants.rs | 7 ++- tests/ui/enum_variants.stderr | 37 +++++-------- 5 files changed, 59 insertions(+), 83 deletions(-) diff --git a/clippy_lints/src/deprecated_lints.rs b/clippy_lints/src/deprecated_lints.rs index dd780ff87..04f3d7746 100644 --- a/clippy_lints/src/deprecated_lints.rs +++ b/clippy_lints/src/deprecated_lints.rs @@ -142,6 +142,16 @@ declare_deprecated_lint! { "this lint has been replaced by `manual_filter_map`, a more specific lint" } +declare_deprecated_lint! { + /// **What it does:** Nothing. This lint has been deprecated. + /// + /// **Deprecation reason:** The `avoid_breaking_exported_api` config option was added, which + /// enables the `enum_variant_names` lint for public items. + /// ``` + pub PUB_ENUM_VARIANT_NAMES, + "set the `avoid_breaking_exported_api` config option to `false` to enable the `enum_variant_names` lint for public items" +} + declare_deprecated_lint! { /// **What it does:** Nothing. This lint has been deprecated. /// diff --git a/clippy_lints/src/enum_variants.rs b/clippy_lints/src/enum_variants.rs index 0ecc0bc3e..b1a105a51 100644 --- a/clippy_lints/src/enum_variants.rs +++ b/clippy_lints/src/enum_variants.rs @@ -3,8 +3,8 @@ use clippy_utils::camel_case; use clippy_utils::diagnostics::{span_lint, span_lint_and_help}; use clippy_utils::source::is_present_in_source; -use rustc_ast::ast::{EnumDef, Item, ItemKind, VisibilityKind}; -use rustc_lint::{EarlyContext, EarlyLintPass, Lint}; +use rustc_hir::{EnumDef, Item, ItemKind}; +use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::source_map::Span; use rustc_span::symbol::Symbol; @@ -39,36 +39,6 @@ declare_clippy_lint! { "enums where all variants share a prefix/postfix" } -declare_clippy_lint! { - /// **What it does:** Detects public enumeration variants that are - /// prefixed or suffixed by the same characters. - /// - /// **Why is this bad?** Public enumeration variant names should specify their variant, - /// not repeat the enumeration name. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// ```rust - /// pub enum Cake { - /// BlackForestCake, - /// HummingbirdCake, - /// BattenbergCake, - /// } - /// ``` - /// Could be written as: - /// ```rust - /// pub enum Cake { - /// BlackForest, - /// Hummingbird, - /// Battenberg, - /// } - /// ``` - pub PUB_ENUM_VARIANT_NAMES, - pedantic, - "public enums where all variants share a prefix/postfix" -} - declare_clippy_lint! { /// **What it does:** Detects type names that are prefixed or suffixed by the /// containing module's name. @@ -127,21 +97,22 @@ declare_clippy_lint! { pub struct EnumVariantNames { modules: Vec<(Symbol, String)>, threshold: u64, + avoid_breaking_exported_api: bool, } impl EnumVariantNames { #[must_use] - pub fn new(threshold: u64) -> Self { + pub fn new(threshold: u64, avoid_breaking_exported_api: bool) -> Self { Self { modules: Vec::new(), threshold, + avoid_breaking_exported_api, } } } impl_lint_pass!(EnumVariantNames => [ ENUM_VARIANT_NAMES, - PUB_ENUM_VARIANT_NAMES, MODULE_NAME_REPETITIONS, MODULE_INCEPTION ]); @@ -167,33 +138,42 @@ fn partial_rmatch(post: &str, name: &str) -> usize { } fn check_variant( - cx: &EarlyContext<'_>, + cx: &LateContext<'_>, threshold: u64, - def: &EnumDef, + def: &EnumDef<'_>, item_name: &str, item_name_chars: usize, span: Span, - lint: &'static Lint, ) { if (def.variants.len() as u64) < threshold { return; } - for var in &def.variants { + for var in def.variants { let name = var.ident.name.as_str(); if partial_match(item_name, &name) == item_name_chars && name.chars().nth(item_name_chars).map_or(false, |c| !c.is_lowercase()) && name.chars().nth(item_name_chars + 1).map_or(false, |c| !c.is_numeric()) { - span_lint(cx, lint, var.span, "variant name starts with the enum's name"); + span_lint( + cx, + ENUM_VARIANT_NAMES, + var.span, + "variant name starts with the enum's name", + ); } if partial_rmatch(item_name, &name) == item_name_chars { - span_lint(cx, lint, var.span, "variant name ends with the enum's name"); + span_lint( + cx, + ENUM_VARIANT_NAMES, + var.span, + "variant name ends with the enum's name", + ); } } let first = &def.variants[0].ident.name.as_str(); let mut pre = &first[..camel_case::until(&*first)]; let mut post = &first[camel_case::from(&*first)..]; - for var in &def.variants { + for var in def.variants { let name = var.ident.name.as_str(); let pre_match = partial_match(pre, &name); @@ -226,7 +206,7 @@ fn check_variant( }; span_lint_and_help( cx, - lint, + ENUM_VARIANT_NAMES, span, &format!("all variants have the same {}fix: `{}`", what, value), None, @@ -261,14 +241,14 @@ fn to_camel_case(item_name: &str) -> String { s } -impl EarlyLintPass for EnumVariantNames { - fn check_item_post(&mut self, _cx: &EarlyContext<'_>, _item: &Item) { +impl LateLintPass<'_> for EnumVariantNames { + fn check_item_post(&mut self, _cx: &LateContext<'_>, _item: &Item<'_>) { let last = self.modules.pop(); assert!(last.is_some()); } #[allow(clippy::similar_names)] - fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { + fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { let item_name = item.ident.name.as_str(); let item_name_chars = item_name.chars().count(); let item_camel = to_camel_case(&item_name); @@ -286,7 +266,7 @@ impl EarlyLintPass for EnumVariantNames { ); } } - if item.vis.kind.is_pub() { + if item.vis.node.is_pub() { let matching = partial_match(mod_camel, &item_camel); let rmatching = partial_rmatch(mod_camel, &item_camel); let nchars = mod_camel.chars().count(); @@ -317,11 +297,9 @@ impl EarlyLintPass for EnumVariantNames { } } if let ItemKind::Enum(ref def, _) = item.kind { - let lint = match item.vis.kind { - VisibilityKind::Public => PUB_ENUM_VARIANT_NAMES, - _ => ENUM_VARIANT_NAMES, - }; - check_variant(cx, self.threshold, def, &item_name, item_name_chars, item.span, lint); + if !(self.avoid_breaking_exported_api && cx.access_levels.is_exported(item.hir_id())) { + check_variant(cx, self.threshold, def, &item_name, item_name_chars, item.span); + } } self.modules.push((item.ident.name, item_camel)); } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 30db45a34..bab389d2c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -505,6 +505,10 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: "clippy::filter_map", "this lint has been replaced by `manual_filter_map`, a more specific lint", ); + store.register_removed( + "clippy::pub_enum_variant_names", + "set the `avoid_breaking_exported_api` config option to `false` to enable the `enum_variant_names` lint for public items", + ); store.register_removed( "clippy::wrong_pub_self_convention", "set the `avoid_breaking_exported_api` config option to `false` to enable the `wrong_self_convention` lint for public items", @@ -622,7 +626,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: enum_variants::ENUM_VARIANT_NAMES, enum_variants::MODULE_INCEPTION, enum_variants::MODULE_NAME_REPETITIONS, - enum_variants::PUB_ENUM_VARIANT_NAMES, eq_op::EQ_OP, eq_op::OP_REF, erasing_op::ERASING_OP, @@ -1080,7 +1083,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(doc::MISSING_PANICS_DOC), LintId::of(empty_enum::EMPTY_ENUM), LintId::of(enum_variants::MODULE_NAME_REPETITIONS), - LintId::of(enum_variants::PUB_ENUM_VARIANT_NAMES), LintId::of(eta_reduction::REDUNDANT_CLOSURE_FOR_METHOD_CALLS), LintId::of(excessive_bools::FN_PARAMS_EXCESSIVE_BOOLS), LintId::of(excessive_bools::STRUCT_EXCESSIVE_BOOLS), @@ -2015,7 +2017,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: let literal_representation_threshold = conf.literal_representation_threshold; store.register_early_pass(move || box literal_representation::DecimalLiteralRepresentation::new(literal_representation_threshold)); 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_late_pass(move || box enum_variants::EnumVariantNames::new(enum_variant_name_threshold, avoid_breaking_exported_api)); store.register_early_pass(|| box tabs_in_doc_comments::TabsInDocComments); 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)); diff --git a/tests/ui/enum_variants.rs b/tests/ui/enum_variants.rs index 4fefc0b43..083f5143e 100644 --- a/tests/ui/enum_variants.rs +++ b/tests/ui/enum_variants.rs @@ -1,5 +1,4 @@ -#![feature(non_ascii_idents)] -#![warn(clippy::enum_variant_names, clippy::pub_enum_variant_names)] +#![warn(clippy::enum_variant_names)] #![allow(non_camel_case_types, clippy::upper_case_acronyms)] enum FakeCallType { @@ -97,8 +96,8 @@ pub enum PubSeall { WithOut, } -#[allow(clippy::pub_enum_variant_names)] -mod allowed { +#[allow(clippy::enum_variant_names)] +pub mod allowed { pub enum PubAllowed { SomeThis, SomeThat, diff --git a/tests/ui/enum_variants.stderr b/tests/ui/enum_variants.stderr index ab7fff450..447fbb9e1 100644 --- a/tests/ui/enum_variants.stderr +++ b/tests/ui/enum_variants.stderr @@ -1,5 +1,5 @@ error: variant name ends with the enum's name - --> $DIR/enum_variants.rs:16:5 + --> $DIR/enum_variants.rs:15:5 | LL | cFoo, | ^^^^ @@ -7,25 +7,25 @@ LL | cFoo, = note: `-D clippy::enum-variant-names` implied by `-D warnings` error: variant name starts with the enum's name - --> $DIR/enum_variants.rs:27:5 + --> $DIR/enum_variants.rs:26:5 | LL | FoodGood, | ^^^^^^^^ error: variant name starts with the enum's name - --> $DIR/enum_variants.rs:28:5 + --> $DIR/enum_variants.rs:27:5 | LL | FoodMiddle, | ^^^^^^^^^^ error: variant name starts with the enum's name - --> $DIR/enum_variants.rs:29:5 + --> $DIR/enum_variants.rs:28:5 | LL | FoodBad, | ^^^^^^^ error: all variants have the same prefix: `Food` - --> $DIR/enum_variants.rs:26:1 + --> $DIR/enum_variants.rs:25:1 | LL | / enum Food { LL | | FoodGood, @@ -37,7 +37,7 @@ LL | | } = help: remove the prefixes and use full paths to the variants instead of glob imports error: all variants have the same prefix: `CallType` - --> $DIR/enum_variants.rs:36:1 + --> $DIR/enum_variants.rs:35:1 | LL | / enum BadCallType { LL | | CallTypeCall, @@ -49,7 +49,7 @@ LL | | } = help: remove the prefixes and use full paths to the variants instead of glob imports error: all variants have the same prefix: `Constant` - --> $DIR/enum_variants.rs:48:1 + --> $DIR/enum_variants.rs:47:1 | LL | / enum Consts { LL | | ConstantInt, @@ -61,7 +61,7 @@ LL | | } = help: remove the prefixes and use full paths to the variants instead of glob imports error: all variants have the same prefix: `With` - --> $DIR/enum_variants.rs:82:1 + --> $DIR/enum_variants.rs:81:1 | LL | / enum Seallll { LL | | WithOutCake, @@ -73,7 +73,7 @@ LL | | } = help: remove the prefixes and use full paths to the variants instead of glob imports error: all variants have the same prefix: `Prefix` - --> $DIR/enum_variants.rs:88:1 + --> $DIR/enum_variants.rs:87:1 | LL | / enum NonCaps { LL | | Prefix的, @@ -84,21 +84,8 @@ LL | | } | = help: remove the prefixes and use full paths to the variants instead of glob imports -error: all variants have the same prefix: `With` - --> $DIR/enum_variants.rs:94:1 - | -LL | / pub enum PubSeall { -LL | | WithOutCake, -LL | | WithOutTea, -LL | | WithOut, -LL | | } - | |_^ - | - = note: `-D clippy::pub-enum-variant-names` implied by `-D warnings` - = help: remove the prefixes and use full paths to the variants instead of glob imports - error: all variants have the same postfix: `IData` - --> $DIR/enum_variants.rs:137:1 + --> $DIR/enum_variants.rs:136:1 | LL | / enum IDataRequest { LL | | PutIData(String), @@ -110,7 +97,7 @@ LL | | } = help: remove the postfixes and use full paths to the variants instead of glob imports error: all variants have the same postfix: `HIData` - --> $DIR/enum_variants.rs:143:1 + --> $DIR/enum_variants.rs:142:1 | LL | / enum HIDataRequest { LL | | PutHIData(String), @@ -121,5 +108,5 @@ LL | | } | = help: remove the postfixes and use full paths to the variants instead of glob imports -error: aborting due to 12 previous errors +error: aborting due to 11 previous errors From 1ce581d70698466879dc7acec0629032fb9006fd Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Thu, 6 May 2021 14:33:23 -0500 Subject: [PATCH 05/10] Use break api config for unnecessary_wraps --- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/unnecessary_wraps.rs | 21 ++++++++++++++++----- tests/ui/unnecessary_wraps.rs | 2 +- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index bab389d2c..9c9eba0d2 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1976,7 +1976,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box redundant_clone::RedundantClone); store.register_late_pass(|| box slow_vector_initialization::SlowVectorInit); store.register_late_pass(|| box unnecessary_sort_by::UnnecessarySortBy); - store.register_late_pass(|| box unnecessary_wraps::UnnecessaryWraps); + store.register_late_pass(move || box unnecessary_wraps::UnnecessaryWraps::new(avoid_breaking_exported_api)); store.register_late_pass(|| box assertions_on_constants::AssertionsOnConstants); store.register_late_pass(|| box transmuting_null::TransmutingNull); store.register_late_pass(|| box path_buf_push_overwrite::PathBufPushOverwrite); diff --git a/clippy_lints/src/unnecessary_wraps.rs b/clippy_lints/src/unnecessary_wraps.rs index f2f1410ae..a85ffa6aa 100644 --- a/clippy_lints/src/unnecessary_wraps.rs +++ b/clippy_lints/src/unnecessary_wraps.rs @@ -8,7 +8,7 @@ use rustc_hir::LangItem::{OptionSome, ResultOk}; use rustc_hir::{Body, ExprKind, FnDecl, HirId, Impl, ItemKind, Node}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; -use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::symbol::sym; use rustc_span::Span; @@ -52,7 +52,19 @@ declare_clippy_lint! { "functions that only return `Ok` or `Some`" } -declare_lint_pass!(UnnecessaryWraps => [UNNECESSARY_WRAPS]); +pub struct UnnecessaryWraps { + avoid_breaking_exported_api: bool, +} + +impl_lint_pass!(UnnecessaryWraps => [UNNECESSARY_WRAPS]); + +impl UnnecessaryWraps { + pub fn new(avoid_breaking_exported_api: bool) -> Self { + Self { + avoid_breaking_exported_api, + } + } +} impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps { fn check_fn( @@ -66,13 +78,12 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps { ) { // Abort if public function/method or closure. match fn_kind { - FnKind::ItemFn(.., visibility) | FnKind::Method(.., Some(visibility)) => { - if visibility.node.is_pub() { + FnKind::ItemFn(..) | FnKind::Method(..) => { + if self.avoid_breaking_exported_api && cx.access_levels.is_exported(hir_id) { return; } }, FnKind::Closure => return, - FnKind::Method(..) => (), } // Abort if the method is implementing a trait or of it a trait method. diff --git a/tests/ui/unnecessary_wraps.rs b/tests/ui/unnecessary_wraps.rs index 54f22e3ee..63648ef58 100644 --- a/tests/ui/unnecessary_wraps.rs +++ b/tests/ui/unnecessary_wraps.rs @@ -65,7 +65,7 @@ fn func10() -> Option<()> { unimplemented!() } -struct A; +pub struct A; impl A { // should not be linted From 55ccc7a8c6c353c3350332114c76b79f3108a9c2 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Thu, 6 May 2021 14:42:55 -0500 Subject: [PATCH 06/10] Use break api config for upper_case_acronyms --- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/upper_case_acronyms.rs | 32 +++++++++++++++---------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 9c9eba0d2..5139b1464 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -2020,7 +2020,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(move || box enum_variants::EnumVariantNames::new(enum_variant_name_threshold, avoid_breaking_exported_api)); store.register_early_pass(|| box tabs_in_doc_comments::TabsInDocComments); 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(move || box upper_case_acronyms::UpperCaseAcronyms::new(avoid_breaking_exported_api, 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); diff --git a/clippy_lints/src/upper_case_acronyms.rs b/clippy_lints/src/upper_case_acronyms.rs index debbd86a5..0b58c6c09 100644 --- a/clippy_lints/src/upper_case_acronyms.rs +++ b/clippy_lints/src/upper_case_acronyms.rs @@ -1,8 +1,8 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use itertools::Itertools; -use rustc_ast::ast::{Item, ItemKind, VisibilityKind}; use rustc_errors::Applicability; -use rustc_lint::{EarlyContext, EarlyLintPass, LintContext}; +use rustc_hir::{Item, ItemKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::symbol::Ident; @@ -38,12 +38,14 @@ declare_clippy_lint! { #[derive(Default)] pub struct UpperCaseAcronyms { + avoid_breaking_exported_api: bool, upper_case_acronyms_aggressive: bool, } impl UpperCaseAcronyms { - pub fn new(aggressive: bool) -> Self { + pub fn new(avoid_breaking_exported_api: bool, aggressive: bool) -> Self { Self { + avoid_breaking_exported_api, upper_case_acronyms_aggressive: aggressive, } } @@ -72,7 +74,7 @@ fn correct_ident(ident: &str) -> String { ident } -fn check_ident(cx: &EarlyContext<'_>, ident: &Ident, be_aggressive: bool) { +fn check_ident(cx: &LateContext<'_>, ident: &Ident, be_aggressive: bool) { let span = ident.span; let ident = &ident.as_str(); let corrected = correct_ident(ident); @@ -96,23 +98,27 @@ fn check_ident(cx: &EarlyContext<'_>, ident: &Ident, be_aggressive: bool) { } } -impl EarlyLintPass for UpperCaseAcronyms { - fn check_item(&mut self, cx: &EarlyContext<'_>, it: &Item) { +impl LateLintPass<'_> for UpperCaseAcronyms { + fn check_item(&mut self, cx: &LateContext<'_>, it: &Item<'_>) { // do not lint public items or in macros - if !in_external_macro(cx.sess(), it.span) && !matches!(it.vis.kind, VisibilityKind::Public) { - if matches!( - it.kind, - ItemKind::TyAlias(..) | ItemKind::Struct(..) | ItemKind::Trait(..) - ) { + if in_external_macro(cx.sess(), it.span) + || (self.avoid_breaking_exported_api && cx.access_levels.is_exported(it.hir_id())) + { + return; + } + match it.kind { + ItemKind::TyAlias(..) | ItemKind::Struct(..) | ItemKind::Trait(..) => { check_ident(cx, &it.ident, self.upper_case_acronyms_aggressive); - } else if let ItemKind::Enum(ref enumdef, _) = it.kind { + }, + ItemKind::Enum(ref enumdef, _) => { // check enum variants seperately because again we only want to lint on private enums and // the fn check_variant does not know about the vis of the enum of its variants enumdef .variants .iter() .for_each(|variant| check_ident(cx, &variant.ident, self.upper_case_acronyms_aggressive)); - } + }, + _ => {}, } } } From c51472b4b09d22bdbb46027f08be54c4b285a725 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Thu, 6 May 2021 15:43:19 -0500 Subject: [PATCH 07/10] Add clippy.toml to project and tests --- clippy.toml | 1 + tests/clippy.toml | 1 + tests/compile-test.rs | 42 ++++++++++++++++++++++++++++++++---------- 3 files changed, 34 insertions(+), 10 deletions(-) create mode 100644 clippy.toml create mode 100644 tests/clippy.toml diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 000000000..cda8d17ee --- /dev/null +++ b/clippy.toml @@ -0,0 +1 @@ +avoid-breaking-exported-api = false diff --git a/tests/clippy.toml b/tests/clippy.toml new file mode 100644 index 000000000..5eb7ac035 --- /dev/null +++ b/tests/clippy.toml @@ -0,0 +1 @@ +# default config for tests, overrides clippy.toml at the project root diff --git a/tests/compile-test.rs b/tests/compile-test.rs index e1110721f..7d266a36b 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -4,8 +4,8 @@ use compiletest_rs as compiletest; use compiletest_rs::common::Mode as TestMode; -use std::env::{self, set_var, var}; -use std::ffi::OsStr; +use std::env::{self, remove_var, set_var, var_os}; +use std::ffi::{OsStr, OsString}; use std::fs; use std::io; use std::path::{Path, PathBuf}; @@ -88,9 +88,11 @@ fn default_config() -> compiletest::Config { config } -fn run_mode(cfg: &mut compiletest::Config) { +fn run_ui(cfg: &mut compiletest::Config) { cfg.mode = TestMode::Ui; cfg.src_base = Path::new("tests").join("ui"); + // use tests/clippy.toml + let _g = VarGuard::set("CARGO_MANIFEST_DIR", std::fs::canonicalize("tests").unwrap()); compiletest::run_tests(cfg); } @@ -114,7 +116,7 @@ fn run_ui_toml(config: &mut compiletest::Config) { continue; } let dir_path = dir.path(); - set_var("CARGO_MANIFEST_DIR", &dir_path); + let _g = VarGuard::set("CARGO_MANIFEST_DIR", &dir_path); for file in fs::read_dir(&dir_path)? { let file = file?; let file_path = file.path(); @@ -145,9 +147,7 @@ fn run_ui_toml(config: &mut compiletest::Config) { let tests = compiletest::make_tests(config); - let manifest_dir = var("CARGO_MANIFEST_DIR").unwrap_or_default(); let res = run_tests(config, tests); - set_var("CARGO_MANIFEST_DIR", &manifest_dir); match res { Ok(true) => {}, Ok(false) => panic!("Some tests failed"), @@ -208,7 +208,7 @@ fn run_ui_cargo(config: &mut compiletest::Config) { Some("main.rs") => {}, _ => continue, } - set_var("CLIPPY_CONF_DIR", case.path()); + let _g = VarGuard::set("CLIPPY_CONF_DIR", case.path()); let paths = compiletest::common::TestPaths { file: file_path, base: config.src_base.clone(), @@ -236,10 +236,8 @@ fn run_ui_cargo(config: &mut compiletest::Config) { let tests = compiletest::make_tests(config); let current_dir = env::current_dir().unwrap(); - let conf_dir = var("CLIPPY_CONF_DIR").unwrap_or_default(); let res = run_tests(config, &config.filters, tests); env::set_current_dir(current_dir).unwrap(); - set_var("CLIPPY_CONF_DIR", conf_dir); match res { Ok(true) => {}, @@ -260,8 +258,32 @@ fn prepare_env() { fn compile_test() { prepare_env(); let mut config = default_config(); - run_mode(&mut config); + run_ui(&mut config); run_ui_toml(&mut config); run_ui_cargo(&mut config); run_internal_tests(&mut config); } + +/// Restores an env var on drop +#[must_use] +struct VarGuard { + key: &'static str, + value: Option, +} + +impl VarGuard { + fn set(key: &'static str, val: impl AsRef) -> Self { + let value = var_os(key); + set_var(key, val); + Self { key, value } + } +} + +impl Drop for VarGuard { + fn drop(&mut self) { + match self.value.as_deref() { + None => remove_var(self.key), + Some(value) => set_var(self.key, value), + } + } +} From 7dd356d9f1c91fb52fb7568ad6439b572a353686 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Thu, 6 May 2021 20:22:10 -0500 Subject: [PATCH 08/10] Eat dogfood --- build.rs | 2 +- rustc_tools_util/src/lib.rs | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/build.rs b/build.rs index 018375dba..b5484bec3 100644 --- a/build.rs +++ b/build.rs @@ -14,6 +14,6 @@ fn main() { ); println!( "cargo:rustc-env=RUSTC_RELEASE_CHANNEL={}", - rustc_tools_util::get_channel().unwrap_or_default() + rustc_tools_util::get_channel() ); } diff --git a/rustc_tools_util/src/lib.rs b/rustc_tools_util/src/lib.rs index ff2a7de57..5f289918a 100644 --- a/rustc_tools_util/src/lib.rs +++ b/rustc_tools_util/src/lib.rs @@ -100,9 +100,9 @@ pub fn get_commit_date() -> Option { } #[must_use] -pub fn get_channel() -> Option { +pub fn get_channel() -> String { match env::var("CFG_RELEASE_CHANNEL") { - Ok(channel) => Some(channel), + Ok(channel) => channel, Err(_) => { // if that failed, try to ask rustc -V, do some parsing and find out match std::process::Command::new("rustc") @@ -113,16 +113,16 @@ pub fn get_channel() -> Option { { Some(rustc_output) => { if rustc_output.contains("beta") { - Some(String::from("beta")) + String::from("beta") } else if rustc_output.contains("stable") { - Some(String::from("stable")) + String::from("stable") } else { // default to nightly if we fail to parse - Some(String::from("nightly")) + String::from("nightly") } }, // default to nightly - None => Some(String::from("nightly")), + None => String::from("nightly"), } }, } From 3af95846a2ab61238f1a8f9c16a52d4a8d2390b0 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Fri, 7 May 2021 11:19:35 -0500 Subject: [PATCH 09/10] Add deprecated lint tests --- tests/ui/deprecated.rs | 2 ++ tests/ui/deprecated.stderr | 14 +++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/ui/deprecated.rs b/tests/ui/deprecated.rs index dbf0b03af..4ba9f0c1f 100644 --- a/tests/ui/deprecated.rs +++ b/tests/ui/deprecated.rs @@ -12,5 +12,7 @@ #[warn(clippy::unknown_clippy_lints)] #[warn(clippy::find_map)] #[warn(clippy::filter_map)] +#[warn(clippy::pub_enum_variant_names)] +#[warn(clippy::wrong_pub_self_convention)] fn main() {} diff --git a/tests/ui/deprecated.stderr b/tests/ui/deprecated.stderr index e5de839db..03c9f4388 100644 --- a/tests/ui/deprecated.stderr +++ b/tests/ui/deprecated.stderr @@ -84,5 +84,17 @@ error: lint `clippy::filter_map` has been removed: this lint has been replaced b LL | #[warn(clippy::filter_map)] | ^^^^^^^^^^^^^^^^^^ -error: aborting due to 14 previous errors +error: lint `clippy::pub_enum_variant_names` has been removed: set the `avoid_breaking_exported_api` config option to `false` to enable the `enum_variant_names` lint for public items + --> $DIR/deprecated.rs:15:8 + | +LL | #[warn(clippy::pub_enum_variant_names)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: lint `clippy::wrong_pub_self_convention` has been removed: set the `avoid_breaking_exported_api` config option to `false` to enable the `wrong_self_convention` lint for public items + --> $DIR/deprecated.rs:16:8 + | +LL | #[warn(clippy::wrong_pub_self_convention)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 16 previous errors From 6eea598645be5489f518f91e4b80a0f04a315fda Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Fri, 7 May 2021 12:07:59 -0500 Subject: [PATCH 10/10] Fix config file lookup --- clippy_lints/src/lib.rs | 11 ----------- clippy_lints/src/utils/conf.rs | 16 +++++++--------- 2 files changed, 7 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 5139b1464..b08a57a09 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -405,7 +405,6 @@ pub fn register_pre_expansion_lints(store: &mut rustc_lint::LintStore) { #[doc(hidden)] pub fn read_conf(sess: &Session) -> Conf { - use std::path::Path; let file_name = match utils::conf::lookup_conf_file() { Ok(Some(path)) => path, Ok(None) => return Conf::default(), @@ -416,16 +415,6 @@ pub fn read_conf(sess: &Session) -> Conf { }, }; - let file_name = if file_name.is_relative() { - sess.local_crate_source_file - .as_deref() - .and_then(Path::parent) - .unwrap_or_else(|| Path::new("")) - .join(file_name) - } else { - file_name - }; - let TryConf { conf, errors } = utils::conf::read(&file_name); // all conf errors are non-fatal, we just use the default conf in case of error for error in errors { diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index d65270a94..1bd38dc04 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -210,15 +210,13 @@ pub fn lookup_conf_file() -> io::Result> { .map_or_else(|| PathBuf::from("."), PathBuf::from); loop { for config_file_name in &CONFIG_FILE_NAMES { - let config_file = current.join(config_file_name); - match fs::metadata(&config_file) { - // Only return if it's a file to handle the unlikely situation of a directory named - // `clippy.toml`. - Ok(ref md) if !md.is_dir() => return Ok(Some(config_file)), - // Return the error if it's something other than `NotFound`; otherwise we didn't - // find the project file yet, and continue searching. - Err(e) if e.kind() != io::ErrorKind::NotFound => return Err(e), - _ => {}, + if let Ok(config_file) = current.join(config_file_name).canonicalize() { + match fs::metadata(&config_file) { + Err(e) if e.kind() == io::ErrorKind::NotFound => {}, + Err(e) => return Err(e), + Ok(md) if md.is_dir() => {}, + Ok(_) => return Ok(Some(config_file)), + } } }