New lints: `impossible_comparisons` and `redundant_comparisons` #10843

MortenLohne · 2023-05-29T20:38:18Z

Inspired by a bug we had in production, like all good lints ;)

Adds two lints for "double" comparisons, specifically when the same expression is being compared against two different constants.

impossible_comparisons checks for expressions that can never be true at all. Example:

status_code <= 400 && status_code > 500

Presumably, the programmer intended to write status_code >= 400 && status_code < 500 in this case.

redundant_comparisons checks for expressions where either half has no effect. Example:

status_code <= 400 && status_code < 500

Works with other literal types like floats and strings, and some cases where the constant is more complex than a literal, see the tests for more.

Limitations and/or future work:

Doesn't work if the LHS can have side-effects at all, for example by being a function call
Only works for exactly two comparison expressions, so x > y && x > z && x < w won't get checked
Doesn't check for comparison expressions combined with ||. Very similar logic could be applied there.

changelog: New lints [impossible_comparisons] and [redundant_comparisons]

rustbot · 2023-05-29T20:38:23Z

r? @Manishearth

(rustbot has picked a reviewer for you, use r? to override)

Manishearth · 2023-05-31T14:27:06Z

clippy_lints/src/operators/double_const_comparison.rs

+        // Ensure that the binary operator is &&
+        if and_op.node == BinOpKind::And;
+
+        let typeck_results = cx.typeck_results();


nit: we should filter by the type first to avoid doing expensive const computations

Thanks for the feedback. This was a bit tricky to implement, at least if we want to detect a > b code where a and b are not the same type. I've pushed a commit that adds test cases for this.

Here's the helper function I came up with for implementing your optimization: (Not committed yet):

// Check that our binary operation is of the form `a CMP b && c CMP d`, // and that the same *type* occurs at least once on both sides of `&&` fn is_double_comparison<'tcx>( typeck: &TypeckResults<'tcx>, left_cond: &'tcx Expr<'tcx>, right_cond: &'tcx Expr<'tcx>, ) -> bool { if_chain! { if let ExprKind::Binary(left_cmp_op, left_left_op, left_right_op) = left_cond.kind; if let ExprKind::Binary(right_cmp_op, right_left_op, right_right_op) = right_cond.kind; if CmpOp::try_from(left_cmp_op.node).is_ok(); if CmpOp::try_from(right_cmp_op.node).is_ok(); then { let left_types = [typeck.expr_ty(left_left_op), typeck.expr_ty(left_right_op)]; let right_types = [typeck.expr_ty(right_left_op), typeck.expr_ty(right_right_op)]; left_types.iter().any(|typ| right_types.contains(typ)) } else { false } } }

Honestly, I think we need to benchmark this. The new code adds a fair bit of complexity, and it's not obvious to me that it's a performance win at all. Do you know a good way to benchmark a single lint on real-world code?

It's kinda annoying, you can just delete all other lint passes and run it and see. The clippy channel in zulip may know.

(The problem is that rustc will run all lints including disabled ones, as one giant interleaved pass.)

I was going to suggest that we just expr_ty the two parts and check against concrete numeric types but I do realize that still needs the splitting. Maybe it doesn't matter, and those operators are used with numeric types most of the time anyway.

I'm ok merging this with the lint name updated and if you'd like you can explore benchmarking and optimizing it later.

I think I could have matched both halves against a list of concrete types, but that might also a bit involved, since the lint also works for string slices, tuples, arrays, basically anything in Constant. I'd like for the lint to keep working on those types, since I think it's easy to makes mistakes with them as well, e.g. the name < "Jennifer" && name > "Shannon" test case.

I wasn't able to get any benchmarking to work, so I settled on a middle solution and implemented the simplest possible optimization: Check that both sides of the root && operator are themselves binary operations. That should filter out most expressions before the expensive const checking stuff.

Manishearth · 2023-05-31T14:29:31Z

clippy_lints/src/operators/mod.rs

+    /// status_code <= 400 && status_code > 500;
+    /// ```
+    #[clippy::version = "1.71.0"]
+    pub IMPOSSIBLE_DOUBLE_CONST_COMPARISONS,


issue: unsure about the name. opened a thread https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/lint.20naming.3A.20double.20comparisons

i think IMPOSSIBLE_DOUBLE_COMPARISONS without the CONST is slightly better

from chat

Maybe something like redundant_comparisons + impossible_comparisons

The double/const part seem more like implementation details to me. Also double made me think of f64 initially

we should probably make these work for all numbers and rename the lints

Manishearth · 2023-06-10T13:45:27Z

cc @Centri3 for additional review

(I'll hopefully rereview soon)

MortenLohne · 2023-06-10T14:07:09Z

I've completed the rename into impossible_comparisons and redundant_comparisons respectively, from discussion on Zulip.

Centri3

Don't have the chance to take a closer look currently, but from what I see of the implementation it looks pretty good to me :) I gave a suggestion for the lints' descriptions.

clippy_lints/src/operators/mod.rs

Centri3 · 2023-06-10T14:19:17Z

clippy_lints/src/operators/mod.rs

+    ///
+    /// ### Why is this bad?
+    /// The whole expression can be replaced by `false`,
+    /// which is probably not the programmer's intention


Here, just add a period to the end of both of these sentences :)

Manishearth

r=me after addressing Centri3's comments

Centri3

yeah this still looks good to me :) Just some minor nits, thanks!

Centri3 · 2023-07-16T17:03:20Z

clippy_lints/src/operators/const_comparisons.rs

+// Extract a comparison between a const and non-const
+// Flip yoda conditionals, turnings expressions like `42 < x` into `x > 42`
+fn comparison_to_const<'tcx>(
+    ctx: &mut ConstEvalLateContext<'_, 'tcx>,


