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

UnnecessaryParentheses: add options to allow in ambiguous cases #4881

Merged
merged 1 commit into from Jul 18, 2022

Conversation

dzirbel
Copy link
Contributor

@dzirbel dzirbel commented May 31, 2022

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).

This idea has been discussed a number of times; most recently in #4495, which suggested that it could integrated into detekt (although perhaps only by porting the functionality from IntelliJ). Previous issues such as #2789 and #1929 concluded that it would be too much of an investment or overcomplicate the rule. I'm happy to stick with that precedent, but as a user who doesn't want to adopt this rule on my team until we can avoid at least some of the more common cases where parentheses can be helpful I thought I'd suggest a concrete implementation with fairly low complexity (although admittedly only addressing some of the lowest hanging fruit). I haven't thought through every edge case or application, so perhaps complexity would explode with a more fleshed-out version.

@github-actions github-actions bot added the rules label May 31, 2022
@codecov
Copy link

codecov bot commented May 31, 2022

Codecov Report

Merging #4881 (96f07db) into main (9146e4d) will decrease coverage by 0.02%.
The diff coverage is 73.80%.

Current head 96f07db differs from pull request most recent head 160716b. Consider uploading reports for the commit 160716b to get more accurate results

@@             Coverage Diff              @@
##               main    #4881      +/-   ##
============================================
- Coverage     84.83%   84.81%   -0.03%     
- Complexity     3512     3517       +5     
============================================
  Files           497      497              
  Lines         11549    11585      +36     
  Branches       2139     2153      +14     
============================================
+ Hits           9798     9826      +28     
- Misses          686      687       +1     
- Partials       1065     1072       +7     
Impacted Files Coverage Δ
...bosch/detekt/rules/style/UnnecessaryParentheses.kt 76.47% <73.80%> (+3.13%) ⬆️

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 9146e4d...160716b. Read the comment docs.


assertThat(UnnecessaryParentheses(config).lint(code)).hasSize(if (allow) 0 else 1)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What would happen with this cases:

  • val local = (1 * 2)
  • val local = (1 * 2 + 3)
  • val local = (a || b)
  • val local = (a || b && c)
  • val local = (a || b) || c

To me all of them should be flagged always.

@Configuration("whether to allow for potentially ambiguous boolean operations such as (x && y) || z")
private val allowInAmbiguousBooleanExpressions: Boolean by config(defaultValue = false)

@Configuration("whether to allow for potentially ambiguous numeric operations such as (x * y) + z")
private val allowInAmbiguousNumericExpressions: Boolean by config(defaultValue = false)
Copy link
Member

Choose a reason for hiding this comment

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

I think naming is (sadly) ambiguous.
It's not immediately clear what's the semantic of AmbiguousBooleanExpressions and AmbiguousNumericExpressions and we're forcing the user to read the example.

I would try to find a better name if possible.

@dzirbel
Copy link
Contributor Author

dzirbel commented Jun 1, 2022

Agreed on both points, this PR was meant more as a concept that something that's quite ready for production. As long as there are no fundamental objections to the idea here, I'll move it to a draft and take another stab at cleaning it up properly soon (maybe over the coming weekend). Sorry for the noise in the meantime.

@dzirbel dzirbel marked this pull request as draft June 1, 2022 18:25
@BraisGabin
Copy link
Collaborator

Sorry for the noise in the meantime.

It's not noise! Thanks for proposing this and create a first draft to give fast feedback.

This is really help us. It doesn't feel good to decline or force someone to change nearly all the code that s/he work for weeks. But we need to do those type of things to keep the project healthy and "managable". So don't worry about sending drafts of ideas we always welcome any help.

@cortinico
Copy link
Member

Sorry for the noise in the meantime.

Don't worry about that :) We're more than happy to receive contribs like yours.

@dzirbel dzirbel force-pushed the unnecessaryParensAmbiguous branch 2 times, most recently from bd071ed to 7cb6c68 Compare June 7, 2022 08:57
@dzirbel
Copy link
Contributor Author

dzirbel commented Jun 7, 2022

OK, I have a new approach heavily modified from IntelliJ's inspection https://github.com/JetBrains/intellij-community/blob/master/plugins/kotlin/idea/src/org/jetbrains/kotlin/idea/inspections/UnclearPrecedenceOfBinaryExpressionInspection.kt which is more generic and should (I believe) cleanly cover cases where parentheses are not strictly required but can be very helpful for clarifying the meaning of the code.

This might even open the door for the inverse warning matching the behavior of IntelliJ's inspection: warning when parentheses could be helpful to clarify precedence but aren't currently present. I don't feel strongly about that idea - probably wouldn't use it myself but perhaps others would.

Is there a standard way of unit testing different rule configurations that I should use here? I wanted to test all the existing cases with the new option applied to make sure it wouldn't change their behavior, but this added a fair bit of boilerplate. Perhaps @ParameterizedTest could clean those up but would make it hard for the tests that have different behavior between configurations.

@BraisGabin
Copy link
Collaborator

BraisGabin commented Jun 7, 2022

I think that to test this properly you could move that code out of the rule. And test the function itself. There you can use a parameterized test to add all the cases you want. Given x code the function should return (true/false).

Then you integrate that function inside the rule and you don't need to test all the cases there, just some to check that the integration is correct.

Edit: I like this new approach a lot. It

