Fix False Positive on UnnecessarySafeCall - #3419
Conversation
cortinico
changed the title
UnnecessarySafeCall
b4cf2ff to
179fe30
Compare
Codecov Report
@@ Coverage Diff @@
## master #3419 +/- ##
=========================================
Coverage 80.37% 80.37%
- Complexity 2748 2749 +1
=========================================
Files 449 449
Lines 8295 8301 +6
Branches 1587 1590 +3
=========================================
+ Hits 6667 6672 +5
Misses 774 774
- Partials 854 855 +1
Continue to review full report at Codecov.
|
BraisGabin
left a comment
BraisGabin
left a comment
There was a problem hiding this comment.
Is there any way to create a unit test?
| if (compilerReport != null) { | ||
| // For external types, if they're not included in the classpath, we still get an Errors.UNNECESSARY_SAFE_CALL. | ||
| // This causes false positives if our users are misconfiguring detekt with Type Resolution. | ||
| // Here we try to check if the compiler reports failed to resolve the nullable type. | ||
| if (compilerReport is DiagnosticWithParameters1<*, *> && compilerReport.a is ErrorType) { | ||
| return | ||
| } |
There was a problem hiding this comment.
For me this is still strange. Why is this popping up only after updating to the newest compiler version?
There was a problem hiding this comment.
We should be able to find the exact DiagnosticWithParameters1 definition of the errors being reported and add this to the documentation.
There was a problem hiding this comment.
For me this is still strange. Why is this popping up only after updating to the newest compiler version?
I believe this is related to us enabling the rule by default:
#3419
| val compilerReport = bindingContext | ||
| .diagnostics | ||
| .forElement(safeAccessElement) | ||
| .firstOrNull { it.factory == Errors.UNNECESSARY_SAFE_CALL } |
There was a problem hiding this comment.
Should we still keep any instead of firstOrNull? I think the code depends on the order for the diagnostics, which seems too fragile.
There was a problem hiding this comment.
Should we still keep
anyinstead offirstOrNull?
I could change it to a forEach as we need to get access to the compilerReport
Found a test case for it 👌 The only drawback is that we can't compile the test snippet as the import will break the compilation. |
|
Turns out the Kotlin compiler was actually wrong here. I've pushed a fix upstream JetBrains/kotlin#4072 so we can revert our patch as soon as Kotlin 1.5 is out 🥳 |
|
How can we be sure that we don't forget? Create the pr with a blocked label and merge it as soon as we move to kotlin 1.5? |
That's really interesting, so this suspicion was right.
I'd suggest this approach. |
Created here #3439 |
4 participants
This is a fix for the various false positives on
UnnecessarySafeCall.After some debugging the problems seems to be with misconfigured class-paths (e.g. carp-dk/carp.core-kotlin#223). If the classpath doesn't contain a third-party types, but still has a safe call operator, the Kotlin Compiler raises an
Errors.UNNECESSARY_SAFE_CALLdetection.Inside the detection the compiler adds information on the non-nullable type that is the receiver of the safe call (in the
.afield). In this patch I'm try to see if the compiler failed to resolve the type => If so, we can skip the report.Fixes #3409
Fixes #3414