[FP] identity_op in front of if #8730

tamaroning · 2022-04-22T03:40:45Z

changelog: FP: [identity_op]: is now allowed in front of if statements, blocks and other expressions where the suggestion would be invalid.

Resolved simular problems with blocks, mathces, and loops.
identity_op always does NOT suggest reducing 0 + if b { 1 } else { 2 } + 3 into if b { 1 } else { 2 } + 3 even in the case that the expression is in f(expr) or let x = expr; for now.

rust-highfive · 2022-04-22T03:40:48Z

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

hellow554 · 2022-04-22T06:04:29Z

tests/ui/identity_op.rs

+    // See #8724
+    let a = 0;
+    let b = true;
+    0 + if b { 1 } else { 2 };


I would like to see a testcase like let _c = 0 + if b { 1 } else { 2 };
Maybe also a function like I wrote in #8724:

pub fn decide(a: bool, b: bool) -> u32 { 0 + if a { 1 } else { 2 } + if b { 3 } else { 5 } }

tamaroning · 2022-04-22T16:17:15Z

Hmm, uitest somehow fails.
~~Despite of attribute #[allow(clippy::identity_op)], the warning of identity_op occurs.~~

...
---- compile_test stdout ----
diff of stderr:

+error: the operation is ineffective. Consider reducing it to `i`
error: test failed, to rerun pass '--test compile-test'
+  --> $DIR/without_loop_counters.rs:104:13
+   |
+LL |         dst[i - 0] = src[i];
+   |             ^^^^^
+   |
+   = note: `-D clippy::identity-op` implied by `-D warnings`
+
...

rust-clippy/tests/ui/manual_memcpy/without_loop_counters.rs

Lines 102 to 105 in ed22428

    
           #[allow(clippy::identity_op)] 
        
           for i in 0..5 { 
        
               dst[i - 0] = src[i]; 
        
           }

4/23
Updated without_loop_counters test

clippy_lints/src/identity_op.rs

