From 83459d0c8afe3531bb545795f919e0b9217970c5 Mon Sep 17 00:00:00 2001 From: Dominic Zirbel Date: Mon, 18 Jul 2022 05:27:32 -0700 Subject: [PATCH] UnnecessaryParentheses: add option to allow when precedence is unclear (#4881) One blocker for adopting the UnnecessaryParentheses is that in some common cases parentheses that may not be strictly required by order of operations are still enormously helpful in parsing code, such as for boolean operations like `(x && y) || z`. While it's possible to `@Suppress` each of these, this adds bloat and encourages developers to avoid these (in my opinion) often very helpful indicators of code flow. Instead, we can add a configuration option to UnnecessaryParentheses to skip over some of the most common cases where unnecessary parentheses can be useful in clarifying the code. This strategy was adopted from IntelliJ's UnclearPrecedenceOfBinaryExpressionInspection inspection: https://github.com/JetBrains/intellij-community/blob/master/plugins/kotlin/idea/src/org/jetbrains/kotlin/idea/inspections/UnclearPrecedenceOfBinaryExpressionInspection.kt with a fair amount of trimming and a different set of operators that have unclear precedence (adding numeric and boolean ones). --- .../main/resources/default-detekt-config.yml | 1 + .../rules/style/UnnecessaryParentheses.kt | 102 +++++++- .../rules/style/UnnecessaryParenthesesSpec.kt | 245 ++++++++++++++---- 3 files changed, 293 insertions(+), 55 deletions(-) diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index e1bff89c1fc5..7574e161edb8 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -666,6 +666,7 @@ style: active: false UnnecessaryParentheses: active: false + allowForUnclearPrecedence: false UntilInsteadOfRangeTo: active: false UnusedImports: diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryParentheses.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryParentheses.kt index c33c408b681a..8731a1c92686 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryParentheses.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryParentheses.kt @@ -7,7 +7,17 @@ import io.gitlab.arturbosch.detekt.api.Entity import io.gitlab.arturbosch.detekt.api.Issue import io.gitlab.arturbosch.detekt.api.Rule import io.gitlab.arturbosch.detekt.api.Severity +import io.gitlab.arturbosch.detekt.api.config +import io.gitlab.arturbosch.detekt.api.internal.Configuration +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.com.intellij.psi.tree.IElementType +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.KtDelegatedSuperTypeEntry +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtIsExpression import org.jetbrains.kotlin.psi.KtParenthesizedExpression import org.jetbrains.kotlin.psi.KtPsiUtil @@ -46,17 +56,97 @@ class UnnecessaryParentheses(config: Config = Config.empty) : Rule(config) { Debt.FIVE_MINS ) + @Configuration( + "allow parentheses when not strictly required but precedence may be unclear, such as `(a && b) || c`" + ) + private val allowForUnclearPrecedence: Boolean by config(defaultValue = false) + + @Suppress("ReturnCount") override fun visitParenthesizedExpression(expression: KtParenthesizedExpression) { super.visitParenthesizedExpression(expression) - if (expression.expression == null) return + val inner = expression.expression ?: return + val outer = expression.parent + + if (outer is KtDelegatedSuperTypeEntry) return + + if (!KtPsiUtil.areParenthesesUseless(expression)) return + + if (allowForUnclearPrecedence && inner.isBinaryOperationPrecedenceUnclearWithParent()) 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)) + } + + companion object { + /** + * Map from operators to a set of other operators between which precedence can be unclear. + * + * This is built from a mapping of [Precedence] to other, greater, [Precedence](s) which should be considered + * unclear when mixed as child binary expressions. + */ + @Suppress("CommentOverPrivateProperty") + private val childToUnclearPrecedenceParentsMapping: Map> = arrayOf( + Precedence.ELVIS to arrayOf( + Precedence.EQUALITY, // (a ?: b) == c + Precedence.COMPARISON, // (a ?: b) > c + Precedence.IN_OR_IS, // (a ?: b) in c + ), + + Precedence.SIMPLE_NAME to arrayOf( + Precedence.ELVIS, // a ?: (b to c) + Precedence.SIMPLE_NAME, // (a to b) to c + ), + + // (a * b) + c + Precedence.MULTIPLICATIVE to arrayOf(Precedence.ADDITIVE), + + // (a && b) || c + Precedence.CONJUNCTION to arrayOf(Precedence.DISJUNCTION), + ) + .onEach { (child, parents) -> + parents.forEach { check(child <= it) } + } + .flatMap { (child, parents) -> + child.operations.types.map { childOp -> + childOp to parents.flatMapTo(mutableSetOf()) { parentOp -> parentOp.operations.types.toList() } + } + } + .toMap() + + /** + * Retrieves the [IElementType] of the binary operation from this element if it is a non-assignment binary + * expression, or null otherwise. + */ + private fun PsiElement.binaryOp(): IElementType? { + return when (this) { + is KtBinaryExpression -> + operationReference.takeUnless { operationToken in KtTokens.ALL_ASSIGNMENTS } + is KtBinaryExpressionWithTypeRHS -> operationReference + is KtIsExpression -> operationReference + else -> null + }?.getReferencedNameElementType() + } + + /** + * Returns either the parent of this [KtExpression] or its first parent expression which is not a + * [KtParenthesizedExpression]. + */ + private fun KtExpression.firstNonParenParent(): PsiElement? { + return generateSequence(parent) { (it as? KtParenthesizedExpression)?.parent } + .firstOrNull { it !is KtParenthesizedExpression } + } - if (expression.parent is KtDelegatedSuperTypeEntry) return + /** + * Determines whether this is a binary expression whose operation precedence is unclear with the parent binary + * operation per [childToUnclearPrecedenceParentsMapping]. + */ + private fun KtExpression.isBinaryOperationPrecedenceUnclearWithParent(): Boolean { + val innerOp = binaryOp() ?: return false + val outerOp = firstNonParenParent()?.binaryOp() ?: return false - if (KtPsiUtil.areParenthesesUseless(expression)) { - val message = "Parentheses in ${expression.text} are unnecessary and can be replaced with: " + - "${KtPsiUtil.deparenthesize(expression)?.text}" - report(CodeSmell(issue, Entity.from(expression), message)) + return childToUnclearPrecedenceParentsMapping[innerOp]?.contains(outerOp) == true } } } diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryParenthesesSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryParenthesesSpec.kt index e896d9c24621..809e94e0bc0d 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryParenthesesSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryParenthesesSpec.kt @@ -1,33 +1,39 @@ package io.gitlab.arturbosch.detekt.rules.style -import io.gitlab.arturbosch.detekt.api.Config +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.Test +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.MethodSource class UnnecessaryParenthesesSpec { - val subject = UnnecessaryParentheses(Config.empty) - - @Test - fun `with unnecessary parentheses on val assignment`() { + @ParameterizedTest + @MethodSource("cases") + fun `with unnecessary parentheses on val assignment`(testCase: RuleTestCase) { val code = "val local = (5)" - assertThat(subject.lint(code)).hasSize(1) + + assertThat(testCase.rule.lint(code)).hasSize(1) } - @Test - fun `with unnecessary parentheses on val assignment operation`() { + @ParameterizedTest + @MethodSource("cases") + fun `with unnecessary parentheses on val assignment operation`(testCase: RuleTestCase) { val code = "val local = (5 + 3)" - assertThat(subject.lint(code)).hasSize(1) + + assertThat(testCase.rule.lint(code)).hasSize(1) } - @Test - fun `with unnecessary parentheses on function call`() { + @ParameterizedTest + @MethodSource("cases") + fun `with unnecessary parentheses on function call`(testCase: RuleTestCase) { val code = "val local = 3.plus((5))" - assertThat(subject.lint(code)).hasSize(1) + + assertThat(testCase.rule.lint(code)).hasSize(1) } - @Test - fun `unnecessary parentheses in other parentheses`() { + @ParameterizedTest + @MethodSource("cases") + fun `unnecessary parentheses in other parentheses`(testCase: RuleTestCase) { val code = """ fun x(a: String, b: String) { if ((a equals b)) { @@ -35,11 +41,13 @@ class UnnecessaryParenthesesSpec { } } """ - assertThat(subject.lint(code)).hasSize(1) + + assertThat(testCase.rule.lint(code)).hasSize(1) } - @Test - fun `does not report unnecessary parentheses around lambdas`() { + @ParameterizedTest + @MethodSource("cases") + fun `does not report unnecessary parentheses around lambdas`(testCase: RuleTestCase) { val code = """ fun function (a: (input: String) -> Unit) { a.invoke("TEST") @@ -49,11 +57,13 @@ class UnnecessaryParenthesesSpec { function({ input -> println(input) }) } """ - assertThat(subject.lint(code)).isEmpty() + + assertThat(testCase.rule.lint(code)).isEmpty() } - @Test - fun `doesn't report function calls containing lambdas and other parameters`() { + @ParameterizedTest + @MethodSource("cases") + fun `doesn't report function calls containing lambdas and other parameters`(testCase: RuleTestCase) { val code = """ fun function (integer: Int, a: (input: String) -> Unit) { a.invoke("TEST") @@ -63,21 +73,25 @@ class UnnecessaryParenthesesSpec { function(1, { input -> println(input) }) } """ - assertThat(subject.lint(code)).isEmpty() + + assertThat(testCase.rule.lint(code)).isEmpty() } - @Test - fun `does not report unnecessary parentheses when assigning a lambda to a val`() { + @ParameterizedTest + @MethodSource("cases") + fun `does not report unnecessary parentheses when assigning a lambda to a val`(testCase: RuleTestCase) { val code = """ fun f() { instance.copy(value = { false }) } """ - assertThat(subject.lint(code)).isEmpty() + + assertThat(testCase.rule.lint(code)).isEmpty() } - @Test - fun `does not report well behaved parentheses`() { + @ParameterizedTest + @MethodSource("cases") + fun `does not report well behaved parentheses`(testCase: RuleTestCase) { val code = """ fun x(a: String, b: String) { if (a equals b) { @@ -85,11 +99,13 @@ class UnnecessaryParenthesesSpec { } } """ - assertThat(subject.lint(code)).isEmpty() + + assertThat(testCase.rule.lint(code)).isEmpty() } - @Test - fun `does not report well behaved parentheses in super constructors`() { + @ParameterizedTest + @MethodSource("cases") + fun `does not report well behaved parentheses in super constructors`(testCase: RuleTestCase) { val code = """ class TestSpek : SubjectSpek({ describe("a simple test") { @@ -98,11 +114,13 @@ class UnnecessaryParenthesesSpec { } }) """ - assertThat(subject.lint(code)).isEmpty() + + assertThat(testCase.rule.lint(code)).isEmpty() } - @Test - fun `does not report well behaved parentheses in constructors`() { + @ParameterizedTest + @MethodSource("cases") + fun `does not report well behaved parentheses in constructors`(testCase: RuleTestCase) { val code = """ class TestSpek({ describe("a simple test") { @@ -111,11 +129,13 @@ class UnnecessaryParenthesesSpec { } }) """ - assertThat(subject.lint(code)).isEmpty() + + assertThat(testCase.rule.lint(code)).isEmpty() } - @Test - fun `should not report lambdas within super constructor calls`() { + @ParameterizedTest + @MethodSource("cases") + fun `should not report lambdas within super constructor calls`(testCase: RuleTestCase) { val code = """ class Clazz( private val func: (X, Y) -> Z @@ -123,11 +143,13 @@ class UnnecessaryParenthesesSpec { constructor() : this({ first, second -> true }) } """ - assertThat(subject.lint(code)).isEmpty() + + assertThat(testCase.rule.lint(code)).isEmpty() } - @Test - fun `should not report call to function with two lambda parameters with one as block body`() { + @ParameterizedTest + @MethodSource("cases") + fun `should not report call to function with two lambda parameters with one as block body`(testCase: RuleTestCase) { val code = """ class Clazz { fun test(first: (Int) -> Unit, second: (Int) -> Unit) { @@ -140,11 +162,13 @@ class UnnecessaryParenthesesSpec { } } """ - assertThat(subject.lint(code)).isEmpty() + + assertThat(testCase.rule.lint(code)).isEmpty() } - @Test - fun `should not report call to function with two lambda parameters`() { + @ParameterizedTest + @MethodSource("cases") + fun `should not report call to function with two lambda parameters`(testCase: RuleTestCase) { val code = """ class Clazz { fun test(first: (Int) -> Unit, second: (Int) -> Unit) { @@ -157,11 +181,15 @@ class UnnecessaryParenthesesSpec { } } """ - assertThat(subject.lint(code)).isEmpty() + + assertThat(testCase.rule.lint(code)).isEmpty() } - @Test - fun `should not report call to function with multiple lambdas as parameters but also other parameters`() { + @ParameterizedTest + @MethodSource("cases") + fun `should not report call to function with multiple lambdas as parameters but also other parameters`( + testCase: RuleTestCase, + ) { val code = """ class Clazz { fun test(text: String, first: () -> Unit, second: () -> Unit) { @@ -174,14 +202,133 @@ class UnnecessaryParenthesesSpec { } } """ - assertThat(subject.lint(code)).isEmpty() + + assertThat(testCase.rule.lint(code)).isEmpty() } - @Test - fun `should not report interface delegation with parenthesis - #3851`() { + @ParameterizedTest + @MethodSource("cases") + fun `should not report interface delegation with parenthesis - #3851`(testCase: RuleTestCase) { val code = """ class Clazz: Comparable by ("hello".filter { it != 'l' }) """ - assertThat(subject.lint(code)).isEmpty() + + assertThat(testCase.rule.lint(code)).isEmpty() + } + + @ParameterizedTest + @MethodSource("cases") + fun `numeric expressions when precedence is unclear`(testCase: RuleTestCase) { + val code = """ + val a1 = (1 * 2) + 3 + val a2 = (1 / 2) + 3 + val a3 = (1 % 2) + 3 + + val b1 = 3 + (1 * 2) + val b2 = 3 + (1 / 2) + val b3 = 3 + (1 % 2) + + val c = (4 + 5) * 3 // parens required + """ + + assertThat(testCase.rule.lint(code)).hasSize(if (testCase.allowForUnclearPrecedence) 0 else 6) + } + + @ParameterizedTest + @MethodSource("cases") + fun `numeric expressions when precedence is clear`(testCase: RuleTestCase) { + val code = """ + val a1 = (1 + 2) + val a2 = (1 * 2) + val a3 = (1 + 2 * 3) + val b1 = (1 + 2) + 3 + val b2 = (1 * 2) * 3 + """ + + assertThat(testCase.rule.lint(code)).hasSize(5) + } + + @ParameterizedTest + @MethodSource("cases") + fun `boolean expressions when precedence is unclear`(testCase: RuleTestCase) { + val code = """ + val a1 = (true && false) || false + val a2 = (true && false) || (false && true) // 2 warnings when disallowed + val b = false || (true && false) + val c = (true || false) && false // parens required + """ + + assertThat(testCase.rule.lint(code)).hasSize(if (testCase.allowForUnclearPrecedence) 0 else 4) + } + + @ParameterizedTest + @MethodSource("cases") + fun `boolean expressions when precedence is clear`(testCase: RuleTestCase) { + val code = """ + val a1 = (true && false) + val a2 = (true || false) + val a3 = (true && false || false) + val b1 = (true && false) && false + val b2 = (true || false) || false + """ + + assertThat(testCase.rule.lint(code)).hasSize(5) + } + + @ParameterizedTest + @MethodSource("cases") + fun `infix operators when precedence is unclear`(testCase: RuleTestCase) { + val code = """ + val d = (true and false) or false + val e = false or (true and false) // parens required + """ + + assertThat(testCase.rule.lint(code)).hasSize(if (testCase.allowForUnclearPrecedence) 0 else 1) + } + + @ParameterizedTest + @MethodSource("cases") + fun `elvis operators when precedence is unclear`(testCase: RuleTestCase) { + val code = """ + val a1 = null ?: (1 to 2) // parens required + val a2 = (null ?: 1) to 2 + + val b1 = null ?: (1 == 2) // parens required + val b2 = (null ?: 1) == 2 + + val c1 = null ?: (1 > 2) // parens required + val c2 = (null ?: 1) > 2 + + val d1 = null ?: (1 in 2) // parens required + val d2 = (null ?: 1) in 2 + """ + + assertThat(testCase.rule.lint(code)).hasSize(if (testCase.allowForUnclearPrecedence) 0 else 4) + } + + @ParameterizedTest + @MethodSource("cases") + fun `multiple wrapping parentheses`(testCase: RuleTestCase) { + val code = """ + val a = ((false || (((true && false))))) + """ + + assertThat(testCase.rule.lint(code)).hasSize(if (testCase.allowForUnclearPrecedence) 4 else 5) + } + + companion object { + class RuleTestCase(val allowForUnclearPrecedence: Boolean) { + val rule = UnnecessaryParentheses( + TestConfig(mapOf("allowForUnclearPrecedence" to allowForUnclearPrecedence)) + ) + } + + @JvmStatic + fun cases(): List { + return listOf( + RuleTestCase(allowForUnclearPrecedence = false), + RuleTestCase(allowForUnclearPrecedence = true), + ) + } } }