Skip to content

Commit

Permalink
Add rule deprecation infrastructure (#9689)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
zanieb committed Jan 30, 2024
1 parent b9b44a1 commit 3f69fb4
Show file tree
Hide file tree
Showing 10 changed files with 279 additions and 21 deletions.
150 changes: 150 additions & 0 deletions crates/ruff/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
10 changes: 9 additions & 1 deletion crates/ruff_dev/src/generate_docs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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,
Expand Down
53 changes: 40 additions & 13 deletions crates/ruff_dev/src/generate_rules_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
//! Used for <https://docs.astral.sh/ruff/rules/>.

use itertools::Itertools;
use ruff_linter::codes::RuleGroup;
use std::borrow::Cow;
use strum::IntoEnumIterator;

Expand All @@ -14,27 +15,40 @@ 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 = "&nbsp;&nbsp;&nbsp;&nbsp;";

fn generate_table(table_out: &mut String, rules: impl IntoIterator<Item = Rule>, linter: &Linter) {
table_out.push_str("| Code | Name | Message | |");
table_out.push('\n');
table_out.push_str("| ---- | ---- | ------- | ------: |");
table_out.push('\n');
for rule in rules {
let status_token = match rule.group() {
RuleGroup::Deprecated => {
format!("<span title='Rule has been deprecated'>{WARNING_SYMBOL}</span>")
}
#[allow(deprecated)]
RuleGroup::Preview | RuleGroup::Nursery => {
format!("<span title='Rule is in preview'>{PREVIEW_SYMBOL}</span>")
}
RuleGroup::Stable => {
// A full opacity checkmark is a bit aggressive for indicating stable
format!("<span title='Rule is stable' style='opacity: 0.6'>{STABLE_SYMBOL}</span>")
}
};

let fix_token = match rule.fixable() {
FixAvailability::Always | FixAvailability::Sometimes => {
format!("<span title='Automatic fix available'>{FIX_SYMBOL}</span>")
}
FixAvailability::None => {
format!("<span style='opacity: 0.1' aria-hidden='true'>{FIX_SYMBOL}</span>")
format!("<span title='Automatic fix not available' style='opacity: 0.1' aria-hidden='true'>{FIX_SYMBOL}</span>")
}
};
let preview_token = if rule.is_preview() || rule.is_nursery() {
format!("<span title='Rule is in preview'>{PREVIEW_SYMBOL}</span>")
} else {
format!("<span style='opacity: 0.1' aria-hidden='true'>{PREVIEW_SYMBOL}</span>")
};
let status_token = format!("{fix_token} {preview_token}");

let tokens = format!("{status_token} {fix_token}");

let rule_name = rule.as_ref();

Expand All @@ -58,7 +72,7 @@ fn generate_table(table_out: &mut String, rules: impl IntoIterator<Item = Rule>,
.then_some(format_args!("[{rule_name}](rules/{rule_name}.md)"))
.unwrap_or(format_args!("{rule_name}")),
message,
status_token,
tokens,
));
table_out.push('\n');
}
Expand All @@ -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("<br />");

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("<br />");

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("<br />");

table_out.push_str(&format!(
"{SPACER}{FIX_SYMBOL}{SPACER} The rule is automatically fixable by the `--fix` command-line option."
));
table_out.push_str("<br />");
table_out.push('\n');

for linter in Linter::iter() {
Expand Down
9 changes: 6 additions & 3 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down
6 changes: 4 additions & 2 deletions crates/ruff_linter/src/rule_selector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 { .. })))
})
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down Expand Up @@ -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.
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;

Expand Down
8 changes: 8 additions & 0 deletions crates/ruff_macros/src/map_codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading

0 comments on commit 3f69fb4

Please sign in to comment.