+    fn reducible_to_right(&mut self, right: &'tcx Expr<'_>) -> bool {
+        if self.top_level_binop {
+            true
+        } else {
+            !if let ExprKind::If(..) | ExprKind::Match(..) = right.kind {
+                self.leftmost
+            } else {
+                false
            }
-            match cmp.node {
-                BinOpKind::Add | BinOpKind::BitOr | BinOpKind::BitXor => {
-                    check(cx, left, 0, e.span, right.span);
-                    check(cx, right, 0, e.span, left.span);
-                },
-                BinOpKind::Shl | BinOpKind::Shr | BinOpKind::Sub => check(cx, right, 0, e.span, left.span),
-                BinOpKind::Mul => {
-                    check(cx, left, 1, e.span, right.span);
-                    check(cx, right, 1, e.span, left.span);
-                },
-                BinOpKind::Div => check(cx, right, 1, e.span, left.span),
-                BinOpKind::BitAnd => {
-                    check(cx, left, -1, e.span, right.span);
-                    check(cx, right, -1, e.span, left.span);
-                },
-                BinOpKind::Rem => check_remainder(cx, left, right, e.span, left.span),
-                _ => (),
+        }
+    }


Manishearth · 2022-04-25T07:39:20Z

r? @Alexendoo

(I'm travelling rn and might not be able to do full reviews, can rubber-stamp after you've had a look at it)

Alexendoo

👋

Alexendoo · 2022-04-25T12:54:11Z

clippy_lints/src/identity_op.rs

+struct ExprVisitor<'a, 'tcx> {
+    cx: &'a LateContext<'tcx>,
+    leftmost: bool,
+    top_level_binop: bool,
+}


I think a visitor is too confusing here, e.g. leftmost is always true by the time it reaches the self.reducible_to_right calls

A simpler alternative would be to go back to using check_expr in the lint pass and checking if e's parent expr is an ExprKind::Binary to determine if it's top level or not

Thanks for your review. I fixed it.

I think the visitor can be removed entirely, at least for all the test cases leftmost is still always true in self.reducible_to_right because the order statements are visited in are

a + b + c + d; // 1 LLLLLLLLL R // 2 LLLLL R // 3 L R

I believe just checking the ExprKind of right as you do in reducible_to_right + is_toplevel_binary is enough

Ohh I misunderstood what you meant. And it seems to work for test cases.

Alexendoo · 2022-04-28T16:01:10Z

clippy_lints/src/identity_op.rs

+// Checks if `left op ..right` can be actually reduced to `right`
+// e.g. `0 + if b { 1 } + else { 2 } +  if b { 3 } + else { 4 }`
+// cannot be reduced to `if b { 1 } + else { 2 } +  if b { 3 } + else { 4 }`
+// See #8724


nit

Suggested change

// Checks if `left op ..right` can be actually reduced to `right`

// e.g. `0 + if b { 1 } + else { 2 } + if b { 3 } + else { 4 }`

// cannot be reduced to `if b { 1 } + else { 2 } + if b { 3 } + else { 4 }`

// See #8724

/// Checks if `left op ..right` can be actually reduced to `right`

/// e.g. `0 + if b { 1 } else { 2 } + if b { 3 } else { 4 }`

/// cannot be reduced to `if b { 1 } else { 2 } + if b { 3 } else { 4 }`

/// See #8724

Thanks. Fixed it.

Alexendoo · 2022-04-28T16:58:52Z

tests/ui/identity_op.rs

+    let _ = 0 + if 0 + 1 > 0 { 1 } else { 2 } + if 0 + 1 > 0 { 3 } else { 4 };
+    let _ = 0 + match 0 + 1 { 0 => 10, _ => 20 } + match 0 + 1  { 0 => 30, _ => 40 };


There's a false negative in here, in contexts where it's unambiguously an expression it is fine to reduce these, e.g.

let b = true; f(0 + if b { 1 } else { 2 } + 3); let _ = 0 + if b { 1 } else { 2 } + 3; const _: i32 = 0 + if b { 1 } else { 2 } + 3;

Perhaps instead of get_parent_expr it should be a get_parent_node, checking the node's variant

Thanks.
But I think it seems to be enough to use get_parent_expr.
Is there need to check node variants, using get_parent_node?

Those cases don't lint currently but they could do is what I mean, as in

// doesn't trigger the lint f(0 + if b { 1 } else { 2 } + 3); // but it could, as the following is valid f(if b { 1 } else { 2 } + 3);

It'd be also fine to leave it as is and make a note of that under a Known problems section in the lint description

I didn't aware of this case. I will leave it as it is and add documents.

Added documents

Alexendoo · 2022-04-28T16:59:20Z

tests/ui/manual_memcpy/without_loop_counters.rs

@@ -99,7 +100,6 @@ pub fn manual_copy(src: &[i32], dst: &mut [i32], dst2: &mut [i32]) {
        dst[i] = src[i - from];
    }

-    #[allow(clippy::identity_op)]


nit: can move this back now

Alexendoo · 2022-04-28T17:00:01Z

clippy_lints/src/identity_op.rs

+// cannot be reduced to `if b { 1 } + else { 2 } +  if b { 3 } + else { 4 }`
+// See #8724
+fn reducible_to_right(cx: &LateContext<'_>, binary: &Expr<'_>, right: &Expr<'_>) -> bool {
+    if let ExprKind::If(..) | ExprKind::Match(..) = right.kind {


Can also include ExprKind::Block and ExprKind::Loop

Manishearth · 2022-04-29T11:19:01Z

@bors deletage=Alexendoo

Manishearth · 2022-04-29T12:21:00Z

@bors delegate=Alexendoo

bors · 2022-04-29T12:21:02Z

✌️ @Alexendoo can now approve this pull request

Alexendoo · 2022-04-30T22:12:06Z

Thanks @tamaroning!

Changes look good, please could you squash your commits into one with a descriptive message and then it'll be good to go

fix Update identity_op.rs ok update without_loop_counters test chore fix chore remove visitor and leftmost fix add document

Alexendoo · 2022-05-02T18:21:28Z

Thanks!

@bors r+

bors · 2022-05-02T18:21:30Z

📌 Commit 6ad810f has been approved by Alexendoo

[FP] identity_op in front of if fix #8724 changelog:

bors · 2022-05-02T18:21:36Z

⌛ Testing commit 6ad810f with merge 59d7a90...

bors · 2022-05-02T18:22:11Z

💔 Test failed - checks-action_test

xFrednet · 2022-05-02T19:35:18Z

Hey, the changelog entry from the PR description is still missing. Could you add one? Then all tests should pass 🙃

tamaroning · 2022-05-03T03:50:34Z

@xFrednet Sorry, could you rerun the test?

xFrednet · 2022-05-03T07:05:47Z

No problem @tamaroning, usually I would edit the changelog entry myself if it's the last thing, but here I wasn't sure anymore how it was exactly fixed. 🙃 Thank you for the changes 👍

@bors retry

bors · 2022-05-03T07:05:53Z

⌛ Testing commit 6ad810f with merge 0ceacbe...

bors · 2022-05-03T07:21:10Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 0ceacbe to master...

…dnet `identity_op`: add parenthesis to suggestions where required changelog: [`identity_op`]: add parenthesis to suggestions where required Follow up to #8730, wraps the cases we can't lint as-is in parenthesis rather than ignoring them Catches a couple new FPs with mixed operator precedences and `as` casts ```rust // such as 0 + { a } * 2; 0 + a as usize; ``` The suggestions are now applied using `span_lint_and_sugg` rather than appearing in just the message and have a `run-rustfix` test

rust-highfive assigned Manishearth Apr 22, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 22, 2022

hellow554 reviewed Apr 22, 2022

View reviewed changes

tamaroning marked this pull request as ready for review April 22, 2022 16:17

tamaroning commented Apr 23, 2022

View reviewed changes

Alexendoo reviewed Apr 25, 2022

View reviewed changes

giraffate assigned giraffate and unassigned Manishearth Apr 27, 2022

Alexendoo reviewed Apr 28, 2022

View reviewed changes

improve identity_op

6ad810f

fix Update identity_op.rs ok update without_loop_counters test chore fix chore remove visitor and leftmost fix add document

tamaroning force-pushed the fix8724 branch from ec16773 to 6ad810f Compare May 1, 2022 02:04

Alexendoo approved these changes May 2, 2022

View reviewed changes

bors added a commit that referenced this pull request May 2, 2022

Auto merge of #8730 - tamaroning:fix8724, r=Alexendoo

59d7a90

[FP] identity_op in front of if fix #8724 changelog:

bors merged commit 0ceacbe into rust-lang:master May 3, 2022

Alexendoo mentioned this pull request May 4, 2022

identity_op: add parenthesis to suggestions where required #8786

Merged

		let _ = 0 + if 0 + 1 > 0 { 1 } else { 2 } + if 0 + 1 > 0 { 3 } else { 4 };
		let _ = 0 + match 0 + 1 { 0 => 10, _ => 20 } + match 0 + 1 { 0 => 30, _ => 40 };

[FP] identity_op in front of if #8730

[FP] identity_op in front of if #8730

Conversation

tamaroning commented Apr 22, 2022 • edited by xFrednet Loading

rust-highfive commented Apr 22, 2022

Choose a reason for hiding this comment

tamaroning commented Apr 22, 2022 • edited Loading

This comment was marked as outdated.

Manishearth commented Apr 25, 2022

Alexendoo left a 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

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

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 Apr 29, 2022

Manishearth commented Apr 29, 2022

bors commented Apr 29, 2022

Alexendoo commented Apr 30, 2022

Alexendoo commented May 2, 2022

bors commented May 2, 2022

bors commented May 2, 2022

bors commented May 2, 2022

xFrednet commented May 2, 2022 • edited Loading

tamaroning commented May 3, 2022

xFrednet commented May 3, 2022

bors commented May 3, 2022

bors commented May 3, 2022

tamaroning commented Apr 22, 2022 •

edited by xFrednet

Loading

tamaroning commented Apr 22, 2022 •

edited

Loading

xFrednet commented May 2, 2022 •

edited

Loading