Auto merge of #7150 - camsteffen:better-conf, r=llogiq

`Conf` macro improvements

changelog: Allow `default_trait_access` in macros

Mainly this is a change to use serde as in [Manually implementing Deserialize for a struct](https://serde.rs/deserialize-struct.html), which opens the door for a cleaner implementation overall.

* Allow `default_trait_access` in macros (tangential, but used in this PR)
* Deserialize into `TryConf { conf, errors }` instead of using a global `ERRORS` variable.
* Improve the `define_Conf!` macro
	* Remove the redundant string literal `(name, "name", ..)`
	* Support deprecated configs with `#[conf_deprecated(message)]`. Message shows in error.
	* Make the default value optional. Use `Default::default()` if omitted.
* Invalid config value error now shows the key (see test output)
* Cleaner `impl Default for Conf` (no `toml::from_str("")`)

clippy_lints/src/default.rs

@@ -1,6 +1,6 @@
 use clippy_utils::diagnostics::{span_lint_and_note, span_lint_and_sugg};
 use clippy_utils::source::snippet_with_macro_callsite;
-use clippy_utils::{any_parent_is_automatically_derived, contains_name, match_def_path, paths};
+use clippy_utils::{any_parent_is_automatically_derived, contains_name, in_macro, match_def_path, paths};
 use if_chain::if_chain;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::Applicability;
@@ -75,6 +75,7 @@ impl_lint_pass!(Default => [DEFAULT_TRAIT_ACCESS, FIELD_REASSIGN_WITH_DEFAULT]);
 impl LateLintPass<'_> for Default {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         if_chain! {
+            if !in_macro(expr.span);
             // Avoid cases already linted by `field_reassign_with_default`
             if !self.reassigned_linted.contains(&expr.span);
             if let ExprKind::Call(path, ..) = expr.kind;

clippy_lints/src/lib.rs

@@ -382,6 +382,7 @@ mod zero_sized_map_values;
 // end lints modules, do not remove this comment, it’s used in `update_lints`
 
 pub use crate::utils::conf::Conf;
+use crate::utils::conf::TryConf;
 
 /// Register all pre expansion lints
 ///
@@ -421,8 +422,7 @@ pub fn read_conf(sess: &Session) -> Conf {
         file_name
     };
 
-    let (conf, errors) = utils::conf::read(&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 {
         sess.struct_err(&format!(

clippy_lints/src/utils/conf.rs

@@ -1,98 +1,111 @@
 //! Read configurations files.
 
-#![deny(clippy::missing_docs_in_private_items)]
+#![allow(clippy::module_name_repetitions)]
 
-use std::lazy::SyncLazy;
+use serde::de::{Deserializer, IgnoredAny, IntoDeserializer, MapAccess, Visitor};
+use serde::Deserialize;
+use std::error::Error;
 use std::path::{Path, PathBuf};
-use std::sync::Mutex;
 use std::{env, fmt, fs, io};
 
-/// Error from reading a configuration file.
-#[derive(Debug)]
-pub enum Error {
-    /// An I/O error.
-    Io(io::Error),
-    /// Not valid toml or doesn't fit the expected config format
-    Toml(String),
+/// Conf with parse errors
+#[derive(Default)]
+pub struct TryConf {
+    pub conf: Conf,
+    pub errors: Vec<String>,
 }
 
-impl fmt::Display for Error {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            Self::Io(err) => err.fmt(f),
-            Self::Toml(err) => err.fmt(f),
+impl TryConf {
+    fn from_error(error: impl Error) -> Self {
+        Self {
+            conf: Conf::default(),
+            errors: vec![error.to_string()],
         }
     }
 }
 
-impl From<io::Error> for Error {
-    fn from(e: io::Error) -> Self {
-        Self::Io(e)
-    }
-}
+macro_rules! define_Conf {
+    ($(
+        #[$doc:meta]
+        $(#[conf_deprecated($dep:literal)])?
+        ($name:ident: $ty:ty $(= $default:expr)?),
+    )*) => {
+        /// Clippy lint configuration
+        pub struct Conf {
+            $(#[$doc] pub $name: $ty,)*
+        }
 
-/// Vec of errors that might be collected during config toml parsing
-static ERRORS: SyncLazy<Mutex<Vec<Error>>> = SyncLazy::new(|| Mutex::new(Vec::new()));
+        mod defaults {
+            $(pub fn $name() -> $ty { define_Conf!(@default $($default)?) })*
+        }
 
-macro_rules! define_Conf {
-    ($(#[$doc:meta] ($config:ident, $config_str:literal: $Ty:ty, $default:expr),)+) => {
-        mod helpers {
-            use serde::Deserialize;
-            /// Type used to store lint configuration.
-            #[derive(Deserialize)]
-            #[serde(rename_all = "kebab-case", deny_unknown_fields)]
-            pub struct Conf {
-                $(
-                    #[$doc]
-                    #[serde(default = $config_str)]
-                    #[serde(with = $config_str)]
-                    pub $config: $Ty,
-                )+
-                #[allow(dead_code)]
-                #[serde(default)]
-                third_party: Option<::toml::Value>,
+        impl Default for Conf {
+            fn default() -> Self {
+                Self { $($name: defaults::$name(),)* }
             }
+        }
 
-            $(
-                mod $config {
-                    use serde::Deserialize;
-                    pub fn deserialize<'de, D: serde::Deserializer<'de>>(deserializer: D) -> Result<$Ty, D::Error> {
-                        use super::super::{ERRORS, Error};
-
-                        Ok(
-                            <$Ty>::deserialize(deserializer).unwrap_or_else(|e| {
-                                ERRORS
-                                    .lock()
-                                    .expect("no threading here")
-                                    .push(Error::Toml(e.to_string()));
-                                super::$config()
-                            })
-                        )
-                    }
-                }
+        impl<'de> Deserialize<'de> for TryConf {
+            fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> where D: Deserializer<'de> {
+                deserializer.deserialize_map(ConfVisitor)
+            }
+        }
+
+        #[derive(Deserialize)]
+        #[serde(field_identifier, rename_all = "kebab-case")]
+        #[allow(non_camel_case_types)]
+        enum Field { $($name,)* third_party, }
+
+        struct ConfVisitor;
+
+        impl<'de> Visitor<'de> for ConfVisitor {
+            type Value = TryConf;
+
+            fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
+                formatter.write_str("Conf")
+            }
 
-                #[must_use]
-                fn $config() -> $Ty {
-                    let x = $default;
-                    x
+            fn visit_map<V>(self, mut map: V) -> Result<Self::Value, V::Error> where V: MapAccess<'de> {
+                let mut errors = Vec::new();
+                $(let mut $name = None;)*
+                // could get `Field` here directly, but get `str` first for diagnostics
+                while let Some(name) = map.next_key::<&str>()? {
+                    match Field::deserialize(name.into_deserializer())? {
+                        $(Field::$name => {
+                            $(errors.push(format!("deprecated field `{}`. {}", name, $dep));)?
+                            match map.next_value() {
+                                Err(e) => errors.push(e.to_string()),
+                                Ok(value) => match $name {
+                                    Some(_) => errors.push(format!("duplicate field `{}`", name)),
+                                    None => $name = Some(value),
+                                }
+                            }
+                        })*
+                        // white-listed; ignore
+                        Field::third_party => drop(map.next_value::<IgnoredAny>())
+                    }
                 }
-            )+
+                let conf = Conf { $($name: $name.unwrap_or_else(defaults::$name),)* };
+                Ok(TryConf { conf, errors })
+            }
         }
     };
+    (@default) => (Default::default());
+    (@default $default:expr) => ($default);
 }
 
-pub use self::helpers::Conf;
 define_Conf! {
     /// 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. The minimum rust version that the project supports
-    (msrv, "msrv": Option<String>, None),
+    (msrv: Option<String>),
     /// Lint: BLACKLISTED_NAME. The list of blacklisted names to lint about. NB: `bar` is not here since it has legitimate uses
-    (blacklisted_names, "blacklisted_names": Vec<String>, ["foo", "baz", "quux"].iter().map(ToString::to_string).collect()),
+    (blacklisted_names: Vec<String> = ["foo", "baz", "quux"].iter().map(ToString::to_string).collect()),
     /// Lint: COGNITIVE_COMPLEXITY. The maximum cognitive complexity a function can have
-    (cognitive_complexity_threshold, "cognitive_complexity_threshold": u64, 25),
+    (cognitive_complexity_threshold: u64 = 25),
     /// DEPRECATED LINT: CYCLOMATIC_COMPLEXITY. Use the Cognitive Complexity lint instead.
-    (cyclomatic_complexity_threshold, "cyclomatic_complexity_threshold": Option<u64>, None),
+    #[conf_deprecated("Please use `cognitive-complexity-threshold` instead.")]
+    (cyclomatic_complexity_threshold: Option<u64>),
     /// Lint: DOC_MARKDOWN. The list of words this lint should not consider as identifiers needing ticks
-    (doc_valid_idents, "doc_valid_idents": Vec<String>, [
+    (doc_valid_idents: Vec<String> = [
         "KiB", "MiB", "GiB", "TiB", "PiB", "EiB",
         "DirectX",
         "ECMAScript",
@@ -113,54 +126,47 @@ define_Conf! {
         "CamelCase",
     ].iter().map(ToString::to_string).collect()),
     /// Lint: TOO_MANY_ARGUMENTS. The maximum number of argument a function or method can have
-    (too_many_arguments_threshold, "too_many_arguments_threshold": u64, 7),
+    (too_many_arguments_threshold: u64 = 7),
     /// Lint: TYPE_COMPLEXITY. The maximum complexity a type can have
-    (type_complexity_threshold, "type_complexity_threshold": u64, 250),
+    (type_complexity_threshold: u64 = 250),
     /// Lint: MANY_SINGLE_CHAR_NAMES. The maximum number of single char bindings a scope may have
-    (single_char_binding_names_threshold, "single_char_binding_names_threshold": u64, 4),
+    (single_char_binding_names_threshold: u64 = 4),
     /// Lint: BOXED_LOCAL, USELESS_VEC. The maximum size of objects (in bytes) that will be linted. Larger objects are ok on the heap
-    (too_large_for_stack, "too_large_for_stack": u64, 200),
+    (too_large_for_stack: u64 = 200),
     /// Lint: ENUM_VARIANT_NAMES. The minimum number of enum variants for the lints about variant names to trigger
-    (enum_variant_name_threshold, "enum_variant_name_threshold": u64, 3),
+    (enum_variant_name_threshold: u64 = 3),
     /// Lint: LARGE_ENUM_VARIANT. The maximum size of a enum's variant to avoid box suggestion
-    (enum_variant_size_threshold, "enum_variant_size_threshold": u64, 200),
+    (enum_variant_size_threshold: u64 = 200),
     /// Lint: VERBOSE_BIT_MASK. The maximum allowed size of a bit mask before suggesting to use 'trailing_zeros'
-    (verbose_bit_mask_threshold, "verbose_bit_mask_threshold": u64, 1),
+    (verbose_bit_mask_threshold: u64 = 1),
     /// Lint: DECIMAL_LITERAL_REPRESENTATION. The lower bound for linting decimal literals
-    (literal_representation_threshold, "literal_representation_threshold": u64, 16384),
+    (literal_representation_threshold: u64 = 16384),
     /// Lint: TRIVIALLY_COPY_PASS_BY_REF. The maximum size (in bytes) to consider a `Copy` type for passing by value instead of by reference.
-    (trivial_copy_size_limit, "trivial_copy_size_limit": Option<u64>, None),
+    (trivial_copy_size_limit: Option<u64>),
     /// Lint: LARGE_TYPE_PASS_BY_MOVE. The minimum size (in bytes) to consider a type for passing by reference instead of by value.
-    (pass_by_value_size_limit, "pass_by_value_size_limit": u64, 256),
+    (pass_by_value_size_limit: u64 = 256),
     /// Lint: TOO_MANY_LINES. The maximum number of lines a function or method can have
-    (too_many_lines_threshold, "too_many_lines_threshold": u64, 100),
+    (too_many_lines_threshold: u64 = 100),
     /// Lint: LARGE_STACK_ARRAYS, LARGE_CONST_ARRAYS. The maximum allowed size for arrays on the stack
-    (array_size_threshold, "array_size_threshold": u64, 512_000),
+    (array_size_threshold: u64 = 512_000),
     /// Lint: VEC_BOX. The size of the boxed type in bytes, where boxing in a `Vec` is allowed
-    (vec_box_size_threshold, "vec_box_size_threshold": u64, 4096),
+    (vec_box_size_threshold: u64 = 4096),
     /// Lint: TYPE_REPETITION_IN_BOUNDS. The maximum number of bounds a trait can have to be linted
-    (max_trait_bounds, "max_trait_bounds": u64, 3),
+    (max_trait_bounds: u64 = 3),
     /// Lint: STRUCT_EXCESSIVE_BOOLS. The maximum number of bools a struct can have
-    (max_struct_bools, "max_struct_bools": u64, 3),
+    (max_struct_bools: u64 = 3),
     /// Lint: FN_PARAMS_EXCESSIVE_BOOLS. The maximum number of bools function parameters can have
-    (max_fn_params_bools, "max_fn_params_bools": u64, 3),
+    (max_fn_params_bools: u64 = 3),
     /// Lint: WILDCARD_IMPORTS. Whether to allow certain wildcard imports (prelude, super in tests).
-    (warn_on_all_wildcard_imports, "warn_on_all_wildcard_imports": bool, false),
+    (warn_on_all_wildcard_imports: bool),
     /// Lint: DISALLOWED_METHOD. The list of disallowed methods, written as fully qualified paths.
-    (disallowed_methods, "disallowed_methods": Vec<String>, Vec::<String>::new()),
+    (disallowed_methods: Vec<String>),
     /// Lint: UNREADABLE_LITERAL. Should the fraction of a decimal be linted to include separators.
-    (unreadable_literal_lint_fractions, "unreadable_literal_lint_fractions": bool, true),
+    (unreadable_literal_lint_fractions: bool = true),
     /// 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),
+    (upper_case_acronyms_aggressive: bool),
     /// 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),
-}
-
-impl Default for Conf {
-    #[must_use]
-    fn default() -> Self {
-        toml::from_str("").expect("we never error on empty config files")
-    }
+    (cargo_ignore_publish: bool),
 }
 
 /// Search for the configuration file.
@@ -194,43 +200,13 @@ pub fn lookup_conf_file() -> io::Result<Option<PathBuf>> {
     }
 }
 
-/// Produces a `Conf` filled with the default values and forwards the errors
-///
-/// Used internally for convenience
-fn default(errors: Vec<Error>) -> (Conf, Vec<Error>) {
-    (Conf::default(), errors)
-}
-
 /// Read the `toml` configuration file.
 ///
 /// In case of error, the function tries to continue as much as possible.
-pub fn read(path: &Path) -> (Conf, Vec<Error>) {
+pub fn read(path: &Path) -> TryConf {
     let content = match fs::read_to_string(path) {
+        Err(e) => return TryConf::from_error(e),
         Ok(content) => content,
-        Err(err) => return default(vec![err.into()]),
     };
-
-    assert!(ERRORS.lock().expect("no threading -> mutex always safe").is_empty());
-    match toml::from_str(&content) {
-        Ok(toml) => {
-            let mut errors = ERRORS.lock().expect("no threading -> mutex always safe").split_off(0);
-
-            let toml_ref: &Conf = &toml;
-
-            let cyc_field: Option<u64> = toml_ref.cyclomatic_complexity_threshold;
-
-            if cyc_field.is_some() {
-                let cyc_err = "found deprecated field `cyclomatic-complexity-threshold`. Please use `cognitive-complexity-threshold` instead.".to_string();
-                errors.push(Error::Toml(cyc_err));
-            }
-
-            (toml, errors)
-        },
-        Err(e) => {
-            let mut errors = ERRORS.lock().expect("no threading -> mutex always safe").split_off(0);
-            errors.push(Error::Toml(e.to_string()));
-
-            default(errors)
-        },
-    }
+    toml::from_str(&content).unwrap_or_else(TryConf::from_error)
 }

tests/ui-toml/bad_toml_type/conf_bad_type.stderr

@@ -1,4 +1,4 @@
-error: error reading Clippy's configuration file `$DIR/clippy.toml`: invalid type: integer `42`, expected a sequence
+error: error reading Clippy's configuration file `$DIR/clippy.toml`: invalid type: integer `42`, expected a sequence for key `blacklisted-names`
 
 error: aborting due to previous error
 

tests/ui-toml/conf_deprecated_key/conf_deprecated_key.stderr

@@ -1,4 +1,4 @@
-error: error reading Clippy's configuration file `$DIR/clippy.toml`: found deprecated field `cyclomatic-complexity-threshold`. Please use `cognitive-complexity-threshold` instead.
+error: error reading Clippy's configuration file `$DIR/clippy.toml`: deprecated field `cyclomatic-complexity-threshold`. Please use `cognitive-complexity-threshold` instead.
 
 error: aborting due to previous error