From b9c8e683d609d204fd1192e236a92099d94cbef2 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Tue, 27 Apr 2021 21:03:48 -0500 Subject: [PATCH 1/2] Disable default_trait_access in macros --- clippy_lints/src/default.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/default.rs b/clippy_lints/src/default.rs index 710da8fe9e03..7a53d390bb45 100644 --- a/clippy_lints/src/default.rs +++ b/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; From 1e22e564e4d272c296a6acb8596831fbaa05bc7b Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Tue, 27 Apr 2021 21:04:06 -0500 Subject: [PATCH 2/2] Refactor config deserialization --- clippy_lints/src/lib.rs | 4 +- clippy_lints/src/utils/conf.rs | 232 ++++++++---------- .../bad_toml_type/conf_bad_type.stderr | 2 +- .../conf_deprecated_key.stderr | 2 +- 4 files changed, 108 insertions(+), 132 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 57843afaddbc..40a793e48cf0 100644 --- a/clippy_lints/src/lib.rs +++ b/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!( diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index e221be635381..747d92dd7423 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/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, } -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 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>> = 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(deserializer: D) -> Result 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(self, mut map: V) -> Result 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::()) + } } - )+ + 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, None), + (msrv: Option), /// 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, ["foo", "baz", "quux"].iter().map(ToString::to_string).collect()), + (blacklisted_names: Vec = ["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, None), + #[conf_deprecated("Please use `cognitive-complexity-threshold` instead.")] + (cyclomatic_complexity_threshold: Option), /// Lint: DOC_MARKDOWN. The list of words this lint should not consider as identifiers needing ticks - (doc_valid_idents, "doc_valid_idents": Vec, [ + (doc_valid_idents: Vec = [ "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, None), + (trivial_copy_size_limit: Option), /// 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, Vec::::new()), + (disallowed_methods: Vec), /// 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> { } } -/// Produces a `Conf` filled with the default values and forwards the errors -/// -/// Used internally for convenience -fn default(errors: Vec) -> (Conf, Vec) { - (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) { +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 = 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) } diff --git a/tests/ui-toml/bad_toml_type/conf_bad_type.stderr b/tests/ui-toml/bad_toml_type/conf_bad_type.stderr index efd02bcbb6e2..c7bc261de6c5 100644 --- a/tests/ui-toml/bad_toml_type/conf_bad_type.stderr +++ b/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 diff --git a/tests/ui-toml/conf_deprecated_key/conf_deprecated_key.stderr b/tests/ui-toml/conf_deprecated_key/conf_deprecated_key.stderr index 34267c0daf7c..8bf9fe64c6d7 100644 --- a/tests/ui-toml/conf_deprecated_key/conf_deprecated_key.stderr +++ b/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