[NullCheckOnMutableProperty] Created rule for when a null-check is performed on a mutable property #4353

severn-everett · 2021-12-02T14:51:16Z

This is for issue #3353. Any advice on how to improve this one is welcome, as this was a bit of a stabbing-in-the-dark solution for me.

codecov · 2021-12-02T15:06:28Z

Codecov Report

Merging #4353 (85edb20) into main (92433ea) will increase coverage by 0.08%.
The diff coverage is 85.07%.

@@             Coverage Diff              @@
##               main    #4353      +/-   ##
============================================
+ Coverage     84.26%   84.35%   +0.08%     
- Complexity     3263     3273      +10     
============================================
  Files           473      474       +1     
  Lines         10336    10405      +69     
  Branches       1827     1845      +18     
============================================
+ Hits           8710     8777      +67     
- Misses          667      668       +1     
- Partials        959      960       +1

Impacted Files	Coverage Δ
...tlab/arturbosch/detekt/rules/KtBinaryExpression.kt	`0.00% <0.00%> (ø)`
...ch/detekt/rules/bugs/NullCheckOnMutableProperty.kt	`87.50% <87.50%> (ø)`
...turbosch/detekt/rules/bugs/PotentialBugProvider.kt	`100.00% <100.00%> (ø)`
...etekt/rules/style/optional/MandatoryBracesLoops.kt	`73.33% <0.00%> (-2.53%)`	⬇️
...ab/arturbosch/detekt/core/config/ValidateConfig.kt	`98.59% <0.00%> (-1.41%)`	⬇️
...n/kotlin/io/gitlab/arturbosch/detekt/api/Entity.kt	`75.00% <0.00%> (-1.20%)`	⬇️
.../detekt/rules/exceptions/ObjectExtendsThrowable.kt	`80.95% <0.00%> (ø)`
...t/generator/collection/RuleSetProviderCollector.kt	`75.94% <0.00%> (+1.26%)`	⬆️
...tlab/arturbosch/detekt/rules/style/UseDataClass.kt	`80.28% <0.00%> (+1.40%)`	⬆️
...osch/detekt/rules/exceptions/SwallowedException.kt	`77.04% <0.00%> (+1.63%)`	⬆️
... and 4 more

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 92433ea...85edb20. Read the comment docs.

BraisGabin

Could you add these tests too:

class A(private var a: Int?) {
    fun foo() {
        if (a != null) {
            println(2 + 2) // this null check should be fine
        } 
    }
}

class A(private val a: Int?) {
    class B(private var a: Int?) {
        fun foo() {
            if (a != null) {
                println(2 + a!!) // should report
            } 
        }
    }

    fun foo() {
        if (a != null) {
            println(2 + a!!) // shouldn't report
        } 
    }
}

class A(private var a: Int?) {
    inner class B {
        fun foo() {
            if (a != null) {
                println(2 + a!!) // should report
            } 
        }
    }
}

I think that you are handling correctly the last two, but just to have tests that ensure it.

BraisGabin · 2021-12-09T12:48:04Z

...orprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt

+ * </compliant>
+ *
+ * <compliant>


I think that we only support one "compliant" block, can you merge both?

Suggested change

* </compliant>

*

* <compliant>

*

The code wouldn't pass the first test case proposed here, as doing so would mean the rule would have to detect both a null-check for the mutable property and a double-bang operation on the variable. This, essentially, would turn the rule into a sub-rule for UnsafeCallOnNullableType: anything that NullCheckOnMutableProperty would pick up would be picked up by that rule as well. It's not a perfectly symmetrical relationship, but I'm not sure that the use case wherein a developer wants to be warned about double-bang operations only if they're being conducted after a null-check is that prevalent.

BraisGabin · 2021-12-09T16:53:37Z

The code wouldn't pass the first test case proposed here, as doing so would mean the rule would have to detect both a null-check for the mutable property and a double-bang operation on the variable. This, essentially, would turn the rule into a sub-rule for UnsafeCallOnNullableType: anything that NullCheckOnMutableProperty would pick up would be picked up by that rule as well. It's not a perfectly symmetrical relationship, but I'm not sure that the use case wherein a developer wants to be warned about double-bang operations only if they're being conducted after a null-check is that prevalent.

I'm going to give more examples because I think that it will be more clear:

class A(private var a: Int?) {
    fun foo() {
        if (a != null) {
            println(a) // report! This could print an unexpected `null`
        }
    }
}

class A(private var a: Int?) {
    fun foo() {
        if (a != null) {
            println(2 + requireNotNull(a)) // report! This could crash
        } 
    }
}

