ExplicitCollectionElementAccessMethod rule fixes#4201
ExplicitCollectionElementAccessMethod rule fixes#4201BraisGabin merged 5 commits intodetekt:mainfrom Whathecode:fix-4122
Conversation
…thod rule This actually simplifies code, since any get/set operator can be treated the same; no need to look up type names. `Map.put()` is a type-specific edge case, which can be replaced by `set`, and thus indexed access, when the return value is not needed. Right now, no check is done whether the return value is use. For both `set` and `put` no code smell should be reported if this is the case.
Codecov Report
@@ Coverage Diff @@
## main #4201 +/- ##
============================================
- Coverage 84.19% 84.16% -0.03%
- Complexity 3233 3241 +8
============================================
Files 468 468
Lines 10185 10182 -3
Branches 1786 1789 +3
============================================
- Hits 8575 8570 -5
+ Misses 671 669 -2
- Partials 939 943 +4
Continue to review full report at Codecov.
|
…sed return values
Also fixed other tests that were failing on build server.
This also includes an early out in case types can't be resolved, rather than iterating over supertypes.
cortinico
left a comment
cortinico
left a comment
There was a problem hiding this comment.
Great work 🚀 The only thing that is needed to update is that now this rule requires type resolution to work correctly
| .isEligibleCollection() | ||
|
|
||
| private fun isOperatorFunction(expression: KtCallExpression): Boolean { | ||
| val function = (expression.getResolvedCall(bindingContext)?.resultingDescriptor as? FunctionDescriptor) |
There was a problem hiding this comment.
Since you're using bindingContext here, you should add a @RequiresTypeResolution to this rule + a if(bindingContext == BindingContext.EMPTY) return check at the beginning of the visit...( method.
There was a problem hiding this comment.
Done! If I understand correctly, this was missing before as well? I.e., the removed LOC right above:
return (caller as? KtElement).getResolvedCall(bindingContext)
?.resultingDescriptor
?.returnType
.isEligibleCollection()
There was a problem hiding this comment.
Done! If I understand correctly, this was missing before as well?
Yes it was a "bug" in the sense that it was a rule with a mixed behavior that we should look into fixing: #2994
cortinico
added
notable changes
3 participants
putare usedgetandsetoperators, which furthermore lead to a more general-purpose implementation (no more elaborate type-checking).set, which is in fact the main use case.putis not a valid index accessor function. The reasonputcan be replaced with an index accessor for maps when the return value is unused is that it essentially behaves the same assetin that case.I think the private functions operating on
KtCallExpressionare best moved more centrally to reusable/tested extension functions. I could not find such a place in the library. I typically don't like creating private functions that are called from a single place, but I imagine similar logic is needed, and may even have been implemented, in other rules. I expect that if these become public helper functions I could write associated unit tests, resolving the decrease in code coverage which currently seems to fail the automated check on this PR.The updated rule found several getters in the detekt codebase, previously not detected given that they weren't of one of the expected types, which had to be be replaced by index accessors.