Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rule deprecation infrastructure #9689

Merged
merged 7 commits into from
Jan 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 />");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of preferred what we had before over a separate section, but I assume you tried to just extend the previous layout and this looked better?

Copy link
Member Author

@zanieb zanieb Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the legend specifically? I didn't like how much blank space there was so I tried removing that and it felt awkward. I find having all of sentences weirdly verbose for documenting the meanings of the icons.

One thing we could do here is link to the legend from the icons in the rule table. I've tried clicking them multiple times :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's interesting. We could even put it at the bottom I guess.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's focus on this separately #9711

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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm less certain on ANN102. I would never enable it myself, but could it have value?

Copy link
Member Author

@zanieb zanieb Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see why. It think the annotations here are either self: Self or cls: type[Self]. Are there situations in which they would differ for the cls case? cc @AlexWaygood

Copy link
Member

@AlexWaygood AlexWaygood Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP-673 has a really annoying restriction that says you can't use typing.Self in metaclasses, so you have to use a custom TypeVar for __new__ methods in metaclasses (in typeshed we work around this in a somewhat-hacky way: we have a custom TypeVar that is literally just defined as Self = TypeVar("Self"), and use that for these situations: https://github.com/python/typeshed/blob/3881b1b81a8118aa6fc2d2e9616c8f541e5101ed/stdlib/builtins.pyi#L194-L196)

You also might need to use a custom type variable for cls if you can't use typing.Self for whatever reason -- maybe you support older Pythons and you can't declare a dependency on typing_extensions?

But I agree that this seems like a pretty pointless rule, honestly. The vast majority of the time, you can leave these unannotated, and no type checker is going to care if you leave them unannotated. If you need to annotate cls, you'll know that you need to do so without a lint rule telling you about it, I think

(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
Loading