Skip to content

Commit

Permalink
Fix false positive of in UnnecessaryParentheses (#5684)
Browse files Browse the repository at this point in the history 
* Fix false positive of in UnnecessaryParentheses

* Split tests into smaller tests
  • Loading branch information
@atulgpt
atulgpt committed Feb 21, 2023
1 parent b2299a9 commit 2c150b9
Show file tree
Hide file tree
Showing 2 changed files with 228 additions and 0 deletions.
30 changes: 30 additions & 0 deletions ...s-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryParentheses.kt
View file
Expand Up @@ -15,10 +15,13 @@ import org.jetbrains.kotlin.lexer.KtTokens
import org.jetbrains.kotlin.parsing.KotlinExpressionParsing.Precedence
import org.jetbrains.kotlin.psi.KtBinaryExpression
import org.jetbrains.kotlin.psi.KtBinaryExpressionWithTypeRHS
import org.jetbrains.kotlin.psi.KtConstantExpression
import org.jetbrains.kotlin.psi.KtDelegatedSuperTypeEntry
import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtIsExpression
import org.jetbrains.kotlin.psi.KtParenthesizedExpression
import org.jetbrains.kotlin.psi.KtPrefixExpression
import org.jetbrains.kotlin.psi.KtPsiUtil

/**
Expand Down Expand Up @@ -74,6 +77,8 @@ class UnnecessaryParentheses(config: Config = Config.empty) : Rule(config) {

if (allowForUnclearPrecedence && inner.isBinaryOperationPrecedenceUnclearWithParent()) return

if (allowForUnclearPrecedence && expression.isUnaryOperationPrecedenceUnclear()) return

val message = "Parentheses in ${expression.text} are unnecessary and can be replaced with: " +
KtPsiUtil.deparenthesize(expression)?.text
report(CodeSmell(issue, Entity.from(expression), message))
Expand Down Expand Up @@ -153,5 +158,30 @@ class UnnecessaryParentheses(config: Config = Config.empty) : Rule(config) {

return childToUnclearPrecedenceParentsMapping[innerOp]?.contains(outerOp) == true
}

/**
* Determines whether this is unary operation whose precedence is unclear
*/
@Suppress("ReturnCount")
private fun KtParenthesizedExpression.isUnaryOperationPrecedenceUnclear(): Boolean {
val parentExpression = this.parent
if (parentExpression !is KtPrefixExpression) return false

if (parentExpression.operationReference.getReferencedNameElementType() in listOf(
KtTokens.PLUSPLUS,
KtTokens.MINUSMINUS,
) && (this.expression as? KtDotQualifiedExpression)?.receiverExpression is KtConstantExpression
) {
return false
}

return parentExpression.operationReference.getReferencedNameElementType() in listOf(
KtTokens.PLUS,
KtTokens.MINUS,
KtTokens.EXCL,
KtTokens.PLUSPLUS,
KtTokens.MINUSMINUS,
)
}
}
}
198 changes: 198 additions & 0 deletions ...yle/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryParenthesesSpec.kt
View file
Expand Up @@ -4,6 +4,7 @@ import io.gitlab.arturbosch.detekt.test.TestConfig
import io.gitlab.arturbosch.detekt.test.lint
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Named
import org.junit.jupiter.api.Test
import org.junit.jupiter.params.ParameterizedTest
import org.junit.jupiter.params.provider.Arguments
import org.junit.jupiter.params.provider.MethodSource
Expand Down Expand Up @@ -321,6 +322,203 @@ class UnnecessaryParenthesesSpec {
assertThat(testCase.rule.lint(code)).hasSize(if (testCase.allowForUnclearPrecedence) 0 else 4)
}

@ParameterizedTest
@MethodSource("cases")
fun `does not report unary operator with value when precedence is clear`(testCase: RuleTestCase) {
val code = """
class Foo(val value: Int) {
operator fun unaryMinus() = Foo(value * -value)
operator fun unaryPlus() = Foo(value * -value)
}
val Int.foo: Foo get() = Foo(value = this)
fun test() {
val a = -2.foo
val b = (-2).foo
val c = +2.foo
val d = (+2).foo
val e = -2.foo.value.foo
val f = (-2).foo.value.foo
}
""".trimIndent()

assertThat(testCase.rule.lint(code)).isEmpty()
}