nit: Can we not use consts::constant instead? It might be marginally slower but ConstEvalLateContext::new does nothing spectacular so doing it twice isn't too impactful. It's at most a micro-optimization. It would just be cleaner.

Centri3 · 2023-07-16T17:05:18Z

clippy_lints/src/operators/const_comparisons.rs

+        then {
+            if left_cmp_op.direction() == right_cmp_op.direction() {
+                let lhs_str = snippet(cx, left_cond.span, "");
+                let rhs_str = snippet(cx, right_cond.span, "");


nit: We should either use snippet_opt or change "" to something like "<lhs>" instead

Centri3 · 2023-07-16T17:06:01Z

clippy_lints/src/operators/const_comparisons.rs

+            else if !comparison_is_possible(left_cmp_op.direction(), ordering) {
+                let expr_str = snippet(cx, left_expr.span, "");
+                let lhs_str = snippet(cx, left_const_expr.span, "");
+                let rhs_str = snippet(cx, right_const_expr.span, "");


…context()' function that it replaced

…ble_const_comparisons`

…ective_double_const_comparisons'

…ns' and 'ineffective_double_const_comparisons' -> 'redundant_comparisons', after discussion on Zulip

…implify the code, after PR suggestion

Co-authored-by: Catherine Flores <[email protected]>

Manishearth · 2023-08-06T17:20:02Z

@bors r=Centri3

bors · 2023-08-06T17:20:04Z

📌 Commit d628046 has been approved by Centri3

It is now in the queue for this repository.

bors · 2023-08-06T17:20:10Z

⌛ Testing commit d628046 with merge 6a2c8a1...

bors · 2023-08-06T17:30:59Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Centri3
Pushing 6a2c8a1 to master...

rustbot assigned Manishearth May 29, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 29, 2023

Manishearth reviewed May 31, 2023

View reviewed changes

MortenLohne changed the title ~~New lints: impossible_double_const_comparisons and ineffective_double_const_comparisons~~ New lints: impossible_comparisons and redundant_comparisons Jun 10, 2023

Centri3 reviewed Jun 10, 2023

View reviewed changes

Manishearth approved these changes Jun 12, 2023

View reviewed changes

Centri3 approved these changes Jul 16, 2023

View reviewed changes

MortenLohne added 10 commits August 5, 2023 21:28

fix: Make ConstEvalLateContext::new() public, to match the 'constant_…

ab1281f

…context()' function that it replaced

New lints: impossible_double_const_comparisons and `ineffective_dou…

046d3df

…ble_const_comparisons`

Add descriptions for 'impossible_double_const_comparisons' and 'ineff…

e16a2ac

…ective_double_const_comparisons'

Add missing variable decl to doc comment

08e1333

Add tests for const comparisons that compare two different types

8f40d09

Optimize by doing a cheap check for double binary expression first

b5ef66f

Rename 'impossible_double_const_comparisons' -> 'impossible_compariso…

1d61fc1

…ns' and 'ineffective_double_const_comparisons' -> 'redundant_comparisons', after discussion on Zulip

Add lifetime parameter to 'Constant', after rebasing on upstream

9646446

Replace ConstEvalLateContext::new() with two calls to constant() to s…

0e064d5

…implify the code, after PR suggestion

Provide fallback code snippets, if the snippet is not available

3157b96

MortenLohne force-pushed the feat/double-const-comparisons branch from ad5ea9c to 3157b96 Compare August 6, 2023 11:49

Update clippy_lints/src/operators/mod.rs

d628046

Co-authored-by: Catherine Flores <[email protected]>

bors merged commit 6a2c8a1 into rust-lang:master Aug 6, 2023

bors mentioned this pull request Aug 6, 2023

Update ui test crate #11239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New lints: `impossible_comparisons` and `redundant_comparisons` #10843

New lints: `impossible_comparisons` and `redundant_comparisons` #10843

MortenLohne commented May 29, 2023 •

edited

Loading

rustbot commented May 29, 2023

Manishearth May 31, 2023

MortenLohne Jun 3, 2023

Manishearth Jun 3, 2023

MortenLohne Jun 10, 2023

Manishearth May 31, 2023

Manishearth May 31, 2023

Manishearth commented Jun 10, 2023 •

edited

Loading

MortenLohne commented Jun 10, 2023

Centri3 left a comment

Centri3 Jun 10, 2023

Manishearth left a comment

Centri3 left a comment

Centri3 Jul 16, 2023 •

edited

Loading

Centri3 Jul 16, 2023

Centri3 Jul 16, 2023

Manishearth commented Aug 6, 2023

bors commented Aug 6, 2023

bors commented Aug 6, 2023

bors commented Aug 6, 2023

New lints: impossible_comparisons and redundant_comparisons #10843

New lints: impossible_comparisons and redundant_comparisons #10843

Conversation

MortenLohne commented May 29, 2023 • edited Loading

rustbot commented May 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Jun 10, 2023 • edited Loading

MortenLohne commented Jun 10, 2023

Centri3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth left a comment

Choose a reason for hiding this comment

Centri3 left a comment

Choose a reason for hiding this comment

Centri3 Jul 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Aug 6, 2023

bors commented Aug 6, 2023

bors commented Aug 6, 2023

bors commented Aug 6, 2023

New lints: `impossible_comparisons` and `redundant_comparisons` #10843

New lints: `impossible_comparisons` and `redundant_comparisons` #10843

MortenLohne commented May 29, 2023 •

edited

Loading

Manishearth commented Jun 10, 2023 •

edited

Loading

Centri3 Jul 16, 2023 •

edited

Loading