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

Add NullableBooleanCheck rule #4872

Merged
merged 1 commit into from May 30, 2022
Merged

Conversation

dzirbel
Copy link
Contributor

@dzirbel dzirbel commented May 28, 2022

Add a new rule which enforces the recommendation of the Kotlin coding conventions to prefer == over ?: when converting a Boolean? into a Boolean: https://kotlinlang.org/docs/coding-conventions.html#nullable-boolean-values-in-conditions. This is simple to implement and enforces consistency for the two essentially equivalent methods of coalescing nullable boolean values.

The coding conventions only specify this preference for usages "in a conditional statement" but the same rationale applies to any statement which converts a Boolean? to a Boolean, so I have implemented it for any situation. An alternative is to add a configuration setting to toggle application only in if statements.

Unfortunately, this rule requires type resolution since there are technically valid usages of e.g. value ?: true for cases where value is not a Boolean? but an Any?. Running without type resolution will not be able to distinguish between these valid usages and ones which could be converted to == false.

@codecov
Copy link

codecov bot commented May 28, 2022

Codecov Report

Merging #4872 (ce0d308) into main (c54453d) will increase coverage by 0.00%.
The diff coverage is 84.00%.

@@            Coverage Diff            @@
##               main    #4872   +/-   ##
=========================================
  Coverage     84.79%   84.80%           
- Complexity     3447     3463   +16     
=========================================
  Files           492      493    +1     
  Lines         11315    11353   +38     
  Branches       2083     2089    +6     
=========================================
+ Hits           9595     9628   +33     
- Misses          673      674    +1     
- Partials       1047     1051    +4     
Impacted Files Coverage Δ
...urbosch/detekt/rules/style/NullableBooleanCheck.kt 83.33% <83.33%> (ø)
...rturbosch/detekt/rules/style/StyleGuideProvider.kt 100.00% <100.00%> (ø)
...rbosch/detekt/rules/style/UnnecessaryInnerClass.kt 91.22% <0.00%> (-0.08%) ⬇️
...n/kotlin/io/github/detekt/parser/DetektPomModel.kt 80.00% <0.00%> (+1.73%) ⬆️

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 c54453d...ce0d308. Read the comment docs.

@schalkms
Copy link
Member

schalkms commented May 28, 2022

The code snippets used in tests are compiled with Jsr223 in this repo to ensure that those code snippets are valid Kotlin code. Some of the code snippets in NullableBooleanCheckSpec seem to be invalid according to the CI run for this PR. Please make sure to correct the code.
Otherwise, the code looks very good and is ready to merge!

Thank you for this nice addition resulting from an official Kotlin convention!

@dzirbel
Copy link
Contributor Author

dzirbel commented May 28, 2022

The code snippets used in tests are compiled with Jsr223 in this repo to ensure that those code snippets are valid Kotlin code. Some of the code snippets in NullableBooleanCheckSpec seem to be invalid according to the CI run for this PR. Please make sure to correct the code. Otherwise, the code looks very good and is ready to merge!

Should be all set now, thanks! compile-snippet-tests took me by surprise, but it's a great setting since I've previously written tests whose code did not compile and so they had unexpected behavior. I'm curious why it's not enabled by default? I'd find it useful on most/all tests when running locally, and if some CI jobs are disabling it for performance that could still be done via -Pcompile-test-snippets=false. The closer running gradle build is to ensuring CI checks pass the easier contribution is, in my experience :)

@schalkms
Copy link
Member

schalkms commented May 29, 2022

That’s because of the increased time for test runs. If you run all the tests locally with the compile-snippet-tests setting enabled, it takes quite some time. Hence, we disabled it by default.

I'm curious why it's not enabled by default?

@BraisGabin
Copy link
Collaborator

BraisGabin commented May 29, 2022

I think that keep this disabled by default on local is good. But it's true that a lot of new contributors are "bitten" byt this issue. I'm going to open a new issue to look for a way to fix it.

@BraisGabin
Copy link
Collaborator

BraisGabin commented May 29, 2022

We can continue this discussion here: #4874. Any feedback is more than welcome.

Add a new rule which enforces the recommendation of the Kotlin coding conventions to prefer `==` over `?:` when converting a `Boolean?` into a `Boolean`: https://kotlinlang.org/docs/coding-conventions.html#nullable-boolean-values-in-conditions. This is simple to implement and enforces consistency for the two essentially equivalent methods of coalescing nullable boolean values.

The coding conventions only specify this preference for usages "in a conditional statement" but the same rationale applies to any statement which converts a Boolean? to a Boolean, so I have implemented it for any situation. An alternative is to add a configuration setting to toggle application only in if statements.

Unfortunately, this rule requires type resolution since there are technically valid usages of e.g. `value ?: true` for cases where `value` is not a `Boolean?` but an `Any?`. Running without type resolution will not be able to distinguish between these valid usages and ones which could be converted to `== false`.
@BraisGabin
Copy link
Collaborator

BraisGabin commented May 30, 2022

Thank you for this rule. It's really nice :)

@BraisGabin BraisGabin merged commit 233b4a6 into detekt:main May 30, 2022
20 checks passed
@dzirbel dzirbel deleted the nullableBooleanCheck branch May 30, 2022
@cortinico cortinico added the notable changes Marker for notable changes in the changelog label May 31, 2022
@cortinico cortinico added this to the 1.21.0 milestone May 31, 2022
@snyman
Copy link

snyman commented Jul 20, 2022

I'm not sure if this is the best place to ask, since the change is merged, but I'm wondering if this rule oversteps the intention of the kotlin style guide. The kotlin style guideline linked in the change says

If you need to use a nullable Boolean in a conditional statement, use if (value == true) or if (value == false) checks.

(Emphasis mine.) To me this is fairly clearly talking only about conditional statements such as if or when, and not about all boolean expressions in general. In particular, when computing a non-null boolean expression that isn't being immediately used inside a conditional, I think the elvis operator (?:) is clearer than ==. For example:

fun nonNullBoolean() = nullableBoolean() ?: false

vs

fun nonNullBoolean() = nullableBoolean() == true

In the former I can immediately see what the returned value is in the case that nullableBoolean() returns null. In the latter, I need to do a bit of extra thinking to figure it out.

@dzirbel
Copy link
Contributor Author

dzirbel commented Jul 20, 2022

These are very good points; the reason why we adopted this style on my team is purely as an anti-bikeshedding measure. I would actually also prefer ?: true and ?: false as being more intuitive, but the Kotlin style guide has settled in the other direction for whatever reason. Since it's just a matter of standardization as I see it, I personally like having it enforced everywhere that a nullable boolean is being converted to non-null; I don't see any particular reason why == true is more intuitive in a conditional statement than in any other line of code.

@BraisGabin
Copy link
Collaborator

BraisGabin commented Jul 23, 2022

I agree with both. It's true that the Kotlin style guideline only talks about conditional statments. But I agree with @dzirbel here that use it everywhere makes the code more consistent.

Anyway, if someone is willing to add a flag that, when enabled, only checks inside conditional statments I would be happy to merge it.

@artem-zinnatullin
Copy link
Contributor

artem-zinnatullin commented Sep 4, 2022

Nice check, found few violations in our project and == is def makes it more clear than ?:

Thanks for your work @dzirbel!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable changes Marker for notable changes in the changelog rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants