-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix false positives for unused_enumerated with result member access #6544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,18 @@ struct UnusedEnumeratedRule: Rule { | |
| Example("list.enumerated().map { ($0.offset, $0.element) }"), | ||
| Example("list.enumerated().map { ($0.0, $0.1) }"), | ||
| Example(""" | ||
| list.enumerated().first { | ||
| $0.element.0.isNumber && | ||
| $0.element.1.isNumber && | ||
| $0.element.0 != $0.element.1 | ||
| }?.offset | ||
| """), | ||
| Example(""" | ||
| list.enumerated().max { | ||
| $0.element < $1.element | ||
| }?.offset | ||
| """), | ||
| Example(""" | ||
| list.enumerated().map { | ||
| $1.enumerated().forEach { print($0, $1) } | ||
| return $0 | ||
|
|
@@ -91,19 +103,21 @@ struct UnusedEnumeratedRule: Rule { | |
|
|
||
| private extension UnusedEnumeratedRule { | ||
| private struct Closure { | ||
| let enumeratedPosition: AbsolutePosition? | ||
| let enumeratedPosition: AbsolutePosition | ||
| let usedEnumeratedResultMembers: (zero: Bool, one: Bool) | ||
| var zeroPosition: AbsolutePosition? | ||
| var onePosition: AbsolutePosition? | ||
| } | ||
|
|
||
| init(enumeratedPosition: AbsolutePosition? = nil) { | ||
| self.enumeratedPosition = enumeratedPosition | ||
| } | ||
| private struct PendingClosure { | ||
| let id: SyntaxIdentifier | ||
| let enumeratedPosition: AbsolutePosition | ||
| let usedEnumeratedResultMembers: (zero: Bool, one: Bool) | ||
| } | ||
|
|
||
| final class Visitor: ViolationsSyntaxVisitor<ConfigurationType> { | ||
| private var nextClosureId: SyntaxIdentifier? | ||
| private var lastEnumeratedPosition: AbsolutePosition? | ||
| private var closures = Stack<Closure>() | ||
| private var pendingClosure: PendingClosure? | ||
| private var closures = Stack<Closure?>() | ||
|
|
||
| override func visitPost(_ node: ForStmtSyntax) { | ||
| guard let tuplePattern = node.pattern.as(TuplePatternSyntax.self), | ||
|
|
@@ -132,7 +146,8 @@ private extension UnusedEnumeratedRule { | |
| guard node.isEnumerated, | ||
| let parent = node.parent, | ||
| parent.as(MemberAccessExprSyntax.self)?.declName.baseName.text != "filter", | ||
| let trailingClosure = parent.parent?.as(FunctionCallExprSyntax.self)?.trailingClosure | ||
| let parentCall = parent.parent?.as(FunctionCallExprSyntax.self), | ||
| let trailingClosure = parentCall.trailingClosure | ||
| else { | ||
| return .visitChildren | ||
| } | ||
|
|
@@ -156,44 +171,55 @@ private extension UnusedEnumeratedRule { | |
| zeroPosition: firstTokenIsUnderscore ? firstElement.positionAfterSkippingLeadingTrivia : nil, | ||
| onePosition: firstTokenIsUnderscore ? nil : secondElement.positionAfterSkippingLeadingTrivia | ||
| ) | ||
| } else { | ||
| nextClosureId = trailingClosure.id | ||
| lastEnumeratedPosition = node.enumeratedPosition | ||
| } else if let enumeratedPosition = node.enumeratedPosition { | ||
| pendingClosure = PendingClosure( | ||
| id: trailingClosure.id, | ||
| enumeratedPosition: enumeratedPosition, | ||
| usedEnumeratedResultMembers: parentCall.usedEnumeratedResultMembers | ||
| ) | ||
| } | ||
|
|
||
| return .visitChildren | ||
| } | ||
|
|
||
| override func visit(_ node: ClosureExprSyntax) -> SyntaxVisitorContinueKind { | ||
| if let nextClosureId, nextClosureId == node.id, let lastEnumeratedPosition { | ||
| closures.push(Closure(enumeratedPosition: lastEnumeratedPosition)) | ||
| self.nextClosureId = nil | ||
| self.lastEnumeratedPosition = nil | ||
| if let pendingClosure, pendingClosure.id == node.id { | ||
| closures.push(Closure( | ||
| enumeratedPosition: pendingClosure.enumeratedPosition, | ||
| usedEnumeratedResultMembers: pendingClosure.usedEnumeratedResultMembers | ||
| )) | ||
| self.pendingClosure = nil | ||
| } else { | ||
| closures.push(Closure()) | ||
| closures.push(nil) | ||
| } | ||
| return .visitChildren | ||
| } | ||
|
|
||
| override func visitPost(_: ClosureExprSyntax) { | ||
| if let closure = closures.pop(), (closure.zeroPosition != nil) != (closure.onePosition != nil) { | ||
| addViolation( | ||
| zeroPosition: closure.onePosition, | ||
| onePosition: closure.zeroPosition, | ||
| enumeratedPosition: closure.enumeratedPosition | ||
| ) | ||
| } | ||
| guard let closure = popTrackedClosure() else { return } | ||
|
|
||
| let zeroPosition = closure.zeroPosition | ||
| ?? (closure.usedEnumeratedResultMembers.zero ? closure.enumeratedPosition : nil) | ||
| let onePosition = closure.onePosition | ||
| ?? (closure.usedEnumeratedResultMembers.one ? closure.enumeratedPosition : nil) | ||
| guard (zeroPosition != nil) != (onePosition != nil) else { return } | ||
|
|
||
| addViolation( | ||
| zeroPosition: onePosition, | ||
| onePosition: zeroPosition, | ||
| enumeratedPosition: closure.enumeratedPosition | ||
| ) | ||
| } | ||
|
|
||
| override func visitPost(_ node: DeclReferenceExprSyntax) { | ||
| guard | ||
| let closure = closures.peek(), | ||
| closure.enumeratedPosition != nil, | ||
| currentTrackedClosure != nil, | ||
| node.baseName.text == "$0" || node.baseName.text == "$1" | ||
| else { | ||
| return | ||
| } | ||
| closures.modifyLast { | ||
|
|
||
| modifyTrackedClosure { | ||
| if node.baseName.text == "$0" { | ||
| let member = node.parent?.as(MemberAccessExprSyntax.self)?.declName.baseName.text | ||
| if member == "element" || member == "1" { | ||
|
|
@@ -210,6 +236,22 @@ private extension UnusedEnumeratedRule { | |
| } | ||
| } | ||
|
|
||
| private var currentTrackedClosure: Closure? { | ||
| closures.peek().flatMap(\.self) | ||
| } | ||
|
|
||
| private func popTrackedClosure() -> Closure? { | ||
| closures.pop().flatMap(\.self) | ||
| } | ||
|
|
||
| private func modifyTrackedClosure(_ modifier: (inout Closure) -> Void) { | ||
| closures.modifyLast { | ||
| guard var closure = $0 else { return } | ||
| modifier(&closure) | ||
| $0 = closure | ||
| } | ||
| } | ||
|
|
||
| private func addViolation( | ||
| zeroPosition: AbsolutePosition?, | ||
| onePosition: AbsolutePosition?, | ||
|
|
@@ -257,6 +299,35 @@ private extension FunctionCallExprSyntax { | |
| && additionalTrailingClosures.isEmpty | ||
| && arguments.isEmpty | ||
| } | ||
|
|
||
| var usedEnumeratedResultMembers: (zero: Bool, one: Bool) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may attach this to |
||
| var currentNode: Syntax? = Syntax(self) | ||
|
|
||
| while let node = currentNode, let parent = node.parent { | ||
| if let memberAccess = parent.as(MemberAccessExprSyntax.self), | ||
| memberAccess.base?.id == node.id { | ||
| switch memberAccess.declName.baseName.text { | ||
| case "offset", "0": | ||
| return (true, false) | ||
| case "element", "1": | ||
| return (false, true) | ||
| default: | ||
| break | ||
| } | ||
| } | ||
|
|
||
| guard parent.is(OptionalChainingExprSyntax.self) | ||
| || parent.is(ForceUnwrapExprSyntax.self) | ||
| || parent.is(MemberAccessExprSyntax.self) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also consider tuple expressions with single elements (i.e. parenthesized expressions). |
||
| else { | ||
| return (false, false) | ||
| } | ||
|
|
||
| currentNode = parent | ||
| } | ||
|
|
||
| return (false, false) | ||
| } | ||
| } | ||
|
|
||
| private extension TuplePatternElementSyntax { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to inline them as each of them is only used once.