Skip to content

Commit e0bc08a

Browse files
committed
Add rule removal infrastructure (#9691)
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">
1 parent a0ef087 commit e0bc08a

File tree

13 files changed

+125
-409
lines changed

13 files changed

+125
-409
lines changed

crates/ruff/tests/integration_test.rs

+33
Original file line numberDiff line numberDiff line change
@@ -1091,6 +1091,39 @@ fn preview_enabled_group_ignore() {
10911091
"###);
10921092
}
10931093

1094+
#[test]
1095+
fn removed_direct() {
1096+
// Selection of a removed rule should fail
1097+
let mut cmd = RuffCheck::default().args(["--select", "PLR1706"]).build();
1098+
assert_cmd_snapshot!(cmd, @r###"
1099+
success: false
1100+
exit_code: 2
1101+
----- stdout -----
1102+
1103+
----- stderr -----
1104+
ruff failed
1105+
Cause: Rule `PLR1706` was removed and cannot be selected.
1106+
"###);
1107+
}
1108+
1109+
#[test]
1110+
fn removed_indirect() {
1111+
// Selection _including_ a removed rule without matching should not fail
1112+
// nor should the rule be used
1113+
let mut cmd = RuffCheck::default().args(["--select", "PLR"]).build();
1114+
assert_cmd_snapshot!(cmd.pass_stdin(r###"
1115+
# This would have been a PLR1706 violation
1116+
x, y = 1, 2
1117+
maximum = x >= y and x or y
1118+
"""###), @r###"
1119+
success: true
1120+
exit_code: 0
1121+
----- stdout -----
1122+
1123+
----- stderr -----
1124+
"###);
1125+
}
1126+
10941127
#[test]
10951128
fn deprecated_direct() {
10961129
// Selection of a deprecated rule without preview enabled should still work

crates/ruff_dev/src/generate_docs.rs

+8
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,14 @@ pub(crate) fn main(args: &Args) -> Result<()> {
4545
output.push('\n');
4646
}
4747

48+
if rule.is_removed() {
49+
output.push_str(
50+
r"**Warning: This rule has been removed and its documentation is only available for historical reasons.**",
51+
);
52+
output.push('\n');
53+
output.push('\n');
54+
}
55+
4856
let fix_availability = rule.fixable();
4957
if matches!(
5058
fix_availability,

crates/ruff_dev/src/generate_rules_table.rs

+21-1
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ use ruff_workspace::options_base::OptionsMetadata;
1515

1616
const FIX_SYMBOL: &str = "🛠️";
1717
const PREVIEW_SYMBOL: &str = "🧪";
18+
const REMOVED_SYMBOL: &str = "❌";
1819
const WARNING_SYMBOL: &str = "⚠️";
1920
const STABLE_SYMBOL: &str = "✔️";
2021
const SPACER: &str = "&nbsp;&nbsp;&nbsp;&nbsp;";
@@ -26,6 +27,9 @@ fn generate_table(table_out: &mut String, rules: impl IntoIterator<Item = Rule>,
2627
table_out.push('\n');
2728
for rule in rules {
2829
let status_token = match rule.group() {
30+
RuleGroup::Removed => {
31+
format!("<span title='Rule has been removed'>{REMOVED_SYMBOL}</span>")
32+
}
2933
RuleGroup::Deprecated => {
3034
format!("<span title='Rule has been deprecated'>{WARNING_SYMBOL}</span>")
3135
}
@@ -62,9 +66,20 @@ fn generate_table(table_out: &mut String, rules: impl IntoIterator<Item = Rule>,
6266
Cow::Borrowed(message)
6367
};
6468

69+
// Start and end of style spans
70+
let mut ss = "";
71+
let mut se = "";
72+
if rule.is_removed() {
73+
ss = "<span style='opacity: 0.5', title='This rule has been removed'>";
74+
se = "</span>";
75+
} else if rule.is_deprecated() {
76+
ss = "<span style='opacity: 0.8', title='This rule has been deprecated'>";
77+
se = "</span>";
78+
}
79+
6580
#[allow(clippy::or_fun_call)]
6681
table_out.push_str(&format!(
67-
"| {0}{1} {{ #{0}{1} }} | {2} | {3} | {4} |",
82+
"| {ss}{0}{1}{se} {{ #{0}{1} }} | {ss}{2}{se} | {ss}{3}{se} | {ss}{4}{se} |",
6883
linter.common_prefix(),
6984
linter.code_for_rule(rule).unwrap(),
7085
rule.explanation()
@@ -101,6 +116,11 @@ pub(crate) fn generate() -> String {
101116
));
102117
table_out.push_str("<br />");
103118

119+
table_out.push_str(&format!(
120+
"{SPACER}{REMOVED_SYMBOL}{SPACER} The rule has been removed only the documentation is available."
121+
));
122+
table_out.push_str("<br />");
123+
104124
table_out.push_str(&format!(
105125
"{SPACER}{FIX_SYMBOL}{SPACER} The rule is automatically fixable by the `--fix` command-line option."
106126
));

crates/ruff_linter/resources/test/fixtures/pylint/and_or_ternary.py

-73
This file was deleted.

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

-3
Original file line numberDiff line numberDiff line change
@@ -1508,9 +1508,6 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
15081508
if checker.enabled(Rule::RepeatedEqualityComparison) {
15091509
pylint::rules::repeated_equality_comparison(checker, bool_op);
15101510
}
1511-
if checker.enabled(Rule::AndOrTernary) {
1512-
pylint::rules::and_or_ternary(checker, bool_op);
1513-
}
15141511
if checker.enabled(Rule::UnnecessaryKeyCheck) {
15151512
ruff::rules::unnecessary_key_check(checker, expr);
15161513
}

crates/ruff_linter/src/codes.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ pub enum RuleGroup {
5555
/// The rule has been deprecated, warnings will be displayed during selection in stable
5656
/// and errors will be raised if used with preview mode enabled.
5757
Deprecated,
58+
/// The rule has been removed, errors will be displayed on use.
59+
Removed,
5860
/// Legacy category for unstable rules, supports backwards compatible selection.
5961
#[deprecated(note = "Use `RuleGroup::Preview` for new rules instead")]
6062
Nursery,
@@ -268,7 +270,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
268270
(Pylint, "R1704") => (RuleGroup::Preview, rules::pylint::rules::RedefinedArgumentFromLocal),
269271
(Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn),
270272
(Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison),
271-
(Pylint, "R1706") => (RuleGroup::Preview, rules::pylint::rules::AndOrTernary),
273+
(Pylint, "R1706") => (RuleGroup::Removed, rules::pylint::rules::AndOrTernary),
272274
(Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias),
273275
(Pylint, "R1733") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDictIndexLookup),
274276
(Pylint, "R1736") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryListIndexLookup),

crates/ruff_linter/src/rule_selector.rs

+14
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,8 @@ impl RuleSelector {
213213
|| (preview_enabled && (matches!(self, RuleSelector::Rule { .. }) || !preview_require_explicit))
214214
// Deprecated rules are excluded in preview mode unless explicitly selected
215215
|| (rule.is_deprecated() && (!preview_enabled || matches!(self, RuleSelector::Rule { .. })))
216+
// Removed rules are included if explicitly selected but will error downstream
217+
|| (rule.is_removed() && matches!(self, RuleSelector::Rule { .. }))
216218
})
217219
}
218220
}
@@ -247,6 +249,8 @@ pub struct PreviewOptions {
247249

248250
#[cfg(feature = "schemars")]
249251
mod schema {
252+
use std::str::FromStr;
253+
250254
use itertools::Itertools;
251255
use schemars::JsonSchema;
252256
use schemars::_serde_json::Value;
@@ -290,6 +294,16 @@ mod schema {
290294
(!prefix.is_empty()).then(|| prefix.to_string())
291295
})),
292296
)
297+
.filter(|p| {
298+
// Exclude any prefixes where all of the rules are removed
299+
if let Ok(Self::Rule { prefix, .. } | Self::Prefix { prefix, .. }) =
300+
RuleSelector::from_str(p)
301+
{
302+
!prefix.rules().all(|rule| rule.is_removed())
303+
} else {
304+
true
305+
}
306+
})
293307
.sorted()
294308
.map(Value::String)
295309
.collect(),

crates/ruff_linter/src/rules/pylint/mod.rs

-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ mod tests {
2020
use crate::settings::LinterSettings;
2121
use crate::test::test_path;
2222

23-
#[test_case(Rule::AndOrTernary, Path::new("and_or_ternary.py"))]
2423
#[test_case(Rule::AssertOnStringLiteral, Path::new("assert_on_string_literal.py"))]
2524
#[test_case(Rule::AwaitOutsideAsync, Path::new("await_outside_async.py"))]
2625
#[test_case(Rule::BadOpenMode, Path::new("bad_open_mode.py"))]
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,10 @@
1-
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
2-
use ruff_macros::{derive_message_formats, violation};
3-
use ruff_python_ast::{
4-
BoolOp, Expr, ExprBoolOp, ExprDictComp, ExprIfExp, ExprListComp, ExprSetComp,
5-
};
6-
use ruff_text_size::{Ranged, TextRange};
7-
8-
use crate::checkers::ast::Checker;
9-
use crate::fix::snippet::SourceCodeSnippet;
1+
use ruff_diagnostics::Violation;
2+
use ruff_macros::violation;
103

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

33+
/// PLR1706
3834
impl Violation for AndOrTernary {
39-
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
40-
41-
#[derive_message_formats]
4235
fn message(&self) -> String {
43-
if let Some(ternary) = self.ternary.full_display() {
44-
format!("Consider using if-else expression (`{ternary}`)")
45-
} else {
46-
format!("Consider using if-else expression")
47-
}
48-
}
49-
50-
fn fix_title(&self) -> Option<String> {
51-
Some(format!("Convert to if-else expression"))
52-
}
53-
}
54-
55-
/// Returns `Some((condition, true_value, false_value))`, if `bool_op` is of the form `condition and true_value or false_value`.
56-
fn parse_and_or_ternary(bool_op: &ExprBoolOp) -> Option<(&Expr, &Expr, &Expr)> {
57-
if bool_op.op != BoolOp::Or {
58-
return None;
36+
unreachable!("PLR1706 has been removed");
5937
}
60-
let [expr, false_value] = bool_op.values.as_slice() else {
61-
return None;
62-
};
63-
let Some(and_op) = expr.as_bool_op_expr() else {
64-
return None;
65-
};
66-
if and_op.op != BoolOp::And {
67-
return None;
68-
}
69-
let [condition, true_value] = and_op.values.as_slice() else {
70-
return None;
71-
};
72-
if false_value.is_bool_op_expr() || true_value.is_bool_op_expr() {
73-
return None;
74-
}
75-
Some((condition, true_value, false_value))
76-
}
77-
78-
/// Returns `true` if the expression is used within a comprehension.
79-
fn is_comprehension_if(parent: Option<&Expr>, expr: &ExprBoolOp) -> bool {
80-
let comprehensions = match parent {
81-
Some(Expr::ListComp(ExprListComp { generators, .. })) => generators,
82-
Some(Expr::SetComp(ExprSetComp { generators, .. })) => generators,
83-
Some(Expr::DictComp(ExprDictComp { generators, .. })) => generators,
84-
_ => {
85-
return false;
86-
}
87-
};
88-
comprehensions
89-
.iter()
90-
.any(|comp| comp.ifs.iter().any(|ifs| ifs.range() == expr.range()))
91-
}
9238

93-
/// PLR1706
94-
pub(crate) fn and_or_ternary(checker: &mut Checker, bool_op: &ExprBoolOp) {
95-
if checker.semantic().current_statement().is_if_stmt() {
96-
return;
97-
}
98-
let parent_expr = checker.semantic().current_expression_parent();
99-
if parent_expr.is_some_and(Expr::is_bool_op_expr) {
100-
return;
39+
fn message_formats() -> &'static [&'static str] {
40+
&["Consider using if-else expression"]
10141
}
102-
let Some((condition, true_value, false_value)) = parse_and_or_ternary(bool_op) else {
103-
return;
104-
};
105-
106-
let if_expr = Expr::IfExp(ExprIfExp {
107-
test: Box::new(condition.clone()),
108-
body: Box::new(true_value.clone()),
109-
orelse: Box::new(false_value.clone()),
110-
range: TextRange::default(),
111-
});
112-
113-
let ternary = if is_comprehension_if(parent_expr, bool_op) {
114-
format!("({})", checker.generator().expr(&if_expr))
115-
} else {
116-
checker.generator().expr(&if_expr)
117-
};
118-
119-
let mut diagnostic = Diagnostic::new(
120-
AndOrTernary {
121-
ternary: SourceCodeSnippet::new(ternary.clone()),
122-
},
123-
bool_op.range,
124-
);
125-
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
126-
ternary,
127-
bool_op.range,
128-
)));
129-
checker.diagnostics.push(diagnostic);
13042
}

0 commit comments

Comments
 (0)