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

Jump to bottom

FP fix and documentation for branches_sharing_code lint #7462

Merged
merged 1 commit into from
Jul 14, 2021
Merged

FP fix and documentation for branches_sharing_code lint #7462

merged 1 commit into from
Jul 14, 2021

Conversation

xFrednet
Copy link
Member

Closes #7369

Related #7452 I'm still thinking about the best way to fix this. I could simply add another visitor to ensure that the moved expressions don't modify values being used in the condition, but I'm not totally happy with this due to the complexity. I therefore only documented it for now

changelog: [branches_sharing_code] fixed false positive where block expressions would sometimes be ignored.

@rust-highfive
Copy link

r? @camsteffen

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 13, 2021
xFrednet
xFrednet commented Jul 13, 2021
View reviewed changes
tests/ui/branches_sharing_code/false_positives.rs Outdated
Comment on lines 28 to 46
// #########################
// # Issue 7452
// #########################
fn test() {
let v = vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 0];
let mut counter = 0;
for i in v {
if counter == 0 {
counter += 1;
println!("first");
} else if counter == 1 {
counter += 1;
println!("second");
} else {
counter += 1;
println!("other: {}", i);
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the issue that is not fixed yet. I found it reasonable to add a test for it (even if it fails) as it's also documented in the lint description

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we gain anything by adding this. Tests are valuable when they verify correct behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wasn't sure with this one that's, the reason why I pointed it out. I'll remove it from this PR and then squash the two commits 🙃

camsteffen
camsteffen reviewed Jul 14, 2021
View reviewed changes
clippy_lints/src/copies.rs Outdated
@@ -225,7 +228,7 @@ fn lint_same_then_else<'tcx>(
blocks,
expr,
);
} else if end_eq != 0 || (has_expr && expr_eq) {
} else if (end_eq != 0 && !has_expr) || (has_expr && expr_eq) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the real problem is that scan_block_for_eq can return end_eq > 0 && expr_eq == false? End statements should not be evaluated if the expressions are not equal.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. scan_block_for_eq even contained an if block for this purpose, but the condition was not quite correct, it's fixed in the latest version. 🙃

@camsteffen
Copy link
Contributor

For #7452, I would maybe first collect all the variables referenced in the condition. Then, when determining if statements are equal, if there is a mutable reference to the variable, the statements are not equal. So I think you would add SpanlessEq::disallowed_mut_locals: FxHashSet<HirId>.

@xFrednet
Copy link
Member Author

For #7452, I would maybe first collect all the variables referenced in the condition. Then, when determining if statements are equal, if there is a mutable reference to the variable, the statements are not equal. So I think you would add SpanlessEq::disallowed_mut_locals: FxHashSet<HirId>.

Interesting idea, I thought about expending the UsedValueFinderVisitor to also collect mutations. Putting it into SpanlessEq feels like the wrong place, as it's not really part of the comparison in my eyes, and I feel like SpanlessEq is already quite complex 🤔

@xFrednet xFrednet force-pushed the 7369-branches-sharing-code-else-expr-fp branch from 20de4f2 to bd2e7ef Compare July 14, 2021 19:29
@xFrednet
Copy link
Member Author

The FP is now fixed in scan_block_for_eq where it originated from (very nice catch!!). I've squashed the commits right away, having a fix up commit that just reverted the previous implementation and the FP test seemed a bit unnecessary. I hope that is okay for you 🙃

@xFrednet xFrednet force-pushed the 7369-branches-sharing-code-else-expr-fp branch from bd2e7ef to 6bfbe5c Compare July 14, 2021 19:37
@xFrednet
Fixed branches_sharing_code FP with block expressions in else
61e2808 
And added `branches_sharing_code` PF note to lint doc for `rust-clippy#7452`
@xFrednet xFrednet force-pushed the 7369-branches-sharing-code-else-expr-fp branch from 6bfbe5c to 61e2808 Compare July 14, 2021 19:37
@camsteffen
Copy link
Contributor

Interesting idea, I thought about expending the UsedValueFinderVisitor to also collect mutations. Putting it into SpanlessEq feels like the wrong place, as it's not really part of the comparison in my eyes, and I feel like SpanlessEq is already quite complex 🤔

Yeah I know what you mean. That would work. One more idea for SpanlessEq - expr_fallback could have a counterpart expr_precondition.

@camsteffen
Copy link
Contributor

Looks good, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jul 14, 2021

📌 Commit 61e2808 has been approved by camsteffen

@bors
Copy link
Contributor

bors commented Jul 14, 2021

⌛ Testing commit 61e2808 with merge 2b193e2...

@xFrednet
Copy link
Member Author

Yeah I know what you mean. That would work. One more idea for SpanlessEq - expr_fallback could have a counterpart expr_precondition.

SpanlessEq - expr_fallback doesn't ring a bell for me right now, but it also has been some time since I’ve dug through the code 🤔 Thank you for the suggestions :)

@bors
Copy link
Contributor

bors commented Jul 14, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 2b193e2 to master...

@bors bors merged commit 2b193e2 into rust-lang:master Jul 14, 2021
@bors bors mentioned this pull request Jul 14, 2021
Merged
@MGlolenstine MGlolenstine mentioned this pull request Jul 22, 2021
Closed
@xFrednet xFrednet deleted the 7369-branches-sharing-code-else-expr-fp branch July 28, 2021 16:41
@xFrednet xFrednet mentioned this pull request Sep 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camsteffen camsteffen camsteffen left review comments

Assignees

@camsteffen camsteffen

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

branches_sharing_code incorrectly detects branches using the same code
4 participants
@xFrednet @rust-highfive @camsteffen @bors