From e5008ca7140fb32cada795c5faa38ec60927b0db Mon Sep 17 00:00:00 2001 From: Zanie Date: Thu, 1 Feb 2024 08:57:47 -0600 Subject: [PATCH] Fix bug where selection included deprecated rules during preview (#9746) Cherry-picked from https://github.com/astral-sh/ruff/pull/9714 which is being abandoned for now because we need to invest more into our redirection infrastructure before it is feasible. Fixes a bug in the implementation where we improperly included deprecated rules in `RuleSelector.rules()` when preview is on. Includes some clean-up of error messages and the implementation. # Conflicts: # crates/ruff/tests/integration_test.rs --- crates/ruff/tests/integration_test.rs | 24 ++++++++++--------- crates/ruff_linter/src/rule_selector.rs | 17 ++++++++----- .../tryceratops/rules/reraise_no_cause.rs | 4 ++-- crates/ruff_workspace/src/configuration.rs | 24 ++++++++----------- 4 files changed, 36 insertions(+), 33 deletions(-) diff --git a/crates/ruff/tests/integration_test.rs b/crates/ruff/tests/integration_test.rs index 2d3e1dc661395..6a0fde5a2b65f 100644 --- a/crates/ruff/tests/integration_test.rs +++ b/crates/ruff/tests/integration_test.rs @@ -977,7 +977,7 @@ fn preview_enabled_direct() { #[test] fn preview_disabled_direct() { - // RUFF911 is preview not nursery so the selection should be empty + // RUFF911 is preview so we should warn without selecting let mut cmd = RuffCheck::default() .args(["--select", "RUF911", "--output-format=concise"]) .build(); @@ -987,7 +987,7 @@ fn preview_disabled_direct() { ----- stdout ----- ----- stderr ----- - warning: Selection `RUF911` has no effect because the `--preview` flag was not included. + warning: Selection `RUF911` has no effect because preview is not enabled. "###); } @@ -1003,7 +1003,7 @@ fn preview_disabled_prefix_empty() { ----- stdout ----- ----- stderr ----- - warning: Selection `RUF91` has no effect because the `--preview` flag was not included. + warning: Selection `RUF91` has no effect because preview is not enabled. "###); } @@ -1135,11 +1135,13 @@ def reciprocal(n): try: return 1 / n except ZeroDivisionError: - raise ValueError + raise ValueError() "###), @r###" - success: true - exit_code: 0 + success: false + exit_code: 1 ----- stdout ----- + -:6:9: TRY200 Use `raise from` to specify exception cause + Found 1 error. ----- stderr ----- warning: Rule `TRY200` is deprecated and will be removed in a future release. @@ -1212,7 +1214,7 @@ def reciprocal(n): try: return 1 / n except ZeroDivisionError: - raise ValueError + raise ValueError() "###), @r###" success: false exit_code: 2 @@ -1220,7 +1222,7 @@ def reciprocal(n): ----- stderr ----- ruff failed - Cause: Selection of deprecated rule `TRY200` is not allowed when preview mode is enabled. + Cause: Selection of deprecated rule `TRY200` is not allowed when preview is enabled. "###); } @@ -1236,7 +1238,7 @@ def reciprocal(n): try: return 1 / n except ZeroDivisionError: - raise ValueError + raise ValueError() "###), @r###" success: true exit_code: 0 @@ -1259,7 +1261,7 @@ def reciprocal(n): try: return 1 / n except ZeroDivisionError: - raise ValueError + raise ValueError() "###), @r###" success: false exit_code: 2 @@ -1267,7 +1269,7 @@ def reciprocal(n): ----- stderr ----- ruff failed - Cause: Selection of deprecated rules is not allowed when preview mode is enabled. Remove selection of: + Cause: Selection of deprecated rules is not allowed when preview is enabled. Remove selection of: - ANN102 - ANN101 diff --git a/crates/ruff_linter/src/rule_selector.rs b/crates/ruff_linter/src/rule_selector.rs index 4dec3ebc0e927..b7e0745d903bc 100644 --- a/crates/ruff_linter/src/rule_selector.rs +++ b/crates/ruff_linter/src/rule_selector.rs @@ -172,7 +172,7 @@ impl Visitor<'_> for SelectorVisitor { } impl RuleSelector { - /// Return all matching rules, regardless of whether they're in preview. + /// Return all matching rules, regardless of rule group filters like preview and deprecated. pub fn all_rules(&self) -> impl Iterator + '_ { match self { RuleSelector::All => RuleSelectorIter::All(Rule::iter()), @@ -198,7 +198,7 @@ impl RuleSelector { } } - /// Returns rules matching the selector, taking into account preview options enabled. + /// Returns rules matching the selector, taking into account rule groups like preview and deprecated. pub fn rules<'a>(&'a self, preview: &PreviewOptions) -> impl Iterator + 'a { let preview_enabled = preview.mode.is_enabled(); let preview_require_explicit = preview.require_explicit; @@ -207,16 +207,21 @@ impl RuleSelector { // Always include stable rules rule.is_stable() // Backwards compatibility allows selection of nursery rules by exact code or dedicated group - || ((matches!(self, RuleSelector::Rule { .. }) || matches!(self, RuleSelector::Nursery { .. })) && rule.is_nursery()) + || ((self.is_exact() || matches!(self, RuleSelector::Nursery { .. })) && rule.is_nursery()) // Enabling preview includes all preview or nursery rules unless explicit selection // is turned on - || (preview_enabled && (matches!(self, RuleSelector::Rule { .. }) || !preview_require_explicit)) + || ((rule.is_preview() || rule.is_nursery()) && preview_enabled && (self.is_exact() || !preview_require_explicit)) // Deprecated rules are excluded in preview mode unless explicitly selected - || (rule.is_deprecated() && (!preview_enabled || matches!(self, RuleSelector::Rule { .. }))) + || (rule.is_deprecated() && (!preview_enabled || self.is_exact())) // Removed rules are included if explicitly selected but will error downstream - || (rule.is_removed() && matches!(self, RuleSelector::Rule { .. })) + || (rule.is_removed() && self.is_exact()) }) } + + /// Returns true if this selector is exact i.e. selects a single rule by code + pub fn is_exact(&self) -> bool { + matches!(self, Self::Rule { .. }) + } } pub enum RuleSelectorIter { diff --git a/crates/ruff_linter/src/rules/tryceratops/rules/reraise_no_cause.rs b/crates/ruff_linter/src/rules/tryceratops/rules/reraise_no_cause.rs index 866894d1d4695..19317109888a5 100644 --- a/crates/ruff_linter/src/rules/tryceratops/rules/reraise_no_cause.rs +++ b/crates/ruff_linter/src/rules/tryceratops/rules/reraise_no_cause.rs @@ -25,7 +25,7 @@ use crate::checkers::ast::Checker; /// try: /// return 1 / n /// except ZeroDivisionError: -/// raise ValueError +/// raise ValueError() /// ``` /// /// Use instead: @@ -34,7 +34,7 @@ use crate::checkers::ast::Checker; /// try: /// return 1 / n /// except ZeroDivisionError as exc: -/// raise ValueError from exc +/// raise ValueError() from exc /// ``` /// /// ## References diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 5a25db58c6580..1014a32c4acf6 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -902,8 +902,8 @@ impl LintConfiguration { // Unstable rules if preview.mode.is_disabled() && kind.is_enable() { - if let RuleSelector::Rule { prefix, .. } = selector { - if prefix.rules().any(|rule| rule.is_nursery()) { + if selector.is_exact() { + if selector.all_rules().all(|rule| rule.is_nursery()) { deprecated_nursery_selectors.insert(selector); } } @@ -915,17 +915,15 @@ impl LintConfiguration { } // Deprecated rules - if kind.is_enable() { - if let RuleSelector::Rule { prefix, .. } = selector { - if prefix.rules().any(|rule| rule.is_deprecated()) { - deprecated_selectors.insert(selector); - } + if kind.is_enable() && selector.is_exact() { + if selector.all_rules().all(|rule| rule.is_deprecated()) { + deprecated_selectors.insert(selector.clone()); } } // Removed rules - if let RuleSelector::Rule { prefix, .. } = selector { - if prefix.rules().any(|rule| rule.is_removed()) { + if selector.is_exact() { + if selector.all_rules().all(|rule| rule.is_removed()) { removed_selectors.insert(selector); } } @@ -1007,10 +1005,10 @@ impl LintConfiguration { [] => (), [selection] => { let (prefix, code) = selection.prefix_and_code(); - return Err(anyhow!("Selection of deprecated rule `{prefix}{code}` is not allowed when preview mode is enabled.")); + return Err(anyhow!("Selection of deprecated rule `{prefix}{code}` is not allowed when preview is enabled.")); } [..] => { - let mut message = "Selection of deprecated rules is not allowed when preview mode is enabled. Remove selection of:".to_string(); + let mut message = "Selection of deprecated rules is not allowed when preview is enabled. Remove selection of:".to_string(); for selection in deprecated_selectors { let (prefix, code) = selection.prefix_and_code(); message.push_str("\n\t- "); @@ -1025,9 +1023,7 @@ impl LintConfiguration { for selection in ignored_preview_selectors { let (prefix, code) = selection.prefix_and_code(); - warn_user!( - "Selection `{prefix}{code}` has no effect because the `--preview` flag was not included.", - ); + warn_user!("Selection `{prefix}{code}` has no effect because preview is not enabled.",); } let mut rules = RuleTable::empty();