Extend CanBeNonNullable rule to check function params #4431

severn-everett · 2021-12-30T13:15:26Z

This addresses issue #4377.

…can be marked as non-nullable

schalkms · 2021-12-30T19:23:14Z

...-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt

+                            val b = a!! + 2
+                        }
+                    """.trimIndent()
+                    Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)


Suggested change

Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)

assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)

detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullable.kt

...-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt

schalkms · 2021-12-30T19:43:56Z

Thanks for reviewing this! 🙂 @BraisGabin

codecov · 2022-01-01T09:46:45Z

Codecov Report

Merging #4431 (9875284) into main (ff7824d) will decrease coverage by 0.22%.
The diff coverage is 69.45%.

@@             Coverage Diff              @@
##               main    #4431      +/-   ##
============================================
- Coverage     84.32%   84.10%   -0.23%     
- Complexity     3293     3299       +6     
============================================
  Files           472      473       +1     
  Lines         10522    10716     +194     
  Branches       1885     1962      +77     
============================================
+ Hits           8873     9013     +140     
- Misses          671      684      +13     
- Partials        978     1019      +41

Impacted Files	Coverage Δ
...ekt/rules/style/SpacingBetweenPackageAndImports.kt	`75.00% <ø> (ø)`
...osch/detekt/rules/bugs/UselessPostfixExpression.kt	`71.73% <33.33%> (-0.99%)`	⬇️
.../arturbosch/detekt/rules/style/CanBeNonNullable.kt	`71.60% <70.52%> (-1.54%)`	⬇️
...urbosch/detekt/rules/performance/ArrayPrimitive.kt	`74.19% <100.00%> (-0.81%)`	⬇️
...etekt/generator/printer/defaultconfig/Exclusion.kt	`96.66% <0.00%> (-0.11%)`	⬇️
...rbosch/detekt/formatting/wrappers/TrailingComma.kt	`100.00% <0.00%> (ø)`
...ab/arturbosch/detekt/formatting/KtLintMultiRule.kt	`95.89% <0.00%> (+0.05%)`	⬆️

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 ff7824d...9875284. Read the comment docs.

BraisGabin

🎉 Thanks! The tests are really good!

severn-everett · 2022-01-01T11:34:09Z

Still got a few more to appease the CodeCov gods 😁, plus that case with override functions that was uncovered in BaseContext.

…coverage

…nd other functions

severn-everett · 2022-01-01T20:23:02Z

@BraisGabin I misread your comment about what to do when checking whether a param isn't null, so this is going to take a bit to rework; I'll let you know when it's ready to be evaluated again. On a side note, is there a way to access the PR labels? It'd be nice to just put a WIP label on this for situations like this.

BraisGabin · 2022-01-02T14:04:22Z

You can move it back to draft

…ic instead of "guilty until proven innocent" for nullable params; Fixed Detekt issue

# Conflicts: # detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt

…-null type if that check is the only expression in the function

…safe extension function; Fixed Detekt issues in SplitPattern.kt

… operator expression

…ceiver is not one of the nullable params

… tests in CanBeNonNullableSpec.kt

severn-everett · 2022-01-04T11:58:34Z

@BraisGabin This should be good for review now.

BraisGabin

Thanks for the hard work! I have a cople of comments more. Tell me what do you think.

BraisGabin · 2022-01-04T13:55:45Z

...n/kotlin/io/gitlab/arturbosch/detekt/rules/exceptions/ExceptionRaisedInUnexpectedLocation.kt

@@ -65,5 +65,5 @@ class ExceptionRaisedInUnexpectedLocation(config: Config = Config.empty) : Rule(
    private fun isPotentialMethod(function: KtNamedFunction) = methodNames.any { function.name == it }

    private fun hasThrowExpression(declaration: KtExpression?) =
-        declaration?.anyDescendantOfType<KtThrowExpression>() == true
+        declaration?.anyDescendantOfType<KtThrowExpression>() ?: false


Is this change needed? Both do the same and I don't see why one should be promoted over the other

This is probably code from an older iteration and should be reverted.

BraisGabin · 2022-01-04T14:00:15Z

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/SplitPattern.kt

-    fun startWith(value: String?): Boolean = excludes.any { value?.startsWith(it) ?: false }
+    fun startWith(value: String?): Boolean {
+        return if (value != null) excludes.any(value::startsWith) else false
+    }


I agree that this code is better (we don't need to loop the collection). But was the previous code flagged by CanBeNonNullable? I don't think it should.

I think this is code from an older iteration and can probably be reverted.

BraisGabin · 2022-01-04T14:19:02Z

...-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt

+
+                            fun foo(a: A?) = a?.foo
+                        """.trimIndent()
+                        assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)


I'm not 100% sure of this. I do agree that I would prefer a to be not null on my code but I don't know if this could be far too strict for a default rule of detekt. At the end of the day you are taking a decision about what to do return when a is null. It just turns out that it's a really "trivial" decission.

That's exactly the same as write:

fun foo(a: A?) = if (a == null) null else a.foo

And as far as I don't like it too much the writer of foo took the decision to do something with the null value.

