Fix false positives for unused_enumerated with result member access#6544
Fix false positives for unused_enumerated with result member access#6544theamodhshetty wants to merge 2 commits intorealm:mainfrom
Conversation
Generated by 🚫 Danger |
|
Pushed a small follow-up in The original version reset |
|
Does anything else need to be done to merge this PR? |
|
I don’t think so from my side. The follow-up for the CI/toolchain issue is already in, and the checks are green now. If there’s anything else you want adjusted before merge, I’m happy to make it. |
SimplyDanny
left a comment
SimplyDanny
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
The idea looks good. I have a few remarks though ...
| guard node.isEnumerated, | ||
| let parent = node.parent, | ||
| parent.as(MemberAccessExprSyntax.self)?.declName.baseName.text != "filter", | ||
| let trailingClosure = parent.parent?.as(FunctionCallExprSyntax.self)?.trailingClosure |
There was a problem hiding this comment.
| let parentCall = parent?.parent?.as(FunctionCallExprSyntax.self) | |
| let trailingClosure = parentCall.trailingClosure |
| lastEnumeratedResultMemberUsage = | ||
| parent.parent?.as(FunctionCallExprSyntax.self)? | ||
| .usedEnumeratedResultMembers ?? (false, false) |
There was a problem hiding this comment.
| lastEnumeratedResultMemberUsage = | |
| parent.parent?.as(FunctionCallExprSyntax.self)? | |
| .usedEnumeratedResultMembers ?? (false, false) | |
| lastEnumeratedResultMemberUsage = parentCall.usedEnumeratedResultMembers ?? (false, false) |
| var current = parent | ||
|
|
||
| while let currentNode = current { | ||
| if let memberAccess = currentNode.as(MemberAccessExprSyntax.self) { |
There was a problem hiding this comment.
If there isn't a member access whatsoever, this will always run up to the file's root. Can we find another condition to stop iteration prematurely?
| case "element", "1": | ||
| return (false, true) | ||
| default: | ||
| return (false, false) |
There was a problem hiding this comment.
| return (false, false) | |
| break |
| ) { | ||
| self.enumeratedPosition = enumeratedPosition | ||
| self.zeroPosition = usesZeroResultMember ? enumeratedPosition : nil | ||
| self.onePosition = usesOneResultMember ? enumeratedPosition : nil |
There was a problem hiding this comment.
This logic looks a bit surprising on first sight. Why would zeroPosition become enumeratedPosition?
I think that actually, enumeratedPosition should always be non-optional, so that we only need to track whether "zero" or "one" was found. In turn, Closure must be Closure? in the stack.
4 participants
Summary
.enumerated()when a higher-order call uses tuple members like?.offsetafter the closure$0.element/$0.0and seed the closure usage from result-member access on the surrounding callTesting
Closes #5881