From 9ab8641373b7798918644a3fb13d2a853c5d572e Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sun, 5 Jan 2025 08:53:21 +0000 Subject: [PATCH 1/7] Better error message when `--config` is given a table key and a non-inline-table value --- crates/ruff/src/args.rs | 25 ++++++++++++++++++++++++- crates/ruff/tests/lint.rs | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/crates/ruff/src/args.rs b/crates/ruff/src/args.rs index 5a3d2f2874106..bd8791d839bb5 100644 --- a/crates/ruff/src/args.rs +++ b/crates/ruff/src/args.rs @@ -9,6 +9,7 @@ use anyhow::bail; use clap::builder::{TypedValueParser, ValueParserFactory}; use clap::{command, Parser, Subcommand}; use colored::Colorize; +use itertools::Itertools; use path_absolutize::path_dedot; use regex::Regex; use ruff_graph::Direction; @@ -24,6 +25,7 @@ use ruff_source_file::{LineIndex, OneIndexed}; use ruff_text_size::TextRange; use ruff_workspace::configuration::{Configuration, RuleSelection}; use ruff_workspace::options::{Options, PycodestyleOptions}; +use ruff_workspace::options_base::{OptionEntry, OptionsMetadata}; use ruff_workspace::resolver::ConfigurationTransformer; use rustc_hash::FxHashMap; use toml; @@ -967,7 +969,28 @@ It looks like you were trying to pass a path to a configuration file. The path `{value}` does not point to a configuration file" )); } - } else if value.contains('=') { + } else if let Some((key, value)) = value.split_once('=') { + let key = key.trim_ascii(); + let value = value.trim_ascii_start(); + + if let Some(OptionEntry::Set(set)) = Options::metadata().find(key) { + if !value.starts_with('{') { + let prefixed_subkeys = format!("{set}") + .trim_ascii() + .split('\n') + .map(|child| format!("{key}.{child}")) + .join("\n"); + + tip.push_str(&format!( + " + +`{key}` is a table. +Did you mean to use one of its subkeys instead? Possible choices: +{prefixed_subkeys}" + )); + } + }; + tip.push_str(&format!( "\n\n{}:\n\n{underlying_error}", config_parse_error.description() diff --git a/crates/ruff/tests/lint.rs b/crates/ruff/tests/lint.rs index 31153d506dab2..8c8f3602485d4 100644 --- a/crates/ruff/tests/lint.rs +++ b/crates/ruff/tests/lint.rs @@ -780,6 +780,45 @@ fn each_toml_option_requires_a_new_flag_2() { "); } +#[test] +fn value_given_to_table_key_is_not_inline_table() { + // https://github.com/astral-sh/ruff/issues/13995 + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(STDIN_BASE_OPTIONS) + // spaces *also* can't be used to delimit different config overrides; + // you need a new --config flag for each override + .args([".", "--config", r#"lint.flake8-pytest-style="csv""#]), + @r#" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + error: invalid value 'lint.flake8-pytest-style="csv"' for '--config ' + + tip: A `--config` flag must either be a path to a `.toml` configuration file + or a TOML ` = ` pair overriding a specific configuration + option + + `lint.flake8-pytest-style` is a table. + Did you mean to use one of its subkeys instead? Possible choices: + lint.flake8-pytest-style.fixture-parentheses + lint.flake8-pytest-style.parametrize-names-type + lint.flake8-pytest-style.parametrize-values-type + lint.flake8-pytest-style.parametrize-values-row-type + lint.flake8-pytest-style.raises-require-match-for + lint.flake8-pytest-style.raises-extend-require-match-for + lint.flake8-pytest-style.mark-parentheses + + Could not parse the supplied argument as a `ruff.toml` configuration option: + + invalid type: string "csv", expected struct Flake8PytestStyleOptions + in `lint` + + For more information, try '--help'. + "#); +} + #[test] fn config_doubly_overridden_via_cli() -> Result<()> { let tempdir = TempDir::new()?; From b524d39f64b95f234c3c3ac26262ef24ff4d09be Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sun, 5 Jan 2025 14:00:55 +0000 Subject: [PATCH 2/7] Per review --- crates/ruff/src/args.rs | 19 +++++++++++-------- crates/ruff/tests/lint.rs | 26 ++++++++++++-------------- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/crates/ruff/src/args.rs b/crates/ruff/src/args.rs index bd8791d839bb5..c908a13e7120c 100644 --- a/crates/ruff/src/args.rs +++ b/crates/ruff/src/args.rs @@ -978,23 +978,26 @@ The path `{value}` does not point to a configuration file" let prefixed_subkeys = format!("{set}") .trim_ascii() .split('\n') - .map(|child| format!("{key}.{child}")) + .map(|child| format!("- `{key}.{child}`")) .join("\n"); tip.push_str(&format!( " -`{key}` is a table. -Did you mean to use one of its subkeys instead? Possible choices: +`{key}` is a table of configuration options. +Did you want to override one of the table's subkeys? + +Possible choices: + {prefixed_subkeys}" )); + } else { + tip.push_str(&format!( + "\n\n{}:\n\n{underlying_error}", + config_parse_error.description() + )); } }; - - tip.push_str(&format!( - "\n\n{}:\n\n{underlying_error}", - config_parse_error.description() - )); } let tip = tip.trim_end().to_owned().into(); diff --git a/crates/ruff/tests/lint.rs b/crates/ruff/tests/lint.rs index 8c8f3602485d4..e96195b3575e5 100644 --- a/crates/ruff/tests/lint.rs +++ b/crates/ruff/tests/lint.rs @@ -800,20 +800,18 @@ fn value_given_to_table_key_is_not_inline_table() { or a TOML ` = ` pair overriding a specific configuration option - `lint.flake8-pytest-style` is a table. - Did you mean to use one of its subkeys instead? Possible choices: - lint.flake8-pytest-style.fixture-parentheses - lint.flake8-pytest-style.parametrize-names-type - lint.flake8-pytest-style.parametrize-values-type - lint.flake8-pytest-style.parametrize-values-row-type - lint.flake8-pytest-style.raises-require-match-for - lint.flake8-pytest-style.raises-extend-require-match-for - lint.flake8-pytest-style.mark-parentheses - - Could not parse the supplied argument as a `ruff.toml` configuration option: - - invalid type: string "csv", expected struct Flake8PytestStyleOptions - in `lint` + `lint.flake8-pytest-style` is a table of configuration options. + Did you want to override one of the table's subkeys? + + Possible choices: + + - `lint.flake8-pytest-style.fixture-parentheses` + - `lint.flake8-pytest-style.parametrize-names-type` + - `lint.flake8-pytest-style.parametrize-values-type` + - `lint.flake8-pytest-style.parametrize-values-row-type` + - `lint.flake8-pytest-style.raises-require-match-for` + - `lint.flake8-pytest-style.raises-extend-require-match-for` + - `lint.flake8-pytest-style.mark-parentheses` For more information, try '--help'. "#); From f918d3a13415429fd3e9b9e9b94ecc1d4c45c7f0 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sun, 5 Jan 2025 14:08:01 +0000 Subject: [PATCH 3/7] Fix --- crates/ruff/src/args.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/ruff/src/args.rs b/crates/ruff/src/args.rs index c908a13e7120c..ef2e4c8fbfa84 100644 --- a/crates/ruff/src/args.rs +++ b/crates/ruff/src/args.rs @@ -991,13 +991,13 @@ Possible choices: {prefixed_subkeys}" )); - } else { - tip.push_str(&format!( - "\n\n{}:\n\n{underlying_error}", - config_parse_error.description() - )); } - }; + } else { + tip.push_str(&format!( + "\n\n{}:\n\n{underlying_error}", + config_parse_error.description() + )); + } } let tip = tip.trim_end().to_owned().into(); From d73bb45df0b2d64199c2dc8c85a35fec6e62cbab Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sun, 5 Jan 2025 14:28:48 +0000 Subject: [PATCH 4/7] Fix --- crates/ruff/src/args.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/crates/ruff/src/args.rs b/crates/ruff/src/args.rs index ef2e4c8fbfa84..dfda74b441d74 100644 --- a/crates/ruff/src/args.rs +++ b/crates/ruff/src/args.rs @@ -973,8 +973,8 @@ The path `{value}` does not point to a configuration file" let key = key.trim_ascii(); let value = value.trim_ascii_start(); - if let Some(OptionEntry::Set(set)) = Options::metadata().find(key) { - if !value.starts_with('{') { + match Options::metadata().find(key) { + Some(OptionEntry::Set(set)) if !value.starts_with('{') => { let prefixed_subkeys = format!("{set}") .trim_ascii() .split('\n') @@ -992,11 +992,12 @@ Possible choices: {prefixed_subkeys}" )); } - } else { - tip.push_str(&format!( - "\n\n{}:\n\n{underlying_error}", - config_parse_error.description() - )); + _ => { + tip.push_str(&format!( + "\n\n{}:\n\n{underlying_error}", + config_parse_error.description() + )); + } } } let tip = tip.trim_end().to_owned().into(); From e7d3a68934e1326d700cb52e1b1e6ff85d634b29 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Mon, 6 Jan 2025 12:52:14 +0000 Subject: [PATCH 5/7] Per review --- crates/ruff/src/args.rs | 12 ++--- crates/ruff/tests/lint.rs | 53 ++++++++++++++++++++++- crates/ruff_workspace/src/options_base.rs | 20 ++++++++- 3 files changed, 78 insertions(+), 7 deletions(-) diff --git a/crates/ruff/src/args.rs b/crates/ruff/src/args.rs index dfda74b441d74..ccdba51293684 100644 --- a/crates/ruff/src/args.rs +++ b/crates/ruff/src/args.rs @@ -975,10 +975,12 @@ The path `{value}` does not point to a configuration file" match Options::metadata().find(key) { Some(OptionEntry::Set(set)) if !value.starts_with('{') => { - let prefixed_subkeys = format!("{set}") - .trim_ascii() - .split('\n') - .map(|child| format!("- `{key}.{child}`")) + let prefixed_subfields = set + .collect_entries() + .iter() + .filter_map(|(name, entry)| { + entry.is_field().then(|| format!("- `{key}.{name}`")) + }) .join("\n"); tip.push_str(&format!( @@ -989,7 +991,7 @@ Did you want to override one of the table's subkeys? Possible choices: -{prefixed_subkeys}" +{prefixed_subfields}" )); } _ => { diff --git a/crates/ruff/tests/lint.rs b/crates/ruff/tests/lint.rs index e96195b3575e5..2a9405a9dd120 100644 --- a/crates/ruff/tests/lint.rs +++ b/crates/ruff/tests/lint.rs @@ -781,7 +781,7 @@ fn each_toml_option_requires_a_new_flag_2() { } #[test] -fn value_given_to_table_key_is_not_inline_table() { +fn value_given_to_table_key_is_not_inline_table_1() { // https://github.com/astral-sh/ruff/issues/13995 assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) .args(STDIN_BASE_OPTIONS) @@ -817,6 +817,57 @@ fn value_given_to_table_key_is_not_inline_table() { "#); } +#[test] +fn value_given_to_table_key_is_not_inline_table_2() { + // https://github.com/astral-sh/ruff/issues/13995 + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(STDIN_BASE_OPTIONS) + // spaces *also* can't be used to delimit different config overrides; + // you need a new --config flag for each override + .args([".", "--config", r#"lint=123"#]), + @r" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + error: invalid value 'lint=123' for '--config ' + + tip: A `--config` flag must either be a path to a `.toml` configuration file + or a TOML ` = ` pair overriding a specific configuration + option + + `lint` is a table of configuration options. + Did you want to override one of the table's subkeys? + + Possible choices: + + - `lint.allowed-confusables` + - `lint.dummy-variable-rgx` + - `lint.extend-ignore` + - `lint.extend-select` + - `lint.extend-fixable` + - `lint.external` + - `lint.fixable` + - `lint.ignore` + - `lint.extend-safe-fixes` + - `lint.extend-unsafe-fixes` + - `lint.ignore-init-module-imports` + - `lint.logger-objects` + - `lint.select` + - `lint.explicit-preview-rules` + - `lint.task-tags` + - `lint.typing-modules` + - `lint.unfixable` + - `lint.per-file-ignores` + - `lint.extend-per-file-ignores` + - `lint.exclude` + - `lint.preview` + + For more information, try '--help'. + "); +} + #[test] fn config_doubly_overridden_via_cli() -> Result<()> { let tempdir = TempDir::new()?; diff --git a/crates/ruff_workspace/src/options_base.rs b/crates/ruff_workspace/src/options_base.rs index a905db867229b..ff9a0c45d5741 100644 --- a/crates/ruff_workspace/src/options_base.rs +++ b/crates/ruff_workspace/src/options_base.rs @@ -42,7 +42,7 @@ where } /// Metadata of an option that can either be a [`OptionField`] or [`OptionSet`]. -#[derive(Clone, PartialEq, Eq, Debug, Serialize)] +#[derive(Clone, PartialEq, Eq, Debug, Serialize, is_macro::Is)] #[serde(untagged)] pub enum OptionEntry { /// A single option. @@ -285,6 +285,24 @@ impl OptionSet { None } } + + pub fn collect_entries(&self) -> Vec<(String, OptionEntry)> { + struct EntryCollector(pub Vec<(String, OptionEntry)>); + + impl Visit for EntryCollector { + fn record_field(&mut self, name: &str, field: OptionField) { + self.0.push((name.to_string(), OptionEntry::Field(field))); + } + + fn record_set(&mut self, name: &str, group: OptionSet) { + self.0.push((name.to_string(), OptionEntry::Set(group))); + } + } + + let mut visitor = EntryCollector(vec![]); + self.record(&mut visitor); + visitor.0 + } } /// Visitor that writes out the names of all fields and sets. From f16eff84b83515ff217ef82ab5ba35779b1b97dc Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 6 Jan 2025 14:11:24 +0100 Subject: [PATCH 6/7] Use `collect_fields`, limit visiblity --- crates/ruff/src/args.rs | 6 ++---- crates/ruff_workspace/src/options_base.rs | 16 +++++++--------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/crates/ruff/src/args.rs b/crates/ruff/src/args.rs index ccdba51293684..b88f3c6746ccf 100644 --- a/crates/ruff/src/args.rs +++ b/crates/ruff/src/args.rs @@ -976,11 +976,9 @@ The path `{value}` does not point to a configuration file" match Options::metadata().find(key) { Some(OptionEntry::Set(set)) if !value.starts_with('{') => { let prefixed_subfields = set - .collect_entries() + .collect_fields() .iter() - .filter_map(|(name, entry)| { - entry.is_field().then(|| format!("- `{key}.{name}`")) - }) + .map(|(name, _)| format!("- `{key}.{name}`")) .join("\n"); tip.push_str(&format!( diff --git a/crates/ruff_workspace/src/options_base.rs b/crates/ruff_workspace/src/options_base.rs index ff9a0c45d5741..e3e4c650a27d7 100644 --- a/crates/ruff_workspace/src/options_base.rs +++ b/crates/ruff_workspace/src/options_base.rs @@ -42,7 +42,7 @@ where } /// Metadata of an option that can either be a [`OptionField`] or [`OptionSet`]. -#[derive(Clone, PartialEq, Eq, Debug, Serialize, is_macro::Is)] +#[derive(Clone, PartialEq, Eq, Debug, Serialize)] #[serde(untagged)] pub enum OptionEntry { /// A single option. @@ -286,20 +286,18 @@ impl OptionSet { } } - pub fn collect_entries(&self) -> Vec<(String, OptionEntry)> { - struct EntryCollector(pub Vec<(String, OptionEntry)>); + pub fn collect_fields(&self) -> Vec<(String, OptionField)> { + struct FieldsCollector(Vec<(String, OptionField)>); - impl Visit for EntryCollector { + impl Visit for FieldsCollector { fn record_field(&mut self, name: &str, field: OptionField) { - self.0.push((name.to_string(), OptionEntry::Field(field))); + self.0.push((name.to_string(), field)); } - fn record_set(&mut self, name: &str, group: OptionSet) { - self.0.push((name.to_string(), OptionEntry::Set(group))); - } + fn record_set(&mut self, _name: &str, _group: OptionSet) {} } - let mut visitor = EntryCollector(vec![]); + let mut visitor = FieldsCollector(vec![]); self.record(&mut visitor); visitor.0 } From 0fdb4f9aeb4765a338de0873b804cb5a079b4945 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Mon, 6 Jan 2025 13:15:20 +0000 Subject: [PATCH 7/7] remove nonsensical comments --- crates/ruff/tests/lint.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/crates/ruff/tests/lint.rs b/crates/ruff/tests/lint.rs index 2a9405a9dd120..84fe98ba6c5d0 100644 --- a/crates/ruff/tests/lint.rs +++ b/crates/ruff/tests/lint.rs @@ -785,8 +785,6 @@ fn value_given_to_table_key_is_not_inline_table_1() { // https://github.com/astral-sh/ruff/issues/13995 assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) .args(STDIN_BASE_OPTIONS) - // spaces *also* can't be used to delimit different config overrides; - // you need a new --config flag for each override .args([".", "--config", r#"lint.flake8-pytest-style="csv""#]), @r#" success: false @@ -822,8 +820,6 @@ fn value_given_to_table_key_is_not_inline_table_2() { // https://github.com/astral-sh/ruff/issues/13995 assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) .args(STDIN_BASE_OPTIONS) - // spaces *also* can't be used to delimit different config overrides; - // you need a new --config flag for each override .args([".", "--config", r#"lint=123"#]), @r" success: false