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.

Sign up for GitHub

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

SIM300: CONSTANT_CASE variables are improperly flagged for yoda violation #9164

Merged
merged 7 commits into from
Dec 20, 2023
Merged

SIM300: CONSTANT_CASE variables are improperly flagged for yoda violation #9164

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
760ff26
sim300 constant likelihood levels
asafamr-mm Dec 16, 2023
935b77e
enum docstrings
asafamr-mm Dec 17, 2023
7879f9e
moved lists and dicts to preview
asafamr-mm Dec 17, 2023
3f36780
Merge branch 'main' into SIM300-CONSTANT-CASE-false-positives
charliermarsh Dec 20, 2023
6e3d5a7
Remove trailing newline
charliermarsh Dec 20, 2023
0a0b042
Tweak some var names
charliermarsh Dec 20, 2023
75564e8
Fix Charlie's typo
charliermarsh Dec 20, 2023
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
8 changes: 7 additions & 1 deletion crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM300.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@
YODA >= age # SIM300
JediOrder.YODA == age # SIM300
0 < (number - 100) # SIM300
SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) # SIM300
B<A[0][0]or B
B or(B)<A[0][0]
['upper'] == UPPER_LIST
{} == DummyHandler.CONFIG

# OK
compare == "yoda"
Expand All @@ -31,3 +32,8 @@
YODA == YODA
age == JediOrder.YODA
(number - 100) > 0
UPPER_LIST == ['upper']
DummyHandler.CONFIG == {}
{"thats": "acceptable"} == DummyHandler.CONFIG
SECONDS_IN_DAY == 60 * 60 * 24
SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60)
51 changes: 43 additions & 8 deletions crates/ruff_linter/src/rules/flake8_simplify/rules/yoda_conditions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::cmp;

use anyhow::Result;
use libcst_native::CompOp;

Expand Down Expand Up @@ -78,18 +80,51 @@ impl Violation for YodaConditions {
}
}

