From e5bf2301bb2a949b628941983b3743240e0bb4cb Mon Sep 17 00:00:00 2001 From: Zanie Date: Tue, 30 Jan 2024 13:11:31 -0600 Subject: [PATCH] Add handling for rules that are both deprecated and redirected --- crates/ruff/tests/integration_test.rs | 97 ++++++++++++++++++++++ crates/ruff_linter/src/rule_redirects.rs | 3 + crates/ruff_linter/src/rule_selector.rs | 35 ++++++++ crates/ruff_workspace/src/configuration.rs | 70 ++++++++++++++-- 4 files changed, 199 insertions(+), 6 deletions(-) diff --git a/crates/ruff/tests/integration_test.rs b/crates/ruff/tests/integration_test.rs index 1b541ae330..68bf81774c 100644 --- a/crates/ruff/tests/integration_test.rs +++ b/crates/ruff/tests/integration_test.rs @@ -1204,6 +1204,26 @@ class Foo: "###); } +#[test] +fn deprecated_redirected() { + // Selection of a redirected deprecated rule without preview enabled should still work + // but a warning should be displayed + let mut cmd = RuffCheck::default().args(["--select", "PGH001"]).build(); + assert_cmd_snapshot!(cmd + .pass_stdin(r###" +x = eval(input("Enter a number: ")) +"###), @r###" + success: false + exit_code: 1 + ----- stdout ----- + -:2:5: S307 Use of possibly insecure function; consider using `ast.literal_eval` + Found 1 error. + + ----- stderr ----- + warning: Rule `PGH001` is deprecated and will be removed in a future release. Use `S307` instead. + "###); +} + #[test] fn deprecated_direct_preview_enabled() { // Direct selection of a deprecated rule in preview should fail @@ -1228,6 +1248,25 @@ def reciprocal(n): "###); } +#[test] +fn deprecated_redirect_preview_enabled() { + // Direct selection of a deprecated rule in preview should fail + let mut cmd = RuffCheck::default() + .args(["--select", "PGH001", "--preview"]) + .build(); + assert_cmd_snapshot!(cmd + .pass_stdin(r###" +x = eval(input("Enter a number: ")) +"###), @r###" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + ruff failed + Cause: Selection of deprecated rule `PGH001` is not allowed when preview mode is enabled. Use `S307` instead. + "###); +} #[test] fn deprecated_indirect_preview_enabled() { // `TRY200` is deprecated and should be off by default in preview. @@ -1250,6 +1289,24 @@ def reciprocal(n): "###); } +#[test] +fn deprecated_indirect_redirect_preview_enabled() { + // e.g. `PGH001` is deprecated and redirected and should be off by default in preview. + let mut cmd = RuffCheck::default() + .args(["--select", "PGH", "--preview"]) + .build(); + assert_cmd_snapshot!(cmd + .pass_stdin(r###" +x = eval(input("Enter a number: ")) +"###), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + "###); +} + #[test] fn deprecated_multiple_direct_preview_enabled() { // Direct selection of the deprecated rules in preview should fail with @@ -1278,6 +1335,46 @@ def reciprocal(n): "###); } +#[test] +fn deprecated_multiple_direct_and_redirected_preview_enabled() { + // Direct selection of the deprecated rules in preview should fail with + // a message listing all of the rule codes + // The redirected rules should include an alternative + let mut cmd = RuffCheck::default() + .args([ + "--select", + "ANN101", + "--select", + "ANN102", + "--select", + "PGH001", + "--preview", + ]) + .build(); + assert_cmd_snapshot!(cmd + .pass_stdin(r###" +x = eval(input("Enter a number: ")) + +def reciprocal(n): + try: + return 1 / n + except ZeroDivisionError: + 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: + - ANN102 + - ANN101 + - PGH001 (use `S307` instead) + + "###); +} + /// An unreadable pyproject.toml in non-isolated mode causes ruff to hard-error trying to build up /// configuration globs #[cfg(unix)] diff --git a/crates/ruff_linter/src/rule_redirects.rs b/crates/ruff_linter/src/rule_redirects.rs index 7115a252a9..7e74bdafd8 100644 --- a/crates/ruff_linter/src/rule_redirects.rs +++ b/crates/ruff_linter/src/rule_redirects.rs @@ -21,6 +21,9 @@ static REDIRECTS: Lazy> = Lazy::new(|| { ("C9", "C90"), ("T1", "T10"), ("T2", "T20"), + // The PGH category is deprecated + ("PGH001", "S307"), + ("PGH002", "G010"), // TODO(charlie): Remove by 2023-02-01. ("R", "RET"), ("R5", "RET5"), diff --git a/crates/ruff_linter/src/rule_selector.rs b/crates/ruff_linter/src/rule_selector.rs index 4dec3ebc0e..37afac41c3 100644 --- a/crates/ruff_linter/src/rule_selector.rs +++ b/crates/ruff_linter/src/rule_selector.rs @@ -126,6 +126,41 @@ impl RuleSelector { RuleSelector::Linter(l) => (l.common_prefix(), ""), } } + + /// Parse [`RuleSelector`] from a string; but do not follow redirects. + pub fn from_str_no_redirect(s: &str) -> Result { + match s { + "ALL" => Ok(Self::All), + #[allow(deprecated)] + "NURSERY" => Ok(Self::Nursery), + "C" => Ok(Self::C), + "T" => Ok(Self::T), + _ => { + let (linter, code) = + Linter::parse_code(s).ok_or_else(|| ParseError::Unknown(s.to_string()))?; + + if code.is_empty() { + return Ok(Self::Linter(linter)); + } + + // Does the selector select a single rule? + let prefix = RuleCodePrefix::parse(&linter, code) + .map_err(|_| ParseError::Unknown(s.to_string()))?; + + if is_single_rule_selector(&prefix) { + Ok(Self::Rule { + prefix, + redirected_from: None, + }) + } else { + Ok(Self::Prefix { + prefix, + redirected_from: None, + }) + } + } + } + } } impl Serialize for RuleSelector { diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 4b51bf2c7a..5cb3bb69ed 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -756,6 +756,7 @@ impl LintConfiguration { let mut redirects = FxHashMap::default(); let mut deprecated_nursery_selectors = FxHashSet::default(); let mut deprecated_selectors = FxHashSet::default(); + let mut deprecated_redirected_selectors = FxHashSet::default(); let mut removed_selectors = FxHashSet::default(); let mut ignored_preview_selectors = FxHashSet::default(); @@ -916,9 +917,31 @@ impl LintConfiguration { // Deprecated rules if kind.is_enable() { - if let RuleSelector::Rule { prefix, .. } = selector { + if let RuleSelector::Rule { + prefix, + redirected_from: None, + } = selector + { if prefix.rules().any(|rule| rule.is_deprecated()) { - deprecated_selectors.insert(selector); + deprecated_selectors.insert(selector.clone()); + } + } + if let RuleSelector::Rule { + prefix: redirected_to, + redirected_from: Some(redirected_from), + } = selector + { + if let Ok(redirected_selector) = + RuleSelector::from_str_no_redirect(redirected_from) + { + if let RuleSelector::Rule { ref prefix, .. } + | RuleSelector::Prefix { ref prefix, .. } = redirected_selector + { + if prefix.rules().all(|rule| rule.is_deprecated()) { + deprecated_redirected_selectors + .insert((redirected_selector, redirected_to)); + } + } } } } @@ -1001,21 +1024,56 @@ impl LintConfiguration { "Rule `{prefix}{code}` is deprecated and will be removed in a future release.", ); } + for (selection, redirected_to) in deprecated_redirected_selectors { + let (prefix, code) = selection.prefix_and_code(); + let (redirect_prefix, redirect_code) = ( + redirected_to.linter().common_prefix(), + redirected_to.short_code(), + ); + warn_user!( + "Rule `{prefix}{code}` is deprecated and will be removed in a future release. Use `{redirect_prefix}{redirect_code}` instead.", + ); + } } else { - let deprecated_selectors = deprecated_selectors.iter().collect::>(); + // Combine the deprecated selectors and those that have been redirected + let deprecated_selectors = deprecated_selectors + .iter() + .map(|selector| (selector, None)) + .chain( + deprecated_redirected_selectors + .iter() + .map(|(selector, redirect)| (selector, Some(redirect))), + ) + .collect::>(); + match deprecated_selectors.as_slice() { [] => (), - [selection] => { + [(selection, redirect)] => { let (prefix, code) = selection.prefix_and_code(); - return Err(anyhow!("Selection of deprecated rule `{prefix}{code}` is not allowed when preview mode is enabled.")); + let err = if let Some(redirect) = redirect { + let (redirect_prefix, redirect_code) = + (redirect.linter().common_prefix(), redirect.short_code()); + anyhow!("Selection of deprecated rule `{prefix}{code}` is not allowed when preview mode is enabled. Use `{redirect_prefix}{redirect_code}` instead.") + } else { + anyhow!("Selection of deprecated rule `{prefix}{code}` is not allowed when preview mode is enabled.") + }; + return Err(err); } [..] => { let mut message = "Selection of deprecated rules is not allowed when preview mode is enabled. Remove selection of:".to_string(); - for selection in deprecated_selectors { + for (selection, redirect) in deprecated_selectors { let (prefix, code) = selection.prefix_and_code(); message.push_str("\n\t- "); message.push_str(prefix); message.push_str(code); + if let Some(redirect) = redirect { + let (redirect_prefix, redirect_code) = + (redirect.linter().common_prefix(), redirect.short_code()); + message.push_str( + format!(" (use `{redirect_prefix}{redirect_code}` instead)") + .as_str(), + ) + } } message.push('\n'); return Err(anyhow!(message));