Rewrite RedundantVoidReturnRule with SwiftSyntax#4192
Conversation
jpsim
left a comment
jpsim
left a comment
There was a problem hiding this comment.
So good. It's hard to write a clean rewriter though. Maybe it's a skill we'll develop over time.
| } | ||
|
|
||
| return [match] | ||
| override func visit(_ node: FunctionSignatureSyntax) -> Syntax { |
There was a problem hiding this comment.
As it is, this won't fix all the violations found, since in the visitor you visit ReturnClauseSyntax nodes and here you visit FunctionSignatureSyntax nodes.
There was a problem hiding this comment.
Duplicating the whole thing for ClosureSignatureSyntax works but is klunky:
diff --git a/Source/SwiftLintFramework/Rules/Idiomatic/RedundantVoidReturnRule.swift b/Source/SwiftLintFramework/Rules/Idiomatic/RedundantVoidReturnRule.swift
index 0fd4108252..38d6d7e59a 100644
--- a/Source/SwiftLintFramework/Rules/Idiomatic/RedundantVoidReturnRule.swift
+++ b/Source/SwiftLintFramework/Rules/Idiomatic/RedundantVoidReturnRule.swift
@@ -101,6 +101,25 @@ private extension RedundantVoidReturnRule {
self.disabledRegions = disabledRegions
}
+ override func visit(_ node: ClosureSignatureSyntax) -> Syntax {
+ guard let output = node.output,
+ let tokenBeforeOutput = output.previousToken,
+ output.containsRedundantVoidViolation else {
+ return super.visit(node)
+ }
+
+ let isInDisabledRegion = disabledRegions.contains { region in
+ region.contains(node.positionAfterSkippingLeadingTrivia, locationConverter: locationConverter)
+ }
+
+ guard !isInDisabledRegion else {
+ return super.visit(node)
+ }
+
+ correctionPositions.append(tokenBeforeOutput.endPositionBeforeTrailingTrivia)
+ return super.visit(node.withOutput(nil).removingTrailingSpaceIfNeeded())
+ }
+
override func visit(_ node: FunctionSignatureSyntax) -> Syntax {
guard let output = node.output,
let tokenBeforeOutput = output.previousToken,
@@ -136,6 +155,32 @@ private extension ReturnClauseSyntax {
}
}
+private extension ClosureSignatureSyntax {
+ /// `withOutput(nil)` adds a `.spaces(1)` trailing trivia, but we don't always want it.
+ func removingTrailingSpaceIfNeeded() -> Self {
+ guard let trivia = trailingTrivia, let nextToken = nextToken else {
+ return self
+ }
+
+ let containsNewLine = nextToken.leadingTrivia.contains { piece in
+ switch piece {
+ case .newlines:
+ return true
+ default:
+ return false
+ }
+ }
+
+ guard containsNewLine else {
+ return self
+ }
+
+ return withTrailingTrivia(
+ Trivia(pieces: Array(trivia.dropFirst()))
+ )
+ }
+}
+
private extension FunctionSignatureSyntax {
/// `withOutput(nil)` adds a `.spaces(1)` trailing trivia, but we don't always want it.
func removingTrailingSpaceIfNeeded() -> Self {
|
I found a false positive in Lyft's codebase with this: let foo = { () -> Void in
bar.baz(to: blorp)
}The type inference fails here. I think this is an issue with |
isn't this something that the existing |
Co-authored-by: Marcelo Fabri <me@marcelofabri.com>
jpsim
force-pushed
the
marcelo/RedundantVoidReturnRule-swift-syntax
branch
from
6e59af1 to
8d5affa
Compare
|
I hope you don't mind @marcelofabri I rebased and updated this branch, it'd be good to get this in. |
🤩 |
|
I’m considering making the extended coverage opt-out since when used in closures, this isn’t strictly redundant since it helps the type checker. |
To see more accurately the difference between the SourceKit and SwiftSyntax versions of the rule
|
When running OSSCheck with
|
This reverts commit bf28469.
3 participants
The correction tests are failing, but wanted to trigger oss-check meanwhile