diff --git a/clippy.toml b/clippy.toml index cda8d17eed44..016e9caefeef 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1 +1,3 @@ +# NOTE: Any changes here must be reversed in `tests/clippy.toml`, expect `workspace` +workspace = true avoid-breaking-exported-api = false diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index dcf1c6f64a4b..02a360c6a837 100644 --- 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; @@ -339,7 +339,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, @@ -362,22 +362,23 @@ 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, Vec)>) -> Conf { +#[expect(clippy::type_complexity)] +pub fn read_conf(sess: &Session, path: &Result<(Vec, Vec), Box>) -> 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 { @@ -385,10 +386,15 @@ pub fn read_conf(sess: &Session, path: &io::Result<(Option, Vec 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 )); } @@ -400,10 +406,15 @@ pub fn read_conf(sess: &Session, path: &io::Result<(Option, Vec 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 index 0a581e0ab9b1..a0b9a72ae3e8 100644 --- 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}; @@ -100,31 +101,42 @@ impl From for TryConf { #[derive(Debug)] pub struct ConfError { pub message: String, + pub file: Option, pub span: Option, } 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, span: Range) -> 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!(); } } @@ -132,6 +144,7 @@ impl From for ConfError { fn from(value: io::Error) -> Self { Self { message: value.to_string(), + file: None, span: None, } } @@ -144,6 +157,7 @@ macro_rules! define_Conf { ($name:ident: $ty:ty = $default:expr), )*) => { /// Clippy lint configuration + #[derive(Deserialize)] pub struct Conf { $($(#[doc = $doc])+ pub $name: $ty,)* } @@ -158,15 +172,20 @@ 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, } + enum Field { $($name,)* third_party, workspace } - struct ConfVisitor<'a>(&'a SourceFile); + struct ConfVisitor<'a> { + file: &'a SourceFile, + conf: &'a mut TryConf, + priority: usize, + workspace: &'a mut bool, + } impl<'de> Visitor<'de> for ConfVisitor<'_> { - type Value = TryConf; + type Value = (); fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { formatter.write_str("Conf") @@ -181,22 +200,22 @@ macro_rules! define_Conf { match Field::deserialize(name.get_ref().as_str().into_deserializer()) { Err(e) => { let e: FieldError = e; - errors.push(ConfError::spanned(self.0, e.0, name.span())); + errors.push(ConfError::spanned(self.file, e.0, name.span())); } $(Ok(Field::$name) => { - $(warnings.push(ConfError::spanned(self.0, format!("deprecated field `{}`. {}", name.get_ref(), $dep), name.span()));)? + $(warnings.push(ConfError::spanned(self.file, format!("deprecated field `{}`. {}", name.get_ref(), $dep), name.span()));)? let raw_value = map.next_value::>()?; let value_span = raw_value.span(); match <$ty>::deserialize(raw_value.into_inner()) { - Err(e) => errors.push(ConfError::spanned(self.0, e.to_string().replace('\n', " ").trim(), value_span)), + Err(e) => errors.push(ConfError::spanned(self.file, e.to_string().replace('\n', " ").trim(), value_span)), Ok(value) => match $name { - Some(_) => errors.push(ConfError::spanned(self.0, format!("duplicate field `{}`", name.get_ref()), name.span())), + Some(_) => errors.push(ConfError::spanned(self.file, format!("duplicate field `{}`", name.get_ref()), name.span())), None => { $name = Some(value); // $new_conf is the same as one of the defined `$name`s, so // this variable is defined in line 2 of this function. $(match $new_conf { - Some(_) => errors.push(ConfError::spanned(self.0, concat!( + Some(_) => errors.push(ConfError::spanned(self.file, concat!( "duplicate field `", stringify!($new_conf), "` (provided as `", stringify!($name), "`)" ), name.span())), @@ -206,12 +225,26 @@ macro_rules! define_Conf { } } })* + // Opt into config inheritence, only allowed on root config + Ok(Field::workspace) => { + if self.priority == 0 { + *self.workspace = true; + } else { + errors.push(ConfError::spanned(self.file, "use of `workspace` field in non-root config", name.span())); + } + }, // ignore contents of the third_party key Ok(Field::third_party) => drop(map.next_value::()) } } - let conf = Conf { $($name: $name.unwrap_or_else(defaults::$name),)* }; - Ok(TryConf { conf, errors, warnings }) + $( + if let Some($name) = $name { + self.conf.conf.$name = $name; + } + )* + self.conf.errors.extend(errors); + self.conf.warnings.extend(warnings); + Ok(()) } } @@ -536,12 +569,17 @@ define_Conf! { (min_ident_chars_threshold: u64 = 1), } -/// 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, Vec)> { +/// Returns any unexpected filesystem error encountered when searching for the config file or when +/// running `cargo metadata`. +pub fn lookup_conf_files() -> Result<(Vec, Vec), Box> { /// Possible filename to search for. const CONFIG_FILE_NAMES: [&str; 2] = [".clippy.toml", "clippy.toml"]; @@ -552,66 +590,84 @@ pub fn lookup_conf_file() -> io::Result<(Option, Vec)> { .map_or_else(|| PathBuf::from("."), PathBuf::from) .canonicalize()?; - let mut found_config: Option = None; + let mut found_configs: Vec = 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); - // TODO: THIS SHOULD BE TESTED, this comment will be gone soon - if conf.conf.allowed_idents_below_min_chars.contains(&"..".to_owned()) { - conf.conf - .allowed_idents_below_min_chars - .extend(DEFAULT_ALLOWED_IDENTS_BELOW_MIN_CHARS.iter().map(ToString::to_string)); - } +pub fn read(sess: &Session, paths: &[PathBuf]) -> TryConf { + let mut conf = TryConf::default(); + let mut workspace = false; + for (priority, file) in paths.iter().rev().enumerate() { + 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: &file, + conf: &mut conf, + priority, + workspace: &mut workspace, + }) { + 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); + if conf.conf.allowed_idents_below_min_chars.contains(&"..".to_owned()) { + conf.conf + .allowed_idents_below_min_chars + .extend(DEFAULT_ALLOWED_IDENTS_BELOW_MIN_CHARS.iter().map(ToString::to_string)); + } + }, + Err(e) => return TryConf::from_toml_error(&file, &e), + } - conf - }, - Err(e) => TryConf::from_toml_error(&file, &e), + // Retain old functionality if not opted into + if priority == 0 && !workspace { + break; + } } + + conf } fn extend_vec_if_indicator_present(vec: &mut Vec, default: &[&str]) { diff --git a/src/driver.rs b/src/driver.rs index 3c5b6e12b968..158cbbf31725 100644 --- 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 index 5eb7ac035419..aa0b6fecb8de 100644 --- 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 new file mode 100644 index 000000000000..e9a3e22da07d --- /dev/null +++ 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 new file mode 100644 index 000000000000..6edf64528ea8 --- /dev/null +++ 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 new file mode 100644 index 000000000000..96ad6203c4b7 --- /dev/null +++ b/tests/ui-cargo/inherit_config/inner/clippy.toml @@ -0,0 +1,4 @@ +too-many-lines-threshold = 3 +msrv = "1.1" +# Will emit an error if uncommented, and revert to default config +# workspace = true diff --git a/tests/ui-cargo/inherit_config/inner/src/main.rs b/tests/ui-cargo/inherit_config/inner/src/main.rs new file mode 100644 index 000000000000..665036d998c6 --- /dev/null +++ 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 new file mode 100644 index 000000000000..26842d9459eb --- /dev/null +++ b/tests/ui-cargo/inherit_config/inner/src/main.stderr @@ -0,0 +1,44 @@ +error: 3 bindings with single-character names in scope + --> $DIR/main.rs:6:10 + | +LL | let (a, b, c) = (1, 2, 3); + | ^ ^ ^ + | + = note: `-D clippy::many-single-char-names` implied by `-D warnings` + +error: this function has too many lines (4/3) + --> $DIR/main.rs:4:1 + | +LL | / fn main() { +LL | | // Inherited from outer config +LL | | let (a, b, c) = (1, 2, 3); +LL | | _ = (); +... | +LL | | // Too many lines, not 1 but 3 because of inner config +LL | | } + | |_^ + | + = note: `-D clippy::too-many-lines` implied by `-D warnings` + +error: this let-binding has unit value + --> $DIR/main.rs:7:5 + | +LL | _ = (); + | ^^^^^^ help: omit the `let` binding: `();` + | + = note: `-D clippy::let-unit-value` implied by `-D warnings` + +error: this let-binding has unit value + --> $DIR/main.rs:8:5 + | +LL | _ = (); + | ^^^^^^ help: omit the `let` binding: `();` + +error: this let-binding has unit value + --> $DIR/main.rs:9:5 + | +LL | _ = (); + | ^^^^^^ help: omit the `let` binding: `();` + +error: aborting due to 5 previous errors + 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 c546d95eb464..74e825b4962e 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -58,6 +58,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect vec-box-size-threshold verbose-bit-mask-threshold warn-on-all-wildcard-imports + workspace --> $DIR/clippy.toml:2:1 | LL | foobar = 42 @@ -123,6 +124,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect vec-box-size-threshold verbose-bit-mask-threshold warn-on-all-wildcard-imports + workspace --> $DIR/clippy.toml:4:1 | LL | barfoo = 53