Skip to content

Commit

Permalink
Fix bug where selection included deprecated rules during preview (#9746)
Browse files Browse the repository at this point in the history
Cherry-picked from #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.
  • Loading branch information
zanieb committed Feb 1, 2024
1 parent 798e72c commit 6117ae7
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 32 deletions.
22 changes: 12 additions & 10 deletions crates/ruff/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1006,7 +1006,7 @@ fn preview_disabled_direct() {
----- stdout -----
----- stderr -----
warning: Selection `FURB145` has no effect because the `--preview` flag was not included.
warning: Selection `FURB145` has no effect because preview is not enabled.
"###);
}

Expand All @@ -1021,7 +1021,7 @@ fn preview_disabled_prefix_empty() {
----- stdout -----
----- stderr -----
warning: Selection `CPY` has no effect because the `--preview` flag was not included.
warning: Selection `CPY` has no effect because preview is not enabled.
"###);
}

Expand Down Expand Up @@ -1139,11 +1139,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.
Expand Down Expand Up @@ -1216,15 +1218,15 @@ def reciprocal(n):
try:
return 1 / n
except ZeroDivisionError:
raise ValueError
raise ValueError()
"###), @r###"
success: false
exit_code: 2
----- stdout -----
----- 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.
"###);
}

Expand All @@ -1240,7 +1242,7 @@ def reciprocal(n):
try:
return 1 / n
except ZeroDivisionError:
raise ValueError
raise ValueError()
"###), @r###"
success: true
exit_code: 0
Expand All @@ -1263,15 +1265,15 @@ def reciprocal(n):
try:
return 1 / n
except ZeroDivisionError:
raise ValueError
raise ValueError()
"###), @r###"
success: false
exit_code: 2
----- stdout -----
----- 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
Expand Down
17 changes: 11 additions & 6 deletions crates/ruff_linter/src/rule_selector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Item = Rule> + '_ {
match self {
RuleSelector::All => RuleSelectorIter::All(Rule::iter()),
Expand All @@ -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<Item = Rule> + 'a {
let preview_enabled = preview.mode.is_enabled();
let preview_require_explicit = preview.require_explicit;
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use crate::checkers::ast::Checker;
/// try:
/// return 1 / n
/// except ZeroDivisionError:
/// raise ValueError
/// raise ValueError()
/// ```
///
/// Use instead:
Expand All @@ -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
Expand Down
24 changes: 10 additions & 14 deletions crates/ruff_workspace/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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- ");
Expand All @@ -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();
Expand Down

0 comments on commit 6117ae7

Please sign in to comment.