@dzirbel dzirbel force-pushed the unnecessaryParensAmbiguous branch from 96f07db to 5981d57 Compare June 9, 2022 03:55
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).
@dzirbel dzirbel force-pushed the unnecessaryParensAmbiguous branch from 5981d57 to 160716b Compare June 9, 2022 03:58
@dzirbel
Copy link
Contributor Author

dzirbel commented Jun 9, 2022

Good ideas - I've refactored a bit and went with a different testing strategy using @ParameterizedTest and providing a different instance of the rule while also making it easy to check whether allowForUnclearPrecedence is active for this test case. This makes most tests easy and avoids needing the extract the functionality or test the integration indirectly. Happy to change that up if you feel that another way to be preferred though.

Copy link
Collaborator

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

I like it :)

There are a lot of tests but I think that's good because there are a lot of cases and I think that you are cheking all of them.

Really good job 👍

@cortinico cortinico added this to the 1.21.0 milestone Jun 10, 2022
@BraisGabin BraisGabin modified the milestones: 1.21.0, 1.22.0 Jul 6, 2022
@BraisGabin
Copy link
Collaborator

I'm moving this to 1.22.0 because it feels like a big change and we are near to release a stable version but as soon as we release 1.21.0 this will be merged.

@BraisGabin BraisGabin merged commit a3a9240 into detekt:main Jul 18, 2022
18 checks passed
@dzirbel dzirbel deleted the unnecessaryParensAmbiguous branch July 18, 2022 16:53
@TWiStErRob
Copy link
Member

This got me, the documentation says the flag exists, but when trying to use it, it fails to execute. Is this normal @BraisGabin that the docs don't match the latest release?

@TWiStErRob
Copy link
Member

@dzirbel I would like to throw another case at you where clarifying parentheses might help:

private fun fakeClock(fixture: JFixture, minOffset: Long, maxOffset: Long): Clock? {
	val now = System.currentTimeMillis()
	val millis = fixture.buildRange((now - minOffset)..(now + maxOffset))
	val zone = fixture.create().fromList(*ZoneId.getAvailableZoneIds().toTypedArray())
	return Clock.fixed(Instant.ofEpochMilli(millis), ZoneId.of(zone))
}

Without parens fixture.buildRange(now - minOffset..now + maxOffset) one might expect it's interpreted as fixture.buildRange(now - (minOffset..now) + maxOffset)

VitalyVPinchuk pushed a commit to VitalyVPinchuk/detekt that referenced this pull request Jul 25, 2022
detekt#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).
dzirbel added a commit to dzirbel/detekt that referenced this pull request Jul 28, 2022
Follow-up to detekt#4881 to allow parentheses clarifying usage of the range operator (`..`). Since for example `IntRange` has a `plus(Int)` operator function, the expression `a..b +c` could be ambiguous (but is interpreted as `a..(b + c)`). We can allow clarifying parentheses for such cases.
@dzirbel
Copy link
Contributor Author

dzirbel commented Jul 28, 2022

This got me, the documentation says the flag exists, but when trying to use it, it fails to execute. Is this normal @BraisGabin that the docs don't match the latest release?

I believe that is the current behavior, yes - new rules, params, etc appear on the website immediately after they are merged rather than when a new version is released. I've always thought a version dropdown like Gradle and others use could be helpful.

@dzirbel I would like to throw another case at you where clarifying parentheses might help:

private fun fakeClock(fixture: JFixture, minOffset: Long, maxOffset: Long): Clock? {
	val now = System.currentTimeMillis()
	val millis = fixture.buildRange((now - minOffset)..(now + maxOffset))
	val zone = fixture.create().fromList(*ZoneId.getAvailableZoneIds().toTypedArray())
	return Clock.fixed(Instant.ofEpochMilli(millis), ZoneId.of(zone))
}

Without parens fixture.buildRange(now - minOffset..now + maxOffset) one might expect it's interpreted as fixture.buildRange(now - (minOffset..now) + maxOffset)

I agree, that's a good case. Opened #5143 to add it.

dzirbel added a commit to dzirbel/detekt that referenced this pull request Jul 28, 2022
Follow-up to detekt#4881 to allow parentheses clarifying usage of the range operator (`..`). Since for example `IntRange` has a `plus(Int)` operator function, the expression `a..b +c` could be ambiguous (but is interpreted as `a..(b + c)`). We can allow clarifying parentheses for such cases.
schalkms pushed a commit that referenced this pull request Aug 1, 2022
Follow-up to #4881 to allow parentheses clarifying usage of the range operator (`..`). Since for example `IntRange` has a `plus(Int)` operator function, the expression `a..b +c` could be ambiguous (but is interpreted as `a..(b + c)`). We can allow clarifying parentheses for such cases.
@BraisGabin
Copy link
Collaborator

This got me, the documentation says the flag exists, but when trying to use it, it fails to execute. Is this normal @BraisGabin that the docs don't match the latest release?

Yes, we need a versioned documentation. We know that docusaurus let us handle it but we didn't have the time for it: #4168. Any help there is more than appreciatted.

@cortinico
Copy link
Member

This got me, the documentation says the flag exists, but when trying to use it, it fails to execute. Is this normal @BraisGabin that the docs don't match the latest release?

Yes, we need a versioned documentation. We know that docusaurus let us handle it but we didn't have the time for it: #4168. Any help there is more than appreciatted.

I can take a stab at #4168

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants