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 false positive (+= overload) in UnusedPrivateMember #3094

Merged
merged 6 commits into from
Sep 23, 2020

Conversation

fourlastor
Copy link
Contributor

Hi! I noticed that detekt emits a false positive when checking for unused members if the function is an operator function, and if it's used via an assignment operation (+= in the test case).

I know that the issue likely lies in io.gitlab.arturbosch.detekt.rules.style.UnusedFunctionVisitor#getUnusedReports:

val operatorToken = OperatorConventions.getOperationSymbolForName(Name.identifier(name))

This will return the token + as the name of the function is plus, so += will get skipped and appear as unused.

I'm not too familiar with detekt internals so I'm not sure how to solve it, I'm open to ideas/suggestions though/happy to let someone else take over the PR.

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Thanks for reporting this scenario to detekt and contributing a test case. 👍
You caught the right line in the source code that prevents this code from not being reported.

The following section in the Kotlin doc describes how this is handled internally by the compiler.
https://kotlinlang.org/docs/reference/operator-overloading.html#assignments

So one has to be careful here. I think there is a simple (stupid) solution here. If there is an operator fun plus available, also add plusAssign to the corresponding reference list. This should be done for += -= *= /= %=.
Overridden plus and plusAssign operator function pairs can't coexist as the following statement in the doc mentions.

If the corresponding binary function (i.e. plus() for plusAssign()) is available too, report error (ambiguity)

@t-kameyama do you have better ideas on how to solve this? Do you know about an existing Kotlin function that already solves this issue?

@fourlastor
Copy link
Contributor Author

@schalkms thanks! that was really helpful

KtTokens.PLUS, KtTokens.MINUS, KtTokens.MUL, KtTokens.DIV, KtTokens.PERC -> operatorValue?.let {
functionReferences[it].orEmpty() + functionReferences["$it="].orEmpty()
}.orEmpty()
else -> operatorValue?.let { functionReferences[it] }.orEmpty()
Copy link
Member

Choose a reason for hiding this comment

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

The code in the whole function could probably be simplified. However, I'd see this out of scope for this PR. Consequently, this refactoring step should go into a new PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

27678ef I did some cleanup

Comment on lines 1059 to 1065
import java.util.Date
class Foo {
var number: Float? = null
fun foo() {
number += 3f
}
private operator fun Float?.plus(other: Float?) = if (this == null && other == null) null else this ?: 0f + (other ?: 0f)
Copy link
Member

Choose a reason for hiding this comment

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

Detekt strives to keep the test code as simple as possible in order to make it more readable and maintainable.
The test snippets should be as simple as possible and test only the rule under test.

Suggested change
import java.util.Date
class Foo {
var number: Float? = null
fun foo() {
number += 3f
}
private operator fun Float?.plus(other: Float?) = if (this == null && other == null) null else this ?: 0f + (other ?: 0f)
class Test {
fun f() {
var number: Int = 0
number += 1
number -= 1
number *= 1
number /= 1
number %= 1
}
private operator fun Int.plus(other: Int) = 1
private operator fun Int.minus(other: Int) = 2
private operator fun Int.times(other: Int) = 3
private operator fun Int.div(other: Int) = 4
private operator fun Int.rem(other: Int) = 5
private operator fun Int.mod(other: Int) = 6
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

b974091 that's a good point. I had to keep the nullable type, as kotlin would otherwise give precedence to the operator function defined in Int. I also didn't add mod as it would be effectively unused (rem would be used instead). I preferred rem over mod as the latter is deprecated.

@schalkms schalkms changed the title Add test case for += Fix false positive (+= overload) in UnusedMember Sep 22, 2020
@schalkms schalkms changed the title Fix false positive (+= overload) in UnusedMember Fix false positive (+= overload) in UnusedPrivateMember Sep 22, 2020
Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for providing the fix in an additional commit! 👍
Please see my suggestion on how to improve the test code a little bit.

@@ -95,7 +96,12 @@ private class UnusedFunctionVisitor(
val referencesViaOperator = if (isOperator) {
val operatorToken = OperatorConventions.getOperationSymbolForName(Name.identifier(name))
val operatorValue = (operatorToken as? KtSingleValueToken)?.value
operatorValue?.let { functionReferences[it] }.orEmpty()
when (operatorToken) {
KtTokens.PLUS, KtTokens.MINUS, KtTokens.MUL, KtTokens.DIV, KtTokens.PERC -> operatorValue?.let {
Copy link
Member

@schalkms schalkms Sep 22, 2020

Choose a reason for hiding this comment

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

Detekt has a dog-fooding process in action, which basically means that we run detekt's rules over it's own codebase.
It detected line 100 as too long (MaxLineLength). Can we please wrap the line in a subsequent commit in order to merge this awesome PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

57e9911 I should have looked at the CI output 😅

@fourlastor
Copy link
Contributor Author

Thank you for the suggestion! I'll address these tomorrow

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Thanks for incorporating the feedback and comments. Well done 👍

@schalkms schalkms merged commit eb666c9 into detekt:master Sep 23, 2020
@fourlastor fourlastor deleted the operator-plus-equals branch September 24, 2020 09:42
@arturbosch arturbosch added this to the 1.14.0 milestone Sep 25, 2020
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.

3 participants