[ruff] implement useless if-else (RUF034) #13218

iamlucasvieira · 2024-09-02T21:16:10Z

Summary

Adds a new rule, RUF102, to check for redundant ternary operators. Message shows when both the true and false branches return the same value, as discussed in #12516

Test Plan

cargo test

github-actions · 2024-09-02T21:54:50Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+2 -0 violations, +0 -0 fixes in 2 projects; 52 projects unchanged)

apache/superset (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ superset/utils/decorators.py:158:9: RUF034 Useless if-else condition

pandas-dev/pandas (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ pandas/tests/io/formats/style/test_style.py:923:26: RUF034 Useless if-else condition

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
RUF034	2	2	0	0	0

RubenVanEldik · 2024-09-02T21:55:43Z

Hi Lucas,

I am new to Ruff an am just reading PRs to get a better understanding of how everything works. I was wondering, how did you decide to name it RUF102. Is there any logic behind the RUF0XX, RUF1XX sets?

MichaReiser

This overall looks great to me but we should change the rule code and its name.

I have some concerns with the scope of the rule. I know it has previously been accepted but I only just now looked closer into it.

There's some overlap with SIM108 that simplifies if expressions
The scope of the rule is very narrow. Should it also apply to if-statements?

@AlexWaygood what are your thoughts on the rule?

It would be great if the rule also provided a fix that removes the if expression with the true or false body.

MichaReiser · 2024-09-03T08:04:10Z

crates/ruff_linter/src/codes.rs

@@ -963,6 +963,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
        (Ruff, "033") => (RuleGroup::Preview, rules::ruff::rules::PostInitDefault),
        (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
        (Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA),
+        (Ruff, "102") => (RuleGroup::Preview, rules::ruff::rules::RedundantTernary),


The 1XX ruff rules are reserved for noqa specific rules. Let's change the code to 034.

@RubenVanEldik I think this answers it 😄. This is also my first time contributing here.

MichaReiser · 2024-09-03T08:08:47Z

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

@@ -1404,6 +1404,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
            if checker.enabled(Rule::IfExpInsteadOfOrOperator) {
                refurb::rules::if_exp_instead_of_or_operator(checker, if_exp);
            }
+            if checker.enabled(Rule::RedundantTernary) {


I quickly checked other "ternary" specific rules and all of them use the term if-expr instead. We also use the term Useless when referring to useless code.

How about: useless-if-expr?

useless-if-expr sounds good to me. I agree that that's more approachable language than ternary

Sounds good, but if we also cover if blocks maybe we will need a different name. For example: useless-if-logic

hmm, good point. Maybe useless-if-else? Or useless-if-condition?

I likeuseless-if-else

I like useless-if-else. It provides room for expanding the rule in the future.

MichaReiser · 2024-09-03T08:16:19Z

The ecosystem check shows an interesting false positive:

https://github.com/apache/superset/blob/548d543efe81ecd6f0a6657550230b765ab4d955/superset/utils/decorators.py#L158

AlexWaygood · 2024-09-03T10:36:26Z

The ecosystem check shows an interesting false positive:

apache/superset@548d543/superset/utils/decorators.py#L158

@MichaReiser is that a false positive? The snippet in question is:

sorted_args = tuple(
    x if hasattr(x, "__repr__") else x for x in [*args, *sorted(kwargs.items())]
)

It seems like each element in the iterable there will be collected whether or not it has a __repr__ method, so I think it can just be simplified to

sorted_args = tuple(x for x in [*args, *sorted(kwargs.items())])

or even just

sorted_args = (*args, *sorted(kwargs.items()))

What they probably wanted to do was

sorted_args = tuple(
    repr(x) if hasattr(x, "__repr__") else x for x in [*args, *sorted(kwargs.items())]
)

But even that doesn't make much sense, because all objects in Python have a __repr__ attribute:

>>> hasattr(object, "__repr__")
True

AlexWaygood · 2024-09-03T10:42:04Z

There's some overlap with SIM108 that simplifies if expressions

I support this being a separate rule to SIM108. SIM108 is quite opinionated IMO: I think it's questionable whether rewriting an if/else block as a ternary expression is always easier to read in Python, or always more idiomatic. This rule, on the other hand, seems like it's pointing out something that you'd almost always want to avoid. A useless ternary expression is almost certainly a straight-up mistake that you didn't mean to write 😄

The scope of the rule is very narrow. Should it also apply to if-statements?

Also applying it to if/else statements sounds reasonable to me.

iamlucasvieira · 2024-09-03T11:28:46Z

It would be great if the rule also provided a fix that removes the if expression with the true or false body.

I don't think a fix would fit this rule because users probably end up in this situation when they mistype one of the ternary ends. Instead of just removing the ternary, they probably need to correct one of the sides.

iamlucasvieira · 2024-09-03T16:23:45Z

The scope of the rule is very narrow. Should it also apply to if-statements?

I wonder if rule SIM114 address that

MichaReiser · 2024-09-03T16:35:31Z

SIM114 only seems to flag if-elif but not if-else. https://play.ruff.rs/ae79ec83-f045-4262-bcfb-dc8a08a19460

AlexWaygood

LGTM. I think we could merge this now and expand it to if/else statements as a followup. @MichaReiser what do you think?

iamlucasvieira added 3 commits September 2, 2024 23:06

[ruff] Add redundant ternary rule

8fb30fd

Fix rule number

068ba48

Regenerate snapshot

b0258e8

MichaReiser reviewed Sep 3, 2024

View reviewed changes

MichaReiser added rule Implementing or modifying a lint rule preview Related to preview mode features labels Sep 3, 2024

Change rule name to RUF034

111f4d0

AlexWaygood changed the title ~~[ruff] implement redundant ternary rule (RUF102)~~ [ruff] implement redundant ternary rule (RUF034) Sep 3, 2024

Rename rule to useless-if-else

9754915

AlexWaygood approved these changes Sep 3, 2024

View reviewed changes

MichaReiser changed the title ~~[ruff] implement redundant ternary rule (RUF034)~~ [ruff] implement useless if-else (RUF034) Sep 4, 2024

MichaReiser merged commit e37bde4 into astral-sh:main Sep 4, 2024
20 checks passed

MichaReiser mentioned this pull request Sep 4, 2024

Complain about redundant ternaries? #12516

Closed

BrewTestBot mentioned this pull request Sep 5, 2024

ruff 0.6.4 Homebrew/homebrew-core#183619

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ruff] implement useless if-else (RUF034) #13218

[ruff] implement useless if-else (RUF034) #13218

iamlucasvieira commented Sep 2, 2024

github-actions bot commented Sep 2, 2024 •

edited

Loading

RubenVanEldik commented Sep 2, 2024

MichaReiser left a comment •

edited

Loading

MichaReiser Sep 3, 2024

iamlucasvieira Sep 3, 2024

MichaReiser Sep 3, 2024

AlexWaygood Sep 3, 2024 •

edited

Loading

iamlucasvieira Sep 3, 2024

AlexWaygood Sep 3, 2024 •

edited

Loading

iamlucasvieira Sep 3, 2024

MichaReiser Sep 3, 2024

MichaReiser commented Sep 3, 2024

AlexWaygood commented Sep 3, 2024 •

edited

Loading

AlexWaygood commented Sep 3, 2024

iamlucasvieira commented Sep 3, 2024

iamlucasvieira commented Sep 3, 2024

MichaReiser commented Sep 3, 2024

AlexWaygood left a comment

[ruff] implement useless if-else (RUF034) #13218

[ruff] implement useless if-else (RUF034) #13218

Conversation

iamlucasvieira commented Sep 2, 2024

Summary

Test Plan

github-actions bot commented Sep 2, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

RubenVanEldik commented Sep 2, 2024

MichaReiser left a comment • edited Loading

Choose a reason for hiding this comment

MichaReiser Sep 3, 2024

Choose a reason for hiding this comment

iamlucasvieira Sep 3, 2024

Choose a reason for hiding this comment

MichaReiser Sep 3, 2024

Choose a reason for hiding this comment

AlexWaygood Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

iamlucasvieira Sep 3, 2024

Choose a reason for hiding this comment

AlexWaygood Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

iamlucasvieira Sep 3, 2024

Choose a reason for hiding this comment

MichaReiser Sep 3, 2024

Choose a reason for hiding this comment

MichaReiser commented Sep 3, 2024

AlexWaygood commented Sep 3, 2024 • edited Loading

AlexWaygood commented Sep 3, 2024

iamlucasvieira commented Sep 3, 2024

iamlucasvieira commented Sep 3, 2024

MichaReiser commented Sep 3, 2024

AlexWaygood left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 2, 2024 •

edited

Loading

`ruff-ecosystem` results

MichaReiser left a comment •

edited

Loading

AlexWaygood Sep 3, 2024 •

edited

Loading

AlexWaygood Sep 3, 2024 •

edited

Loading

AlexWaygood commented Sep 3, 2024 •

edited

Loading