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

Implement rule UnnecessaryPartOfBinaryExpression #5203

Merged
merged 7 commits into from Sep 7, 2022

Conversation

VovaStelmashchuk
Copy link
Contributor

@VovaStelmashchuk VovaStelmashchuk commented Aug 8, 2022

Implement the UnnecessaryPartOfBinaryExpression. The rule works with all binary expression include if and when condition. The rule also works with all predicates.

@github-actions github-actions bot added the rules label Aug 8, 2022
@github-actions
Copy link

github-actions bot commented Aug 8, 2022

Warnings
⚠️ This PR is approved with no milestone set. If merged, it won't appear in the Detekt release notes.
Messages
📖 Thanks for adding a new rule to Detekt ❤️

Generated by 🚫 dangerJS against 6b79951

@cortinico cortinico changed the title Implement check for unnecessary part of binary expression Implement rule UnnecessaryPartOfBinaryExpression Aug 8, 2022
super.visitBinaryExpression(expression)
val children = expression.children.map { it.text.replace(Regex("\\s"), "") }

if (children.size != children.distinct().size) {
Copy link
Member

Choose a reason for hiding this comment

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

Are we missing any potential false positives here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I can see possibility for true negative case. In case when condition has extra brackets. Like a (foo && (foo)). I did not add a test for the case because detekt has a forbidden brackets rule.

We can change the condition, and compare contents or add some preprocessing to the children remove the && and ||.

Do we need this improve ?

Copy link
Member

Choose a reason for hiding this comment

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

Do we need this improve ?

Not necessarily. The rule "looked" too easy and I wanted to collect some feedback around it. Let's wait for other maintainers' opinions but it should be good to go 👍

…detekt/rules/performance/UnnecessaryPartOfBinaryExpression.kt

Co-authored-by: Nicola Corti <corti.nico@gmail.com>
@marschwar
Copy link
Contributor

Don't get me wrong, but is this really a problem in die wild that needs its own rule?

I know this is not a binary expression but this is essentially the same thing and not detected by the rule.

fun bar() {
    val foo = true
    val baz = false
    if (foo || baz || foo) {
        //TODO    
    }
}

I know that it does not make much sense but an xor would also be flagged but removing the second part would not yield the same result.

@VovaStelmashchuk
Copy link
Contributor Author

@marschwar The rule will report an issue in the line if (foo || baz || foo) {

@marschwar
Copy link
Contributor

@marschwar The rule will report an issue in the line if (foo || baz || foo) {

I tried and it does not.

@VovaStelmashchuk
Copy link
Contributor Author

Ok, I added the test case, and fix them.

@VovaStelmashchuk
Copy link
Contributor Author

@marschwar I added your test case and several new tests and rewrite the rule.

@cortinico cortinico added this to the 1.22.0 milestone Aug 18, 2022
@BraisGabin
Copy link
Collaborator

Don't get me wrong, but is this really a problem in die wild that needs its own rule?

I have same the impression... Does this rule "borns" from a real problem in your code base? Could you explain your use case for this rule?

@VovaStelmashchuk
Copy link
Contributor Author

Don't get me wrong, but is this really a problem in die wild that needs its own rule?

I have same the impression... Does this rule "borns" from a real problem in your code base? Could you explain your use case for this rule?

Yes, I started a rule development because I detect something like if (foo || foo) into own code base.

@cortinico
Copy link
Member

@BraisGabin are you fine merging this?

@BraisGabin
Copy link
Collaborator

BraisGabin commented Sep 7, 2022

Yes, I started a rule development because I detect something like if (foo || foo) into own code base.

Thanks!

@BraisGabin are you fine merging this?

Yes, I'm :)

@BraisGabin BraisGabin merged commit c1178f2 into detekt:main Sep 7, 2022
21 checks passed
@VovaStelmashchuk
Copy link
Contributor Author

The check is red after merge the pull request. It happened because the merge request pipeline executed before new rules applied to the repository.

Do I need to create a new MR for fix the issues ?

Is the currenty pipline for pull request correct ? (Looks like the repo need one more check [is branch are up to date with main])

@BraisGabin
Copy link
Collaborator

Yes, please, could you create a PR solving that?

And about a new check... Yes, we could have it but I think that we should rethink our pipelines in general, we have far too much. They are overwhelming to me so they could be even scary for a new contributor.

@VovaStelmashchuk
Copy link
Contributor Author

will be done today

@cortinico
Copy link
Member

(Looks like the repo need one more check [is branch are up to date with main])

This can be enforced at the Github level (inside the branch protection rule). It would create a lot of friction though as it will require every PR to be rebased before merging.

I'm fine with some failures like this one from time to time as the number of PR we receive is not that high to require extra tooling (like merge queues or other checks).

@VovaStelmashchuk
Copy link
Contributor Author

@BraisGabin fixed #5280

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants