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

Add UnusedUnaryOperator rule #3499

Merged
merged 3 commits into from
Mar 7, 2021

Conversation

t-kameyama
Copy link
Contributor

Fixes #312

@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #3499 (471372e) into master (c5f65a9) will decrease coverage by 0.04%.
The diff coverage is 62.50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3499      +/-   ##
============================================
- Coverage     77.63%   77.58%   -0.05%     
- Complexity     2814     2822       +8     
============================================
  Files           462      463       +1     
  Lines          8709     8732      +23     
  Branches       1688     1697       +9     
============================================
+ Hits           6761     6775      +14     
- Misses         1036     1037       +1     
- Partials        912      920       +8     
Impacted Files Coverage Δ Complexity Δ
...rturbosch/detekt/rules/bugs/UnusedUnaryOperator.kt 59.09% <59.09%> (ø) 8.00 <8.00> (?)
...turbosch/detekt/rules/bugs/PotentialBugProvider.kt 100.00% <100.00%> (ø) 3.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5f65a9...1737770. Read the comment docs.

operator fun Foo.unaryMinus() = Foo(-x)
fun test() {
val p = Foo(1) + Foo(2)
- Foo(3)
Copy link
Member

Choose a reason for hiding this comment

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

This case could have false negatives, but that's better than false postives for sure.


if (expression.baseExpression == null) return
val operationToken = expression.operationToken
if (operationToken != KtTokens.PLUS && operationToken != KtTokens.MINUS) return
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why .not() (the !) unary operator is not considered in this rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

! cannot be used as a binary operator.

…etekt/rules/bugs/UnusedUnaryOperatorSpec.kt

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

chao2zhang commented Feb 27, 2021

I talked with my colleagus about this rule and found out some interesting discussions:

With some manual testing

val d = 1
    + 1

would fail because of unary-op-spacing + 1 should be +1.

But even we change it to +1

val d = 1
    +1

still failed because of indent Unexpected indentation (8) (should be 4)

I believe this new rule is already covered by the combination of unary-op-spacing and indent, which does not require type resolution. Therefore my proposal would be not to add duplicate rules.

@t-kameyama
Copy link
Contributor Author

ktlint reports nothing in the following case:

fun test() {
    val x = 1 + 2
    +3
}

@chao2zhang
Copy link
Member

ktlint reports nothing in the following case:

fun test() {
    val x = 1 + 2
    +3
}

In this scenario, +3 is clearly a separate line instead of the continuation of the previous line.

@t-kameyama
Copy link
Contributor Author

I think this problem is easier to understand with one detekt rule than with a combination of two ktlint rules, but if you don't need this rule, close this PR.

@chao2zhang
Copy link
Member

@cortinico @BraisGabin What do you guys think? If both of you support this rule, I am fine with merging this in.

@cortinico
Copy link
Member

What do you guys think? If both of you support this rule, I am fine with merging this in.

I don't have a strong opinion honestly. I would lean towards merging this. Mostly because is disabled by default and because explicit is better than implicit, we offer a rule that covers this exact scenario rather than "suggesting" a combination of two.

@BraisGabin
Copy link
Member

I think that this is a code smell problem, not a formatting problem. So I vote to merge it.

It would be nice to have "experimental rules". Rules that we are not sure if we want to keep or not because they can have far too much false positive or they could be too subjective... But I don't think that this rule should wait for us to implement something like that.

Copy link
Member

@chao2zhang chao2zhang left a comment

Choose a reason for hiding this comment

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

Thank you for the great test coverage. I played around with the code a little bit and found that those test cases were quite exhaustive for false positives.

This was referenced Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule: Possible Semicolon Inference Issues
4 participants