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 of in UnnecessaryParentheses #5684

Merged
merged 2 commits into from Feb 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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,
)
}
}
}
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 @@ -322,6 +323,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