CanBeNonNullable: fix false positives for parameterized types

[dzirbel]

CanBeNonNullable reports that properties of parameterized types can be marked as non-nullable even when this is impossible, just because the parameterized type is itself nullable (i.e. does not inherit from a non-nullable type). Instead, the rule should only report cases where a property is explicitly marked nullable but does not need to be.

[codecov]

Codecov Report

Merging #4870 (0f1f1ac) into main (fcd9c48) will increase coverage by 0.02%.
The diff coverage is 75.00%.

@@             Coverage Diff              @@
##               main    #4870      +/-   ##
============================================
+ Coverage     84.79%   84.81%   +0.02%     
- Complexity     3447     3453       +6     
============================================
  Files           492      492              
  Lines         11315    11330      +15     
  Branches       2083     2087       +4     
============================================
+ Hits           9595     9610      +15     
  Misses          673      673              
  Partials       1047     1047              

Impacted Files	Coverage Δ
.../arturbosch/detekt/rules/style/CanBeNonNullable.kt	73.22% <75.00%> (+0.60%)	⬆️
...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 fcd9c48...0f1f1ac. Read the comment docs.

[dzirbel]

Hm, I forced pushed two additional tests and codecov is now complaining it's under the coverage target? 🤔 Did I do something wrong? I'm not as familiar with PRs in GitHub as other tools

[BraisGabin]

Did I do something wrong?

Yet another caveat on our CI configuration... We usualy don't pay attention to that (unless the number is near to 0). Probably we should do something about it to avoid this type of confusions.

@schalkms What do you think we could do here?

[BraisGabin]

LGTM

[schalkms]

I guess you @BraisGabin and @dzirbel mean the two codecov CI runs.
Codecov provides detekt with code coverage reports. One report yields the code coverage for the whole project. The other report called patch just yields the code coverage for the additions and changed lines of code. The patch report fails when the code coverage is below the 80 % target. 80 % constitutes a sensible target for well tested code. However, this report is not 100 % accurate for every PR. Usually, I pay attention to this code coverage report during code reviews.
In this case, it's fine to merge with 75 %, I guess. Nevertheless, if you @dzirbel have an idea on how to test a currently uncovered path, we'd ask you to add tests for it.

Yet another caveat on our CI configuration... We usualy don't pay attention to that (unless the number is near to 0). Probably we should do something about it to avoid this type of confusions.

Maybe, we could spend one or two sentences in the Contributing.md file. I don't know whether this addition helps. What do you think?

@schalkms What do you think we could do here?

[dzirbel]

I guess you @BraisGabin and @dzirbel mean the two codecov CI runs. Codecov provides detekt with code coverage reports. One report yields the code coverage for the whole project. The other report called patch just yields the code coverage for the additions and changed lines of code. The patch report fails when the code coverage is below the 80 % target. 80 % constitutes a sensible target for well tested code. However, this report is not 100 % accurate for every PR. Usually, I pay attention to this code coverage report during code reviews. In this case, it's fine to merge with 75 %, I guess. Nevertheless, if you @dzirbel have an idea on how to test a currently uncovered path, we'd ask you to add tests for it.

Right, if the report can be inaccurate that all makes sense. In this case, coverage was above 80% initially (I believe, since all the checks had passed IIRC) and I force pushed two additional tests, after which coverage apparently dropped to 75%, so I wanted to double check that my PR method was correct (maybe I'd messed up rebases or something).

Thanks both of you for the quick and helpful reviews on my PRs this weekend :)

[schalkms]

You are welcome 🤗