Skip to content

Commit 5d41c84

Browse files
authored
SIM300: CONSTANT_CASE variables are improperly flagged for yoda violation (#9164)
## Summary fixes #6956 details in issue Following an advice in #6956 (comment), this change separates expressions to 3 levels of "constant likelihood": * literals, empty dict and tuples... (definitely constant, level 2) * CONSTANT_CASE vars (probably constant, 1) * all other expressions (0) a comparison is marked yoda if the level is strictly higher on its left hand side following #6956 (comment) marking compound expressions of literals (e.g. `60 * 60` ) as constants this change current behaviour on `SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60)` in the fixture from error to ok
1 parent cbe3bf9 commit 5d41c84

File tree

5 files changed

+682
-268
lines changed

5 files changed

+682
-268
lines changed

crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM300.py

+11-2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
# Errors
22
"yoda" == compare # SIM300
3-
"yoda" == compare # SIM300
43
42 == age # SIM300
54
("a", "b") == compare # SIM300
65
"yoda" <= compare # SIM300
@@ -13,10 +12,17 @@
1312
YODA >= age # SIM300
1413
JediOrder.YODA == age # SIM300
1514
0 < (number - 100) # SIM300
16-
SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) # SIM300
1715
B<A[0][0]or B
1816
B or(B)<A[0][0]
1917

18+
# Errors in preview
19+
['upper'] == UPPER_LIST
20+
{} == DummyHandler.CONFIG
21+
22+
# Errors in stable
23+
UPPER_LIST == ['upper']
24+
DummyHandler.CONFIG == {}
25+
2026
# OK
2127
compare == "yoda"
2228
age == 42
@@ -31,3 +37,6 @@
3137
YODA == YODA
3238
age == JediOrder.YODA
3339
(number - 100) > 0
40+
SECONDS_IN_DAY == 60 * 60 * 24 # Error in 0.1.8
41+
SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) # Error in 0.1.8
42+
{"non-empty-dict": "is-ok"} == DummyHandler.CONFIG

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

+1
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ mod tests {
5656
}
5757

5858
#[test_case(Rule::InDictKeys, Path::new("SIM118.py"))]
59+
#[test_case(Rule::YodaConditions, Path::new("SIM300.py"))]
5960
#[test_case(Rule::IfElseBlockInsteadOfDictGet, Path::new("SIM401.py"))]
6061
#[test_case(Rule::DictGetWithNoneDefault, Path::new("SIM910.py"))]
6162
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {

crates/ruff_linter/src/rules/flake8_simplify/rules/yoda_conditions.rs

+65-13
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
use std::cmp;
2+
13
use anyhow::Result;
24
use libcst_native::CompOp;
35

@@ -14,6 +16,7 @@ use crate::cst::helpers::or_space;
1416
use crate::cst::matchers::{match_comparison, transform_expression};
1517
use crate::fix::edits::pad;
1618
use crate::fix::snippet::SourceCodeSnippet;
19+
use crate::settings::types::PreviewMode;
1720

1821
/// ## What it does
1922
/// Checks for conditions that position a constant on the left-hand side of the
@@ -78,18 +81,65 @@ impl Violation for YodaConditions {
7881
}
7982
}
8083

81-
/// Return `true` if an [`Expr`] is a constant or a constant-like name.
82-
fn is_constant_like(expr: &Expr) -> bool {
83-
match expr {
84-
Expr::Attribute(ast::ExprAttribute { attr, .. }) => str::is_cased_uppercase(attr),
85-
Expr::Tuple(ast::ExprTuple { elts, .. }) => elts.iter().all(is_constant_like),
86-
Expr::Name(ast::ExprName { id, .. }) => str::is_cased_uppercase(id),
87-
Expr::UnaryOp(ast::ExprUnaryOp {
88-
op: UnaryOp::UAdd | UnaryOp::USub | UnaryOp::Invert,
89-
operand,
90-
range: _,
91-
}) => operand.is_literal_expr(),
92-
_ => expr.is_literal_expr(),
84+
/// Comparisons left-hand side must not be more [`ConstantLikelihood`] than the right-hand side.
85+
#[derive(PartialEq, Eq, PartialOrd, Ord, Debug)]
86+
enum ConstantLikelihood {
87+
/// The expression is unlikely to be a constant (e.g., `foo` or `foo(bar)`).
88+
Unlikely = 0,
89+
90+
/// The expression is likely to be a constant (e.g., `FOO`).
91+
Probably = 1,
92+
93+
/// The expression is definitely a constant (e.g., `42` or `"foo"`).
94+
Definitely = 2,
95+
}
96+
97+
impl ConstantLikelihood {
98+
/// Determine the [`ConstantLikelihood`] of an expression.
99+
fn from_expression(expr: &Expr, preview: PreviewMode) -> Self {
100+
match expr {
101+
_ if expr.is_literal_expr() => ConstantLikelihood::Definitely,
102+
Expr::Attribute(ast::ExprAttribute { attr, .. }) => {
103+
ConstantLikelihood::from_identifier(attr)
104+
}
105+
Expr::Name(ast::ExprName { id, .. }) => ConstantLikelihood::from_identifier(id),
106+
Expr::Tuple(ast::ExprTuple { elts, .. }) => elts
107+
.iter()
108+
.map(|expr| ConstantLikelihood::from_expression(expr, preview))
109+
.min()
110+
.unwrap_or(ConstantLikelihood::Definitely),
111+
Expr::List(ast::ExprList { elts, .. }) if preview.is_enabled() => elts
112+
.iter()
113+
.map(|expr| ConstantLikelihood::from_expression(expr, preview))
114+
.min()
115+
.unwrap_or(ConstantLikelihood::Definitely),
116+
Expr::Dict(ast::ExprDict { values: vs, .. }) if preview.is_enabled() => {
117+
if vs.is_empty() {
118+
ConstantLikelihood::Definitely
119+
} else {
120+
ConstantLikelihood::Probably
121+
}
122+
}
123+
Expr::BinOp(ast::ExprBinOp { left, right, .. }) => cmp::min(
124+
ConstantLikelihood::from_expression(left, preview),
125+
ConstantLikelihood::from_expression(right, preview),
126+
),
127+
Expr::UnaryOp(ast::ExprUnaryOp {
128+
op: UnaryOp::UAdd | UnaryOp::USub | UnaryOp::Invert,
129+
operand,
130+
range: _,
131+
}) => ConstantLikelihood::from_expression(operand, preview),
132+
_ => ConstantLikelihood::Unlikely,
133+
}
134+
}
135+
136+
/// Determine the [`ConstantLikelihood`] of an identifier.
137+
fn from_identifier(identifier: &str) -> Self {
138+
if str::is_cased_uppercase(identifier) {
139+
ConstantLikelihood::Probably
140+
} else {
141+
ConstantLikelihood::Unlikely
142+
}
93143
}
94144
}
95145

@@ -180,7 +230,9 @@ pub(crate) fn yoda_conditions(
180230
return;
181231
}
182232

183-
if !is_constant_like(left) || is_constant_like(right) {
233+
if ConstantLikelihood::from_expression(left, checker.settings.preview)
234+
<= ConstantLikelihood::from_expression(right, checker.settings.preview)
235+
{
184236
return;
185237
}
186238

0 commit comments

Comments
 (0)