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
This commit is contained in:
Zanie 2024-02-01 08:57:47 -06:00 committed by Zanie Blue
parent 85a7edcc70
commit e5008ca714
4 changed files with 36 additions and 33 deletions

View file

@ -977,7 +977,7 @@ fn preview_enabled_direct() {
#[test] #[test]
fn preview_disabled_direct() { 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() let mut cmd = RuffCheck::default()
.args(["--select", "RUF911", "--output-format=concise"]) .args(["--select", "RUF911", "--output-format=concise"])
.build(); .build();
@ -987,7 +987,7 @@ fn preview_disabled_direct() {
----- stdout ----- ----- stdout -----
----- stderr ----- ----- 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 ----- ----- stdout -----
----- stderr ----- ----- 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: try:
return 1 / n return 1 / n
except ZeroDivisionError: except ZeroDivisionError:
raise ValueError raise ValueError()
"###), @r###" "###), @r###"
success: true success: false
exit_code: 0 exit_code: 1
----- stdout ----- ----- stdout -----
-:6:9: TRY200 Use `raise from` to specify exception cause
Found 1 error.
----- stderr ----- ----- stderr -----
warning: Rule `TRY200` is deprecated and will be removed in a future release. warning: Rule `TRY200` is deprecated and will be removed in a future release.
@ -1212,7 +1214,7 @@ def reciprocal(n):
try: try:
return 1 / n return 1 / n
except ZeroDivisionError: except ZeroDivisionError:
raise ValueError raise ValueError()
"###), @r###" "###), @r###"
success: false success: false
exit_code: 2 exit_code: 2
@ -1220,7 +1222,7 @@ def reciprocal(n):
----- stderr ----- ----- stderr -----
ruff failed 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: try:
return 1 / n return 1 / n
except ZeroDivisionError: except ZeroDivisionError:
raise ValueError raise ValueError()
"###), @r###" "###), @r###"
success: true success: true
exit_code: 0 exit_code: 0
@ -1259,7 +1261,7 @@ def reciprocal(n):
try: try:
return 1 / n return 1 / n
except ZeroDivisionError: except ZeroDivisionError:
raise ValueError raise ValueError()
"###), @r###" "###), @r###"
success: false success: false
exit_code: 2 exit_code: 2
@ -1267,7 +1269,7 @@ def reciprocal(n):
----- stderr ----- ----- stderr -----
ruff failed 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 - ANN102
- ANN101 - ANN101

View file

@ -172,7 +172,7 @@ impl Visitor<'_> for SelectorVisitor {
} }
impl RuleSelector { 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> + '_ { pub fn all_rules(&self) -> impl Iterator<Item = Rule> + '_ {
match self { match self {
RuleSelector::All => RuleSelectorIter::All(Rule::iter()), 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<Item = Rule> + 'a { pub fn rules<'a>(&'a self, preview: &PreviewOptions) -> impl Iterator<Item = Rule> + 'a {
let preview_enabled = preview.mode.is_enabled(); let preview_enabled = preview.mode.is_enabled();
let preview_require_explicit = preview.require_explicit; let preview_require_explicit = preview.require_explicit;
@ -207,16 +207,21 @@ impl RuleSelector {
// Always include stable rules // Always include stable rules
rule.is_stable() rule.is_stable()
// Backwards compatibility allows selection of nursery rules by exact code or dedicated group // 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 // Enabling preview includes all preview or nursery rules unless explicit selection
// is turned on // 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 // 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 // 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 { pub enum RuleSelectorIter {

View file

@ -25,7 +25,7 @@ use crate::checkers::ast::Checker;
/// try: /// try:
/// return 1 / n /// return 1 / n
/// except ZeroDivisionError: /// except ZeroDivisionError:
/// raise ValueError /// raise ValueError()
/// ``` /// ```
/// ///
/// Use instead: /// Use instead:
@ -34,7 +34,7 @@ use crate::checkers::ast::Checker;
/// try: /// try:
/// return 1 / n /// return 1 / n
/// except ZeroDivisionError as exc: /// except ZeroDivisionError as exc:
/// raise ValueError from exc /// raise ValueError() from exc
/// ``` /// ```
/// ///
/// ## References /// ## References

View file

@ -902,8 +902,8 @@ impl LintConfiguration {
// Unstable rules // Unstable rules
if preview.mode.is_disabled() && kind.is_enable() { if preview.mode.is_disabled() && kind.is_enable() {
if let RuleSelector::Rule { prefix, .. } = selector { if selector.is_exact() {
if prefix.rules().any(|rule| rule.is_nursery()) { if selector.all_rules().all(|rule| rule.is_nursery()) {
deprecated_nursery_selectors.insert(selector); deprecated_nursery_selectors.insert(selector);
} }
} }
@ -915,17 +915,15 @@ impl LintConfiguration {
} }
// Deprecated rules // Deprecated rules
if kind.is_enable() { if kind.is_enable() && selector.is_exact() {
if let RuleSelector::Rule { prefix, .. } = selector { if selector.all_rules().all(|rule| rule.is_deprecated()) {
if prefix.rules().any(|rule| rule.is_deprecated()) { deprecated_selectors.insert(selector.clone());
deprecated_selectors.insert(selector);
}
} }
} }
// Removed rules // Removed rules
if let RuleSelector::Rule { prefix, .. } = selector { if selector.is_exact() {
if prefix.rules().any(|rule| rule.is_removed()) { if selector.all_rules().all(|rule| rule.is_removed()) {
removed_selectors.insert(selector); removed_selectors.insert(selector);
} }
} }
@ -1007,10 +1005,10 @@ impl LintConfiguration {
[] => (), [] => (),
[selection] => { [selection] => {
let (prefix, code) = selection.prefix_and_code(); 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 { for selection in deprecated_selectors {
let (prefix, code) = selection.prefix_and_code(); let (prefix, code) = selection.prefix_and_code();
message.push_str("\n\t- "); message.push_str("\n\t- ");
@ -1025,9 +1023,7 @@ impl LintConfiguration {
for selection in ignored_preview_selectors { for selection in ignored_preview_selectors {
let (prefix, code) = selection.prefix_and_code(); let (prefix, code) = selection.prefix_and_code();
warn_user!( warn_user!("Selection `{prefix}{code}` has no effect because preview is not enabled.",);
"Selection `{prefix}{code}` has no effect because the `--preview` flag was not included.",
);
} }
let mut rules = RuleTable::empty(); let mut rules = RuleTable::empty();