/// Return `true` if an [`Expr`] is a constant or a constant-like name.
fn is_constant_like(expr: &Expr) -> bool {
/// Comparisons left hand side must not be more [`ConstantLikelihood`] than their right hand side
#[derive(PartialEq, Eq, PartialOrd, Ord, Debug)]
enum ConstantLikelihood {
Unlikely = 0,
Probably = 1, // CAMEL_CASED vars
Definitely = 2, // literals, empty dicts and tuples...
Copy link
Member

Choose a reason for hiding this comment

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

Nit: make these rustdocs? Like:

/// The expression is unlikely to be a constant (e.g., `foo` or `foo(bar)`).
Unlikely = 0
/// The expression is likely to be a constant (e.g., `FOO`).
Probably = 1,
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. feel free to correct my English there...

}

fn how_likely_constant_str(s: &str) -> ConstantLikelihood {
if str::is_cased_uppercase(s) {
ConstantLikelihood::Probably
} else {
ConstantLikelihood::Unlikely
}
}

/// Return [`Expr`] [`ConstantLikelihood`] level depending on simple heuristics.
fn how_likely_constant(expr: &Expr) -> ConstantLikelihood {
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably add an associated method on ConstantLikelihood, like:

impl ConstantLikelihood {
  fn from_expression(expr: &Expr) -> Self { ... }
}

It helps with discoverability, since this is kind of a constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like this?

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

Lines 96 to 143 in 7879f9e

impl ConstantLikelihood {
fn from_expression(expr: &Expr, is_preview_enabled: bool) -> Self {
match expr {
_ if expr.is_literal_expr() => ConstantLikelihood::Definitely,
Expr::Attribute(ast::ExprAttribute { attr, .. }) => {
ConstantLikelihood::from_identifier(attr)
}
Expr::Name(ast::ExprName { id, .. }) => ConstantLikelihood::from_identifier(id),
Expr::Tuple(ast::ExprTuple { elts, .. }) => elts
.iter()
.map(|x| ConstantLikelihood::from_expression(x, is_preview_enabled))
.min()
.unwrap_or(ConstantLikelihood::Definitely),
Expr::List(ast::ExprList { elts, .. }) if is_preview_enabled => elts
.iter()
.map(|x| ConstantLikelihood::from_expression(x, is_preview_enabled))
.min()
.unwrap_or(ConstantLikelihood::Definitely),
Expr::Dict(ast::ExprDict { values: vs, .. }) if is_preview_enabled => {
if vs.is_empty() {
ConstantLikelihood::Definitely
} else {
ConstantLikelihood::Probably
}
}
Expr::BinOp(ast::ExprBinOp { left, right, .. }) => cmp::min(
ConstantLikelihood::from_expression(left, is_preview_enabled),
ConstantLikelihood::from_expression(right, is_preview_enabled),
),
Expr::UnaryOp(ast::ExprUnaryOp {
op: UnaryOp::UAdd | UnaryOp::USub | UnaryOp::Invert,
operand,
range: _,
}) => ConstantLikelihood::from_expression(operand, is_preview_enabled),
_ => ConstantLikelihood::Unlikely,
}
}
fn from_identifier(identifier: &str) -> Self {
if str::is_cased_uppercase(identifier) {
ConstantLikelihood::Probably
} else {
ConstantLikelihood::Unlikely
}
}
}
/// Generate a fix to reverse a comparison.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, exactly!

Copy link
Member

Choose a reason for hiding this comment

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

If we didn't need preview, we could also do...

impl From<&Expr> for ConstantLikelihood {
  ...
}

That would enable us to do things like expr.into().

if expr.is_literal_expr() {
return ConstantLikelihood::Definitely;
}
Copy link
Member

Choose a reason for hiding this comment

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

Entirely personal style but I'd probably just put this in the match with all the cases rather than adding an early return. I just find it a bit clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to correct me, I'm here to learn 🙂
Would you say this is idiomatic:

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

Lines 98 to 100 in 7879f9e

match expr {
_ if expr.is_literal_expr() => ConstantLikelihood::Definitely,
Expr::Attribute(ast::ExprAttribute { attr, .. }) => {

(trying to DRY this invocation from multiple patterns)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think what you have here with the match is a nice approach.

match expr {
Expr::Attribute(ast::ExprAttribute { attr, .. }) => str::is_cased_uppercase(attr),
Expr::Tuple(ast::ExprTuple { elts, .. }) => elts.iter().all(is_constant_like),
Expr::Name(ast::ExprName { id, .. }) => str::is_cased_uppercase(id),
Expr::Attribute(ast::ExprAttribute { attr, .. }) => how_likely_constant_str(attr),
Expr::Name(ast::ExprName { id, .. }) => how_likely_constant_str(id),
Expr::Tuple(ast::ExprTuple { elts, .. }) | Expr::List(ast::ExprList { elts, .. }) => elts
Copy link
Member

Choose a reason for hiding this comment

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

I think we should consider omitting lists and dictionaries, or at the very least they should be added under preview mode to avoid new violations.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I guess the use of list was actually the motivating issue here. So, alternatively, can we try gating this whole change under preview, to avoid disruption for folks on stable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved lists and dicts to preview

.iter()
.map(how_likely_constant)
.min()
.unwrap_or(ConstantLikelihood::Definitely),
Expr::Dict(ast::ExprDict { values: vs, .. }) => {
if vs.is_empty() {
ConstantLikelihood::Definitely
} else {
ConstantLikelihood::Probably
}
}
Expr::BinOp(ast::ExprBinOp { left, right, .. }) => {
cmp::min(how_likely_constant(left), how_likely_constant(right))
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice.

Expr::UnaryOp(ast::ExprUnaryOp {
op: UnaryOp::UAdd | UnaryOp::USub | UnaryOp::Invert,
operand,
range: _,
}) => operand.is_literal_expr(),
_ => expr.is_literal_expr(),
}) => how_likely_constant(operand),
_ => ConstantLikelihood::Unlikely,
}
}

Expand Down Expand Up @@ -180,7 +215,7 @@ pub(crate) fn yoda_conditions(
return;
}

if !is_constant_like(left) || is_constant_like(right) {
if how_likely_constant(left) <= how_likely_constant(right) {
return;
}

Expand Down
117 changes: 69 additions & 48 deletions ...ake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM300_SIM300.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ SIM300.py:13:1: SIM300 [*] Yoda conditions are discouraged, use `age <= YODA` in
13 |+age <= YODA # SIM300
14 14 | JediOrder.YODA == age # SIM300
15 15 | 0 < (number - 100) # SIM300
16 16 | SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) # SIM300
16 16 | B<A[0][0]or B

SIM300.py:14:1: SIM300 [*] Yoda conditions are discouraged, use `age == JediOrder.YODA` instead
|
Expand All @@ -256,7 +256,7 @@ SIM300.py:14:1: SIM300 [*] Yoda conditions are discouraged, use `age == JediOrde
14 | JediOrder.YODA == age # SIM300
| ^^^^^^^^^^^^^^^^^^^^^ SIM300
15 | 0 < (number - 100) # SIM300
16 | SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) # SIM300
16 | B<A[0][0]or B
|
= help: Replace Yoda condition with `age == JediOrder.YODA`

Expand All @@ -267,17 +267,17 @@ SIM300.py:14:1: SIM300 [*] Yoda conditions are discouraged, use `age == JediOrde
14 |-JediOrder.YODA == age # SIM300
14 |+age == JediOrder.YODA # SIM300
15 15 | 0 < (number - 100) # SIM300
16 16 | SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) # SIM300
17 17 | B<A[0][0]or B
16 16 | B<A[0][0]or B
17 17 | B or(B)<A[0][0]

SIM300.py:15:1: SIM300 [*] Yoda conditions are discouraged, use `(number - 100) > 0` instead
|
13 | YODA >= age # SIM300
14 | JediOrder.YODA == age # SIM300
15 | 0 < (number - 100) # SIM300
| ^^^^^^^^^^^^^^^^^^ SIM300
16 | SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) # SIM300
17 | B<A[0][0]or B
16 | B<A[0][0]or B
17 | B or(B)<A[0][0]
|
= help: Replace Yoda condition with `(number - 100) > 0`

