allow multiple clippy.tomls and make them inherit from eachother

Centri3 · Centri3 · commit 560b6b7122f8 · 2023-06-11T14:11:00.000-05:00
diff --git a/clippy.toml b/clippy.toml
@@ -1 +1,2 @@
+# NOTE: Any changes here must be reversed in `tests/clippy.toml`
 avoid-breaking-exported-api = false
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -48,7 +48,7 @@ extern crate clippy_utils;
 #[macro_use]
 extern crate declare_clippy_lint;
 
-use std::io;
+use std::error::Error;
 use std::path::PathBuf;
 
 use clippy_utils::msrvs::Msrv;
@@ -337,7 +337,7 @@ mod zero_div_zero;
 mod zero_sized_map_values;
 // end lints modules, do not remove this comment, it’s used in `update_lints`
 
-pub use crate::utils::conf::{lookup_conf_file, Conf};
+pub use crate::utils::conf::{lookup_conf_files, Conf};
 use crate::utils::{
     conf::{metadata::get_configuration_metadata, TryConf},
     FindAll,
@@ -360,33 +360,39 @@ pub fn register_pre_expansion_lints(store: &mut rustc_lint::LintStore, sess: &Se
 }
 
 #[doc(hidden)]
-pub fn read_conf(sess: &Session, path: &io::Result<(Option<PathBuf>, Vec<String>)>) -> Conf {
+#[expect(clippy::type_complexity)]
+pub fn read_conf(sess: &Session, path: &Result<(Vec<PathBuf>, Vec<String>), Box<dyn Error + Send + Sync>>) -> Conf {
     if let Ok((_, warnings)) = path {
         for warning in warnings {
             sess.warn(warning.clone());
         }
     }
-    let file_name = match path {
-        Ok((Some(path), _)) => path,
-        Ok((None, _)) => return Conf::default(),
+    let file_names = match path {
+        Ok((file_names, _)) if file_names.is_empty() => return Conf::default(),
+        Ok((file_names, _)) => file_names,
         Err(error) => {
             sess.err(format!("error finding Clippy's configuration file: {error}"));
             return Conf::default();
         },
     };
 
-    let TryConf { conf, errors, warnings } = utils::conf::read(sess, file_name);
+    let TryConf { conf, errors, warnings } = utils::conf::read(sess, file_names);
     // all conf errors are non-fatal, we just use the default conf in case of error
     for error in errors {
         if let Some(span) = error.span {
             sess.span_err(
                 span,
                 format!("error reading Clippy's configuration file: {}", error.message),
             );
-        } else {
+        } else if let Some(file) = error.file {
             sess.err(format!(
                 "error reading Clippy's configuration file `{}`: {}",
-                file_name.display(),
+                file.display(),
+                error.message
+            ));
+        } else {
+            sess.err(format!(
+                "error reading any of Clippy's configuration files: {}",
                 error.message
             ));
         }
@@ -398,10 +404,15 @@ pub fn read_conf(sess: &Session, path: &io::Result<(Option<PathBuf>, Vec<String>
                 span,
                 format!("error reading Clippy's configuration file: {}", warning.message),
             );
-        } else {
-            sess.warn(format!(
+        } else if let Some(file) = warning.file {
+            sess.err(format!(
                 "error reading Clippy's configuration file `{}`: {}",
-                file_name.display(),
+                file.display(),
+                warning.message
+            ));
+        } else {
+            sess.err(format!(
+                "error reading any of Clippy's configuration files: {}",
                 warning.message
             ));
         }
diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs
@@ -3,12 +3,13 @@
 #![allow(clippy::module_name_repetitions)]
 
 use rustc_session::Session;
-use rustc_span::{BytePos, Pos, SourceFile, Span, SyntaxContext};
+use rustc_span::{BytePos, FileName, Pos, SourceFile, Span, SyntaxContext};
 use serde::de::{Deserializer, IgnoredAny, IntoDeserializer, MapAccess, Visitor};
 use serde::Deserialize;
+use std::error::Error;
 use std::fmt::{Debug, Display, Formatter};
 use std::ops::Range;
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 use std::str::FromStr;
 use std::{cmp, env, fmt, fs, io};
 
@@ -99,38 +100,50 @@ impl From<io::Error> for TryConf {
 #[derive(Debug)]
 pub struct ConfError {
     pub message: String,
+    pub file: Option<PathBuf>,
     pub span: Option<Span>,
 }
 
 impl ConfError {
     fn from_toml(file: &SourceFile, error: &toml::de::Error) -> Self {
         if let Some(span) = error.span() {
-            Self::spanned(file, error.message(), span)
-        } else {
-            Self {
-                message: error.message().to_string(),
-                span: None,
-            }
+            return Self::spanned(file, error.message(), span);
+        } else if let FileName::Real(filename) = &file.name
+            && let Some(filename) = filename.local_path()
+        {
+                return Self {
+                    message: error.message().to_string(),
+                    file: Some(filename.to_owned()),
+                    span: None,
+                };
         }
+
+        unreachable!();
     }
 
     fn spanned(file: &SourceFile, message: impl Into<String>, span: Range<usize>) -> Self {
-        Self {
-            message: message.into(),
-            span: Some(Span::new(
-                file.start_pos + BytePos::from_usize(span.start),
-                file.start_pos + BytePos::from_usize(span.end),
-                SyntaxContext::root(),
-                None,
-            )),
+        if let FileName::Real(filename) = &file.name && let Some(filename) = filename.local_path() {
+            return Self {
+                message: message.into(),
+                file: Some(filename.to_owned()),
+                span: Some(Span::new(
+                    file.start_pos + BytePos::from_usize(span.start),
+                    file.start_pos + BytePos::from_usize(span.end),
+                    SyntaxContext::root(),
+                    None,
+                )),
+            };
         }
+
+        unreachable!();
     }
 }
 
 impl From<io::Error> for ConfError {
     fn from(value: io::Error) -> Self {
         Self {
             message: value.to_string(),
+            file: None,
             span: None,
         }
     }
@@ -143,6 +156,7 @@ macro_rules! define_Conf {
         ($name:ident: $ty:ty = $default:expr),
     )*) => {
         /// Clippy lint configuration
+        #[derive(Deserialize)]
         pub struct Conf {
             $($(#[doc = $doc])+ pub $name: $ty,)*
         }
@@ -157,15 +171,15 @@ macro_rules! define_Conf {
             }
         }
 
+        #[allow(non_camel_case_types)]
         #[derive(Deserialize)]
         #[serde(field_identifier, rename_all = "kebab-case")]
-        #[allow(non_camel_case_types)]
         enum Field { $($name,)* third_party, }
 
-        struct ConfVisitor<'a>(&'a SourceFile);
+        struct ConfVisitor<'a>(&'a SourceFile, &'a mut TryConf);
 
         impl<'de> Visitor<'de> for ConfVisitor<'_> {
-            type Value = TryConf;
+            type Value = ();
 
             fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
                 formatter.write_str("Conf")
@@ -209,8 +223,14 @@ macro_rules! define_Conf {
                         Ok(Field::third_party) => drop(map.next_value::<IgnoredAny>())
                     }
                 }
-                let conf = Conf { $($name: $name.unwrap_or_else(defaults::$name),)* };
-                Ok(TryConf { conf, errors, warnings })
+                $(
+                    if let Some($name) = $name {
+                        self.1.conf.$name = $name;
+                    }
+                )*
+                self.1.errors.extend(errors);
+                self.1.warnings.extend(warnings);
+                Ok(())
             }
         }
 
@@ -524,12 +544,17 @@ define_Conf! {
     (allow_private_module_inception: bool = false),
 }
 
-/// Search for the configuration file.
+/// Search for any configuration files. The index corresponds to the priority; the higher the index,
+/// the lower the priority.
+///
+/// Note: It's up to the caller to reverse the priority of configuration files, otherwise the last
+/// configuration file will have the highest priority.
 ///
 /// # Errors
 ///
-/// Returns any unexpected filesystem error encountered when searching for the config file
-pub fn lookup_conf_file() -> io::Result<(Option<PathBuf>, Vec<String>)> {
+/// Returns any unexpected filesystem error encountered when searching for the config file or when
+/// running `cargo metadata`.
+pub fn lookup_conf_files() -> Result<(Vec<PathBuf>, Vec<String>), Box<dyn Error + Send + Sync>> {
     /// Possible filename to search for.
     const CONFIG_FILE_NAMES: [&str; 2] = [".clippy.toml", "clippy.toml"];
 
@@ -540,60 +565,68 @@ pub fn lookup_conf_file() -> io::Result<(Option<PathBuf>, Vec<String>)> {
         .map_or_else(|| PathBuf::from("."), PathBuf::from)
         .canonicalize()?;
 
-    let mut found_config: Option<PathBuf> = None;
+    let mut found_configs: Vec<PathBuf> = vec![];
     let mut warnings = vec![];
 
+    // TODO: This will continue searching even outside of the workspace, and even add an erroneous
+    // configuration file to the list! Is it worth fixing this? `workspace_root` on `cargo metadata`
+    // doesn't work for clippy_lints' clippy.toml in cwd. We likely can't just use cwd as what if
+    // it's called in src?
     loop {
         for config_file_name in &CONFIG_FILE_NAMES {
             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),
+                    Err(e) => return Err(e.into()),
                     Ok(md) if md.is_dir() => {},
                     Ok(_) => {
-                        // warn if we happen to find two config files #8323
-                        if let Some(ref found_config) = found_config {
+                        // Warn if we happen to find two config files #8323
+                        if let [.., last_config] = &*found_configs
+                            && let Some(last_config_dir) = last_config.parent()
+                            && let Some(config_file_dir) = config_file.parent()
+                            && last_config_dir == config_file_dir
+                        {
                             warnings.push(format!(
                                 "using config file `{}`, `{}` will be ignored",
-                                found_config.display(),
+                                last_config.display(),
                                 config_file.display()
                             ));
                         } else {
-                            found_config = Some(config_file);
+                            found_configs.push(config_file);
                         }
                     },
                 }
             }
         }
 
-        if found_config.is_some() {
-            return Ok((found_config, warnings));
-        }
-
-        // If the current directory has no parent, we're done searching.
         if !current.pop() {
-            return Ok((None, warnings));
+            break;
         }
     }
+
+    Ok((found_configs, warnings))
 }
 
 /// Read the `toml` configuration file.
 ///
 /// In case of error, the function tries to continue as much as possible.
-pub fn read(sess: &Session, path: &Path) -> TryConf {
-    let file = match sess.source_map().load_file(path) {
-        Err(e) => return e.into(),
-        Ok(file) => file,
-    };
-    match toml::de::Deserializer::new(file.src.as_ref().unwrap()).deserialize_map(ConfVisitor(&file)) {
-        Ok(mut conf) => {
-            extend_vec_if_indicator_present(&mut conf.conf.doc_valid_idents, DEFAULT_DOC_VALID_IDENTS);
-            extend_vec_if_indicator_present(&mut conf.conf.disallowed_names, DEFAULT_DISALLOWED_NAMES);
-
-            conf
-        },
-        Err(e) => TryConf::from_toml_error(&file, &e),
+pub fn read(sess: &Session, paths: &[PathBuf]) -> TryConf {
+    let mut conf = TryConf::default();
+    for file in paths.iter().rev() {
+        let file = match sess.source_map().load_file(file) {
+            Err(e) => return e.into(),
+            Ok(file) => file,
+        };
+        match toml::de::Deserializer::new(file.src.as_ref().unwrap()).deserialize_map(ConfVisitor(&file, &mut conf)) {
+            Ok(_) => {
+                extend_vec_if_indicator_present(&mut conf.conf.doc_valid_idents, DEFAULT_DOC_VALID_IDENTS);
+                extend_vec_if_indicator_present(&mut conf.conf.disallowed_names, DEFAULT_DISALLOWED_NAMES);
+            },
+            Err(e) => return TryConf::from_toml_error(&file, &e),
+        }
     }
+
+    conf
 }
 
 fn extend_vec_if_indicator_present(vec: &mut Vec<String>, default: &[&str]) {
diff --git a/src/driver.rs b/src/driver.rs
@@ -119,10 +119,9 @@ struct ClippyCallbacks {
 }
 
 impl rustc_driver::Callbacks for ClippyCallbacks {
-    // JUSTIFICATION: necessary in clippy driver to set `mir_opt_level`
-    #[allow(rustc::bad_opt_access)]
+    #[allow(rustc::bad_opt_access, reason = "necessary in clippy driver to set `mir_opt_level`")]
     fn config(&mut self, config: &mut interface::Config) {
-        let conf_path = clippy_lints::lookup_conf_file();
+        let conf_path = clippy_lints::lookup_conf_files();
         let previous = config.register_lints.take();
         let clippy_args_var = self.clippy_args_var.take();
         config.parse_sess_created = Some(Box::new(move |parse_sess| {
diff --git a/tests/clippy.toml b/tests/clippy.toml
@@ -1 +1,2 @@
 # default config for tests, overrides clippy.toml at the project root
+avoid-breaking-exported-api = true
diff --git a/tests/ui-cargo/inherit_config/clippy.toml b/tests/ui-cargo/inherit_config/clippy.toml
@@ -0,0 +1,2 @@
+too-many-lines-threshold = 1
+single-char-binding-names-threshold = 1
diff --git a/tests/ui-cargo/inherit_config/inner/Cargo.toml b/tests/ui-cargo/inherit_config/inner/Cargo.toml
@@ -0,0 +1,9 @@
+[package]
+name = "inherit-config"
+version = "0.1.0"
+edition = "2018"
+publish = false
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
diff --git a/tests/ui-cargo/inherit_config/inner/clippy.toml b/tests/ui-cargo/inherit_config/inner/clippy.toml
@@ -0,0 +1,2 @@
+too-many-lines-threshold = 3
+msrv = "1.1"
diff --git a/tests/ui-cargo/inherit_config/inner/src/main.rs b/tests/ui-cargo/inherit_config/inner/src/main.rs
@@ -0,0 +1,11 @@
+//@compile-flags: --crate-name=inherit_config
+#![warn(clippy::many_single_char_names, clippy::too_many_lines)]
+
+fn main() {
+    // Inherited from outer config
+    let (a, b, c) = (1, 2, 3);
+    _ = ();
+    _ = ();
+    _ = ();
+    // Too many lines, not 1 but 3 because of inner config
+}
diff --git a/tests/ui-cargo/inherit_config/inner/src/main.stderr b/tests/ui-cargo/inherit_config/inner/src/main.stderr

Original file line number	Diff line number	Diff line change
`@@ -1 +1,2 @@`
	`1`	+# NOTE: Any changes here must be reversed in `tests/clippy.toml`
`1`	`2`	`avoid-breaking-exported-api = false`
Original file line number	Diff line number	Diff line change
`@@ -1 +1,2 @@`
`1`	`1`	`# default config for tests, overrides clippy.toml at the project root`
	`2`	`+avoid-breaking-exported-api = true`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+too-many-lines-threshold = 1`
	`2`	`+single-char-binding-names-threshold = 1`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+too-many-lines-threshold = 3`
	`2`	`+msrv = "1.1"`