Skip to content
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

Add unused_capture_list rule #2719

Merged

Conversation

daltonclaybrook
Copy link
Contributor

Resolves #2715

I'm using an ASTRule in order to find closures, but I was unable to find any first-class parsing support within SourceKitten for capture lists, so I am doing this with regex and string manipulation. Let me know if I missed something.

@SwiftLintBot
Copy link

SwiftLintBot commented Apr 14, 2019

4 Warnings
⚠️ This PR introduced a violation in Firefox: /Users/vsts/agent/2.149.2/work/1/s/osscheck/Firefox/Client/Frontend/Settings/AppSettingsOptions.swift:630:84: warning: Unused Capture List Violation: Unused reference self in a capture list should be removed. (unused_capture_list)
⚠️ This PR introduced a violation in Kickstarter: /Users/vsts/agent/2.149.2/work/1/s/osscheck/Kickstarter/Kickstarter-iOS/Views/Controllers/ProjectPamphletContentViewControllerTests.swift:224:8: warning: Unused Capture List Violation: Unused reference Device.phone4_7inch in a capture list should be removed. (unused_capture_list)
⚠️ This PR introduced a violation in Kickstarter: /Users/vsts/agent/2.149.2/work/1/s/osscheck/Kickstarter/Kickstarter-iOS/Views/Controllers/ProjectPamphletContentViewControllerTests.swift:224:29: warning: Unused Capture List Violation: Unused reference Device.phone5_8inch in a capture list should be removed. (unused_capture_list)
⚠️ This PR introduced a violation in Kickstarter: /Users/vsts/agent/2.149.2/work/1/s/osscheck/Kickstarter/Kickstarter-iOS/Views/Controllers/ProjectPamphletContentViewControllerTests.swift:224:50: warning: Unused Capture List Violation: Unused reference Device.pad in a capture list should be removed. (unused_capture_list)
12 Messages
📖 Linting Aerial with this PR took 2.36s vs 2.11s on master (11% slower)
📖 Linting Alamofire with this PR took 5.22s vs 5.23s on master (0% faster)
📖 Linting Firefox with this PR took 17.16s vs 16.17s on master (6% slower)
📖 Linting Kickstarter with this PR took 28.34s vs 27.4s on master (3% slower)
📖 Linting Moya with this PR took 2.48s vs 2.5s on master (0% faster)
📖 Linting Nimble with this PR took 2.32s vs 2.29s on master (1% slower)
📖 Linting Quick with this PR took 0.71s vs 0.75s on master (5% faster)
📖 Linting Realm with this PR took 4.21s vs 4.38s on master (3% faster)
📖 Linting SourceKitten with this PR took 1.51s vs 1.48s on master (2% slower)
📖 Linting Sourcery with this PR took 5.07s vs 5.17s on master (1% faster)
📖 Linting Swift with this PR took 34.34s vs 32.8s on master (4% slower)
📖 Linting WordPress with this PR took 29.91s vs 24.17s on master (23% slower)

Generated by 🚫 Danger

]
)

private let captureListRegex = regex("^\\{\\h*\\[([^\\]]+)\\].*\\bin\\b")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any particular reason to use \h here? although not common, the capture list could be in a new line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, good point.

identifier: "unused_capture_list",
name: "Unused Capture List",
description: "Unused reference in a capture list should be removed.",
kind: .lint,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add swiftVersion: .fourDotTwo? SwiftSyntaxKind.closure is only returned by SourceKit in 4.2+

}
}

private func identifierStrings(in file: File, byteRange: NSRange) -> [String] {
Copy link
Collaborator

@marcelofabri marcelofabri Apr 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: this could return a Set<String>

@marcelofabri
Copy link
Collaborator

I'm using an ASTRule in order to find closures, but I was unable to find any first-class parsing support within SourceKitten for capture lists

They're reported as local variables inside the closure, but you'd need to be creative to figure out if it's a captured variable (i.e. check if the location is before the in token location).

@marcelofabri marcelofabri merged commit 87d9097 into realm:master Apr 15, 2019
Copy link
Collaborator

@marcelofabri marcelofabri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for another great contribution, @daltonclaybrook! 🥇

@daltonclaybrook daltonclaybrook deleted the dc-unused-capture-list-rule branch April 15, 2019 17:41
@jpsim
Copy link
Collaborator

jpsim commented Apr 20, 2019

Thanks for the contribution @daltonclaybrook, and this is indeed a useful rule, although there are some false positives (3/4 of the violations reported by OSSCheck are false positives). If we can't avoid those false positives, I think we should make this rule opt-in (disabled by default).

@daltonclaybrook
Copy link
Contributor Author

@jpsim oh whoops, I guess I missed those warnings. I may have some time this afternoon to take a look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule Request: unused value in capture group
4 participants