Added rule CanBeNonNullable by severn-everett · Pull Request #4379 · detekt/detekt

severn-everett · 2021-12-17T10:47:49Z

This addresses issue #4376

codecov · 2021-12-17T10:56:13Z

Codecov Report

Merging #4379 (9202e76) into main (7d8acae) will decrease coverage by 0.04%.
The diff coverage is 73.52%.

@@             Coverage Diff              @@
##               main    #4379      +/-   ##
============================================
- Coverage     84.30%   84.26%   -0.05%     
+ Complexity     3273     3272       -1     
============================================
  Files           473      474       +1     
  Lines         10340    10408      +68     
  Branches       1830     1857      +27     
============================================
+ Hits           8717     8770      +53     
- Misses          667      670       +3     
- Partials        956      968      +12

Impacted Files	Coverage Δ
.../arturbosch/detekt/rules/style/CanBeNonNullable.kt	`73.13% <73.13%> (ø)`
...rturbosch/detekt/rules/style/StyleGuideProvider.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%)`	⬆️
...b/arturbosch/detekt/core/reporting/OutputFacade.kt	`90.90% <0.00%> (+4.54%)`	⬆️
... and 1 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 7d8acae...9202e76. Read the comment docs.

severn-everett · 2021-12-17T12:42:21Z

@BraisGabin The latest Detekt run on the branch had CanBeNonNullableProperty flag this call in DetektExtension.kt:

    @Deprecated("Use reportsDir which is equivalent", ReplaceWith("reportsDir"))
    val customReportsDir: File?
        get() = reportsDir

The problem is that Kotlin determines that reportsDir is non-nullable, even though it is Java code that could return a nullable. I'd say to have CanBeNonNullableProperty disqualify any property that derives its value from Java source code, but I'm not sure how I'd be able to test that in the Spec files. Any suggestions for how to compile Java code alongside Kotlin code in the tests, or should I just suppress the warning on customReportsDir?

cortinico

I'd say to have CanBeNonNullableProperty disqualify any property that derives its value from Java source code

+1 on this

cortinico · 2021-12-17T14:55:23Z

+        Debt.TEN_MINS
+    )
+
+    override fun visitKtFile(file: KtFile) {


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

Why would this be necessary? The NonNullableCheckVisitor instance conducts the actual check of the file for the rule, not CanBeNonNullableProperty itself.

Why would this be necessary?

That's part of the Rule contract and in general follows the visitor design pattern implementation. If by any chance we decide to implement a new feature on detekt that relies on the visitKtFile and lives inside the Rule top level class (or any of its ancestors), this rule will break this behavior.

cortinico · 2021-12-17T14:56:17Z

+                    CodeSmell(
+                        issue,
+                        Entity.from(property),
+                        "A nullable property can be made non-nullable."


nit: You could improve this to mention the name of the property as you have it

cortinico · 2021-12-17T14:57:59Z

+                    }
+                }
+                """
+            Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(5)


Can you split into smaller tests?

cortinico · 2021-12-17T14:58:24Z

+                    }
+                }
+                """
+            Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(5)


Same as above

…s; Reorganized tests and split up basic var/val evaluation tests

BraisGabin · 2021-12-17T23:21:15Z

but I'm not sure how I'd be able to test that in the Spec files. Any suggestions for how to compile Java code alongside Kotlin code in the tests, or should I just suppress the warning on customReportsDir?

@t-kameyama implemented the option to do that in #4203. I think that you can use the same pattern.

BraisGabin

Run the tests with -Pcompile-test-snippets=true in the gradle command. That will make the tests run slower but it will ensure that your snippet codes compile (It is necessary to merge the PR)

This rule is really good! But I'm wondering if this same rule should check for the same pattern in local variables or should we use other rule. I think that this rule is ready to merge now but, if we want to extend it later with the local variables we should change its name before merging it. I mean, it's not needed to implement the local variable thing now but if we want both things in the same ruel it can't be called "property".

BraisGabin · 2021-12-17T23:23:31Z


 style:
+  CanBeNonNullableProperty:
+    active: false


I think that it's safe to enable this. Or, if it isn't, remove these lines

…bles in the future; Re-enabled usage of rule for Detekt codebase; Fixed test code snippets that weren't compiling

severn-everett · 2021-12-18T10:37:12Z

Should be good to go, but something happened on the Windows Java 8 build. It looks like Gradle crashed for whatever reason, not sure if it's got anything to do with the code.

DHosseiny · 2022-11-20T12:54:35Z

It seems No doc for this in detekt.dev

cortinico · 2022-11-20T13:10:22Z

It seems No doc for this in detekt.dev

Doc is here: https://detekt.dev/docs/rules/style/#canbenonnullable

Added rule CanBeNonNullableProperty

d4818a5

Added processing for open properties and delegated properties

6f9c65e

cortinico reviewed Dec 17, 2021

View reviewed changes

cortinico added this to the 1.20.0 milestone Dec 17, 2021

cortinico added the rules label Dec 17, 2021

severn-everett added 5 commits December 17, 2021 21:05

Fixed check for nullable types that didn't cover bang-punctuated type…

b08f1ec

…s; Reorganized tests and split up basic var/val evaluation tests

Fix Detekt issues

a67703d

Added check to exclude evaluating interfaces

1c9c60e

Set rule to not be active by default

a772919

Fixed Detekt issue

f6a184b

BraisGabin reviewed Dec 17, 2021

View reviewed changes

Changed name of rule to make it potentially applicable to local varia…

9202e76

…bles in the future; Re-enabled usage of rule for Detekt codebase; Fixed test code snippets that weren't compiling

BraisGabin approved these changes Dec 18, 2021

View reviewed changes

severn-everett mentioned this pull request Dec 18, 2021

New rule: Function paramater should not be nullable #4377

Closed

BraisGabin merged commit f5d2a17 into detekt:main Dec 26, 2021

BraisGabin mentioned this pull request Dec 26, 2021

Disable CI for Windows & JDK8 #4403

Merged

severn-everett deleted the can_be_non_nullable_property branch December 27, 2021 08:38

cortinico mentioned this pull request Jan 3, 2022

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

Closed

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

cortinico changed the title ~~Added rule CanBeNonNullableProperty~~ Added rule CanBeNonNullable Nov 20, 2022

Uh oh!

Conversation

severn-everett commented Dec 17, 2021

Uh oh!

codecov Bot commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

severn-everett commented Dec 17, 2021

Uh oh!

cortinico left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cortinico Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

severn-everett Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

cortinico Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

cortinico Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

cortinico Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

cortinico Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

BraisGabin commented Dec 17, 2021

Uh oh!

BraisGabin left a comment

Choose a reason for hiding this comment

Uh oh!

BraisGabin Dec 17, 2021

Choose a reason for hiding this comment

Uh oh!

severn-everett commented Dec 18, 2021

Uh oh!

DHosseiny commented Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cortinico commented Nov 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Dec 17, 2021 •

edited

Loading

DHosseiny commented Nov 20, 2022 •

edited

Loading