Add warnings for nursery and preview rule selection (#7210)

## Summary Adds warnings for cases where: - A selector does not include any rules because preview is disabled - A nursery rule is selected without the preview flag ## Test plan Add integration tests
astral-sh · Sep 13, 2023 · ebd1b29 · ebd1b29
1 parent 1373e1c
commit ebd1b29
Show file tree

Hide file tree

Showing 2 changed files with 229 additions and 3 deletions.
diff --git a/crates/ruff_cli/tests/integration_test.rs b/crates/ruff_cli/tests/integration_test.rs
@@ -300,6 +300,195 @@ fn nursery_direct() {
     -:1:2: E225 Missing whitespace around operator
     Found 1 error.
 
+    ----- stderr -----
+    warning: Selection of nursery rule `E225` without the `--preview` flag is deprecated.
+    "###);
+}
+
+#[test]
+fn nursery_group_selector() {
+    // Only nursery rules should be detected e.g. E225 and a warning should be displayed
+    let args = ["--select", "NURSERY"];
+    assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+        .args(STDIN_BASE_OPTIONS)
+        .args(args)
+        .pass_stdin("I=42\n"), @r###"
+    success: false
+    exit_code: 1
+    ----- stdout -----
+    -:1:1: CPY001 Missing copyright notice at top of file
+    -:1:2: E225 Missing whitespace around operator
+    Found 2 errors.
+
+    ----- stderr -----
+    warning: The `NURSERY` selector has been deprecated. Use the `--preview` flag instead.
+    "###);
+}
+
+#[test]
+fn nursery_group_selector_preview_enabled() {
+    // Only nursery rules should be detected e.g. E225 and a warning should be displayed
+    let args = ["--select", "NURSERY", "--preview"];
+    assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+        .args(STDIN_BASE_OPTIONS)
+        .args(args)
+        .pass_stdin("I=42\n"), @r###"
+    success: false
+    exit_code: 1
+    ----- stdout -----
+    -:1:1: CPY001 Missing copyright notice at top of file
+    -:1:2: E225 Missing whitespace around operator
+    Found 2 errors.
+
+    ----- stderr -----
+    warning: The `NURSERY` selector has been deprecated. Use the `PREVIEW` selector instead.
+    "###);
+}
+
+#[test]
+fn preview_enabled_prefix() {
+    // E741 and E225 (preview) should both be detected
+    let args = ["--select", "E", "--preview"];
+    assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+        .args(STDIN_BASE_OPTIONS)
+        .args(args)
+        .pass_stdin("I=42\n"), @r###"
+    success: false
+    exit_code: 1
+    ----- stdout -----
+    -:1:1: E741 Ambiguous variable name: `I`
+    -:1:2: E225 Missing whitespace around operator
+    Found 2 errors.
+
+    ----- stderr -----
+    "###);
+}
+
+#[test]
+fn preview_enabled_all() {
+    let args = ["--select", "ALL", "--preview"];
+    assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+        .args(STDIN_BASE_OPTIONS)
+        .args(args)
+        .pass_stdin("I=42\n"), @r###"
+    success: false
+    exit_code: 1
+    ----- stdout -----
+    -:1:1: E741 Ambiguous variable name: `I`
+    -:1:1: D100 Missing docstring in public module
+    -:1:1: CPY001 Missing copyright notice at top of file
+    -:1:2: E225 Missing whitespace around operator
+    Found 4 errors.
+
+    ----- stderr -----
+    warning: `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Ignoring `one-blank-line-before-class`.
+    warning: `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible. Ignoring `multi-line-summary-second-line`.
+    "###);
+}
+
+#[test]
+fn preview_enabled_direct() {
+    // E225 should be detected without warning
+    let args = ["--select", "E225", "--preview"];
+    assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+        .args(STDIN_BASE_OPTIONS)
+        .args(args)
+        .pass_stdin("I=42\n"), @r###"
+    success: false
+    exit_code: 1
+    ----- stdout -----
+    -:1:2: E225 Missing whitespace around operator
+    Found 1 error.
+
+    ----- stderr -----
+    "###);
+}
+
+#[test]
+fn preview_disabled_direct() {
+    // FURB145 is preview not nursery so selecting should be empty
+    let args = ["--select", "FURB145"];
+    assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+        .args(STDIN_BASE_OPTIONS)
+        .args(args)
+        .pass_stdin("a = l[:]\n"), @r###"
+    success: true
+    exit_code: 0
+    ----- stdout -----
+
+    ----- stderr -----
+    warning: Selection `FURB145` has no effect because the `--preview` flag was not included.
+    "###);
+}
+
+#[test]
+fn preview_disabled_prefix_empty() {
+    // Warns that the selection is empty since all of the CPY rules are in preview
+    let args = ["--select", "CPY"];
+    assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+        .args(STDIN_BASE_OPTIONS)
+        .args(args)
+        .pass_stdin("I=42\n"), @r###"
+    success: true
+    exit_code: 0
+    ----- stdout -----
+
+    ----- stderr -----
+    warning: Selection `CPY` has no effect because the `--preview` flag was not included.
+    "###);
+}
+
+#[test]
+fn preview_disabled_group_selector() {
+    // `--select PREVIEW` should warn without the `--preview` flag
+    let args = ["--select", "PREVIEW"];
+    assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+        .args(STDIN_BASE_OPTIONS)
+        .args(args)
+        .pass_stdin("I=42\n"), @r###"
+    success: true
+    exit_code: 0
+    ----- stdout -----
+
+    ----- stderr -----
+    warning: Selection `PREVIEW` has no effect because the `--preview` flag was not included.
+    "###);
+}
+
+#[test]
+fn preview_enabled_group_selector() {
+    // `--select PREVIEW` is okay with the `--preview` flag and shouldn't warn
+    let args = ["--select", "PREVIEW", "--preview"];
+    assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+        .args(STDIN_BASE_OPTIONS)
+        .args(args)
+        .pass_stdin("I=42\n"), @r###"
+    success: false
+    exit_code: 1
+    ----- stdout -----
+    -:1:1: CPY001 Missing copyright notice at top of file
+    -:1:2: E225 Missing whitespace around operator
+    Found 2 errors.
+
+    ----- stderr -----
+    "###);
+}
+
+#[test]
+fn preview_enabled_group_ignore() {
+    // `--select E --ignore PREVIEW` should detect E741 and E225, which is in preview but "E" is more specific.
+    let args = ["--select", "E", "--ignore", "PREVIEW", "--preview"];
+    assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+        .args(STDIN_BASE_OPTIONS)
+        .args(args)
+        .pass_stdin("I=42\n"), @r###"
+    success: false
+    exit_code: 1
+    ----- stdout -----
+    -:1:1: E741 Ambiguous variable name: `I`
+    -:1:2: E225 Missing whitespace around operator
+    Found 2 errors.
+
     ----- stderr -----
     "###);
 }

diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs
@@ -27,7 +27,7 @@ use ruff::settings::types::{
     Version,
 };
 use ruff::settings::{defaults, resolve_per_file_ignores, AllSettings, CliSettings, Settings};
-use ruff::{fs, warn_user_once_by_id, RuleSelector, RUFF_PKG_VERSION};
+use ruff::{fs, warn_user, warn_user_once, warn_user_once_by_id, RuleSelector, RUFF_PKG_VERSION};
 use ruff_cache::cache_dir;
 use rustc_hash::{FxHashMap, FxHashSet};
 use shellexpand;
@@ -460,7 +460,10 @@ impl Configuration {
         let mut carryover_ignores: Option<&[RuleSelector]> = None;
         let mut carryover_unfixables: Option<&[RuleSelector]> = None;
 
+        // Store selectors for displaying warnings
         let mut redirects = FxHashMap::default();
+        let mut deprecated_nursery_selectors = FxHashSet::default();
+        let mut ignored_preview_selectors = FxHashSet::default();
 
         for selection in &self.rule_selections {
             // If a selection only specifies extend-select we cannot directly
@@ -571,8 +574,7 @@ impl Configuration {
                 }
             }
 
-            // We insert redirects into the hashmap so that we
-            // can warn the users about remapped rule codes.
+            // Check for selections that require a warning
             for selector in selection
                 .select
                 .iter()
@@ -583,6 +585,29 @@ impl Configuration {
                 .chain(selection.unfixable.iter())
                 .chain(selection.extend_fixable.iter())
             {
+                #[allow(deprecated)]
+                if matches!(selector, RuleSelector::Nursery) {
+                    let suggestion = if preview.is_disabled() {
+                        "Use the `--preview` flag instead."
+                    } else {
+                        "Use the `PREVIEW` selector instead."
+                    };
+                    warn_user_once!("The `NURSERY` selector has been deprecated. {suggestion}");
+                }
+
+                if preview.is_disabled() {
+                    if let RuleSelector::Rule { prefix, .. } = selector {
+                        if prefix.rules().any(|rule| rule.is_nursery()) {
+                            deprecated_nursery_selectors.insert(selector);
+                        }
+                    }
+
+                    // Check if the selector is empty because preview mode is disabled
+                    if selector.rules(PreviewMode::Disabled).next().is_none() {
+                        ignored_preview_selectors.insert(selector);
+                    }
+                }
+
                 if let RuleSelector::Prefix {
                     prefix,
                     redirected_from: Some(redirect_from),
@@ -603,6 +628,18 @@ impl Configuration {
             );
         }
 
+        for selection in deprecated_nursery_selectors {
+            let (prefix, code) = selection.prefix_and_code();
+            warn_user!("Selection of nursery rule `{prefix}{code}` without the `--preview` flag is deprecated.",);
+        }
+
+        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.",
+            );
+        }
+
         let mut rules = RuleTable::empty();
 
         for rule in select_set {