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.

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

Fix FP for suspicious_arithmetic_impl from suspicious_trait_impl #5820

Merged
merged 1 commit into from
Jul 26, 2020

Conversation

ThibsG
Copy link
Contributor

@ThibsG ThibsG commented Jul 18, 2020

As discussed in #3215, the suspicious_trait_impl lint causes too many false positives, as it is complex to find out if binary operations are suspicious or not.

This PR restricts the number of binary operations to at most one, otherwise we don't lint.
This can be seen as very conservative, but at least FP can be reduced to bare minimum.

Fixes: #3215

changelog: limit the suspicious_arithmetic_impl lint to one binop, to avoid many FPs

@rust-highfive
Copy link

r? @flip1995

(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 18, 2020
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

I generally agree with this change, just want your input to a thought that I just had: What do you think about a configuration option aggressive = true | false (default: false), that will keep the old behavior?

@flip1995
Copy link
Member

Nvm, I don't think that this would give much benefit.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 26, 2020

📌 Commit 442c8ae has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jul 26, 2020

⌛ Testing commit 442c8ae with merge f5d429c...

@bors
Copy link
Contributor

bors commented Jul 26, 2020

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

@bors bors merged commit f5d429c into rust-lang:master Jul 26, 2020
@ThibsG ThibsG deleted the FixSuspiciousArithmeticImpl branch August 24, 2020 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spurious suspicious_arithmetic_impl
4 participants