New Lint: `branches_sharing_code` #6463

xFrednet · 2020-12-17T23:28:53Z

This lint checks if all if-blocks contain some statements that are the same and can be moved out of the blocks to prevent code duplication. Here is an example:

let _ = if ... {
    println!("Start"); // <-- Lint for code duplication
    let _a = 99;
    println!("End"); // <-- Lint for code duplication
    false
} else {
    println!("Start");
    let _b = 17;
    println!("End"); 
    false
};

This could be written as:

println!("Start");

let _ = if ... {
    let _a = 99;
    false
} else {
    let _b = 17;
    false
};

println!("End");

This lint will get masked by the IF_SAME_THEN_ELSE lint. I think it makes more sense to only emit one lint per if block. This means that the folloing example:

if ... {
    let _a = 17;
} else {
    let _a = 17;
}

Will only trigger the IF_SAME_THEN_ELSE lint and not the SHARED_CODE_IN_IF_BLOCKS lint.

closes: #5234

changelog: Added a new lint: branches_sharing_code

And hello to the one that is writing the changelog for this release :D

rust-highfive · 2020-12-17T23:28:56Z

r? @flip1995

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

xFrednet · 2020-12-18T00:25:02Z

I'll have to fix some if statements in the project. This is a good example what I mean with "this lint is a bit aggressive". I'll do that in a separate commit tomorrow 🙃

flip1995

I like the concept of this lint. But I also think that this will have many FPs. So this should be allow-by-default.

clippy_lints/src/copies.rs

flip1995 · 2020-12-27T16:03:34Z

tests/ui/shared_code_in_if_blocks.rs

+    let _z = if x == 8 {
+        println!("Branch 1");
+        let z = 10;
+        foo!();
+        z
+    } else {
+        println!("Branch 2");
+        let z = 10;
+        foo!();
+        z
+    };


What does the lint do, if you add a slightly different variant of this test:

let _z = if x == 8 { println!("Branch 1"); let mut z = 1; z += 10; foo!(); z } else { println!("Branch 2"); let mut z = 2; z += 10; foo!(); z };

The lint would suggest to move the last three lines out of the blocks to prevent code duplication:

Lint output

error: All if blocks contain the same code at the end --> $DIR/test_file.rs:174:9 | LL | / z += 10; LL | | foo!(); LL | | z LL | | } else { | |_____^ | = help: Consider moving the code out of the if statement to prevent code duplication

This is an excellent example why this lint is aggressive and why the suggestions are not as automatically applicable. A possible rewrite would be:

let _z = { let mut z = if x == 8 { println!("Branch 1"); 1 } else { println!("Branch 2"); 2 }; z += 10; foo!(); z }

This is not always the nicest way especially for simple statements like these. This can also be linked to the discussion in #3770 about the if_same_then_else what could should actually be moved out. I sadly don't have a better suggestion right now and I believe that this is okay for now since the lint is allowed by default.

So this case could be prevented, if we could make sure that:

The definition of the assigned variable is before the if-else-block (aka not in the if-else-block) OR

the definition of the assigned variable is contained in the end block this lint suggests to move out of the if-else-block.

1. implies 2. here. So collecting all of the definitions in the if-else and then check if the assigned to variable is in that collection should be a good heuristic.

The hard part of implementing this was to decide what should be lined and what not. I see your suggestion and I'll try to implement it :)

flip1995 · 2020-12-27T16:07:16Z

tests/ui/shared_code_in_if_blocks.rs

+        println!("End of block");
+        false
+    };
+}


I have a feeling that many tests are missing. Mainly tests where the returned value is present somewhere in the code that should be moved out. I think we should just not lint if the return value is used in the end stmts, but can lint if it is only used in the start stmts.

I think that even these cases should be included in the lint since it can still be code duplication. My previous comment shows a rewrite where the return value is not a part of the linted code.

The main reason why I'm not in favor of your suggestion is the implementation. This has enabled me to reuse the provided SpanlessEq implementation with very few changes. Adding the simple rule that the return value needs to be in the moved code would in my opinion add a lot of edge cases and limit the use of SpanlessEq since it wasn't designed for value tracking. It can quickly end up in a lot of complex code which is difficult to maintain for little benefit.

