Auto merge of rust-lang#10607 - beetrees:toml-spans, r=giraffate

Add spans to `clippy.toml` error messages

Adds spans to errors and warnings encountered when parsing `clippy.toml`.

changelog: Errors and warnings generated when parsing `clippy.toml` now point to the location in the TOML file the error/warning occurred.

Author: bors

Cargo.toml

@@ -30,7 +30,7 @@ termize = "0.1"
 compiletest_rs = { version = "0.10", features = ["tmp"] }
 tester = "0.9"
 regex = "1.5"
-toml = "0.5"
+toml = "0.7.3"
 walkdir = "2.3"
 # This is used by the `collect-metadata` alias.
 filetime = "0.2"

clippy_lints/Cargo.toml

@@ -21,7 +21,7 @@ regex-syntax = "0.7"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = { version = "1.0", optional = true }
 tempfile = { version = "3.3.0", optional = true }
-toml = "0.5"
+toml = "0.7.3"
 unicode-normalization = "0.1"
 unicode-script = { version = "0.5", default-features = false }
 semver = "1.0"

clippy_lints/src/lib.rs

@@ -336,7 +336,7 @@ mod zero_sized_map_values;
 
 pub use crate::utils::conf::{lookup_conf_file, Conf};
 use crate::utils::{
-    conf::{format_error, metadata::get_configuration_metadata, TryConf},
+    conf::{metadata::get_configuration_metadata, TryConf},
     FindAll,
 };
 
@@ -372,23 +372,36 @@ pub fn read_conf(sess: &Session, path: &io::Result<(Option<PathBuf>, Vec<String>
         },
     };
 
-    let TryConf { conf, errors, warnings } = utils::conf::read(file_name);
+    let TryConf { conf, errors, warnings } = utils::conf::read(sess, file_name);
     // all conf errors are non-fatal, we just use the default conf in case of error
     for error in errors {
-        sess.err(format!(
-            "error reading Clippy's configuration file `{}`: {}",
-            file_name.display(),
-            format_error(error)
-        ));
+        if let Some(span) = error.span {
+            sess.span_err(
+                span,
+                format!("error reading Clippy's configuration file: {}", error.message),
+            );
+        } else {
+            sess.err(format!(
+                "error reading Clippy's configuration file `{}`: {}",
+                file_name.display(),
+                error.message
+            ));
+        }
     }
 
     for warning in warnings {
-        sess.struct_warn(format!(
-            "error reading Clippy's configuration file `{}`: {}",
-            file_name.display(),
-            format_error(warning)
-        ))
-        .emit();
+        if let Some(span) = warning.span {
+            sess.span_warn(
+                span,
+                format!("error reading Clippy's configuration file: {}", warning.message),
+            );
+        } else {
+            sess.warn(format!(
+                "error reading Clippy's configuration file `{}`: {}",
+                file_name.display(),
+                warning.message
+            ));
+        }
     }
 
     conf

clippy_lints/src/nonstandard_macro_braces.rs

@@ -241,7 +241,7 @@ impl<'de> Deserialize<'de> for MacroMatcher {
                 V: de::MapAccess<'de>,
             {
                 let mut name = None;
-                let mut brace: Option<&str> = None;
+                let mut brace: Option<String> = None;
                 while let Some(key) = map.next_key()? {
                     match key {
                         Field::Name => {

clippy_lints/src/utils/conf.rs

@@ -2,12 +2,15 @@
 
 #![allow(clippy::module_name_repetitions)]
 
+use rustc_session::Session;
+use rustc_span::{BytePos, 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::str::FromStr;
-use std::{cmp, env, fmt, fs, io, iter};
+use std::{cmp, env, fmt, fs, io};
 
 #[rustfmt::skip]
 const DEFAULT_DOC_VALID_IDENTS: &[&str] = &[
@@ -67,33 +70,70 @@ impl DisallowedPath {
 #[derive(Default)]
 pub struct TryConf {
     pub conf: Conf,
-    pub errors: Vec<Box<dyn Error>>,
-    pub warnings: Vec<Box<dyn Error>>,
+    pub errors: Vec<ConfError>,
+    pub warnings: Vec<ConfError>,
 }
 
 impl TryConf {
-    fn from_error(error: impl Error + 'static) -> Self {
+    fn from_toml_error(file: &SourceFile, error: &toml::de::Error) -> Self {
+        ConfError::from_toml(file, error).into()
+    }
+}
+
+impl From<ConfError> for TryConf {
+    fn from(value: ConfError) -> Self {
         Self {
             conf: Conf::default(),
-            errors: vec![Box::new(error)],
+            errors: vec![value],
             warnings: vec![],
         }
     }
 }
 
+impl From<io::Error> for TryConf {
+    fn from(value: io::Error) -> Self {
+        ConfError::from(value).into()
+    }
+}
+
 #[derive(Debug)]
-struct ConfError(String);
+pub struct ConfError {
+    pub message: String,
+    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,
+            }
+        }
+    }
 
-impl fmt::Display for ConfError {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        <String as fmt::Display>::fmt(&self.0, f)
+    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,
+            )),
+        }
     }
 }
 
-impl Error for ConfError {}
-
-fn conf_error(s: impl Into<String>) -> Box<dyn Error> {
-    Box::new(ConfError(s.into()))
+impl From<io::Error> for ConfError {
+    fn from(value: io::Error) -> Self {
+        Self {
+            message: value.to_string(),
+            span: None,
+        }
+    }
 }
 
 macro_rules! define_Conf {
@@ -117,20 +157,14 @@ macro_rules! define_Conf {
             }
         }
 
-        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;
+        struct ConfVisitor<'a>(&'a SourceFile);
 
-        impl<'de> Visitor<'de> for ConfVisitor {
+        impl<'de> Visitor<'de> for ConfVisitor<'_> {
             type Value = TryConf;
 
             fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
@@ -141,32 +175,38 @@ macro_rules! define_Conf {
                 let mut errors = Vec::new();
                 let mut warnings = 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 => {
-                            $(warnings.push(conf_error(format!("deprecated field `{}`. {}", name, $dep)));)?
-                            match map.next_value() {
-                                Err(e) => errors.push(conf_error(e.to_string())),
+                // could get `Field` here directly, but get `String` first for diagnostics
+                while let Some(name) = map.next_key::<toml::Spanned<String>>()? {
+                    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()));
+                        }
+                        $(Ok(Field::$name) => {
+                            $(warnings.push(ConfError::spanned(self.0, format!("deprecated field `{}`. {}", name.get_ref(), $dep), name.span()));)?
+                            let raw_value = map.next_value::<toml::Spanned<toml::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)),
                                 Ok(value) => match $name {
-                                    Some(_) => errors.push(conf_error(format!("duplicate field `{}`", name))),
+                                    Some(_) => errors.push(ConfError::spanned(self.0, 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(conf_error(concat!(
+                                            Some(_) => errors.push(ConfError::spanned(self.0, concat!(
                                                 "duplicate field `", stringify!($new_conf),
                                                 "` (provided as `", stringify!($name), "`)"
-                                            ))),
+                                            ), name.span())),
                                             None => $new_conf = $name.clone(),
                                         })?
                                     },
                                 }
                             }
                         })*
-                        // white-listed; ignore
-                        Field::third_party => drop(map.next_value::<IgnoredAny>())
+                        // ignore contents of the third_party key
+                        Ok(Field::third_party) => drop(map.next_value::<IgnoredAny>())
                     }
                 }
                 let conf = Conf { $($name: $name.unwrap_or_else(defaults::$name),)* };
