Skip to content

Commit

Permalink
Add rule removal infrastructure (#9691)
Browse files Browse the repository at this point in the history
Similar to #9689 — retains removed
rules for better error messages and documentation but removed rules
_cannot_ be used in any context.

Removes PLR1706 as a useful test case and something we want to
accomplish in #9680 anyway. The rule was in preview so we do not need to
deprecate it first.

Closes #9007

## Test plan

<img width="1110" alt="Rules table"
src="https://github.com/astral-sh/ruff/assets/2586601/ac9fa682-623c-44aa-8e51-d8ab0d308355">

<img width="1110" alt="Rule page"
src="https://github.com/astral-sh/ruff/assets/2586601/05850b2d-7ca5-49bb-8df8-bb931bab25cd">
  • Loading branch information
zanieb committed Jan 30, 2024
1 parent 3f69fb4 commit 1a485b5
Show file tree
Hide file tree
Showing 13 changed files with 125 additions and 409 deletions.
33 changes: 33 additions & 0 deletions crates/ruff/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1095,6 +1095,39 @@ fn preview_enabled_group_ignore() {
"###);
}

#[test]
fn removed_direct() {
// Selection of a removed rule should fail
let mut cmd = RuffCheck::default().args(["--select", "PLR1706"]).build();
assert_cmd_snapshot!(cmd, @r###"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
ruff failed
Cause: Rule `PLR1706` was removed and cannot be selected.
"###);
}

#[test]
fn removed_indirect() {
// Selection _including_ a removed rule without matching should not fail
// nor should the rule be used
let mut cmd = RuffCheck::default().args(["--select", "PLR"]).build();
assert_cmd_snapshot!(cmd.pass_stdin(r###"
# This would have been a PLR1706 violation
x, y = 1, 2
maximum = x >= y and x or y
"""###), @r###"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
"###);
}

#[test]
fn deprecated_direct() {
// Selection of a deprecated rule without preview enabled should still work
Expand Down
8 changes: 8 additions & 0 deletions crates/ruff_dev/src/generate_docs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ pub(crate) fn main(args: &Args) -> Result<()> {
output.push('\n');
}

if rule.is_removed() {
output.push_str(
r"**Warning: This rule has been removed and its documentation is only available for historical reasons.**",
);
output.push('\n');
output.push('\n');
}

let fix_availability = rule.fixable();
if matches!(
fix_availability,
Expand Down
22 changes: 21 additions & 1 deletion crates/ruff_dev/src/generate_rules_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use ruff_workspace::options_base::OptionsMetadata;

const FIX_SYMBOL: &str = "🛠️";
const PREVIEW_SYMBOL: &str = "🧪";
const REMOVED_SYMBOL: &str = "❌";
const WARNING_SYMBOL: &str = "⚠️";
const STABLE_SYMBOL: &str = "✔️";
const SPACER: &str = "&nbsp;&nbsp;&nbsp;&nbsp;";
Expand All @@ -26,6 +27,9 @@ fn generate_table(table_out: &mut String, rules: impl IntoIterator<Item = Rule>,
table_out.push('\n');
for rule in rules {
let status_token = match rule.group() {
RuleGroup::Removed => {
format!("<span title='Rule has been removed'>{REMOVED_SYMBOL}</span>")
}
RuleGroup::Deprecated => {
format!("<span title='Rule has been deprecated'>{WARNING_SYMBOL}</span>")
}
Expand Down Expand Up @@ -62,9 +66,20 @@ fn generate_table(table_out: &mut String, rules: impl IntoIterator<Item = Rule>,
Cow::Borrowed(message)
};

// Start and end of style spans
let mut ss = "";
let mut se = "";
if rule.is_removed() {
ss = "<span style='opacity: 0.5', title='This rule has been removed'>";
se = "</span>";
} else if rule.is_deprecated() {
ss = "<span style='opacity: 0.8', title='This rule has been deprecated'>";
se = "</span>";
}

#[allow(clippy::or_fun_call)]
table_out.push_str(&format!(
"| {0}{1} {{ #{0}{1} }} | {2} | {3} | {4} |",
"| {ss}{0}{1}{se} {{ #{0}{1} }} | {ss}{2}{se} | {ss}{3}{se} | {ss}{4}{se} |",
linter.common_prefix(),
linter.code_for_rule(rule).unwrap(),
rule.explanation()
Expand Down Expand Up @@ -101,6 +116,11 @@ pub(crate) fn generate() -> String {
));
table_out.push_str("<br />");

table_out.push_str(&format!(
"{SPACER}{REMOVED_SYMBOL}{SPACER} The rule has been removed only the documentation is available."
));
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."
));
Expand Down

This file was deleted.

3 changes: 0 additions & 3 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1508,9 +1508,6 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::RepeatedEqualityComparison) {
pylint::rules::repeated_equality_comparison(checker, bool_op);
}
if checker.enabled(Rule::AndOrTernary) {
pylint::rules::and_or_ternary(checker, bool_op);
}
if checker.enabled(Rule::UnnecessaryKeyCheck) {
ruff::rules::unnecessary_key_check(checker, expr);
}
Expand Down
4 changes: 3 additions & 1 deletion crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ pub enum RuleGroup {
/// 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,
/// The rule has been removed, errors will be displayed on use.
Removed,
/// Legacy category for unstable rules, supports backwards compatible selection.
#[deprecated(note = "Use `RuleGroup::Preview` for new rules instead")]
Nursery,
Expand Down Expand Up @@ -268,7 +270,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "R1704") => (RuleGroup::Preview, rules::pylint::rules::RedefinedArgumentFromLocal),
(Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn),
(Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison),
(Pylint, "R1706") => (RuleGroup::Preview, rules::pylint::rules::AndOrTernary),
(Pylint, "R1706") => (RuleGroup::Removed, rules::pylint::rules::AndOrTernary),
(Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias),
(Pylint, "R1733") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDictIndexLookup),
(Pylint, "R1736") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryListIndexLookup),
Expand Down
14 changes: 14 additions & 0 deletions crates/ruff_linter/src/rule_selector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ impl RuleSelector {
|| (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 { .. })))
// Removed rules are included if explicitly selected but will error downstream
|| (rule.is_removed() && matches!(self, RuleSelector::Rule { .. }))
})
}
}
Expand Down Expand Up @@ -247,6 +249,8 @@ pub struct PreviewOptions {

#[cfg(feature = "schemars")]
mod schema {
use std::str::FromStr;

use itertools::Itertools;
use schemars::JsonSchema;
use schemars::_serde_json::Value;
Expand Down Expand Up @@ -290,6 +294,16 @@ mod schema {
(!prefix.is_empty()).then(|| prefix.to_string())
})),
)
.filter(|p| {
// Exclude any prefixes where all of the rules are removed
if let Ok(Self::Rule { prefix, .. } | Self::Prefix { prefix, .. }) =
RuleSelector::from_str(p)
{
!prefix.rules().all(|rule| rule.is_removed())
} else {
true
}
})
.sorted()
.map(Value::String)
.collect(),
Expand Down
1 change: 0 additions & 1 deletion crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ mod tests {
use crate::settings::LinterSettings;
use crate::test::test_path;

#[test_case(Rule::AndOrTernary, Path::new("and_or_ternary.py"))]
#[test_case(Rule::AssertOnStringLiteral, Path::new("assert_on_string_literal.py"))]
#[test_case(Rule::AwaitOutsideAsync, Path::new("await_outside_async.py"))]
#[test_case(Rule::BadOpenMode, Path::new("bad_open_mode.py"))]
Expand Down
110 changes: 11 additions & 99 deletions crates/ruff_linter/src/rules/pylint/rules/and_or_ternary.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{
BoolOp, Expr, ExprBoolOp, ExprDictComp, ExprIfExp, ExprListComp, ExprSetComp,
};
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;
use crate::fix::snippet::SourceCodeSnippet;
use ruff_diagnostics::Violation;
use ruff_macros::violation;

/// ## Removal
/// This rule was removed from Ruff because it was common for it to introduce behavioral changes.
/// See [#9007](https://github.com/astral-sh/ruff/issues/9007) for more information.
///
/// ## What it does
/// Checks for uses of the known pre-Python 2.5 ternary syntax.
///
Expand All @@ -31,100 +28,15 @@ use crate::fix::snippet::SourceCodeSnippet;
/// maximum = x if x >= y else y
/// ```
#[violation]
pub struct AndOrTernary {
ternary: SourceCodeSnippet,
}
pub struct AndOrTernary;

/// PLR1706
impl Violation for AndOrTernary {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
if let Some(ternary) = self.ternary.full_display() {
format!("Consider using if-else expression (`{ternary}`)")
} else {
format!("Consider using if-else expression")
}
}

fn fix_title(&self) -> Option<String> {
Some(format!("Convert to if-else expression"))
}
}

/// Returns `Some((condition, true_value, false_value))`, if `bool_op` is of the form `condition and true_value or false_value`.
fn parse_and_or_ternary(bool_op: &ExprBoolOp) -> Option<(&Expr, &Expr, &Expr)> {
if bool_op.op != BoolOp::Or {
return None;
unreachable!("PLR1706 has been removed");
}
let [expr, false_value] = bool_op.values.as_slice() else {
return None;
};
let Some(and_op) = expr.as_bool_op_expr() else {
return None;
};
if and_op.op != BoolOp::And {
return None;
}
let [condition, true_value] = and_op.values.as_slice() else {
return None;
};
if false_value.is_bool_op_expr() || true_value.is_bool_op_expr() {
return None;
}
Some((condition, true_value, false_value))
}

/// Returns `true` if the expression is used within a comprehension.
fn is_comprehension_if(parent: Option<&Expr>, expr: &ExprBoolOp) -> bool {
let comprehensions = match parent {
Some(Expr::ListComp(ExprListComp { generators, .. })) => generators,
Some(Expr::SetComp(ExprSetComp { generators, .. })) => generators,
Some(Expr::DictComp(ExprDictComp { generators, .. })) => generators,
_ => {
return false;
}
};
comprehensions
.iter()
.any(|comp| comp.ifs.iter().any(|ifs| ifs.range() == expr.range()))
}

/// PLR1706
pub(crate) fn and_or_ternary(checker: &mut Checker, bool_op: &ExprBoolOp) {
if checker.semantic().current_statement().is_if_stmt() {
return;
}
let parent_expr = checker.semantic().current_expression_parent();
if parent_expr.is_some_and(Expr::is_bool_op_expr) {
return;
fn message_formats() -> &'static [&'static str] {
&["Consider using if-else expression"]
}
let Some((condition, true_value, false_value)) = parse_and_or_ternary(bool_op) else {
return;
};

let if_expr = Expr::IfExp(ExprIfExp {
test: Box::new(condition.clone()),
body: Box::new(true_value.clone()),
orelse: Box::new(false_value.clone()),
range: TextRange::default(),
});

let ternary = if is_comprehension_if(parent_expr, bool_op) {
format!("({})", checker.generator().expr(&if_expr))
} else {
checker.generator().expr(&if_expr)
};

let mut diagnostic = Diagnostic::new(
AndOrTernary {
ternary: SourceCodeSnippet::new(ternary.clone()),
},
bool_op.range,
);
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
ternary,
bool_op.range,
)));
checker.diagnostics.push(diagnostic);
}

0 comments on commit 1a485b5

Please sign in to comment.