class A(private var a: Int?) {
    fun foo() {
        if (a != null) {
            println("a is not null") // this code is completely fine
        }
    }
}

This code is not related with the usage of !!. We just need to check if the property is used or not inside the if. If it's used, we should flag the check. If it is not used, we shouldn't flag it.

Other missing test:

class A(private var a: Int?) {
    fun foo() {
        if (a != null && true) {
            println("a is not null") // this code is completely fine
        }
    }
}

severn-everett · 2021-12-09T17:18:50Z

Gotcha, I'll see what I can do

…enced within the if-expression block; Moved all relevant inspection code to within a private class to ensure resource cleanup after visiting a Kotlin file

…llCheckOnMutablePropertySpec

…perties

BraisGabin

🎉🎉🎉 Great job! I really like this rule. This will flag a lot of difficult to spot issues. 👏👏👏

Do you think this is ready to be merged?

BraisGabin · 2021-12-10T19:19:40Z

...one/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutablePropertySpec.kt

+                        } 
+                    }
+                }
+                """


It would be nice to have another rule to check something like this. But it would be difficult and with lots of false positives.

BraisGabin · 2021-12-10T19:21:46Z

...one/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutablePropertySpec.kt

+        it("should not report a null-check on a non-mutable class property") {
+            val code = """
+                class A {
+                    private val a: Int? = 5


private val a: Int? get() = 5

Maybe this could be flagged too.

severn-everett · 2021-12-10T19:45:53Z

The code should be good to go now. The other two points could be made into other issues/tasks to be tackled later.

BraisGabin · 2021-12-12T16:27:45Z

Ok, I'll wait a bit to get the review form other maintainer.

I think that this case should be handled:

private val a: Int?
  get() = 5

But I agree that it could be handled in other PR. Can you do it? Or should we open an issue so we don't forget about it?

severn-everett · 2021-12-12T16:46:02Z

I'd say that it's a different issue, as it wouldn't be "error-prone": the value is going to be non-null, no matter how many times it's checked. Maybe this would be a style issue, e.g. some rule like UnnecessaryNullable?

BraisGabin · 2021-12-15T20:28:20Z

Sorry, I wasn't clear with my example. It was far too simplified. I mean something like this:

private val a: Int?
  get() = foo()

a is val but its value could change between calls so it is a mutable property.

…lly-nullable values

…ossesses a getter

BraisGabin · 2021-12-16T19:32:14Z

Why did you closed and removed it?

severn-everett · 2021-12-16T19:37:25Z

My apologies, I thought it was merged - thanks for the heads-up!

cortinico

Great work :)

cortinico · 2021-12-17T15:10:02Z

...orprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt

+ *
+ * class A(private var a: Int?) {
+ *   fun foo() {
+ *     val a = a


That's not the best snippet to put in the compliant block. Is doing name shadowing and should not be suggested.

cortinico · 2021-12-17T15:10:11Z

...orprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt

+ * }
+ * </compliant>
+ */
+


Extra white line?

cortinico · 2021-12-17T15:10:39Z

...orprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt

+    )
+
+    override fun visitKtFile(file: KtFile) {
+        if (bindingContext == BindingContext.EMPTY) return


You're missing a super.visitKtFile(file) here

cortinico · 2021-12-17T15:13:34Z

...orprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt

+                    candidateProperties.getOrPut(candidateFqName) { ArrayDeque() }.apply { add(expression) }
+                }
+            }
+            super.visitIfExpression(expression)


Commenting here but valid for the whole NullCheckVisitor:

Is there a reason why you're calling super. methods at arbitrary locations inside the function blocks? Ideally you should always call the super method as first statement and then add your logic.

In this line you're calling super.visitIfExpression(expression) in the middle of the block.

The line needs to be in the middle of the block in order to analyze whether the descendant expressions use any candidate properties identified in the if-expression, with the visitIfExpression() removing any if-statements added to candidateProperties afterwards. I've added a comment that explains as much in the code.

cortinico · 2021-12-17T15:14:16Z

...orprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt

+                                CodeSmell(
+                                    issue,
+                                    Entity.from(ifExpression),
+                                    "Null-check is being called on a mutable property."


nit: if you could customize the message with the name of the property would be 👌

cortinico · 2021-12-31T13:57:19Z

Are we good to merge this?

Created rule for when a null-check is performed on a mutable property

556166d

severn-everett added 3 commits December 2, 2021 16:13

Fixed Detekt issue

c83d3da

Added tests to improve code coverage

1beced5

Fixed Detekt issues

d3995e5

BraisGabin reviewed Dec 9, 2021

View reviewed changes

severn-everett added 5 commits December 9, 2021 20:38

Rule will now only fire when the candidate variable is actually refer…

db26bac

…enced within the if-expression block; Moved all relevant inspection code to within a private class to ensure resource cleanup after visiting a Kotlin file

Consolidated null-checking on candidateFqName

c6e6137

More consolidation within visitIfExpression; Fixed Detekt issue in Nu…

1e7b0cf

…llCheckOnMutablePropertySpec

Switch to interface type specification in declaration of candidatePro…

60c747d

…perties

Merge branch 'master' into create_null_check_on_mutable_property

7373b14

severn-everett mentioned this pull request Dec 10, 2021

UnnecessaryAbstractClass should report empty abstract classes #4357

Closed

severn-everett added 2 commits December 10, 2021 16:56

Added checks for multi-clause if-expressions

dee8d9c

Fixed Detekt issue

0b1ba46

BraisGabin approved these changes Dec 10, 2021

View reviewed changes

severn-everett added 4 commits December 16, 2021 14:06

Added check for val properties that have getters that return potentia…

614657b

…lly-nullable values

Address Detekt issue

4713eb9

Fix failures in test compilation

2b80d7a

Simplified check on getter - will now only report if a val property p…

625afc1

…ossesses a getter

BraisGabin approved these changes Dec 16, 2021

View reviewed changes

BraisGabin mentioned this pull request Dec 16, 2021

New rule: Nullable val can be non-nullable #4376

Closed

severn-everett closed this Dec 16, 2021

severn-everett deleted the create_null_check_on_mutable_property branch December 16, 2021 19:23

severn-everett restored the create_null_check_on_mutable_property branch December 16, 2021 19:36

severn-everett reopened this Dec 16, 2021

cortinico approved these changes Dec 17, 2021

View reviewed changes

severn-everett added 2 commits December 18, 2021 13:54

Addressing PR comments

609faa5

Fixed typo

85edb20

cortinico added this to the 1.20.0 milestone Dec 31, 2021

cortinico added the rules label Dec 31, 2021

cortinico changed the title ~~Created rule for when a null-check is performed on a mutable property~~ [NullCheckOnMutableProperty] Created rule for when a null-check is performed on a mutable property Dec 31, 2021

BraisGabin merged commit f6440e3 into detekt:main Dec 31, 2021

cortinico added the notable changes Marker for notable changes in the changelog label Jan 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NullCheckOnMutableProperty] Created rule for when a null-check is performed on a mutable property #4353

[NullCheckOnMutableProperty] Created rule for when a null-check is performed on a mutable property #4353

severn-everett commented Dec 2, 2021

codecov bot commented Dec 2, 2021 •

edited

Loading

BraisGabin left a comment

BraisGabin Dec 9, 2021

severn-everett Dec 9, 2021

BraisGabin commented Dec 9, 2021

severn-everett commented Dec 9, 2021

BraisGabin left a comment

BraisGabin Dec 10, 2021

BraisGabin Dec 10, 2021

severn-everett commented Dec 10, 2021

BraisGabin commented Dec 12, 2021

severn-everett commented Dec 12, 2021

BraisGabin commented Dec 15, 2021 •

edited

Loading

BraisGabin commented Dec 16, 2021

severn-everett commented Dec 16, 2021

cortinico left a comment

cortinico Dec 17, 2021

cortinico Dec 17, 2021

cortinico Dec 17, 2021

cortinico Dec 17, 2021

severn-everett Dec 18, 2021

cortinico Dec 17, 2021

cortinico commented Dec 31, 2021

[NullCheckOnMutableProperty] Created rule for when a null-check is performed on a mutable property #4353

[NullCheckOnMutableProperty] Created rule for when a null-check is performed on a mutable property #4353

Conversation

severn-everett commented Dec 2, 2021

codecov bot commented Dec 2, 2021 • edited Loading

Codecov Report

BraisGabin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BraisGabin commented Dec 9, 2021

severn-everett commented Dec 9, 2021

BraisGabin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

severn-everett commented Dec 10, 2021

BraisGabin commented Dec 12, 2021

severn-everett commented Dec 12, 2021

BraisGabin commented Dec 15, 2021 • edited Loading

BraisGabin commented Dec 16, 2021

severn-everett commented Dec 16, 2021

cortinico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cortinico commented Dec 31, 2021

codecov bot commented Dec 2, 2021 •

edited

Loading

BraisGabin commented Dec 15, 2021 •

edited

Loading