@@ -532,19 +572,19 @@ pub fn lookup_conf_file() -> io::Result<(Option<PathBuf>, Vec<String>)> {
 /// Read the `toml` configuration file.
 ///
 /// In case of error, the function tries to continue as much as possible.
-pub fn read(path: &Path) -> TryConf {
-    let content = match fs::read_to_string(path) {
-        Err(e) => return TryConf::from_error(e),
-        Ok(content) => content,
+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::from_str::<TryConf>(&content) {
+    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_error(e),
+        Err(e) => TryConf::from_toml_error(&file, &e),
     }
 }
 
@@ -556,65 +596,42 @@ fn extend_vec_if_indicator_present(vec: &mut Vec<String>, default: &[&str]) {
 
 const SEPARATOR_WIDTH: usize = 4;
 
-// Check whether the error is "unknown field" and, if so, list the available fields sorted and at
-// least one per line, more if `CLIPPY_TERMINAL_WIDTH` is set and allows it.
-pub fn format_error(error: Box<dyn Error>) -> String {
-    let s = error.to_string();
-
-    if_chain! {
-        if error.downcast::<toml::de::Error>().is_ok();
-        if let Some((prefix, mut fields, suffix)) = parse_unknown_field_message(&s);
-        then {
-            use fmt::Write;
-
-            fields.sort_unstable();
-
-            let (rows, column_widths) = calculate_dimensions(&fields);
-
-            let mut msg = String::from(prefix);
-            for row in 0..rows {
-                writeln!(msg).unwrap();
-                for (column, column_width) in column_widths.iter().copied().enumerate() {
-                    let index = column * rows + row;
-                    let field = fields.get(index).copied().unwrap_or_default();
-                    write!(
-                        msg,
-                        "{:SEPARATOR_WIDTH$}{field:column_width$}",
-                        " "
-                    )
-                    .unwrap();
-                }
-            }
-            write!(msg, "\n{suffix}").unwrap();
-            msg
-        } else {
-            s
-        }
+#[derive(Debug)]
+struct FieldError(String);
+
+impl std::error::Error for FieldError {}
+
+impl Display for FieldError {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        f.pad(&self.0)
     }
 }
 
-// `parse_unknown_field_message` will become unnecessary if
-// https://github.com/alexcrichton/toml-rs/pull/364 is merged.
-fn parse_unknown_field_message(s: &str) -> Option<(&str, Vec<&str>, &str)> {
-    // An "unknown field" message has the following form:
-    //   unknown field `UNKNOWN`, expected one of `FIELD0`, `FIELD1`, ..., `FIELDN` at line X column Y
-    //                                           ^^      ^^^^                     ^^
-    if_chain! {
-        if s.starts_with("unknown field");
-        let slices = s.split("`, `").collect::<Vec<_>>();
-        let n = slices.len();
-        if n >= 2;
-        if let Some((prefix, first_field)) = slices[0].rsplit_once(" `");
-        if let Some((last_field, suffix)) = slices[n - 1].split_once("` ");
-        then {
-            let fields = iter::once(first_field)
-                .chain(slices[1..n - 1].iter().copied())
-                .chain(iter::once(last_field))
-                .collect::<Vec<_>>();
-            Some((prefix, fields, suffix))
-        } else {
-            None
+impl serde::de::Error for FieldError {
+    fn custom<T: Display>(msg: T) -> Self {
+        Self(msg.to_string())
+    }
+
+    fn unknown_field(field: &str, expected: &'static [&'static str]) -> Self {
+        // List the available fields sorted and at least one per line, more if `CLIPPY_TERMINAL_WIDTH` is
+        // set and allows it.
+        use fmt::Write;
+
+        let mut expected = expected.to_vec();
+        expected.sort_unstable();
+
+        let (rows, column_widths) = calculate_dimensions(&expected);
+
+        let mut msg = format!("unknown field `{field}`, expected one of");
+        for row in 0..rows {
+            writeln!(msg).unwrap();
+            for (column, column_width) in column_widths.iter().copied().enumerate() {
+                let index = column * rows + row;
+                let field = expected.get(index).copied().unwrap_or_default();
+                write!(msg, "{:SEPARATOR_WIDTH$}{field:column_width$}", " ").unwrap();
+            }
         }
+        Self(msg)
     }
 }
 

lintcheck/Cargo.toml

@@ -22,7 +22,7 @@ rayon = "1.5.1"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0.85"
 tar = "0.4"
-toml = "0.5"
+toml = "0.7.3"
 ureq = "2.2"
 walkdir = "2.3"