That's my opinion with my current knowledge, but I might have overlooked something. I'm still fairly new to clippy and I still learn something every time I work on it 🙃.

I've implemented your other suggestions. Thank you for those, they were definitely helpful 👍. I'll wait on an answer on this comment before adding a bunch of tests (and probably separating them into different files to make it more readable) 🙃

I think rewriting it like you suggested above makes it way harder to read and we shouldn't suggest that. Also this is not always possible. For example if z is modified, but not returned. With my suggested change, I don't think you have to modify existing code, but rather just add a visitor and an additional check.

In general, I don't think a lint just saying "this is duplicated" without an easy suggestion how to improve it, is a good lint. We shouldn't say: "this is wrong, have fun rewriting it" without giving guidance on how to do it.

Jup, the rewrite is definitely not pretty 😅.

The lint currently only suggests moving the code out of the block. Making a correct concrete suggestion seamed difficult as the lint didn't make sure that the moved values will also be available after the load. This should now be possible with your suggested change to make sure that the return value is included in the moved code or at least available in the outer scope. It will need some fine-tuning and probably some testing but should be possible 🙃.

xFrednet · 2020-12-27T16:42:47Z

So this should be allow-by-default.

How can I make it allow by default, do I just change the lint category to pedantic or is there another way?

Your suggestions look god at a first glance I'll probably work on them tomorrow 🙃 Thank you for the review!

flip1995 · 2020-12-27T16:47:40Z

Yes, just put it in the pedantic group. 👍

flip1995 · 2021-01-04T09:34:23Z

Making a correct concrete suggestion seamed difficult as the lint didn't make sure that the moved values will also be available after the load.

Yes, I can definitely see that, and you don't have to provide an auto applicable suggestion here. My argument is, that if we can't even provide a suggestion, because it would be too hard, we also shouldn't lint.

If you're stuck or finished with the thing I requested and want the next review, let me know by pinging me or requesting a review via the GitHub UI.

bors · 2021-01-04T12:58:35Z