@ParameterizedTest
@MethodSource("cases")
fun `unary operator with value when precedence is unclear`(testCase: RuleTestCase) {
val code = """
class Foo(val value: Int) {
operator fun unaryMinus() = Foo(value * -value)
operator fun unaryPlus() = Foo(value * -value)
}
val Int.foo: Foo get() = Foo(value = this)
fun test() {
val a = -(2.foo)
val b = +(2.foo)
val c = -(2.foo.value.foo)
val d = +(2.foo.value.foo)
}
""".trimIndent()

assertThat(testCase.rule.lint(code)).hasSize(if (testCase.allowForUnclearPrecedence) 0 else 4)
}

@ParameterizedTest
@MethodSource("cases")
fun `not operator with value when precedence is clear`(testCase: RuleTestCase) {
val code = """
class Bar(var value: Boolean) {
operator fun not() = false
}
val Boolean.bar get() = Bar(false)
fun test() {
val someBool = false
val a = !someBool.bar
val b = (!someBool).bar
}
""".trimIndent()

assertThat(testCase.rule.lint(code)).isEmpty()
}

@ParameterizedTest
@MethodSource("cases")
fun `not operator with value when precedence is unclear`(testCase: RuleTestCase) {
val code = """
class Bar(var value: Boolean) {
operator fun not() = false
}
val Boolean.bar get() = Bar(false)
fun test() {
val someBool = false
val a = !(someBool.bar)
}
""".trimIndent()

assertThat(testCase.rule.lint(code)).hasSize(if (testCase.allowForUnclearPrecedence) 0 else 1)
}

@ParameterizedTest
@MethodSource("cases")
fun `does report unnecessary parens in case of constant literal when using inc operator`(testCase: RuleTestCase) {
val code = """
class Foo(var value: Int) {
operator fun inc() = Foo(value + 1)
}
var Int.foo: Foo
get() = Foo(value = this)
set(value) {}
fun test() {
val violation = ++(2.foo)
}
""".trimIndent()

assertThat(testCase.rule.lint(code)).hasSize(1)
}

@Test
fun `given allowForUnclearPrecedence allowed, does report unnecessary outer parens in case of inc operator`() {
val code = """
class Foo(var value: Int) {
operator fun inc() = Foo(value + 1)
}
var Int.foo: Foo
get() = Foo(value = this)
set(value) {}
fun test() {
var a = 2.foo
val violation1 = ((++a).value)
val violation2 = (++(a.value))
val violation3 = ++((a.value))
}
""".trimIndent()

assertThat(RuleTestCase(allowForUnclearPrecedence = true).rule.lint(code)).hasSize(3)
}

@Test
fun `given allowForUnclearPrecedence not allowed, does report unnecessary outer and clarifying inner parens`() {
val code = """
class Foo(var value: Int) {
operator fun inc() = Foo(value + 1)
}
var Int.foo: Foo
get() = Foo(value = this)
set(value) {}
fun test() {
var a = 2.foo
val violation1 = ((++a).value)
val violation2 = (++(a.value))
val violation3 = ++((a.value))
}
""".trimIndent()
assertThat(RuleTestCase(allowForUnclearPrecedence = false).rule.lint(code)).hasSize(5)
}

@ParameterizedTest
@MethodSource("cases")
fun `inc operator with value when precedence is clear`(testCase: RuleTestCase) {
val code = """
class Foo(var value: Int) {
operator fun inc() = Foo(value + 1)
operator fun dec() = Foo(value - 1)
}
var Int.foo: Foo
get() = Foo(value = this)
set(value) {}
fun test() {
var a = 2.foo
val noViolations = ++a.value
val noViolations1 = (++a).value
}
""".trimIndent()

assertThat(testCase.rule.lint(code)).isEmpty()
}

@ParameterizedTest
@MethodSource("cases")
fun `inc operator with value when precedence is unclear`(testCase: RuleTestCase) {
val code = """
class Foo(var value: Int) {
operator fun inc() = Foo(value + 1)
operator fun dec() = Foo(value - 1)
}
var Int.foo: Foo
get() = Foo(value = this)
set(value) {}
fun test() {
var a = 2.foo
val b = ++(a.value)
val c = ++(a.value.foo.value)
val d = ++(a.value.foo)
}
""".trimIndent()

assertThat(testCase.rule.lint(code)).hasSize(if (testCase.allowForUnclearPrecedence) 0 else 3)
}

@ParameterizedTest
@MethodSource("cases")
fun `multiple wrapping parentheses`(testCase: RuleTestCase) {
Expand Down

0 comments on commit 2c150b9

Please sign in to comment.