From 564725775bd200de131b96f4d93a36bd64508e72 Mon Sep 17 00:00:00 2001 From: "Samuel E. Moelius III" Date: Thu, 12 May 2022 12:31:17 -0400 Subject: [PATCH] Improve "unknown field" error messages --- Cargo.toml | 1 + clippy_lints/src/lib.rs | 4 +- clippy_lints/src/utils/conf.rs | 129 +++++++++++++++++- src/main.rs | 4 + .../toml_unknown_key/conf_unknown_key.stderr | 41 +++++- 5 files changed, 169 insertions(+), 10 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index dd6518d5241..03e6bf03afe 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,6 +25,7 @@ clippy_lints = { path = "clippy_lints" } semver = "1.0" rustc_tools_util = { path = "rustc_tools_util" } tempfile = { version = "3.2", optional = true } +termize = "0.1" [dev-dependencies] compiletest_rs = { version = "0.7.1", features = ["tmp"] } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 1c502ef1d44..44c42d03e60 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -418,7 +418,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; +use crate::utils::conf::{format_error, TryConf}; /// Register all pre expansion lints /// @@ -463,7 +463,7 @@ pub fn read_conf(sess: &Session) -> Conf { sess.struct_err(&format!( "error reading Clippy's configuration file `{}`: {}", file_name.display(), - error + format_error(error) )) .emit(); } diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index bdcd76d153f..cd4d16fe95f 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -6,7 +6,8 @@ use serde::de::{Deserializer, IgnoredAny, IntoDeserializer, MapAccess, Visitor}; use serde::Deserialize; use std::error::Error; use std::path::{Path, PathBuf}; -use std::{env, fmt, fs, io}; +use std::str::FromStr; +use std::{cmp, env, fmt, fs, io, iter}; /// Holds information used by `MISSING_ENFORCED_IMPORT_RENAMES` lint. #[derive(Clone, Debug, Deserialize)] @@ -43,18 +44,33 @@ pub enum DisallowedType { #[derive(Default)] pub struct TryConf { pub conf: Conf, - pub errors: Vec, + pub errors: Vec>, } impl TryConf { - fn from_error(error: impl Error) -> Self { + fn from_error(error: impl Error + 'static) -> Self { Self { conf: Conf::default(), - errors: vec![error.to_string()], + errors: vec![Box::new(error)], } } } +#[derive(Debug)] +struct ConfError(String); + +impl fmt::Display for ConfError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + ::fmt(&self.0, f) + } +} + +impl Error for ConfError {} + +fn conf_error(s: String) -> Box { + Box::new(ConfError(s)) +} + macro_rules! define_Conf { ($( $(#[doc = $doc:literal])+ @@ -103,11 +119,11 @@ macro_rules! define_Conf { while let Some(name) = map.next_key::<&str>()? { match Field::deserialize(name.into_deserializer())? { $(Field::$name => { - $(errors.push(format!("deprecated field `{}`. {}", name, $dep));)? + $(errors.push(conf_error(format!("deprecated field `{}`. {}", name, $dep)));)? match map.next_value() { - Err(e) => errors.push(e.to_string()), + Err(e) => errors.push(conf_error(e.to_string())), Ok(value) => match $name { - Some(_) => errors.push(format!("duplicate field `{}`", name)), + Some(_) => errors.push(conf_error(format!("duplicate field `{}`", name))), None => $name = Some(value), } } @@ -383,3 +399,102 @@ pub fn read(path: &Path) -> TryConf { }; toml::from_str(&content).unwrap_or_else(TryConf::from_error) } + +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) -> String { + let s = error.to_string(); + + if_chain! { + if error.downcast::().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 { + write!(msg, "\n").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_width$}", + " ", + field, + separator_width = SEPARATOR_WIDTH, + field_width = column_width + ) + .unwrap(); + } + } + write!(msg, "\n{}", suffix).unwrap(); + msg + } else { + s + } + } +} + +// `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::>(); + 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::>(); + Some((prefix, fields, suffix)) + } else { + None + } + } +} + +fn calculate_dimensions(fields: &[&str]) -> (usize, Vec) { + let columns = env::var("CLIPPY_TERMINAL_WIDTH") + .ok() + .and_then(|s| ::from_str(&s).ok()) + .map_or(1, |terminal_width| { + let max_field_width = fields.iter().map(|field| field.len()).max().unwrap(); + cmp::max(1, terminal_width / (SEPARATOR_WIDTH + max_field_width)) + }); + + let rows = (fields.len() + (columns - 1)) / columns; + + let column_widths = (0..columns) + .map(|column| { + if column < columns - 1 { + (0..rows) + .map(|row| { + let index = column * rows + row; + let field = fields.get(index).copied().unwrap_or_default(); + field.len() + }) + .max() + .unwrap() + } else { + // Avoid adding extra space to the last column. + 0 + } + }) + .collect::>(); + + (rows, column_widths) +} diff --git a/src/main.rs b/src/main.rs index 240e233420f..9ee4a40cbf2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -123,8 +123,12 @@ impl ClippyCmd { .map(|arg| format!("{}__CLIPPY_HACKERY__", arg)) .collect(); + // Currently, `CLIPPY_TERMINAL_WIDTH` is used only to format "unknown field" error messages. + let terminal_width = termize::dimensions().map_or(0, |(w, _)| w); + cmd.env("RUSTC_WORKSPACE_WRAPPER", Self::path()) .env("CLIPPY_ARGS", clippy_args) + .env("CLIPPY_TERMINAL_WIDTH", terminal_width.to_string()) .arg(self.cargo_subcommand) .args(&self.args); 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 65277dd03e8..92838aa57df 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -1,4 +1,43 @@ -error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `avoid-breaking-exported-api`, `msrv`, `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `pass-by-value-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `vec-box-size-threshold`, `max-trait-bounds`, `max-struct-bools`, `max-fn-params-bools`, `warn-on-all-wildcard-imports`, `disallowed-methods`, `disallowed-types`, `unreadable-literal-lint-fractions`, `upper-case-acronyms-aggressive`, `cargo-ignore-publish`, `standard-macro-braces`, `enforced-import-renames`, `allowed-scripts`, `enable-raw-pointer-heuristic-for-send`, `max-suggested-slice-pattern-length`, `await-holding-invalid-types`, `max-include-file-size`, `allow-expect-in-tests`, `allow-unwrap-in-tests`, `third-party` at line 5 column 1 +error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of + allow-expect-in-tests + allow-unwrap-in-tests + allowed-scripts + array-size-threshold + avoid-breaking-exported-api + await-holding-invalid-types + blacklisted-names + cargo-ignore-publish + cognitive-complexity-threshold + cyclomatic-complexity-threshold + disallowed-methods + disallowed-types + doc-valid-idents + enable-raw-pointer-heuristic-for-send + enforced-import-renames + enum-variant-name-threshold + enum-variant-size-threshold + literal-representation-threshold + max-fn-params-bools + max-include-file-size + max-struct-bools + max-suggested-slice-pattern-length + max-trait-bounds + msrv + pass-by-value-size-limit + single-char-binding-names-threshold + standard-macro-braces + third-party + too-large-for-stack + too-many-arguments-threshold + too-many-lines-threshold + trivial-copy-size-limit + type-complexity-threshold + unreadable-literal-lint-fractions + upper-case-acronyms-aggressive + vec-box-size-threshold + verbose-bit-mask-threshold + warn-on-all-wildcard-imports + at line 5 column 1 error: aborting due to previous error