☔ The latest upstream changes (presumably #6544) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2021-01-15T10:01:18Z

☔ The latest upstream changes (presumably #6586) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet · 2021-01-16T20:12:36Z

Hey @flip1995, thank you for sending me back to the drawing board with the previous implementation of this lint. The implementation is now a bit more complex but way better in my opinion. It checks that all used values can be assessed in the new moved-to scope. It also adds some helpful notes if the suggestion is most likely not 100% correct. You can see some examples in the test output. This implementation is now up for review if the pipeline passes :)

I changed the lint category to nursery as it seems to be pretty stable right now. We can of course change this later, but I would like to at least try to have it in the nursery category for a bit if that's okay?

@rustbot label -S-waiting-on-author

flip1995 · 2021-01-17T17:16:07Z

Thanks for putting all this work into this! Sadly I have to defer the review to another time (hopefully next weekend). I'm already reviewing for 4h and my brain is mush.

I changed the lint category to nursery as it seems to be pretty stable right now.

yeah sure.

…else ifs

…tcheck

xFrednet · 2021-04-05T11:42:38Z

The rebase was simple and the PR is ready for merge from my side. Thank you for the review! I know that it was quite a monster to begin with 😅 . The next ones will be smaller 🙃

phansch · 2021-04-05T12:52:01Z

@bors r+ thanks a lot! 😄

bors · 2021-04-05T12:52:02Z

📌 Commit a6f54f5 has been approved by phansch

bors · 2021-04-05T12:52:09Z

⌛ Testing commit a6f54f5 with merge ead70c3...

New Lint: `branches_sharing_code` This lint checks if all `if`-blocks contain some statements that are the same and can be moved out of the blocks to prevent code duplication. Here is an example: ```rust let _ = if ... { println!("Start"); // <-- Lint for code duplication let _a = 99; println!("End"); // <-- Lint for code duplication false } else { println!("Start"); let _b = 17; println!("End"); false }; ``` This could be written as: ```rust println!("Start"); let _ = if ... { let _a = 99; false } else { let _b = 17; false }; println!("End"); ``` --- This lint will get masked by the `IF_SAME_THEN_ELSE` lint. I think it makes more sense to only emit one lint per if block. This means that the folloing example: ```rust if ... { let _a = 17; } else { let _a = 17; } ``` Will only trigger the `IF_SAME_THEN_ELSE` lint and not the `SHARED_CODE_IN_IF_BLOCKS` lint. --- closes: #5234 changelog: Added a new lint: `branches_sharing_code` And hello to the one that is writing the changelog for this release :D

bors · 2021-04-05T14:22:09Z

💥 Test timed out

xFrednet · 2021-04-05T14:31:27Z

Homu seems to be stuck, another PR from Rust itself is also struck in the pending stage for the last hour 🤔 bors queue

camsteffen · 2021-04-05T14:47:43Z

It only tests one PR at a time.

@bors retry

xFrednet · 2021-04-05T14:53:19Z

Yeah, this PR remained in the pending stage for the two hours. I've asked in the infra zulip stream if humo might be stuck. #6905 seems to be in the same state. It's also interesting that retry isn't working.

bors · 2021-04-05T16:18:28Z

⌛ Testing commit a6f54f5 with merge d34cb85...

New Lint: `branches_sharing_code` This lint checks if all `if`-blocks contain some statements that are the same and can be moved out of the blocks to prevent code duplication. Here is an example: ```rust let _ = if ... { println!("Start"); // <-- Lint for code duplication let _a = 99; println!("End"); // <-- Lint for code duplication false } else { println!("Start"); let _b = 17; println!("End"); false }; ``` This could be written as: ```rust println!("Start"); let _ = if ... { let _a = 99; false } else { let _b = 17; false }; println!("End"); ``` --- This lint will get masked by the `IF_SAME_THEN_ELSE` lint. I think it makes more sense to only emit one lint per if block. This means that the folloing example: ```rust if ... { let _a = 17; } else { let _a = 17; } ``` Will only trigger the `IF_SAME_THEN_ELSE` lint and not the `SHARED_CODE_IN_IF_BLOCKS` lint. --- closes: #5234 changelog: Added a new lint: `branches_sharing_code` And hello to the one that is writing the changelog for this release :D

bors · 2021-04-05T17:48:29Z

💥 Test timed out

phansch · 2021-04-05T18:49:00Z

@bors retry

bors · 2021-04-05T19:00:45Z

⌛ Testing commit a6f54f5 with merge d91da40...

bors · 2021-04-05T19:14:52Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: phansch
Pushing d91da40 to master...

rust-highfive assigned flip1995 Dec 17, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 17, 2020

xFrednet changed the title ~~New Lint: if_shares_code_with_else~~ WIP: New Lint: if_shares_code_with_else Dec 17, 2020

xFrednet marked this pull request as draft December 17, 2020 23:42

xFrednet force-pushed the 5234-shared-code-in-if-blocks branch from 511c888 to 4637f05 Compare December 18, 2020 00:13

xFrednet marked this pull request as ready for review December 18, 2020 23:22

xFrednet changed the title ~~WIP: New Lint: if_shares_code_with_else~~ New Lint: if_shares_code_with_else Dec 18, 2020

flip1995 reviewed Dec 27, 2020

View reviewed changes

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 27, 2020

xFrednet force-pushed the 5234-shared-code-in-if-blocks branch from 1ff5e48 to 04db249 Compare December 28, 2020 18:49

xFrednet marked this pull request as draft December 30, 2020 22:48

xFrednet changed the title ~~New Lint: if_shares_code_with_else~~ WIP: New Lint: if_shares_code_with_else Dec 30, 2020

xFrednet force-pushed the 5234-shared-code-in-if-blocks branch 2 times, most recently from 6b13418 to d7195a8 Compare January 11, 2021 23:28

xFrednet force-pushed the 5234-shared-code-in-if-blocks branch from d2b051e to aa88925 Compare January 16, 2021 13:04

xFrednet requested a review from flip1995 January 16, 2021 20:13

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 16, 2021

xFrednet marked this pull request as ready for review January 16, 2021 20:14

rustbot removed the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Jan 16, 2021

xFrednet changed the title ~~WIP: New Lint: if_shares_code_with_else~~ New Lint: if_shares_code_with_else Jan 16, 2021

xFrednet added 9 commits April 5, 2021 13:33

The shared_code_in_if_blocks lint only operats on entire if expr not …

469ff96

…else ifs

Improved shared_code_in_if_blocks output readability and added tests

8efc6ac

Improved shared_code_in_if_blocks message and added test stderrs

b1d26e5

Updated code for dogfood

65ed5a6

Adapted the lint to use the new SpanlessEq

c74e49e

Moving shared_code_in_if_blocks to clippy::complexity and running lin…

617c65b

…tcheck

Only running shared_code_in_if_blocks only for if statements

8c0b4d7

Test for empty blocks and update from master

7c9e192

Renaming the lint to branches_sharing_code and fixing typos

a6f54f5

xFrednet force-pushed the 5234-shared-code-in-if-blocks branch from 610535c to a6f54f5 Compare April 5, 2021 11:41

bors merged commit d91da40 into rust-lang:master Apr 5, 2021

bors mentioned this pull request Apr 5, 2021

Fix all occurences needless_borrow internally #6931

Merged

This was referenced Apr 9, 2021

False positive branches_sharing_code: different inferred type #7053

Closed

False positive branches_sharing_code: trailing expression in else of if-let #7054

Closed

xFrednet deleted the 5234-shared-code-in-if-blocks branch July 28, 2021 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Lint: `branches_sharing_code` #6463

New Lint: `branches_sharing_code` #6463

xFrednet commented Dec 17, 2020 •

edited

Loading

rust-highfive commented Dec 17, 2020

xFrednet commented Dec 18, 2020

flip1995 left a comment

flip1995 Dec 27, 2020

xFrednet Dec 28, 2020

flip1995 Dec 30, 2020

xFrednet Dec 30, 2020

flip1995 Dec 27, 2020

xFrednet Dec 28, 2020 •

edited

Loading

flip1995 Dec 30, 2020

xFrednet Dec 30, 2020

xFrednet commented Dec 27, 2020

flip1995 commented Dec 27, 2020

flip1995 commented Jan 4, 2021

bors commented Jan 4, 2021

bors commented Jan 15, 2021

xFrednet commented Jan 16, 2021 •

edited

Loading

flip1995 commented Jan 17, 2021

xFrednet commented Apr 5, 2021

phansch commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

xFrednet commented Apr 5, 2021

camsteffen commented Apr 5, 2021

xFrednet commented Apr 5, 2021 •

edited

Loading

bors commented Apr 5, 2021

bors commented Apr 5, 2021

phansch commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

New Lint: branches_sharing_code #6463

New Lint: branches_sharing_code #6463

Conversation

xFrednet commented Dec 17, 2020 • edited Loading

rust-highfive commented Dec 17, 2020

xFrednet commented Dec 18, 2020

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 Dec 27, 2020

Choose a reason for hiding this comment

xFrednet Dec 28, 2020

Choose a reason for hiding this comment

flip1995 Dec 30, 2020

Choose a reason for hiding this comment

xFrednet Dec 30, 2020

Choose a reason for hiding this comment

flip1995 Dec 27, 2020

Choose a reason for hiding this comment

xFrednet Dec 28, 2020 • edited Loading

Choose a reason for hiding this comment

flip1995 Dec 30, 2020

Choose a reason for hiding this comment

xFrednet Dec 30, 2020

Choose a reason for hiding this comment

xFrednet commented Dec 27, 2020

flip1995 commented Dec 27, 2020

flip1995 commented Jan 4, 2021

bors commented Jan 4, 2021

bors commented Jan 15, 2021

xFrednet commented Jan 16, 2021 • edited Loading

flip1995 commented Jan 17, 2021

xFrednet commented Apr 5, 2021

phansch commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

xFrednet commented Apr 5, 2021

camsteffen commented Apr 5, 2021

xFrednet commented Apr 5, 2021 • edited Loading

bors commented Apr 5, 2021

bors commented Apr 5, 2021

phansch commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

New Lint: `branches_sharing_code` #6463

New Lint: `branches_sharing_code` #6463

xFrednet commented Dec 17, 2020 •

edited

Loading

xFrednet Dec 28, 2020 •

edited

Loading

xFrednet commented Jan 16, 2021 •

edited

Loading

xFrednet commented Apr 5, 2021 •

edited

Loading