Add tests for redirected rules (#9754)

Extends #9752 adding internal test rules for redirection Fixes a bug where we did not see warnings for exact codes that are redirected (just prefixes)
astral-sh · Feb 1, 2024 · 003a360 · 003a360
1 parent dcc69f2
commit 003a360
Show file tree

Hide file tree

Showing 6 changed files with 195 additions and 6 deletions.
diff --git a/crates/ruff/tests/integration_test.rs b/crates/ruff/tests/integration_test.rs
@@ -826,7 +826,8 @@ fn nursery_prefix() {
     -:1:1: RUF903 Hey this is a stable test rule with a display only fix.
     -:1:1: RUF920 Hey this is a deprecated test rule.
     -:1:1: RUF921 Hey this is another deprecated test rule.
-    Found 6 errors.
+    -:1:1: RUF950 Hey this is a test rule that was redirected from another.
+    Found 7 errors.
     [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
 
     ----- stderr -----
@@ -850,7 +851,8 @@ fn nursery_all() {
     -:1:1: RUF903 Hey this is a stable test rule with a display only fix.
     -:1:1: RUF920 Hey this is a deprecated test rule.
     -:1:1: RUF921 Hey this is another deprecated test rule.
-    Found 7 errors.
+    -:1:1: RUF950 Hey this is a test rule that was redirected from another.
+    Found 8 errors.
     [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
 
     ----- stderr -----
@@ -929,7 +931,8 @@ fn preview_enabled_prefix() {
     -:1:1: RUF903 Hey this is a stable test rule with a display only fix.
     -:1:1: RUF911 Hey this is a preview test rule.
     -:1:1: RUF912 Hey this is a nursery test rule.
-    Found 6 errors.
+    -:1:1: RUF950 Hey this is a test rule that was redirected from another.
+    Found 7 errors.
     [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
 
     ----- stderr -----
@@ -953,7 +956,8 @@ fn preview_enabled_all() {
     -:1:1: RUF903 Hey this is a stable test rule with a display only fix.
     -:1:1: RUF911 Hey this is a preview test rule.
     -:1:1: RUF912 Hey this is a nursery test rule.
-    Found 8 errors.
+    -:1:1: RUF950 Hey this is a test rule that was redirected from another.
+    Found 9 errors.
     [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
 
     ----- stderr -----
@@ -1088,7 +1092,8 @@ fn preview_enabled_group_ignore() {
     -:1:1: RUF903 Hey this is a stable test rule with a display only fix.
     -:1:1: RUF911 Hey this is a preview test rule.
     -:1:1: RUF912 Hey this is a nursery test rule.
-    Found 6 errors.
+    -:1:1: RUF950 Hey this is a test rule that was redirected from another.
+    Found 7 errors.
     [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
 
     ----- stderr -----
@@ -1145,6 +1150,53 @@ fn removed_indirect() {
     "###);
 }
 
+#[test]
+fn redirect_direct() {
+    // Selection of a redirected rule directly should use the new rule and warn
+    let mut cmd = RuffCheck::default().args(["--select", "RUF940"]).build();
+    assert_cmd_snapshot!(cmd, @r###"
+    success: false
+    exit_code: 1
+    ----- stdout -----
+    -:1:1: RUF950 Hey this is a test rule that was redirected from another.
+    Found 1 error.
+
+    ----- stderr -----
+    warning: `RUF940` has been remapped to `RUF950`.
+    "###);
+}
+
+#[test]
+fn redirect_indirect() {
+    // Selection _including_ a redirected rule without matching should not fail
+    // nor should the rule be used
+    let mut cmd = RuffCheck::default().args(["--select", "RUF94"]).build();
+    assert_cmd_snapshot!(cmd, @r###"
+    success: true
+    exit_code: 0
+    ----- stdout -----
+
+    ----- stderr -----
+    "###);
+}
+
+#[test]
+fn redirect_prefix() {
+    // Selection using a redirected prefix should switch to all rules in the
+    // new prefix
+    let mut cmd = RuffCheck::default().args(["--select", "RUF96"]).build();
+    assert_cmd_snapshot!(cmd, @r###"
+    success: false
+    exit_code: 1
+    ----- stdout -----
+    -:1:1: RUF950 Hey this is a test rule that was redirected from another.
+    Found 1 error.
+
+    ----- stderr -----
+    warning: `RUF96` has been remapped to `RUF95`.
+    "###);
+}
+
 #[test]
 fn deprecated_direct() {
     // Selection of a deprecated rule without preview enabled should still work
@@ -1770,7 +1822,8 @@ extend-safe-fixes = ["RUF9"]
     -:1:1: RUF903 Hey this is a stable test rule with a display only fix.
     -:1:1: RUF920 Hey this is a deprecated test rule.
     -:1:1: RUF921 Hey this is another deprecated test rule.
-    Found 6 errors.
+    -:1:1: RUF950 Hey this is a test rule that was redirected from another.
+    Found 7 errors.
     [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
 
     ----- stderr -----

diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs
@@ -959,6 +959,12 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Ruff, "930") => (RuleGroup::Removed, rules::ruff::rules::RemovedTestRule),
         #[cfg(feature = "test-rules")]
         (Ruff, "931") => (RuleGroup::Removed, rules::ruff::rules::AnotherRemovedTestRule),
+        #[cfg(feature = "test-rules")]
+        (Ruff, "940") => (RuleGroup::Removed, rules::ruff::rules::RedirectedFromTestRule),
+        #[cfg(feature = "test-rules")]
+        (Ruff, "950") => (RuleGroup::Stable, rules::ruff::rules::RedirectedToTestRule),
+        #[cfg(feature = "test-rules")]
+        (Ruff, "960") => (RuleGroup::Removed, rules::ruff::rules::RedirectedFromPrefixTestRule),
 
 
         // flake8-django

diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs
@@ -246,6 +246,15 @@ pub fn check_path(
                 Rule::AnotherRemovedTestRule => {
                     test_rules::AnotherRemovedTestRule::diagnostic(locator, indexer)
                 }
+                Rule::RedirectedToTestRule => {
+                    test_rules::RedirectedToTestRule::diagnostic(locator, indexer)
+                }
+                Rule::RedirectedFromTestRule => {
+                    test_rules::RedirectedFromTestRule::diagnostic(locator, indexer)
+                }
+                Rule::RedirectedFromPrefixTestRule => {
+                    test_rules::RedirectedFromPrefixTestRule::diagnostic(locator, indexer)
+                }
                 _ => unreachable!("All test rules must have an implementation"),
             };
             if let Some(diagnostic) = diagnostic {

diff --git a/crates/ruff_linter/src/rule_redirects.rs b/crates/ruff_linter/src/rule_redirects.rs
@@ -100,5 +100,11 @@ static REDIRECTS: Lazy<HashMap<&'static str, &'static str>> = Lazy::new(|| {
         ("T004", "FIX004"),
         ("RUF011", "B035"),
         ("TCH006", "TCH010"),
+        // Test redirect by exact code
+        #[cfg(feature = "test-rules")]
+        ("RUF940", "RUF950"),
+        // Test redirect by prefix
+        #[cfg(feature = "test-rules")]
+        ("RUF96", "RUF95"),
     ])
 });
diff --git a/crates/ruff_linter/src/rules/ruff/rules/test_rules.rs b/crates/ruff_linter/src/rules/ruff/rules/test_rules.rs
@@ -43,6 +43,9 @@ pub(crate) const TEST_RULES: &[Rule] = &[
     Rule::AnotherDeprecatedTestRule,
     Rule::RemovedTestRule,
     Rule::AnotherRemovedTestRule,
+    Rule::RedirectedFromTestRule,
+    Rule::RedirectedToTestRule,
+    Rule::RedirectedFromPrefixTestRule,
 ];
 
 pub(crate) trait TestRule {
@@ -438,3 +441,111 @@ impl TestRule for AnotherRemovedTestRule {
         ))
     }
 }
+
+/// ## What it does
+/// Fake rule for testing.
+///
+/// ## Why is this bad?
+/// Tests must pass!
+///
+/// ## Example
+/// ```python
+/// foo
+/// ```
+///
+/// Use instead:
+/// ```python
+/// bar
+/// ```
+#[violation]
+pub struct RedirectedFromTestRule;
+
+impl Violation for RedirectedFromTestRule {
+    const FIX_AVAILABILITY: FixAvailability = FixAvailability::None;
+
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("Hey this is a test rule that was redirected to another.")
+    }
+}
+
+impl TestRule for RedirectedFromTestRule {
+    fn diagnostic(_locator: &Locator, _indexer: &Indexer) -> Option<Diagnostic> {
+        Some(Diagnostic::new(
+            RedirectedFromTestRule,
+            ruff_text_size::TextRange::default(),
+        ))
+    }
+}
+
+/// ## What it does
+/// Fake rule for testing.
+///
+/// ## Why is this bad?
+/// Tests must pass!
+///
+/// ## Example
+/// ```python
+/// foo
+/// ```
+///
+/// Use instead:
+/// ```python
+/// bar
+/// ```
+#[violation]
+pub struct RedirectedToTestRule;
+
+impl Violation for RedirectedToTestRule {
+    const FIX_AVAILABILITY: FixAvailability = FixAvailability::None;
+
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("Hey this is a test rule that was redirected from another.")
+    }
+}
+
+impl TestRule for RedirectedToTestRule {
+    fn diagnostic(_locator: &Locator, _indexer: &Indexer) -> Option<Diagnostic> {
+        Some(Diagnostic::new(
+            RedirectedToTestRule,
+            ruff_text_size::TextRange::default(),
+        ))
+    }
+}
+
+/// ## What it does
+/// Fake rule for testing.
+///
+/// ## Why is this bad?
+/// Tests must pass!
+///
+/// ## Example
+/// ```python
+/// foo
+/// ```
+///
+/// Use instead:
+/// ```python
+/// bar
+/// ```
+#[violation]
+pub struct RedirectedFromPrefixTestRule;
+
+impl Violation for RedirectedFromPrefixTestRule {
+    const FIX_AVAILABILITY: FixAvailability = FixAvailability::None;
+
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("Hey this is a test rule that was redirected to another by prefix.")
+    }
+}
+
+impl TestRule for RedirectedFromPrefixTestRule {
+    fn diagnostic(_locator: &Locator, _indexer: &Indexer) -> Option<Diagnostic> {
+        Some(Diagnostic::new(
+            RedirectedFromPrefixTestRule,
+            ruff_text_size::TextRange::default(),
+        ))
+    }
+}
diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs
@@ -940,6 +940,10 @@ impl LintConfiguration {
                 if let RuleSelector::Prefix {
                     prefix,
                     redirected_from: Some(redirect_from),
+                }
+                | RuleSelector::Rule {
+                    prefix,
+                    redirected_from: Some(redirect_from),
                 } = selector
                 {
                     redirects.insert(redirect_from, prefix);