My thought was that it'd move the null-check to outside of the function so that the function call could be skipped entirely - see the code in UselessPostfixExpression for how it'd look using the rule as it now is. However, you're right that the code in your example wouldn't be flagged, so it'd be better to revert this rule aspect and keep things as symmetrical as possible.

BraisGabin · 2022-01-04T16:14:05Z

...-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt

+                                return aObj?.foo
+                            }
+                        """.trimIndent()
+                        assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)


BraisGabin · 2022-01-05T08:35:27Z

To me this is good to go now.

severn-everett added 3 commits December 30, 2021 14:12

Expanded CanBeNonNullable rule to check for function parameters that …

47e858e

…can be marked as non-nullable

Added code comment

7abf6ae

Addressing Detekt issue

9197f1b

schalkms requested a review from BraisGabin December 30, 2021 19:13

schalkms reviewed Dec 30, 2021

View reviewed changes

BraisGabin reviewed Dec 30, 2021

View reviewed changes

detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullable.kt Show resolved Hide resolved

...-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt Show resolved Hide resolved

severn-everett added 2 commits January 1, 2022 10:41

Reworked CanBeNonNullable check for params based on PR discussion

7a50a9a

Merge branch 'master' into function_parameters_non_nullable

0992e7d

Address Detekt issues

b642aa9

BraisGabin approved these changes Jan 1, 2022

View reviewed changes

severn-everett added 4 commits January 1, 2022 12:38

Added check to ignore override function; Added tests to improve code …

e5ecb4a

…coverage

Enabled nullable params to be passed through to non-local variables a…

b0a3cda

…nd other functions

Addressing Detekt issue

82f5f85

Fixed Detekt issue

2d48819

severn-everett added 11 commits January 3, 2022 13:30

Reworked CanBeNonNullable so that it only calls on a specific heurist…

6a2bdc8

…ic instead of "guilty until proven innocent" for nullable params; Fixed Detekt issue

Merge branch 'master' into function_parameters_non_nullable

408e7ec

# Conflicts: # detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt

Fixed merge conflict

a260193

CanBeNonNullable will only flag a param that has been checked for non…

8a381b5

…-null type if that check is the only expression in the function

Added check for when a single-expression function finishes on a null-…

d8aef95

…safe extension function; Fixed Detekt issues in SplitPattern.kt

Addressing Detekt issues in SplitPattern.kt

e95d7ab

Single expression check now recursively evaluates lambdas

73790f1

Address Detekt issues

b3445d9

CanBeNonNullable will now ignore params that are involved in an elvis…

9d22662

… operator expression

CanBeNonNullable will now ignore single expression calls where the re…

c69f3c4

…ceiver is not one of the nullable params

Simplified checks for single-expression functions; Consolidated param…

f9e938e

… tests in CanBeNonNullableSpec.kt

severn-everett added 2 commits January 4, 2022 12:32

Address Detekt issues

08bc3fe

Clean up CanBeNonNullableSpec tests

9f07ecc

BraisGabin reviewed Jan 4, 2022

View reviewed changes

Addressing PR comments

9875284

BraisGabin approved these changes Jan 5, 2022

View reviewed changes

cortinico added this to the 1.20.0 milestone Jan 5, 2022

cortinico added the rules label Jan 5, 2022

cortinico changed the title ~~Enable CanBeNonNullable rule to check function params~~ Extend CanBeNonNullable rule to check function params Jan 5, 2022

cortinico approved these changes Jan 12, 2022

View reviewed changes

cortinico merged commit 27205a1 into detekt:main Jan 12, 2022

cortinico mentioned this pull request Jan 12, 2022

New rule: Function paramater should not be nullable #4377

Closed

cortinico mentioned this pull request Jan 28, 2022

CanBeNonNullable false-negative #4492

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend CanBeNonNullable rule to check function params #4431

Extend CanBeNonNullable rule to check function params #4431

severn-everett commented Dec 30, 2021

schalkms Dec 30, 2021

schalkms commented Dec 30, 2021

codecov bot commented Jan 1, 2022 •

edited

BraisGabin left a comment

severn-everett commented Jan 1, 2022

severn-everett commented Jan 1, 2022

BraisGabin commented Jan 2, 2022

severn-everett commented Jan 4, 2022

BraisGabin left a comment

BraisGabin Jan 4, 2022

severn-everett Jan 4, 2022

BraisGabin Jan 4, 2022

severn-everett Jan 4, 2022

BraisGabin Jan 4, 2022

severn-everett Jan 4, 2022

BraisGabin Jan 4, 2022

BraisGabin commented Jan 5, 2022

	Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
	assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)

Extend CanBeNonNullable rule to check function params #4431

Extend CanBeNonNullable rule to check function params #4431

Conversation

severn-everett commented Dec 30, 2021

Choose a reason for hiding this comment

schalkms commented Dec 30, 2021

codecov bot commented Jan 1, 2022 • edited

Codecov Report

BraisGabin left a comment

Choose a reason for hiding this comment

severn-everett commented Jan 1, 2022

severn-everett commented Jan 1, 2022

BraisGabin commented Jan 2, 2022

severn-everett commented Jan 4, 2022

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

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

BraisGabin commented Jan 5, 2022

codecov bot commented Jan 1, 2022 •

edited