From 8d5affad35b170e5f6fe5f94d01bbe9cf4ab21bf Mon Sep 17 00:00:00 2001 From: JP Simard Date: Mon, 30 Oct 2023 10:48:27 -0400 Subject: [PATCH 1/4] Rewrite RedundantVoidReturnRule rule using SwiftSyntax Co-authored-by: Marcelo Fabri --- CHANGELOG.md | 4 + .../Idiomatic/RedundantVoidReturnRule.swift | 94 +++++++++++-------- .../Helpers/LintableFilesVisitor.swift | 2 +- tools/oss-check | 4 + 4 files changed, 66 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4da3be8be7..7a601f237d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,10 @@ * Rewrite `cyclomatic_complexity` rule using SwiftSyntax. [Marcelo Fabri](https://github.com/marcelofabri) +* Rewrite `redundant_void_return` rule using SwiftSyntax. + [Marcelo Fabri](https://github.com/marcelofabri) + [JP Simard](https://github.com/jpsim) + #### Bug Fixes * Fix correction of `explicit_init` rule by keeping significant trivia. diff --git a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantVoidReturnRule.swift b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantVoidReturnRule.swift index f5cbb0a278..6dd55af09f 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantVoidReturnRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantVoidReturnRule.swift @@ -1,7 +1,7 @@ -import Foundation -import SourceKittenFramework +import SwiftSyntax -struct RedundantVoidReturnRule: SubstitutionCorrectableASTRule { +@SwiftSyntaxRule(explicitRewriter: true) +struct RedundantVoidReturnRule: Rule { var configuration = SeverityConfiguration(.warning) static let description = RuleDescription( @@ -42,6 +42,16 @@ struct RedundantVoidReturnRule: SubstitutionCorrectableASTRule { protocol Foo { func foo()↓ -> () } + """), + Example(""" + doSomething { arg↓ -> () in + print(arg) + } + """), + Example(""" + doSomething { arg↓ -> Void in + print(arg) + } """) ], corrections: [ @@ -53,50 +63,60 @@ struct RedundantVoidReturnRule: SubstitutionCorrectableASTRule { Example("protocol Foo {\n #if true\n func foo()\n #endif\n}") ] ) +} - private let pattern = "\\s*->\\s*(?:Void\\b|\\(\\s*\\))(?![?!])" - private let excludingKinds = SyntaxKind.allKinds.subtracting([.typeidentifier]) - private let functionKinds = SwiftDeclarationKind.functionKinds.subtracting([.functionSubscript]) - - func validate(file: SwiftLintFile, kind: SwiftDeclarationKind, - dictionary: SourceKittenDictionary) -> [StyleViolation] { - return violationRanges(in: file, kind: kind, dictionary: dictionary).map { - StyleViolation(ruleDescription: Self.description, - severity: configuration.severity, - location: Location(file: file, characterOffset: $0.location)) +private extension RedundantVoidReturnRule { + final class Visitor: ViolationsSyntaxVisitor { + override func visitPost(_ node: ReturnClauseSyntax) { + if node.containsRedundantVoidViolation, + let tokenBeforeOutput = node.previousToken(viewMode: .sourceAccurate) { + violations.append(tokenBeforeOutput.endPositionBeforeTrailingTrivia) + } } } - func violationRanges(in file: SwiftLintFile, kind: SwiftDeclarationKind, - dictionary: SourceKittenDictionary) -> [NSRange] { - guard functionKinds.contains(kind), - containsVoidReturnTypeBasedOnTypeName(dictionary: dictionary), - let nameOffset = dictionary.nameOffset, - let nameLength = dictionary.nameLength, - let length = dictionary.length, - let offset = dictionary.offset, - case let start = nameOffset + nameLength, - case let end = dictionary.bodyOffset ?? offset + length, - case let contents = file.stringView, - case let byteRange = ByteRange(location: start, length: end - start), - let range = contents.byteRangeToNSRange(byteRange), - file.match(pattern: "->", excludingSyntaxKinds: excludingKinds, range: range).count == 1, - let match = file.match(pattern: pattern, excludingSyntaxKinds: excludingKinds, range: range).first else { - return [] - } + final class Rewriter: ViolationsSyntaxRewriter { + override func visit(_ node: FunctionSignatureSyntax) -> FunctionSignatureSyntax { + guard let output = node.returnClause, + let tokenBeforeOutput = output.previousToken(viewMode: .sourceAccurate), + output.containsRedundantVoidViolation + else { + return super.visit(node) + } - return [match] + correctionPositions.append(tokenBeforeOutput.endPositionBeforeTrailingTrivia) + return super.visit(node.with(\.returnClause, nil).removingTrailingSpaceIfNeeded()) + } } +} - func substitution(for violationRange: NSRange, in file: SwiftLintFile) -> (NSRange, String)? { - return (violationRange, "") +private extension ReturnClauseSyntax { + var containsRedundantVoidViolation: Bool { + if parent?.is(FunctionTypeSyntax.self) == true { + return false + } else if let simpleReturnType = type.as(IdentifierTypeSyntax.self) { + return simpleReturnType.typeName == "Void" + } else if let tupleReturnType = type.as(TupleTypeSyntax.self) { + return tupleReturnType.elements.isEmpty + } else { + return false + } } +} - private func containsVoidReturnTypeBasedOnTypeName(dictionary: SourceKittenDictionary) -> Bool { - guard let typeName = dictionary.typeName else { - return false +private extension FunctionSignatureSyntax { + /// `withOutput(nil)` adds a `.spaces(1)` trailing trivia, but we don't always want it. + func removingTrailingSpaceIfNeeded() -> Self { + guard + let nextToken = nextToken(viewMode: .sourceAccurate), + nextToken.leadingTrivia.containsNewlines() + else { + return self } - return typeName == "Void" || typeName.components(separatedBy: .whitespaces).joined() == "()" + return with( + \.trailingTrivia, + Trivia(pieces: trailingTrivia.dropFirst()) + ) } } diff --git a/Source/swiftlint/Helpers/LintableFilesVisitor.swift b/Source/swiftlint/Helpers/LintableFilesVisitor.swift index b353d7726a..97272773bf 100644 --- a/Source/swiftlint/Helpers/LintableFilesVisitor.swift +++ b/Source/swiftlint/Helpers/LintableFilesVisitor.swift @@ -59,7 +59,7 @@ enum LintOrAnalyzeModeWithCompilerArguments { } private func resolveParamsFiles(args: [String]) -> [String] { - return args.reduce(into: []) { (allArgs: inout [String], arg: String) -> Void in + return args.reduce(into: []) { (allArgs: inout [String], arg: String) in if arg.hasPrefix("@"), let contents = try? String(contentsOfFile: String(arg.dropFirst())) { allArgs.append(contentsOf: resolveParamsFiles(args: contents.split(separator: "\n").map(String.init))) } else { diff --git a/tools/oss-check b/tools/oss-check index a378c95aa3..5f0903c230 100755 --- a/tools/oss-check +++ b/tools/oss-check @@ -276,6 +276,10 @@ def set_globals end.compact.sort # True iff the only Swift files that were changed are SwiftLint rules, and that number is one or greater @only_rules_changed = !@rules_changed.empty? && @changed_swift_files.count == @rules_changed.count + + # Force OSSCheck to only measure the impact of the `redundant_void_return` rule + @rules_changed = ['redundant_void_return'] + @only_rules_changed = true end def print_rules_changed From edeffceb7bf95f0e3cfd69299805c134c38c4f8e Mon Sep 17 00:00:00 2001 From: JP Simard Date: Mon, 30 Oct 2023 12:39:49 -0400 Subject: [PATCH 2/4] Add RedundantVoidReturnConfiguration --- CHANGELOG.md | 5 +- .../Idiomatic/RedundantVoidReturnRule.swift | 51 +++++++++++++++++-- .../RedundantVoidReturnConfiguration.swift | 9 ++++ 3 files changed, 59 insertions(+), 6 deletions(-) create mode 100644 Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/RedundantVoidReturnConfiguration.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a601f237d..0313178fe0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,10 @@ * Rewrite `cyclomatic_complexity` rule using SwiftSyntax. [Marcelo Fabri](https://github.com/marcelofabri) -* Rewrite `redundant_void_return` rule using SwiftSyntax. +* Rewrite `redundant_void_return` rule using SwiftSyntax. + Also include redundant void return clauses for closures in addition to + functions. This can be disabled by configuring the rule with + `include_closures: false`. [Marcelo Fabri](https://github.com/marcelofabri) [JP Simard](https://github.com/jpsim) diff --git a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantVoidReturnRule.swift b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantVoidReturnRule.swift index 6dd55af09f..2e59c55836 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantVoidReturnRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Idiomatic/RedundantVoidReturnRule.swift @@ -1,8 +1,8 @@ import SwiftSyntax -@SwiftSyntaxRule(explicitRewriter: true) -struct RedundantVoidReturnRule: Rule { - var configuration = SeverityConfiguration(.warning) +@SwiftSyntaxRule +struct RedundantVoidReturnRule: SwiftSyntaxCorrectableRule { + var configuration = RedundantVoidReturnConfiguration() static let description = RuleDescription( identifier: "redundant_void_return", @@ -27,7 +27,12 @@ struct RedundantVoidReturnRule: Rule { print(key) } } - """) + """), + Example(""" + doSomething { arg -> Void in + print(arg) + } + """, configuration: ["include_closures": false]) ], triggeringExamples: [ Example("func foo()↓ -> Void {}"), @@ -63,11 +68,23 @@ struct RedundantVoidReturnRule: Rule { Example("protocol Foo {\n #if true\n func foo()\n #endif\n}") ] ) + + func makeRewriter(file: SwiftLintFile) -> (some ViolationsSyntaxRewriter)? { + Rewriter( + configuration: configuration, + locationConverter: file.locationConverter, + disabledRegions: disabledRegions(file: file) + ) + } } private extension RedundantVoidReturnRule { final class Visitor: ViolationsSyntaxVisitor { override func visitPost(_ node: ReturnClauseSyntax) { + if !configuration.includeClosures && node.parent?.is(ClosureSignatureSyntax.self) == true { + return + } + if node.containsRedundantVoidViolation, let tokenBeforeOutput = node.previousToken(viewMode: .sourceAccurate) { violations.append(tokenBeforeOutput.endPositionBeforeTrailingTrivia) @@ -76,6 +93,30 @@ private extension RedundantVoidReturnRule { } final class Rewriter: ViolationsSyntaxRewriter { + let configuration: ConfigurationType + + init( + configuration: ConfigurationType, + locationConverter: SourceLocationConverter, + disabledRegions: [SourceRange] + ) { + self.configuration = configuration + super.init(locationConverter: locationConverter, disabledRegions: disabledRegions) + } + + override func visit(_ node: ClosureSignatureSyntax) -> ClosureSignatureSyntax { + guard configuration.includeClosures, + let output = node.returnClause, + let tokenBeforeOutput = output.previousToken(viewMode: .sourceAccurate), + output.containsRedundantVoidViolation + else { + return super.visit(node) + } + + correctionPositions.append(tokenBeforeOutput.endPositionBeforeTrailingTrivia) + return super.visit(node.with(\.returnClause, nil).removingTrailingSpaceIfNeeded()) + } + override func visit(_ node: FunctionSignatureSyntax) -> FunctionSignatureSyntax { guard let output = node.returnClause, let tokenBeforeOutput = output.previousToken(viewMode: .sourceAccurate), @@ -104,7 +145,7 @@ private extension ReturnClauseSyntax { } } -private extension FunctionSignatureSyntax { +private extension SyntaxProtocol { /// `withOutput(nil)` adds a `.spaces(1)` trailing trivia, but we don't always want it. func removingTrailingSpaceIfNeeded() -> Self { guard diff --git a/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/RedundantVoidReturnConfiguration.swift b/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/RedundantVoidReturnConfiguration.swift new file mode 100644 index 0000000000..c81e36723a --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/RedundantVoidReturnConfiguration.swift @@ -0,0 +1,9 @@ +@AutoApply +struct RedundantVoidReturnConfiguration: SeverityBasedRuleConfiguration { + typealias Parent = RedundantVoidReturnRule + + @ConfigurationElement(key: "severity") + private(set) var severityConfiguration = SeverityConfiguration(.warning) + @ConfigurationElement(key: "include_closures") + private(set) var includeClosures = true +} From bf28469ae17cf8b3b283efff84d83a4d93c4021a Mon Sep 17 00:00:00 2001 From: JP Simard Date: Mon, 30 Oct 2023 12:56:49 -0400 Subject: [PATCH 3/4] REVERT ME: Run OSSCheck with closures disabled To see more accurately the difference between the SourceKit and SwiftSyntax versions of the rule --- tools/oss-check | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/oss-check b/tools/oss-check index 5f0903c230..cb6bc5eb12 100755 --- a/tools/oss-check +++ b/tools/oss-check @@ -149,8 +149,8 @@ def setup_repos end if @only_rules_changed && @rules_changed File.open(swiftlint_config, 'w') do |file| - file.puts('only_rules:') - file.puts(@rules_changed.map { |rule| " - #{rule}" }) + file.puts('only_rules: [redundant_void_return]') + file.puts('redundant_void_return: {include_closures: false}') end end Dir.chdir(dir) do From 9fcf91971256bb7219a3e634655b85efdb779a57 Mon Sep 17 00:00:00 2001 From: JP Simard Date: Mon, 30 Oct 2023 13:14:42 -0400 Subject: [PATCH 4/4] Revert "REVERT ME: Run OSSCheck with closures disabled" This reverts commit bf28469ae17cf8b3b283efff84d83a4d93c4021a. --- tools/oss-check | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tools/oss-check b/tools/oss-check index cb6bc5eb12..a378c95aa3 100755 --- a/tools/oss-check +++ b/tools/oss-check @@ -149,8 +149,8 @@ def setup_repos end if @only_rules_changed && @rules_changed File.open(swiftlint_config, 'w') do |file| - file.puts('only_rules: [redundant_void_return]') - file.puts('redundant_void_return: {include_closures: false}') + file.puts('only_rules:') + file.puts(@rules_changed.map { |rule| " - #{rule}" }) end end Dir.chdir(dir) do @@ -276,10 +276,6 @@ def set_globals end.compact.sort # True iff the only Swift files that were changed are SwiftLint rules, and that number is one or greater @only_rules_changed = !@rules_changed.empty? && @changed_swift_files.count == @rules_changed.count - - # Force OSSCheck to only measure the impact of the `redundant_void_return` rule - @rules_changed = ['redundant_void_return'] - @only_rules_changed = true end def print_rules_changed