Expand All @@ -287,70 +287,91 @@ SIM300.py:15:1: SIM300 [*] Yoda conditions are discouraged, use `(number - 100)
14 14 | JediOrder.YODA == age # SIM300
15 |-0 < (number - 100) # SIM300
15 |+(number - 100) > 0 # SIM300
16 16 | SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) # SIM300
17 17 | B<A[0][0]or B
18 18 | B or(B)<A[0][0]
16 16 | B<A[0][0]or B
17 17 | B or(B)<A[0][0]
18 18 | ['upper'] == UPPER_LIST

SIM300.py:16:1: SIM300 [*] Yoda conditions are discouraged
SIM300.py:16:1: SIM300 [*] Yoda conditions are discouraged, use `A[0][0] > B` instead
|
14 | JediOrder.YODA == age # SIM300
15 | 0 < (number - 100) # SIM300
16 | SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) # SIM300
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SIM300
17 | B<A[0][0]or B
18 | B or(B)<A[0][0]
16 | B<A[0][0]or B
| ^^^^^^^^^ SIM300
17 | B or(B)<A[0][0]
18 | ['upper'] == UPPER_LIST
|
= help: Replace Yoda condition
= help: Replace Yoda condition with `A[0][0] > B`

ℹ Safe fix
13 13 | YODA >= age # SIM300
14 14 | JediOrder.YODA == age # SIM300
15 15 | 0 < (number - 100) # SIM300
16 |-SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) # SIM300
16 |+(60 * 60) < SomeClass().settings.SOME_CONSTANT_VALUE # SIM300
17 17 | B<A[0][0]or B
18 18 | B or(B)<A[0][0]
19 19 |
16 |-B<A[0][0]or B
16 |+A[0][0] > B or B
17 17 | B or(B)<A[0][0]
18 18 | ['upper'] == UPPER_LIST
19 19 | {} == DummyHandler.CONFIG

SIM300.py:17:1: SIM300 [*] Yoda conditions are discouraged, use `A[0][0] > B` instead
SIM300.py:17:5: SIM300 [*] Yoda conditions are discouraged, use `A[0][0] > (B)` instead
|
15 | 0 < (number - 100) # SIM300
16 | SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) # SIM300
17 | B<A[0][0]or B
| ^^^^^^^^^ SIM300
18 | B or(B)<A[0][0]
16 | B<A[0][0]or B
17 | B or(B)<A[0][0]
| ^^^^^^^^^^^ SIM300
18 | ['upper'] == UPPER_LIST
19 | {} == DummyHandler.CONFIG
|
= help: Replace Yoda condition with `A[0][0] > B`
= help: Replace Yoda condition with `A[0][0] > (B)`

ℹ Safe fix
14 14 | JediOrder.YODA == age # SIM300
15 15 | 0 < (number - 100) # SIM300
16 16 | SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) # SIM300
17 |-B<A[0][0]or B
17 |+A[0][0] > B or B
18 18 | B or(B)<A[0][0]
19 19 |
20 20 | # OK

SIM300.py:18:5: SIM300 [*] Yoda conditions are discouraged, use `A[0][0] > (B)` instead
16 16 | B<A[0][0]or B
17 |-B or(B)<A[0][0]
17 |+B or A[0][0] > (B)
18 18 | ['upper'] == UPPER_LIST
19 19 | {} == DummyHandler.CONFIG
20 20 |

SIM300.py:18:1: SIM300 [*] Yoda conditions are discouraged, use `UPPER_LIST == ['upper']` instead
|
16 | SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) # SIM300
17 | B<A[0][0]or B
18 | B or(B)<A[0][0]
| ^^^^^^^^^^^ SIM300
19 |
20 | # OK
16 | B<A[0][0]or B
17 | B or(B)<A[0][0]
18 | ['upper'] == UPPER_LIST
| ^^^^^^^^^^^^^^^^^^^^^^^ SIM300
19 | {} == DummyHandler.CONFIG
|
= help: Replace Yoda condition with `A[0][0] > (B)`
= help: Replace Yoda condition with `UPPER_LIST == ['upper']`

ℹ Safe fix
15 15 | 0 < (number - 100) # SIM300
16 16 | SomeClass().settings.SOME_CONSTANT_VALUE > (60 * 60) # SIM300
17 17 | B<A[0][0]or B
18 |-B or(B)<A[0][0]
18 |+B or A[0][0] > (B)
19 19 |
20 20 | # OK
21 21 | compare == "yoda"
16 16 | B<A[0][0]or B
17 17 | B or(B)<A[0][0]
18 |-['upper'] == UPPER_LIST
18 |+UPPER_LIST == ['upper']
19 19 | {} == DummyHandler.CONFIG
20 20 |
21 21 | # OK

SIM300.py:19:1: SIM300 [*] Yoda conditions are discouraged, use `DummyHandler.CONFIG == {}` instead
|
17 | B or(B)<A[0][0]
18 | ['upper'] == UPPER_LIST
19 | {} == DummyHandler.CONFIG
| ^^^^^^^^^^^^^^^^^^^^^^^^^ SIM300
20 |
21 | # OK
|
= help: Replace Yoda condition with `DummyHandler.CONFIG == {}`

ℹ Safe fix
16 16 | B<A[0][0]or B
17 17 | B or(B)<A[0][0]
18 18 | ['upper'] == UPPER_LIST
19 |-{} == DummyHandler.CONFIG
19 |+DummyHandler.CONFIG == {}
20 20 |
21 21 | # OK
22 22 | compare == "yoda"


Loading