From 5c75da980f83d09478441ae65124e683b79ba0be Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Tue, 30 Jan 2024 11:38:25 -0600 Subject: [PATCH] Add rule deprecation infrastructure (#9689) Adds a new `Deprecated` rule group in addition to `Stable` and `Preview`. Deprecated rules: - Warn on explicit selection without preview - Error on explicit selection with preview - Are excluded when selected by prefix with preview Deprecates `TRY200`, `ANN101`, and `ANN102` as a proof of concept. We can consider deprecating them separately. --- crates/ruff/tests/integration_test.rs | 150 ++++++++++++++++++ crates/ruff_dev/src/generate_docs.rs | 10 +- crates/ruff_dev/src/generate_rules_table.rs | 53 +++++-- crates/ruff_linter/src/codes.rs | 9 +- crates/ruff_linter/src/rule_selector.rs | 6 +- .../flake8_annotations/rules/definition.rs | 8 + .../tryceratops/rules/reraise_no_cause.rs | 5 + crates/ruff_macros/src/map_codes.rs | 8 + crates/ruff_workspace/src/configuration.rs | 46 +++++- docs/preview.md | 5 + 10 files changed, 279 insertions(+), 21 deletions(-) diff --git a/crates/ruff/tests/integration_test.rs b/crates/ruff/tests/integration_test.rs index 0b3ce01702b03..051087ede1fda 100644 --- a/crates/ruff/tests/integration_test.rs +++ b/crates/ruff/tests/integration_test.rs @@ -1095,6 +1095,156 @@ fn preview_enabled_group_ignore() { "###); } +#[test] +fn deprecated_direct() { + // Selection of a deprecated rule without preview enabled should still work + // but a warning should be displayed + let mut cmd = RuffCheck::default().args(["--select", "TRY200"]).build(); + assert_cmd_snapshot!(cmd + .pass_stdin(r###" +def reciprocal(n): + try: + return 1 / n + except ZeroDivisionError: + raise ValueError +"###), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + warning: Rule `TRY200` is deprecated and will be removed in a future release. + "###); +} + +#[test] +fn deprecated_multiple_direct() { + let mut cmd = RuffCheck::default() + .args(["--select", "ANN101", "--select", "ANN102"]) + .build(); + assert_cmd_snapshot!(cmd + .pass_stdin(r###" +class Foo: + def a(self): + pass + + @classmethod + def b(cls): + pass +"###), @r###" + success: false + exit_code: 1 + ----- stdout ----- + -:3:11: ANN101 Missing type annotation for `self` in method + -:7:11: ANN102 Missing type annotation for `cls` in classmethod + Found 2 errors. + + ----- stderr ----- + warning: Rule `ANN102` is deprecated and will be removed in a future release. + warning: Rule `ANN101` is deprecated and will be removed in a future release. + "###); +} + +#[test] +fn deprecated_indirect() { + // `ANN` includes deprecated rules `ANN101` and `ANN102` but should not warn + // since it is not a "direct" selection + let mut cmd = RuffCheck::default().args(["--select", "ANN1"]).build(); + assert_cmd_snapshot!(cmd + .pass_stdin(r###" +class Foo: + def a(self): + pass + + @classmethod + def b(cls): + pass +"###), @r###" + success: false + exit_code: 1 + ----- stdout ----- + -:3:11: ANN101 Missing type annotation for `self` in method + -:7:11: ANN102 Missing type annotation for `cls` in classmethod + Found 2 errors. + + ----- stderr ----- + "###); +} + +#[test] +fn deprecated_direct_preview_enabled() { + // Direct selection of a deprecated rule in preview should fail + let mut cmd = RuffCheck::default() + .args(["--select", "TRY200", "--preview"]) + .build(); + assert_cmd_snapshot!(cmd + .pass_stdin(r###" +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 rule `TRY200` is not allowed when preview mode is enabled. + "###); +} + +#[test] +fn deprecated_indirect_preview_enabled() { + // `TRY200` is deprecated and should be off by default in preview. + let mut cmd = RuffCheck::default() + .args(["--select", "TRY", "--preview"]) + .build(); + assert_cmd_snapshot!(cmd + .pass_stdin(r###" +def reciprocal(n): + try: + return 1 / n + except ZeroDivisionError: + raise ValueError +"###), @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 + // a message listing all of the rule codes + let mut cmd = RuffCheck::default() + .args(["--select", "ANN101", "--select", "ANN102", "--preview"]) + .build(); + assert_cmd_snapshot!(cmd + .pass_stdin(r###" +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 + + "###); +} + /// 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_dev/src/generate_docs.rs b/crates/ruff_dev/src/generate_docs.rs index 293f99c5dbca7..45421ba1907d4 100644 --- a/crates/ruff_dev/src/generate_docs.rs +++ b/crates/ruff_dev/src/generate_docs.rs @@ -26,9 +26,9 @@ pub(crate) fn main(args: &Args) -> Result<()> { for rule in Rule::iter() { if let Some(explanation) = rule.explanation() { let mut output = String::new(); + output.push_str(&format!("# {} ({})", rule.as_ref(), rule.noqa_code())); output.push('\n'); - output.push('\n'); let (linter, _) = Linter::parse_code(&rule.noqa_code().to_string()).unwrap(); if linter.url().is_some() { @@ -37,6 +37,14 @@ pub(crate) fn main(args: &Args) -> Result<()> { output.push('\n'); } + if rule.is_deprecated() { + output.push_str( + r"**Warning: This rule is deprecated and will be removed in a future release.**", + ); + output.push('\n'); + output.push('\n'); + } + let fix_availability = rule.fixable(); if matches!( fix_availability, diff --git a/crates/ruff_dev/src/generate_rules_table.rs b/crates/ruff_dev/src/generate_rules_table.rs index 8499a6d900d81..ad0aaa4e658f0 100644 --- a/crates/ruff_dev/src/generate_rules_table.rs +++ b/crates/ruff_dev/src/generate_rules_table.rs @@ -3,6 +3,7 @@ //! Used for . use itertools::Itertools; +use ruff_linter::codes::RuleGroup; use std::borrow::Cow; use strum::IntoEnumIterator; @@ -14,6 +15,9 @@ use ruff_workspace::options_base::OptionsMetadata; const FIX_SYMBOL: &str = "🛠️"; const PREVIEW_SYMBOL: &str = "🧪"; +const WARNING_SYMBOL: &str = "⚠️"; +const STABLE_SYMBOL: &str = "✔️"; +const SPACER: &str = "    "; fn generate_table(table_out: &mut String, rules: impl IntoIterator, linter: &Linter) { table_out.push_str("| Code | Name | Message | |"); @@ -21,20 +25,30 @@ fn generate_table(table_out: &mut String, rules: impl IntoIterator, table_out.push_str("| ---- | ---- | ------- | ------: |"); table_out.push('\n'); for rule in rules { + let status_token = match rule.group() { + RuleGroup::Deprecated => { + format!("{WARNING_SYMBOL}") + } + #[allow(deprecated)] + RuleGroup::Preview | RuleGroup::Nursery => { + format!("{PREVIEW_SYMBOL}") + } + RuleGroup::Stable => { + // A full opacity checkmark is a bit aggressive for indicating stable + format!("{STABLE_SYMBOL}") + } + }; + let fix_token = match rule.fixable() { FixAvailability::Always | FixAvailability::Sometimes => { format!("{FIX_SYMBOL}") } FixAvailability::None => { - format!("") + format!("") } }; - let preview_token = if rule.is_preview() || rule.is_nursery() { - format!("{PREVIEW_SYMBOL}") - } else { - format!("") - }; - let status_token = format!("{fix_token} {preview_token}"); + + let tokens = format!("{status_token} {fix_token}"); let rule_name = rule.as_ref(); @@ -58,7 +72,7 @@ fn generate_table(table_out: &mut String, rules: impl IntoIterator, .then_some(format_args!("[{rule_name}](rules/{rule_name}.md)")) .unwrap_or(format_args!("{rule_name}")), message, - status_token, + tokens, )); table_out.push('\n'); } @@ -69,15 +83,28 @@ pub(crate) fn generate() -> String { // Generate the table string. let mut table_out = String::new(); - table_out.push_str(&format!( - "The {FIX_SYMBOL} emoji indicates that a rule is automatically fixable by the `--fix` command-line option.")); - table_out.push('\n'); + table_out.push_str("### Legend"); table_out.push('\n'); table_out.push_str(&format!( - "The {PREVIEW_SYMBOL} emoji indicates that a rule is in [\"preview\"](faq.md#what-is-preview)." + "{SPACER}{STABLE_SYMBOL}{SPACER} The rule is stable." )); - table_out.push('\n'); + table_out.push_str("
"); + + table_out.push_str(&format!( + "{SPACER}{PREVIEW_SYMBOL}{SPACER} The rule is unstable and is in [\"preview\"](faq.md#what-is-preview)." + )); + table_out.push_str("
"); + + table_out.push_str(&format!( + "{SPACER}{WARNING_SYMBOL}{SPACER} The rule has been deprecated and will be removed in a future release." + )); + table_out.push_str("
"); + + table_out.push_str(&format!( + "{SPACER}{FIX_SYMBOL}{SPACER} The rule is automatically fixable by the `--fix` command-line option." + )); + table_out.push_str("
"); table_out.push('\n'); for linter in Linter::iter() { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 558d55b9738df..b89b2082b9b9c 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -52,6 +52,9 @@ pub enum RuleGroup { Stable, /// The rule is unstable, and preview mode must be enabled for usage. Preview, + /// The rule has been deprecated, warnings will be displayed during selection in stable + /// and errors will be raised if used with preview mode enabled. + Deprecated, /// Legacy category for unstable rules, supports backwards compatible selection. #[deprecated(note = "Use `RuleGroup::Preview` for new rules instead")] Nursery, @@ -424,8 +427,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Annotations, "001") => (RuleGroup::Stable, rules::flake8_annotations::rules::MissingTypeFunctionArgument), (Flake8Annotations, "002") => (RuleGroup::Stable, rules::flake8_annotations::rules::MissingTypeArgs), (Flake8Annotations, "003") => (RuleGroup::Stable, rules::flake8_annotations::rules::MissingTypeKwargs), - (Flake8Annotations, "101") => (RuleGroup::Stable, rules::flake8_annotations::rules::MissingTypeSelf), - (Flake8Annotations, "102") => (RuleGroup::Stable, rules::flake8_annotations::rules::MissingTypeCls), + (Flake8Annotations, "101") => (RuleGroup::Deprecated, rules::flake8_annotations::rules::MissingTypeSelf), + (Flake8Annotations, "102") => (RuleGroup::Deprecated, rules::flake8_annotations::rules::MissingTypeCls), (Flake8Annotations, "201") => (RuleGroup::Stable, rules::flake8_annotations::rules::MissingReturnTypeUndocumentedPublicFunction), (Flake8Annotations, "202") => (RuleGroup::Stable, rules::flake8_annotations::rules::MissingReturnTypePrivateFunction), (Flake8Annotations, "204") => (RuleGroup::Stable, rules::flake8_annotations::rules::MissingReturnTypeSpecialMethod), @@ -842,7 +845,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Tryceratops, "002") => (RuleGroup::Stable, rules::tryceratops::rules::RaiseVanillaClass), (Tryceratops, "003") => (RuleGroup::Stable, rules::tryceratops::rules::RaiseVanillaArgs), (Tryceratops, "004") => (RuleGroup::Stable, rules::tryceratops::rules::TypeCheckWithoutTypeError), - (Tryceratops, "200") => (RuleGroup::Stable, rules::tryceratops::rules::ReraiseNoCause), + (Tryceratops, "200") => (RuleGroup::Deprecated, rules::tryceratops::rules::ReraiseNoCause), (Tryceratops, "201") => (RuleGroup::Stable, rules::tryceratops::rules::VerboseRaise), (Tryceratops, "300") => (RuleGroup::Stable, rules::tryceratops::rules::TryConsiderElse), (Tryceratops, "301") => (RuleGroup::Stable, rules::tryceratops::rules::RaiseWithinTry), diff --git a/crates/ruff_linter/src/rule_selector.rs b/crates/ruff_linter/src/rule_selector.rs index efd105bb60112..9cfdd52eadb98 100644 --- a/crates/ruff_linter/src/rule_selector.rs +++ b/crates/ruff_linter/src/rule_selector.rs @@ -204,13 +204,15 @@ impl RuleSelector { let preview_require_explicit = preview.require_explicit; #[allow(deprecated)] self.all_rules().filter(move |rule| { - // Always include rules that are not in preview or the nursery - !(rule.is_preview() || rule.is_nursery()) + // 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()) // Enabling preview includes all preview or nursery rules unless explicit selection // is turned on || (preview_enabled && (matches!(self, RuleSelector::Rule { .. }) || !preview_require_explicit)) + // Deprecated rules are excluded in preview mode unless explicitly selected + || (rule.is_deprecated() && (!preview_enabled || matches!(self, RuleSelector::Rule { .. }))) }) } } diff --git a/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs b/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs index 607d07cd5f82f..d8a9ea42ee3bb 100644 --- a/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs +++ b/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs @@ -111,6 +111,10 @@ impl Violation for MissingTypeKwargs { } } +/// ## Deprecation +/// This rule is commonly disabled because type checkers can infer this type without annotation. +/// It will be removed in a future release. +/// /// ## What it does /// Checks that instance method `self` arguments have type annotations. /// @@ -148,6 +152,10 @@ impl Violation for MissingTypeSelf { } } +/// ## Deprecation +/// This rule is commonly disabled because type checkers can infer this type without annotation. +/// It will be removed in a future release. +/// /// ## What it does /// Checks that class method `cls` arguments have type annotations. /// diff --git a/crates/ruff_linter/src/rules/tryceratops/rules/reraise_no_cause.rs b/crates/ruff_linter/src/rules/tryceratops/rules/reraise_no_cause.rs index 523e97235e649..866894d1d4695 100644 --- a/crates/ruff_linter/src/rules/tryceratops/rules/reraise_no_cause.rs +++ b/crates/ruff_linter/src/rules/tryceratops/rules/reraise_no_cause.rs @@ -7,6 +7,9 @@ use ruff_python_ast::statement_visitor::StatementVisitor; use crate::checkers::ast::Checker; +/// ## Deprecation +/// This rule is identical to [B904] which should be used instead. +/// /// ## What it does /// Checks for exceptions that are re-raised without specifying the cause via /// the `from` keyword. @@ -36,6 +39,8 @@ use crate::checkers::ast::Checker; /// /// ## References /// - [Python documentation: Exception context](https://docs.python.org/3/library/exceptions.html#exception-context) +/// +/// [B904]: https://docs.astral.sh/ruff/rules/raise-without-from-inside-except/ #[violation] pub struct ReraiseNoCause; diff --git a/crates/ruff_macros/src/map_codes.rs b/crates/ruff_macros/src/map_codes.rs index 3a68af8e25258..1c353ed519642 100644 --- a/crates/ruff_macros/src/map_codes.rs +++ b/crates/ruff_macros/src/map_codes.rs @@ -315,10 +315,18 @@ See also https://github.com/astral-sh/ruff/issues/2186. matches!(self.group(), RuleGroup::Preview) } + pub fn is_stable(&self) -> bool { + matches!(self.group(), RuleGroup::Stable) + } + #[allow(deprecated)] pub fn is_nursery(&self) -> bool { matches!(self.group(), RuleGroup::Nursery) } + + pub fn is_deprecated(&self) -> bool { + matches!(self.group(), RuleGroup::Deprecated) + } } impl Linter { diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index cd8f8b84c3b9c..894b3f1fb427b 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -755,6 +755,7 @@ impl LintConfiguration { // Store selectors for displaying warnings let mut redirects = FxHashMap::default(); let mut deprecated_nursery_selectors = FxHashSet::default(); + let mut deprecated_selectors = FxHashSet::default(); let mut ignored_preview_selectors = FxHashSet::default(); // Track which docstring rules are specifically enabled @@ -895,8 +896,10 @@ impl LintConfiguration { return Err(anyhow!("The `NURSERY` selector was removed.{suggestion}")); }; - // Only warn for the following selectors if used to enable rules - // e.g. use with `--ignore` or `--fixable` is okay + // Some of these checks are only for `Kind::Enable` which means only `--select` will warn + // and use with, e.g., `--ignore` or `--fixable` is okay + + // Unstable rules if preview.mode.is_disabled() && kind.is_enable() { if let RuleSelector::Rule { prefix, .. } = selector { if prefix.rules().any(|rule| rule.is_nursery()) { @@ -910,6 +913,16 @@ 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); + } + } + } + + // Redirected rules if let RuleSelector::Prefix { prefix, redirected_from: Some(redirect_from), @@ -950,6 +963,35 @@ impl LintConfiguration { } } + if preview.mode.is_disabled() { + for selection in deprecated_selectors { + let (prefix, code) = selection.prefix_and_code(); + warn_user!( + "Rule `{prefix}{code}` is deprecated and will be removed in a future release.", + ); + } + } else { + let deprecated_selectors = deprecated_selectors.iter().collect::>(); + match deprecated_selectors.as_slice() { + [] => (), + [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.")); + } + [..] => { + 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 { + let (prefix, code) = selection.prefix_and_code(); + message.push_str("\n\t- "); + message.push_str(prefix); + message.push_str(code); + } + message.push('\n'); + return Err(anyhow!(message)); + } + } + } + for selection in ignored_preview_selectors { let (prefix, code) = selection.prefix_and_code(); warn_user!( diff --git a/docs/preview.md b/docs/preview.md index 048fcbe9b6510..2321500f55a90 100644 --- a/docs/preview.md +++ b/docs/preview.md @@ -143,3 +143,8 @@ In our previous example, `--select` with `ALL` `HYP`, `HYP0`, or `HYP00` would n rule will need to be selected with its exact code, e.g. `--select ALL,HYP001`. If preview mode is not enabled, this setting has no effect. + +## Deprecated rules + +When preview mode is enabled, deprecated rules will be disabled. If a deprecated rule is selected explicitly, an +error will be raised. Deprecated rules will not be included if selected via a rule category or prefix.