From db633ae1a8afbc8793091f69eca489b105d1e5ec Mon Sep 17 00:00:00 2001 From: Martin Fischer Date: Sat, 28 Jan 2023 20:49:15 +0100 Subject: [PATCH] Improve rule config resolution Ruff allows rules to be enabled with `select` and disabled with `ignore`, where the more specific rule selector takes precedence, for example: `--select ALL --ignore E501` selects all rules except E501 `--ignore ALL --select E501` selects only E501 (If both selectors have the same specificity ignore selectors take precedence.) Ruff always had two quirks: * If `pyproject.toml` specified `ignore = ["E501"]` then you could previously not override that with `--select E501` on the command-line (since the resolution didn't take into account that the select was specified after the ignore). * If `pyproject.toml` specified `select = ["E501"]` then you could previously not override that with `--ignore E` on the command-line (since the resolution didn't take into account that the ignore was specified after the select). Since d067efe2656dfa86c8ac2baa87f43f408e862a3a (#1245) `extend-select` and `extend-ignore` always override `select` and `ignore` and are applied iteratively in pairs, which introduced another quirk: * If some `pyproject.toml` file specified `extend-select` or `extend-ignore`, `select` and `ignore` became pretty much unreliable after that with no way of resetting that. This commit fixes all of these quirks by making later configuration sources take precedence over earlier configuration sources. While this is a breaking change, we expect most ruff configuration files to not rely on the previous unintutive behavior. --- README.md | 13 +- ruff.schema.json | 4 +- ruff_cli/src/args.rs | 46 ++---- src/rule_selector.rs | 3 +- src/settings/configuration.rs | 51 +++---- src/settings/mod.rs | 272 +++++++++++++++++++--------------- src/settings/options.rs | 13 +- 7 files changed, 205 insertions(+), 197 deletions(-) diff --git a/README.md b/README.md index f246c2228adfa1..59b2e32e7b18ce 100644 --- a/README.md +++ b/README.md @@ -2138,11 +2138,8 @@ extend-exclude = ["tests", "src/bad.py"] A list of rule codes or prefixes to ignore, in addition to those specified by `ignore`. -Note that `extend-ignore` is applied after resolving rules from -`ignore`/`select` and a less specific rule in `extend-ignore` -would overwrite a more specific rule in `select`. It is -recommended to only use `extend-ignore` when extending a -`pyproject.toml` file via `extend`. +This option has been DEPRECATED in favor of `ignore` +since its usage is now interchangeable with `ignore`. **Default value**: `[]` @@ -2163,12 +2160,6 @@ extend-ignore = ["F841"] A list of rule codes or prefixes to enable, in addition to those specified by `select`. -Note that `extend-select` is applied after resolving rules from -`ignore`/`select` and a less specific rule in `extend-select` -would overwrite a more specific rule in `ignore`. It is -recommended to only use `extend-select` when extending a -`pyproject.toml` file via `extend`. - **Default value**: `[]` **Type**: `Vec` diff --git a/ruff.schema.json b/ruff.schema.json index 6d3c91bcb1f2dc..7ccb46d3803394 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -67,7 +67,7 @@ } }, "extend-ignore": { - "description": "A list of rule codes or prefixes to ignore, in addition to those specified by `ignore`.\n\nNote that `extend-ignore` is applied after resolving rules from `ignore`/`select` and a less specific rule in `extend-ignore` would overwrite a more specific rule in `select`. It is recommended to only use `extend-ignore` when extending a `pyproject.toml` file via `extend`.", + "description": "A list of rule codes or prefixes to ignore, in addition to those specified by `ignore`.\n\nThis option has been DEPRECATED in favor of `ignore` since its usage is now interchangeable with `ignore`.", "type": [ "array", "null" @@ -77,7 +77,7 @@ } }, "extend-select": { - "description": "A list of rule codes or prefixes to enable, in addition to those specified by `select`.\n\nNote that `extend-select` is applied after resolving rules from `ignore`/`select` and a less specific rule in `extend-select` would overwrite a more specific rule in `ignore`. It is recommended to only use `extend-select` when extending a `pyproject.toml` file via `extend`.", + "description": "A list of rule codes or prefixes to enable, in addition to those specified by `select`.", "type": [ "array", "null" diff --git a/ruff_cli/src/args.rs b/ruff_cli/src/args.rs index b821468ee387df..5334a137a2a524 100644 --- a/ruff_cli/src/args.rs +++ b/ruff_cli/src/args.rs @@ -5,6 +5,7 @@ use regex::Regex; use ruff::logging::LogLevel; use ruff::registry::Rule; use ruff::resolver::ConfigProcessor; +use ruff::settings::configuration::RuleSelection; use ruff::settings::types::{ FilePattern, PatternPrefixPair, PerFileIgnore, PythonVersion, SerializationFormat, }; @@ -116,7 +117,8 @@ pub struct CheckArgs { long, value_delimiter = ',', value_name = "RULE_CODE", - help_heading = "Rule selection" + help_heading = "Rule selection", + hide = true )] pub extend_ignore: Option>, /// List of mappings from file pattern to code to exclude @@ -422,18 +424,25 @@ impl ConfigProcessor for &Overrides { if let Some(fix_only) = &self.fix_only { config.fix_only = Some(*fix_only); } - if let Some(fixable) = &self.fixable { - config.fixable = Some(fixable.clone()); - } + config.rule_selections.push(RuleSelection { + select: self.select.clone(), + ignore: self + .ignore + .iter() + .cloned() + .chain(self.extend_ignore.iter().cloned().into_iter()) + .flatten() + .collect(), + extend_select: self.extend_select.clone().unwrap_or_default(), + fixable: self.fixable.clone(), + unfixable: self.unfixable.clone().unwrap_or_default(), + }); if let Some(format) = &self.format { config.format = Some(*format); } if let Some(force_exclude) = &self.force_exclude { config.force_exclude = Some(*force_exclude); } - if let Some(ignore) = &self.ignore { - config.ignore = Some(ignore.clone()); - } if let Some(line_length) = &self.line_length { config.line_length = Some(*line_length); } @@ -443,38 +452,15 @@ impl ConfigProcessor for &Overrides { if let Some(respect_gitignore) = &self.respect_gitignore { config.respect_gitignore = Some(*respect_gitignore); } - if let Some(select) = &self.select { - config.select = Some(select.clone()); - } if let Some(show_source) = &self.show_source { config.show_source = Some(*show_source); } if let Some(target_version) = &self.target_version { config.target_version = Some(*target_version); } - if let Some(unfixable) = &self.unfixable { - config.unfixable = Some(unfixable.clone()); - } if let Some(update_check) = &self.update_check { config.update_check = Some(*update_check); } - // Special-case: `extend_ignore` and `extend_select` are parallel arrays, so - // push an empty array if only one of the two is provided. - match (&self.extend_ignore, &self.extend_select) { - (Some(extend_ignore), Some(extend_select)) => { - config.extend_ignore.push(extend_ignore.clone()); - config.extend_select.push(extend_select.clone()); - } - (Some(extend_ignore), None) => { - config.extend_ignore.push(extend_ignore.clone()); - config.extend_select.push(Vec::new()); - } - (None, Some(extend_select)) => { - config.extend_ignore.push(Vec::new()); - config.extend_select.push(extend_select.clone()); - } - (None, None) => {} - } } } diff --git a/src/rule_selector.rs b/src/rule_selector.rs index 8d52a1bfb979a3..6c2ddb222b37ff 100644 --- a/src/rule_selector.rs +++ b/src/rule_selector.rs @@ -6,6 +6,7 @@ use schemars::JsonSchema; use serde::de::{self, Visitor}; use serde::{Deserialize, Serialize}; use strum::IntoEnumIterator; +use strum_macros::EnumIter; use crate::registry::{Rule, RuleCodePrefix, RuleIter}; use crate::rule_redirects::get_redirect; @@ -171,7 +172,7 @@ impl RuleSelector { } } -#[derive(PartialEq, Eq, PartialOrd, Ord)] +#[derive(EnumIter, PartialEq, Eq, PartialOrd, Ord)] pub(crate) enum Specificity { All, Linter, diff --git a/src/settings/configuration.rs b/src/settings/configuration.rs index 08de2314544519..e6ffa42b765c80 100644 --- a/src/settings/configuration.rs +++ b/src/settings/configuration.rs @@ -27,13 +27,17 @@ use crate::settings::types::{ }; #[derive(Debug, Default)] -pub struct Configuration { +pub struct RuleSelection { pub select: Option>, - pub ignore: Option>, - pub extend_select: Vec>, - pub extend_ignore: Vec>, + pub ignore: Vec, + pub extend_select: Vec, pub fixable: Option>, - pub unfixable: Option>, + pub unfixable: Vec, +} + +#[derive(Debug, Default)] +pub struct Configuration { + pub rule_selections: Vec, pub per_file_ignores: Option>, pub allowed_confusables: Option>, @@ -88,6 +92,18 @@ impl Configuration { pub fn from_options(options: Options, project_root: &Path) -> Result { Ok(Configuration { + rule_selections: vec![RuleSelection { + select: options.select, + ignore: options + .ignore + .into_iter() + .flatten() + .chain(options.extend_ignore.into_iter().flatten()) + .collect(), + extend_select: options.extend_select.unwrap_or_default(), + fixable: options.fixable, + unfixable: options.unfixable.unwrap_or_default(), + }], allowed_confusables: options.allowed_confusables, builtins: options.builtins, cache_dir: options @@ -132,15 +148,11 @@ impl Configuration { .collect() }) .unwrap_or_default(), - extend_ignore: vec![options.extend_ignore.unwrap_or_default()], - extend_select: vec![options.extend_select.unwrap_or_default()], external: options.external, fix: options.fix, fix_only: options.fix_only, - fixable: options.fixable, format: options.format, force_exclude: options.force_exclude, - ignore: options.ignore, ignore_init_module_imports: options.ignore_init_module_imports, line_length: options.line_length, namespace_packages: options @@ -157,7 +169,6 @@ impl Configuration { }), required_version: options.required_version, respect_gitignore: options.respect_gitignore, - select: options.select, show_source: options.show_source, src: options .src @@ -166,7 +177,6 @@ impl Configuration { target_version: options.target_version, task_tags: options.task_tags, typing_modules: options.typing_modules, - unfixable: options.unfixable, update_check: options.update_check, // Plugins flake8_annotations: options.flake8_annotations, @@ -194,6 +204,11 @@ impl Configuration { #[must_use] pub fn combine(self, config: Configuration) -> Self { Self { + rule_selections: config + .rule_selections + .into_iter() + .chain(self.rule_selections.into_iter()) + .collect(), allowed_confusables: self.allowed_confusables.or(config.allowed_confusables), builtins: self.builtins.or(config.builtins), cache_dir: self.cache_dir.or(config.cache_dir), @@ -205,23 +220,11 @@ impl Configuration { .into_iter() .chain(self.extend_exclude.into_iter()) .collect(), - extend_ignore: config - .extend_ignore - .into_iter() - .chain(self.extend_ignore.into_iter()) - .collect(), - extend_select: config - .extend_select - .into_iter() - .chain(self.extend_select.into_iter()) - .collect(), external: self.external.or(config.external), fix: self.fix.or(config.fix), fix_only: self.fix_only.or(config.fix_only), - fixable: self.fixable.or(config.fixable), format: self.format.or(config.format), force_exclude: self.force_exclude.or(config.force_exclude), - ignore: self.ignore.or(config.ignore), ignore_init_module_imports: self .ignore_init_module_imports .or(config.ignore_init_module_imports), @@ -230,13 +233,11 @@ impl Configuration { per_file_ignores: self.per_file_ignores.or(config.per_file_ignores), required_version: self.required_version.or(config.required_version), respect_gitignore: self.respect_gitignore.or(config.respect_gitignore), - select: self.select.or(config.select), show_source: self.show_source.or(config.show_source), src: self.src.or(config.src), target_version: self.target_version.or(config.target_version), task_tags: self.task_tags.or(config.task_tags), typing_modules: self.typing_modules.or(config.typing_modules), - unfixable: self.unfixable.or(config.unfixable), update_check: self.update_check.or(config.update_check), // Plugins flake8_annotations: self.flake8_annotations.or(config.flake8_annotations), diff --git a/src/settings/mod.rs b/src/settings/mod.rs index 3eb5f67e78f4f4..56a5444eb31291 100644 --- a/src/settings/mod.rs +++ b/src/settings/mod.rs @@ -7,6 +7,7 @@ use std::path::{Path, PathBuf}; use anyhow::{anyhow, Result}; use globset::Glob; use rustc_hash::{FxHashMap, FxHashSet}; +use strum::IntoEnumIterator; use self::hashable::{HashableGlobMatcher, HashableGlobSet, HashableHashSet, HashableRegex}; use self::rule_table::RuleTable; @@ -236,29 +237,110 @@ impl Settings { impl From<&Configuration> for RuleTable { fn from(config: &Configuration) -> Self { - let mut rules = RuleTable::empty(); - - let fixable = resolve_codes([RuleCodeSpec { - select: config.fixable.as_deref().unwrap_or(&[RuleSelector::All]), - ignore: config.unfixable.as_deref().unwrap_or_default(), - }]); + // The select_set keeps track of which rules have been selected. + let mut select_set: FxHashSet = defaults::PREFIXES.iter().flatten().collect(); + // The fixable set keeps track of which rules are fixable. + let mut fixable_set: FxHashSet = RuleSelector::All.into_iter().collect(); + + let mut redirects = FxHashMap::default(); + + for selection in &config.rule_selections { + // We do not have an extend-fixable option, so fixable and unfixable + // selectors can simply be applied directly to fixable_set. + if selection.fixable.is_some() { + fixable_set.clear(); + } - for code in resolve_codes( - [RuleCodeSpec { - select: config.select.as_deref().unwrap_or(defaults::PREFIXES), - ignore: config.ignore.as_deref().unwrap_or_default(), - }] - .into_iter() - .chain( - config - .extend_select + // If a selection only specifies extend-select we cannot directly + // apply its rule selectors to the select_set because we firstly have + // to resolve the effectively selected rules within the current rule selection + // (taking specificity into account since more specific selectors take + // precedence over less specific selectors within a rule selection). + // We do this via the following HashMap where the bool indicates + // whether to enable or disable the given rule. + let mut select_map_updates: FxHashMap = FxHashMap::default(); + + for spec in Specificity::iter() { + for selector in selection + .select + .iter() + .flatten() + .chain(selection.extend_select.iter()) + .filter(|s| s.specificity() == spec) + { + for rule in selector { + select_map_updates.insert(rule, true); + } + } + for selector in selection.ignore.iter().filter(|s| s.specificity() == spec) { + for rule in selector { + select_map_updates.insert(rule, false); + } + } + if let Some(fixable) = &selection.fixable { + fixable_set + .extend(fixable.iter().filter(|s| s.specificity() == spec).flatten()); + } + for selector in selection + .unfixable .iter() - .zip(config.extend_ignore.iter()) - .map(|(select, ignore)| RuleCodeSpec { select, ignore }), - ), - ) { - let fix = fixable.contains(&code); - rules.enable(code, fix); + .filter(|s| s.specificity() == spec) + { + for rule in selector { + fixable_set.remove(&rule); + } + } + } + + if selection.select.is_some() { + // If the `select` option is given we reassign the whole select_set + // (overriding everything that has been defined previously). + select_set = select_map_updates + .into_iter() + .filter_map(|(rule, enabled)| enabled.then_some(rule)) + .collect(); + } else { + // Otherwise we apply the updates on top of the existing select_set. + for (rule, enabled) in select_map_updates { + if enabled { + select_set.insert(rule); + } else { + select_set.remove(&rule); + } + } + } + + // We insert redirects into the hashmap so that we + // can warn the users about remapped rule codes. + for selector in selection + .select + .iter() + .chain(selection.fixable.iter()) + .flatten() + .chain(selection.ignore.iter()) + .chain(selection.extend_select.iter()) + .chain(selection.unfixable.iter()) + { + if let RuleSelector::Prefix { + prefix, + redirected_from: Some(redirect_from), + } = selector + { + redirects.insert(redirect_from, prefix); + } + } + } + + for (from, target) in redirects { + // TODO(martin): This belongs into the ruff_cli crate. + crate::warn_user!("`{from}` has been remapped to `{}`.", target.as_ref()); + } + + let mut rules = RuleTable::empty(); + + for rule in select_set { + let fix = fixable_set.contains(&rule); + rules.enable(rule.clone(), fix); } // If a docstring convention is specified, force-disable any incompatible error @@ -309,68 +391,6 @@ pub fn resolve_per_file_ignores( .collect() } -#[derive(Debug)] -struct RuleCodeSpec<'a> { - select: &'a [RuleSelector], - ignore: &'a [RuleSelector], -} - -/// Given a set of selected and ignored prefixes, resolve the set of enabled -/// rule codes. -fn resolve_codes<'a>(specs: impl IntoIterator>) -> FxHashSet { - let mut rules: FxHashSet = FxHashSet::default(); - - let mut redirects = FxHashMap::default(); - - for spec in specs { - for specificity in [ - Specificity::All, - Specificity::Linter, - Specificity::Code1Char, - Specificity::Code2Chars, - Specificity::Code3Chars, - Specificity::Code4Chars, - Specificity::Code5Chars, - ] { - for selector in spec.select { - if selector.specificity() == specificity { - rules.extend(selector); - } - - if let RuleSelector::Prefix { - prefix, - redirected_from: Some(redirect_from), - } = selector - { - redirects.insert(redirect_from, prefix); - } - } - for selector in spec.ignore { - if selector.specificity() == specificity { - for rule in selector { - rules.remove(&rule); - } - } - - if let RuleSelector::Prefix { - prefix, - redirected_from: Some(redirect_from), - } = selector - { - redirects.insert(redirect_from, prefix); - } - } - } - } - - for (from, target) in redirects { - // TODO(martin): This belongs into the ruff_cli crate. - crate::warn_user!("`{from}` has been remapped to `{}`.", target.as_ref()); - } - - rules -} - #[cfg(test)] mod tests { use rustc_hash::FxHashSet; @@ -379,17 +399,26 @@ mod tests { use crate::settings::configuration::Configuration; use crate::settings::rule_table::RuleTable; + use super::configuration::RuleSelection; + #[allow(clippy::needless_pass_by_value)] - fn resolve_rules(config: Configuration) -> FxHashSet { - RuleTable::from(&config).iter_enabled().cloned().collect() + fn resolve_rules(selections: impl IntoIterator) -> FxHashSet { + RuleTable::from(&Configuration { + rule_selections: selections.into_iter().collect(), + ..Configuration::default() + }) + .iter_enabled() + .cloned() + .collect() } #[test] fn rule_codes() { - let actual = resolve_rules(Configuration { + let actual = resolve_rules([RuleSelection { select: Some(vec![RuleCodePrefix::W.into()]), - ..Configuration::default() - }); + ..RuleSelection::default() + }]); + let expected = FxHashSet::from_iter([ Rule::NoNewLineAtEndOfFile, Rule::DocLineTooLong, @@ -397,44 +426,48 @@ mod tests { ]); assert_eq!(actual, expected); - let actual = resolve_rules(Configuration { + let actual = resolve_rules([RuleSelection { select: Some(vec![RuleCodePrefix::W6.into()]), - ..Configuration::default() - }); + ..RuleSelection::default() + }]); let expected = FxHashSet::from_iter([Rule::InvalidEscapeSequence]); assert_eq!(actual, expected); - let actual = resolve_rules(Configuration { + let actual = resolve_rules([RuleSelection { select: Some(vec![RuleCodePrefix::W.into()]), - ignore: Some(vec![RuleCodePrefix::W292.into()]), - ..Configuration::default() - }); + ignore: vec![RuleCodePrefix::W292.into()], + ..RuleSelection::default() + }]); let expected = FxHashSet::from_iter([Rule::DocLineTooLong, Rule::InvalidEscapeSequence]); assert_eq!(actual, expected); - let actual = resolve_rules(Configuration { + let actual = resolve_rules([RuleSelection { select: Some(vec![RuleCodePrefix::W292.into()]), - ignore: Some(vec![RuleCodePrefix::W.into()]), - ..Configuration::default() - }); + ignore: vec![RuleCodePrefix::W.into()], + ..RuleSelection::default() + }]); let expected = FxHashSet::from_iter([Rule::NoNewLineAtEndOfFile]); assert_eq!(actual, expected); - let actual = resolve_rules(Configuration { + let actual = resolve_rules([RuleSelection { select: Some(vec![RuleCodePrefix::W605.into()]), - ignore: Some(vec![RuleCodePrefix::W605.into()]), - ..Configuration::default() - }); + ignore: vec![RuleCodePrefix::W605.into()], + ..RuleSelection::default() + }]); let expected = FxHashSet::from_iter([]); assert_eq!(actual, expected); - let actual = resolve_rules(Configuration { - select: Some(vec![RuleCodePrefix::W.into()]), - ignore: Some(vec![RuleCodePrefix::W292.into()]), - extend_select: vec![vec![RuleCodePrefix::W292.into()]], - extend_ignore: vec![vec![]], - ..Configuration::default() - }); + let actual = resolve_rules([ + RuleSelection { + select: Some(vec![RuleCodePrefix::W.into()]), + ignore: vec![RuleCodePrefix::W292.into()], + ..RuleSelection::default() + }, + RuleSelection { + extend_select: vec![RuleCodePrefix::W292.into()], + ..RuleSelection::default() + }, + ]); let expected = FxHashSet::from_iter([ Rule::NoNewLineAtEndOfFile, Rule::DocLineTooLong, @@ -442,13 +475,18 @@ mod tests { ]); assert_eq!(actual, expected); - let actual = resolve_rules(Configuration { - select: Some(vec![RuleCodePrefix::W.into()]), - ignore: Some(vec![RuleCodePrefix::W292.into()]), - extend_select: vec![vec![RuleCodePrefix::W292.into()]], - extend_ignore: vec![vec![RuleCodePrefix::W.into()]], - ..Configuration::default() - }); + let actual = resolve_rules([ + RuleSelection { + select: Some(vec![RuleCodePrefix::W.into()]), + ignore: vec![RuleCodePrefix::W292.into()], + ..RuleSelection::default() + }, + RuleSelection { + extend_select: vec![RuleCodePrefix::W292.into()], + ignore: vec![RuleCodePrefix::W.into()], + ..RuleSelection::default() + }, + ]); let expected = FxHashSet::from_iter([Rule::NoNewLineAtEndOfFile]); assert_eq!(actual, expected); } diff --git a/src/settings/options.rs b/src/settings/options.rs index 2a4d9463a42a4d..e43f4a030214b2 100644 --- a/src/settings/options.rs +++ b/src/settings/options.rs @@ -145,11 +145,8 @@ pub struct Options { /// A list of rule codes or prefixes to ignore, in addition to those /// specified by `ignore`. /// - /// Note that `extend-ignore` is applied after resolving rules from - /// `ignore`/`select` and a less specific rule in `extend-ignore` - /// would overwrite a more specific rule in `select`. It is - /// recommended to only use `extend-ignore` when extending a - /// `pyproject.toml` file via `extend`. + /// This option has been DEPRECATED in favor of `ignore` + /// since its usage is now interchangeable with `ignore`. pub extend_ignore: Option>, #[option( default = "[]", @@ -161,12 +158,6 @@ pub struct Options { )] /// A list of rule codes or prefixes to enable, in addition to those /// specified by `select`. - /// - /// Note that `extend-select` is applied after resolving rules from - /// `ignore`/`select` and a less specific rule in `extend-select` - /// would overwrite a more specific rule in `ignore`. It is - /// recommended to only use `extend-select` when extending a - /// `pyproject.toml` file via `extend`. pub extend_select: Option>, #[option( default = "[]",