Auto merge of #7187 - camsteffen:avoid-break-exported, r=flip1995,phansch

Add avoid_breaking_exported_api config option

changelog: Add `avoid_breaking_exported_api` config option for [`enum_variant_names`], [`large_types_passed_by_value`], [`trivially_copy_pass_by_ref`], [`unnecessary_wraps`], [`upper_case_acronyms`] and [`wrong_self_convention`].

changelog: Deprecates [`pub_enum_variant_names`] and [`wrong_pub_self_convention`] as the non-pub variants are now configurable.

changelog: Fix various false negatives for `pub` items that are not exported from the crate.

A couple changes to late passes in order to use `cx.access_levels.is_exported` rather than `item.vis.kind.is_pub`.

I'm not sure how to better document the config option or lints that are (not) affected (see comments in #6806). Suggestions are welcome. cc `@rust-lang/clippy`

I added `/clippy.toml` to use the config internally and `/tests/clippy.toml` to maintain a default config in ui tests.

Closes #6806
Closes #4504

Author: bors

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
 ```

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()
     );
 }

clippy.toml

@@ -0,0 +1 @@
+avoid-breaking-exported-api = false

clippy_lints/src/deprecated_lints.rs

@@ -141,3 +141,22 @@ 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 `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.
+    ///
+    /// **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"
+}

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));
     }

clippy_lints/src/lib.rs

@@ -393,7 +393,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(),
@@ -404,16 +403,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 {
@@ -493,6 +482,14 @@ 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",
+    );
     // 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`
@@ -606,7 +603,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,
@@ -790,7 +786,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,
@@ -1014,7 +1009,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),
@@ -1066,7 +1060,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),
@@ -1850,7 +1843,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));
@@ -1932,6 +1926,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);
@@ -1958,7 +1953,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);
@@ -1999,10 +1994,10 @@ 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));
+    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);