diff --git a/CHANGELOG.md b/CHANGELOG.md index a05492c575..5c4bd5528d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,10 @@ [mt00chikin](https://github.com/mt00chikin) [#3173](https://github.com/realm/SwiftLint/issues/3173) +* The `implicit_return` rule now supports the kinds `subscript` and + `initializer` in the `included` configuration list. + [SimplyDanny](https://github.com/SimplyDanny) + * Add `unneeded_override` rule to remove function overrides that only call super. [keith](https://github.com/keith) @@ -85,6 +89,11 @@ * Document `exclude_ranges` option for `number_separator` rule. [SimplyDanny](https://github.com/SimplyDanny) +* Rewrite `implicit_return` rule with SwiftSyntax fixing a few false positives + and false negatives in the process. + [SimplyDanny](https://github.com/SimplyDanny) + [#5161](https://github.com/realm/SwiftLint/issues/5161) + * Make sure `severity` is configurable for `type_contents_order` rule. [SimplyDanny](https://github.com/SimplyDanny) diff --git a/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/ImplicitReturnConfiguration.swift b/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/ImplicitReturnConfiguration.swift index d89dcd9bb0..2ff391d54a 100644 --- a/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/ImplicitReturnConfiguration.swift +++ b/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/ImplicitReturnConfiguration.swift @@ -7,6 +7,8 @@ struct ImplicitReturnConfiguration: SeverityBasedRuleConfiguration, Equatable { case closure case function case getter + case `subscript` + case initializer func asOption() -> OptionType { .symbol(rawValue) } diff --git a/Source/SwiftLintBuiltInRules/Rules/Style/ImplicitReturnRule.swift b/Source/SwiftLintBuiltInRules/Rules/Style/ImplicitReturnRule.swift index 07e325a18d..11840252df 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Style/ImplicitReturnRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Style/ImplicitReturnRule.swift @@ -1,7 +1,6 @@ -import Foundation -import SourceKittenFramework +import SwiftSyntax -struct ImplicitReturnRule: ConfigurationProviderRule, SubstitutionCorrectableRule, OptInRule { +struct ImplicitReturnRule: SwiftSyntaxCorrectableRule, ConfigurationProviderRule, OptInRule { var configuration = ImplicitReturnConfiguration() static let description = RuleDescription( @@ -14,72 +13,78 @@ struct ImplicitReturnRule: ConfigurationProviderRule, SubstitutionCorrectableRul corrections: ImplicitReturnRuleExamples.corrections ) - func validate(file: SwiftLintFile) -> [StyleViolation] { - return violationRanges(in: file).compactMap { - StyleViolation(ruleDescription: Self.description, - severity: configuration.severityConfiguration.severity, - location: Location(file: file, characterOffset: $0.location)) - } + func makeVisitor(file: SwiftLintFile) -> ViolationsSyntaxVisitor { + Visitor(config: configuration) } +} - func substitution(for violationRange: NSRange, in file: SwiftLintFile) -> (NSRange, String)? { - return (violationRange, "") - } +private extension ImplicitReturnRule { + private class Visitor: ViolationsSyntaxVisitor { + private let config: ConfigurationType - func violationRanges(in file: SwiftLintFile) -> [NSRange] { - let pattern = "(?:\\bin|\\{)\\s+(return\\s+)" - let contents = file.stringView + override var skippableDeclarations: [DeclSyntaxProtocol.Type] { [ProtocolDeclSyntax.self] } - return file.matchesAndSyntaxKinds(matching: pattern).compactMap { result, kinds in - let range = result.range - guard kinds == [.keyword, .keyword] || kinds == [.keyword], - let byteRange = contents.NSRangeToByteRange(start: range.location, length: range.length), - case let kinds = file.structureDictionary.kinds(forByteOffset: byteRange.location), - let outerKindString = kinds.lastExcludingBrace()?.kind - else { - return nil - } + init(config: ConfigurationType) { + self.config = config + super.init(viewMode: .sourceAccurate) + } - func isKindIncluded(_ kind: ImplicitReturnConfiguration.ReturnKind) -> Bool { - return self.configuration.isKindIncluded(kind) + override func visitPost(_ node: AccessorDeclSyntax) { + if config.isKindIncluded(.getter), + node.accessorSpecifier.tokenKind == .keyword(.get), + let body = node.body { + collectViolation(in: body.statements) } + } - if let outerKind = SwiftExpressionKind(rawValue: outerKindString), - isKindIncluded(.closure), - [.call, .argument, .closure].contains(outerKind) { - return result.range(at: 1) + override func visitPost(_ node: ClosureExprSyntax) { + if config.isKindIncluded(.closure) { + collectViolation(in: node.statements) } + } - if let outerKind = SwiftDeclarationKind(rawValue: outerKindString), - (isKindIncluded(.function) && SwiftDeclarationKind.functionKinds.contains(outerKind)) || - (isKindIncluded(.getter) && SwiftDeclarationKind.variableKinds.contains(outerKind)) { - return result.range(at: 1) + override func visitPost(_ node: FunctionDeclSyntax) { + if config.isKindIncluded(.function), + let body = node.body { + collectViolation(in: body.statements) } - - return nil } - } -} -private extension Array where Element == (kind: String, byteRange: ByteRange) { - func lastExcludingBrace() -> Element? { - guard SwiftVersion.current >= .fiveDotFour else { - return last + override func visitPost(_ node: InitializerDeclSyntax) { + if config.isKindIncluded(.initializer), + let body = node.body { + collectViolation(in: body.statements) + } } - guard let last else { - return nil + override func visitPost(_ node: PatternBindingSyntax) { + if config.isKindIncluded(.getter), + case let .getter(itemList) = node.accessorBlock?.accessors { + collectViolation(in: itemList) + } } - guard last.kind == "source.lang.swift.stmt.brace", count > 1 else { - return last + override func visitPost(_ node: SubscriptDeclSyntax) { + if config.isKindIncluded(.subscript), + case let .getter(itemList) = node.accessorBlock?.accessors { + collectViolation(in: itemList) + } } - let secondLast = self[endIndex - 2] - if SwiftExpressionKind(rawValue: secondLast.kind) == .closure { - return secondLast + private func collectViolation(in itemList: CodeBlockItemListSyntax) { + guard let returnStmt = itemList.onlyElement?.item.as(ReturnStmtSyntax.self) else { + return + } + let returnKeyword = returnStmt.returnKeyword + violations.append(returnKeyword.positionAfterSkippingLeadingTrivia) + violationCorrections.append( + ViolationCorrection( + start: returnKeyword.positionAfterSkippingLeadingTrivia, + end: returnKeyword.endPositionBeforeTrailingTrivia + .advanced(by: returnStmt.expression == nil ? 0 : 1), + replacement: "" + ) + ) } - - return last } } diff --git a/Source/SwiftLintBuiltInRules/Rules/Style/ImplicitReturnRuleExamples.swift b/Source/SwiftLintBuiltInRules/Rules/Style/ImplicitReturnRuleExamples.swift index 1f5d0d5cfa..eaf6b8cd71 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Style/ImplicitReturnRuleExamples.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Style/ImplicitReturnRuleExamples.swift @@ -1,9 +1,5 @@ -internal struct ImplicitReturnRuleExamples { - internal struct GenericExamples { - static let nonTriggeringExamples = [Example("if foo {\n return 0\n}")] - } - - internal struct ClosureExamples { +struct ImplicitReturnRuleExamples { + struct ClosureExamples { static let nonTriggeringExamples = [ Example("foo.map { $0 + 1 }"), Example("foo.map({ $0 + 1 })"), @@ -18,31 +14,57 @@ internal struct ImplicitReturnRuleExamples { static let triggeringExamples = [ Example(""" foo.map { value in - return value + 1 + ↓return value + 1 } """), Example(""" foo.map { - return $0 + 1 + ↓return $0 + 1 } """), - Example("foo.map({ return $0 + 1})"), + Example("foo.map({ ↓return $0 + 1})"), Example(""" [1, 2].first(where: { - return true + ↓return true }) """) ] static let corrections = [ - Example("foo.map { value in\n return value + 1\n}"): Example("foo.map { value in\n value + 1\n}"), - Example("foo.map {\n return $0 + 1\n}"): Example("foo.map {\n $0 + 1\n}"), - Example("foo.map({ return $0 + 1})"): Example("foo.map({ $0 + 1})"), - Example("[1, 2].first(where: {\n return true })"): Example("[1, 2].first(where: {\n true })") + Example(""" + foo.map { value in + // Important comment + return value + 1 + } + """): Example(""" + foo.map { value in + // Important comment + value + 1 + } + """), + Example(""" + foo.map { + return $0 + 1 + } + """): Example(""" + foo.map { + $0 + 1 + } + """), + Example("foo.map({ return $0 + 1 })"): Example("foo.map({ $0 + 1 })"), + Example(""" + [1, 2].first(where: { + return true + }) + """): Example(""" + [1, 2].first(where: { + true + }) + """) ] } - internal struct FunctionExamples { + struct FunctionExamples { static let nonTriggeringExamples = [ Example(""" func foo() -> Int { @@ -62,30 +84,83 @@ internal struct ImplicitReturnRuleExamples { return nil } } + """), + Example(""" + func f() -> Int { + let i = 4 + return i + } + """), + Example(""" + func f() -> Int { + return 3 + let i = 2 + } + """), + Example(""" + func f() -> Int { + return g() + func g() -> Int { 4 } + } """) ] static let triggeringExamples = [ Example(""" func foo() -> Int { - return 0 + ↓return 0 } """), Example(""" class Foo { - func foo() -> Int { return 0 } + func foo() -> Int { ↓return 0 } } + """), + Example(""" + func f() { ↓return } """) ] static let corrections = [ - Example("func foo() -> Int {\n return 0\n}"): Example("func foo() -> Int {\n 0\n}"), - // swiftlint:disable:next line_length - Example("class Foo {\n func foo() -> Int {\n return 0\n }\n}"): Example("class Foo {\n func foo() -> Int {\n 0\n }\n}") + Example(""" + func foo() -> Int { + return 0 + } + """): Example(""" + func foo() -> Int { + 0 + } + """), + Example(""" + class Foo { + func foo() -> Int { + return 0 + } + } + """): Example(""" + class Foo { + func foo() -> Int { + 0 + } + } + """), + Example(""" + func f() { + // Comment + ↓return + // Another comment + } + """): Example(""" + func f() { + // Comment + \("") + // Another comment + } + """) ] } - internal struct GetterExamples { + struct GetterExamples { static let nonTriggeringExamples = [ Example("var foo: Bool { true }"), Example(""" @@ -107,12 +182,12 @@ internal struct ImplicitReturnRuleExamples { ] static let triggeringExamples = [ - Example("var foo: Bool { return true }"), + Example("var foo: Bool { ↓return true }"), Example(""" class Foo { var bar: Int { get { - return 0 + ↓return 0 } } } @@ -120,7 +195,7 @@ internal struct ImplicitReturnRuleExamples { Example(""" class Foo { static var bar: Int { - return 0 + ↓return 0 } } """) @@ -128,28 +203,159 @@ internal struct ImplicitReturnRuleExamples { static let corrections = [ Example("var foo: Bool { return true }"): Example("var foo: Bool { true }"), - // swiftlint:disable:next line_length - Example("class Foo {\n var bar: Int {\n get {\n return 0\n }\n }\n}"): Example("class Foo {\n var bar: Int {\n get {\n 0\n }\n }\n}") + Example(""" + class Foo { + var bar: Int { + get { + return 0 + } + } + } + """): Example(""" + class Foo { + var bar: Int { + get { + 0 + } + } + } + """) + ] + } + + struct InitializerExamples { + static let nonTriggeringExamples = [ + Example(""" + class C { + let i: Int + init(i: Int) { + if i < 3 { + self.i = 1 + return + } + self.i = 2 + } + } + """), + Example(""" + class C { + init?() { + let i = 1 + return nil + } + } + """) + ] + + static let triggeringExamples = [ + Example(""" + class C { + init() { + ↓return + } + } + """), + Example(""" + class C { + init?() { + ↓return nil + } + } + """) + ] + + static let corrections = [ + Example(""" + class C { + init() { + ↓return + } + } + """): Example(""" + class C { + init() { + \("") + } + } + """), + Example(""" + class C { + init?() { + ↓return nil + } + } + """): Example(""" + class C { + init?() { + nil + } + } + """) + ] + } + + struct SubscriptExamples { + static let nonTriggeringExamples = [ + Example(""" + class C { + subscript(i: Int) -> Int { + let res = i + return res + } + } + """) + ] + + static let triggeringExamples = [ + Example(""" + class C { + subscript(i: Int) -> Int { + ↓return i + } + } + """) + ] + + static let corrections = [ + Example(""" + class C { + subscript(i: Int) -> Int { + ↓return i + } + } + """): Example(""" + class C { + subscript(i: Int) -> Int { + i + } + } + """) ] } - static let nonTriggeringExamples = GenericExamples.nonTriggeringExamples + + static let nonTriggeringExamples = ClosureExamples.nonTriggeringExamples + FunctionExamples.nonTriggeringExamples + - GetterExamples.nonTriggeringExamples + GetterExamples.nonTriggeringExamples + + InitializerExamples.nonTriggeringExamples + + SubscriptExamples.nonTriggeringExamples - static let triggeringExamples = ClosureExamples.triggeringExamples + + static let triggeringExamples = + ClosureExamples.triggeringExamples + FunctionExamples.triggeringExamples + - GetterExamples.triggeringExamples + GetterExamples.triggeringExamples + + InitializerExamples.triggeringExamples + + SubscriptExamples.triggeringExamples static var corrections: [Example: Example] { - let corrections: [[Example: Example]] = [ + [ ClosureExamples.corrections, FunctionExamples.corrections, - GetterExamples.corrections + GetterExamples.corrections, + InitializerExamples.corrections, + SubscriptExamples.corrections ] - - return corrections.reduce(into: [:]) { result, element in + .reduce(into: [:]) { result, element in result.merge(element) { _, _ in preconditionFailure("Duplicate correction in implicit return rule examples.") } diff --git a/Source/SwiftLintCore/Models/Example.swift b/Source/SwiftLintCore/Models/Example.swift index 4d19d6a4ef..0cf669215a 100644 --- a/Source/SwiftLintCore/Models/Example.swift +++ b/Source/SwiftLintCore/Models/Example.swift @@ -167,4 +167,9 @@ public extension Array where Element == Example { func skipDisableCommandTests() -> Self { map { $0.skipDisableCommandTest() } } + + /// Remove all violation markers from the examples. + func removingViolationMarker() -> Self { + map { $0.removingViolationMarkers() } + } } diff --git a/Tests/SwiftLintFrameworkTests/ImplicitReturnConfigurationTests.swift b/Tests/SwiftLintFrameworkTests/ImplicitReturnConfigurationTests.swift index 5181fb6e5c..4d2ae93798 100644 --- a/Tests/SwiftLintFrameworkTests/ImplicitReturnConfigurationTests.swift +++ b/Tests/SwiftLintFrameworkTests/ImplicitReturnConfigurationTests.swift @@ -9,7 +9,9 @@ class ImplicitReturnConfigurationTests: SwiftLintTestCase { "included": [ "closure", "function", - "getter" + "getter", + "initializer", + "subscript" ] ] @@ -17,7 +19,9 @@ class ImplicitReturnConfigurationTests: SwiftLintTestCase { let expectedKinds: Set = Set([ .closure, .function, - .getter + .getter, + .initializer, + .subscript ]) XCTAssertEqual(configuration.severityConfiguration.severity, .error) XCTAssertEqual(configuration.includedKinds, expectedKinds) diff --git a/Tests/SwiftLintFrameworkTests/ImplicitReturnRuleTests.swift b/Tests/SwiftLintFrameworkTests/ImplicitReturnRuleTests.swift index 3dea06d957..7e5870a3ab 100644 --- a/Tests/SwiftLintFrameworkTests/ImplicitReturnRuleTests.swift +++ b/Tests/SwiftLintFrameworkTests/ImplicitReturnRuleTests.swift @@ -2,60 +2,91 @@ class ImplicitReturnRuleTests: SwiftLintTestCase { func testOnlyClosureKindIncluded() { - let nonTriggeringExamples = ImplicitReturnRuleExamples.GenericExamples.nonTriggeringExamples + - ImplicitReturnRuleExamples.ClosureExamples.nonTriggeringExamples + - ImplicitReturnRuleExamples.FunctionExamples.nonTriggeringExamples + - ImplicitReturnRuleExamples.FunctionExamples.triggeringExamples + - ImplicitReturnRuleExamples.GetterExamples.nonTriggeringExamples + - ImplicitReturnRuleExamples.GetterExamples.triggeringExamples - let triggeringExamples = ImplicitReturnRuleExamples.ClosureExamples.triggeringExamples - let corrections = ImplicitReturnRuleExamples.ClosureExamples.corrections + var nonTriggeringExamples = ImplicitReturnRuleExamples.nonTriggeringExamples + + ImplicitReturnRuleExamples.triggeringExamples + nonTriggeringExamples.removeAll( + where: ImplicitReturnRuleExamples.ClosureExamples.triggeringExamples.contains + ) - let description = ImplicitReturnRule.description - .with(nonTriggeringExamples: nonTriggeringExamples) - .with(triggeringExamples: triggeringExamples) - .with(corrections: corrections) - - self.verifyRule(description, returnKind: .closure) + verifySubset( + nonTriggeringExamples: nonTriggeringExamples, + triggeringExamples: ImplicitReturnRuleExamples.ClosureExamples.triggeringExamples, + corrections: ImplicitReturnRuleExamples.ClosureExamples.corrections, + kind: .closure + ) } func testOnlyFunctionKindIncluded() { - let nonTriggeringExamples = ImplicitReturnRuleExamples.GenericExamples.nonTriggeringExamples + - ImplicitReturnRuleExamples.ClosureExamples.nonTriggeringExamples + - ImplicitReturnRuleExamples.ClosureExamples.triggeringExamples + - ImplicitReturnRuleExamples.FunctionExamples.nonTriggeringExamples + - ImplicitReturnRuleExamples.GetterExamples.nonTriggeringExamples + - ImplicitReturnRuleExamples.GetterExamples.triggeringExamples - let triggeringExamples = ImplicitReturnRuleExamples.FunctionExamples.triggeringExamples - let corrections = ImplicitReturnRuleExamples.FunctionExamples.corrections + var nonTriggeringExamples = ImplicitReturnRuleExamples.nonTriggeringExamples + + ImplicitReturnRuleExamples.triggeringExamples + nonTriggeringExamples.removeAll( + where: ImplicitReturnRuleExamples.FunctionExamples.triggeringExamples.contains + ) - let description = ImplicitReturnRule.description - .with(nonTriggeringExamples: nonTriggeringExamples) - .with(triggeringExamples: triggeringExamples) - .with(corrections: corrections) - - self.verifyRule(description, returnKind: .function) + verifySubset( + nonTriggeringExamples: nonTriggeringExamples, + triggeringExamples: ImplicitReturnRuleExamples.FunctionExamples.triggeringExamples, + corrections: ImplicitReturnRuleExamples.FunctionExamples.corrections, + kind: .function + ) } func testOnlyGetterKindIncluded() { - let nonTriggeringExamples = ImplicitReturnRuleExamples.GenericExamples.nonTriggeringExamples + - ImplicitReturnRuleExamples.ClosureExamples.nonTriggeringExamples + - ImplicitReturnRuleExamples.ClosureExamples.triggeringExamples + - ImplicitReturnRuleExamples.FunctionExamples.nonTriggeringExamples + - ImplicitReturnRuleExamples.FunctionExamples.triggeringExamples + - ImplicitReturnRuleExamples.GetterExamples.nonTriggeringExamples - let triggeringExamples = ImplicitReturnRuleExamples.GetterExamples.triggeringExamples - let corrections = ImplicitReturnRuleExamples.GetterExamples.corrections + var nonTriggeringExamples = ImplicitReturnRuleExamples.nonTriggeringExamples + + ImplicitReturnRuleExamples.triggeringExamples + nonTriggeringExamples.removeAll( + where: ImplicitReturnRuleExamples.GetterExamples.triggeringExamples.contains + ) + + verifySubset( + nonTriggeringExamples: nonTriggeringExamples, + triggeringExamples: ImplicitReturnRuleExamples.GetterExamples.triggeringExamples, + corrections: ImplicitReturnRuleExamples.GetterExamples.corrections, + kind: .getter + ) + } + + func testOnlyInitializerKindIncluded() { + var nonTriggeringExamples = ImplicitReturnRuleExamples.nonTriggeringExamples + + ImplicitReturnRuleExamples.triggeringExamples + nonTriggeringExamples.removeAll( + where: ImplicitReturnRuleExamples.InitializerExamples.triggeringExamples.contains + ) + + verifySubset( + nonTriggeringExamples: nonTriggeringExamples, + triggeringExamples: ImplicitReturnRuleExamples.InitializerExamples.triggeringExamples, + corrections: ImplicitReturnRuleExamples.InitializerExamples.corrections, + kind: .initializer + ) + } + + func testOnlySubscriptKindIncluded() { + var nonTriggeringExamples = ImplicitReturnRuleExamples.nonTriggeringExamples + + ImplicitReturnRuleExamples.triggeringExamples + nonTriggeringExamples.removeAll( + where: ImplicitReturnRuleExamples.SubscriptExamples.triggeringExamples.contains + ) + verifySubset( + nonTriggeringExamples: nonTriggeringExamples, + triggeringExamples: ImplicitReturnRuleExamples.SubscriptExamples.triggeringExamples, + corrections: ImplicitReturnRuleExamples.SubscriptExamples.corrections, + kind: .subscript + ) + } + + private func verifySubset( + nonTriggeringExamples: [Example], + triggeringExamples: [Example], + corrections: [Example: Example], + kind: ImplicitReturnConfiguration.ReturnKind + ) { let description = ImplicitReturnRule.description - .with(nonTriggeringExamples: nonTriggeringExamples) + .with(nonTriggeringExamples: nonTriggeringExamples.removingViolationMarker()) .with(triggeringExamples: triggeringExamples) .with(corrections: corrections) - self.verifyRule(description, returnKind: .getter) - } - - private func verifyRule(_ ruleDescription: RuleDescription, returnKind: ImplicitReturnConfiguration.ReturnKind) { - self.verifyRule(ruleDescription, ruleConfiguration: ["included": [returnKind.rawValue]]) + self.verifyRule(description, ruleConfiguration: ["included": [kind.rawValue]]) } }