From e4227c5a46f5d6c513344c4409096b771c1790ce Mon Sep 17 00:00:00 2001 From: Marcelo Fabri Date: Sat, 16 Apr 2022 23:32:23 -0700 Subject: [PATCH 1/5] Add back `void_function_in_ternary` rule --- CHANGELOG.md | 4 + .../Models/PrimaryRuleList.swift | 1 + .../VoidFunctionInTernaryConditionRule.swift | 112 ++++++++++++++++++ .../AutomaticRuleTests.generated.swift | 6 + 4 files changed, 123 insertions(+) create mode 100644 Source/SwiftLintFramework/Rules/Idiomatic/VoidFunctionInTernaryConditionRule.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index fe6a0845c1..6c220ba608 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -90,6 +90,10 @@ [Juozas Valancius](https://github.com/juozasvalancius) [#3840](https://github.com/realm/SwiftLint/issues/3840) +* Add back `void_function_in_ternary` opt-in rule to warn against using + a ternary operator to call `Void` functions. + [Marcelo Fabri](https://github.com/marcelofabri) + * Add back `return_value_from_void_function` opt-in rule to warn against using `return ` in a function that returns `Void`. [Marcelo Fabri](https://github.com/marcelofabri) diff --git a/Source/SwiftLintFramework/Models/PrimaryRuleList.swift b/Source/SwiftLintFramework/Models/PrimaryRuleList.swift index a07eb5f5a0..5c68d3f029 100644 --- a/Source/SwiftLintFramework/Models/PrimaryRuleList.swift +++ b/Source/SwiftLintFramework/Models/PrimaryRuleList.swift @@ -210,6 +210,7 @@ public let primaryRuleList = RuleList(rules: [ VerticalWhitespaceClosingBracesRule.self, VerticalWhitespaceOpeningBracesRule.self, VerticalWhitespaceRule.self, + VoidFunctionInTernaryConditionRule.self, VoidReturnRule.self, WeakDelegateRule.self, XCTFailMessageRule.self, diff --git a/Source/SwiftLintFramework/Rules/Idiomatic/VoidFunctionInTernaryConditionRule.swift b/Source/SwiftLintFramework/Rules/Idiomatic/VoidFunctionInTernaryConditionRule.swift new file mode 100644 index 0000000000..d14722a344 --- /dev/null +++ b/Source/SwiftLintFramework/Rules/Idiomatic/VoidFunctionInTernaryConditionRule.swift @@ -0,0 +1,112 @@ +import SourceKittenFramework +import SwiftSyntax + +public struct VoidFunctionInTernaryConditionRule: ConfigurationProviderRule, AutomaticTestableRule { + public var configuration = SeverityConfiguration(.warning) + + public init() {} + + public static let description = RuleDescription( + identifier: "void_function_in_ternary", + name: "Void Function in Ternary", + description: "Using ternary to call Void functions should be avoided.", + kind: .idiomatic, + minSwiftVersion: .fiveDotOne, + nonTriggeringExamples: [ + Example("let result = success ? foo() : bar()"), + Example(""" + if success { + askQuestion() + } else { + exit() + } + """), + Example(""" + var price: Double { + return hasDiscount ? calculatePriceWithDiscount() : calculateRegularPrice() + } + """), + Example("foo(x == 2 ? a() : b())"), + Example(""" + chevronView.image = collapsed ? .icon(.mediumChevronDown) : .icon(.mediumChevronUp) + """), + Example(""" + array.map { elem in + elem.isEmpty() ? .emptyValue() : .number(elem) + } + """) + ], + triggeringExamples: [ + Example("success ↓? askQuestion() : exit()"), + Example(""" + perform { elem in + elem.isEmpty() ↓? .emptyValue() : .number(elem) + return 1 + } + """), + Example(""" + DispatchQueue.main.async { + self.sectionViewModels[section].collapsed.toggle() + self.sectionViewModels[section].collapsed + ↓? self.tableView.deleteRows(at: [IndexPath(row: 0, section: section)], with: .automatic) + : self.tableView.insertRows(at: [IndexPath(row: 0, section: section)], with: .automatic) + self.tableView.scrollToRow(at: IndexPath(row: NSNotFound, section: section), at: .top, animated: true) + } + """) + ] + ) + + public func validate(file: SwiftLintFile) -> [StyleViolation] { + guard let syntaxTree = file.syntaxTree else { + return [] + } + + let visitor = VoidFunctionInTernaryConditionVisitor() + visitor.walk(syntaxTree) + return visitor.violations(for: self, in: file) + } +} + +private class VoidFunctionInTernaryConditionVisitor: SyntaxVisitor { + private var positions = [AbsolutePosition]() + + override func visitPost(_ node: TernaryExprSyntax) { + guard node.firstChoice.is(FunctionCallExprSyntax.self), + node.secondChoice.is(FunctionCallExprSyntax.self), + let parent = node.parent?.as(ExprListSyntax.self), + !parent.containsAssignment, + let grandparent = parent.parent, + grandparent.is(SequenceExprSyntax.self), + let blockItem = grandparent.parent?.as(CodeBlockItemSyntax.self), + !blockItem.isClosureImplictReturn else { + return + } + + positions.append(node.questionMark.positionAfterSkippingLeadingTrivia) + } + + func violations(for rule: VoidFunctionInTernaryConditionRule, in file: SwiftLintFile) -> [StyleViolation] { + return positions.map { position in + StyleViolation(ruleDescription: type(of: rule).description, + severity: rule.configuration.severity, + location: Location(file: file, byteOffset: ByteCount(position))) + } + } +} + +private extension ExprListSyntax { + var containsAssignment: Bool { + return children.contains(where: { $0.is(AssignmentExprSyntax.self) }) + } +} + +private extension CodeBlockItemSyntax { + var isClosureImplictReturn: Bool { + guard let parent = parent?.as(CodeBlockItemListSyntax.self), + let grandparent = parent.parent else { + return false + } + + return parent.children.count == 1 && grandparent.is(ClosureExprSyntax.self) + } +} diff --git a/Tests/SwiftLintFrameworkTests/AutomaticRuleTests.generated.swift b/Tests/SwiftLintFrameworkTests/AutomaticRuleTests.generated.swift index c70291fea4..6305caadf6 100644 --- a/Tests/SwiftLintFrameworkTests/AutomaticRuleTests.generated.swift +++ b/Tests/SwiftLintFrameworkTests/AutomaticRuleTests.generated.swift @@ -923,6 +923,12 @@ class VerticalWhitespaceOpeningBracesRuleTests: XCTestCase { } } +class VoidFunctionInTernaryConditionRuleTests: XCTestCase { + func testWithDefaultConfiguration() { + verifyRule(VoidFunctionInTernaryConditionRule.description) + } +} + class VoidReturnRuleTests: XCTestCase { func testWithDefaultConfiguration() { verifyRule(VoidReturnRule.description) From aea1748f7589d58333d61589178a774fcf60c2cd Mon Sep 17 00:00:00 2001 From: Marcelo Fabri Date: Sun, 8 May 2022 19:37:34 -0700 Subject: [PATCH 2/5] Handle implicit returns in getters and subscripts --- .../VoidFunctionInTernaryConditionRule.swift | 137 +++++++++++++++++- 1 file changed, 130 insertions(+), 7 deletions(-) diff --git a/Source/SwiftLintFramework/Rules/Idiomatic/VoidFunctionInTernaryConditionRule.swift b/Source/SwiftLintFramework/Rules/Idiomatic/VoidFunctionInTernaryConditionRule.swift index d14722a344..ca9a22e454 100644 --- a/Source/SwiftLintFramework/Rules/Idiomatic/VoidFunctionInTernaryConditionRule.swift +++ b/Source/SwiftLintFramework/Rules/Idiomatic/VoidFunctionInTernaryConditionRule.swift @@ -34,6 +34,40 @@ public struct VoidFunctionInTernaryConditionRule: ConfigurationProviderRule, Aut array.map { elem in elem.isEmpty() ? .emptyValue() : .number(elem) } + """), + Example(""" + func compute(data: [Int]) -> Int { + data.isEmpty ? 0 : expensiveComputation(data) + } + """), + Example(""" + func get() -> Int { 1 } + func get(from elements: [Int]) -> Int { elements[0] } + func f(elements: [Int]) -> Int { + elements.isEmpty ? get() : get(from: elements) + } + """), + Example(""" + var value: Int { + mode == .fast ? fastComputation() : expensiveComputation() + } + """), + Example(""" + var value: Int { + get { + mode == .fast ? fastComputation() : expensiveComputation() + } + } + """), + Example(""" + subscript(index: Int) -> Int { + get { + index == 0 ? defaultValue() : compute(index) + } + """), + Example(""" + subscript(index: Int) -> Int { + index == 0 ? defaultValue() : compute(index) """) ], triggeringExamples: [ @@ -52,18 +86,41 @@ public struct VoidFunctionInTernaryConditionRule: ConfigurationProviderRule, Aut : self.tableView.insertRows(at: [IndexPath(row: 0, section: section)], with: .automatic) self.tableView.scrollToRow(at: IndexPath(row: NSNotFound, section: section), at: .top, animated: true) } + """), + Example(""" + subscript(index: Int) -> Int { + index == 0 ↓? something() : somethingElse(index) + return index + """), + Example(""" + var value: Int { + mode == .fast ↓? something() : somethingElse() + return 0 + } + """), + Example(""" + var value: Int { + get { + mode == .fast ↓? something() : somethingElse() + return 0 + } + } + """), + Example(""" + subscript(index: Int) -> Int { + get { + index == 0 ↓? something() : somethingElse(index) + return index + } """) ] ) public func validate(file: SwiftLintFile) -> [StyleViolation] { - guard let syntaxTree = file.syntaxTree else { - return [] - } - let visitor = VoidFunctionInTernaryConditionVisitor() - visitor.walk(syntaxTree) - return visitor.violations(for: self, in: file) + return visitor.walk(file: file) { visitor in + visitor.violations(for: self, in: file) + } } } @@ -78,7 +135,7 @@ private class VoidFunctionInTernaryConditionVisitor: SyntaxVisitor { let grandparent = parent.parent, grandparent.is(SequenceExprSyntax.self), let blockItem = grandparent.parent?.as(CodeBlockItemSyntax.self), - !blockItem.isClosureImplictReturn else { + !blockItem.isImplicitReturn else { return } @@ -101,6 +158,12 @@ private extension ExprListSyntax { } private extension CodeBlockItemSyntax { + var isImplicitReturn: Bool { + isClosureImplictReturn || isFunctionImplicitReturn || + isVariableImplicitReturn || isSubscriptImplicitReturn || + isAcessorImplicitReturn + } + var isClosureImplictReturn: Bool { guard let parent = parent?.as(CodeBlockItemListSyntax.self), let grandparent = parent.parent else { @@ -109,4 +172,64 @@ private extension CodeBlockItemSyntax { return parent.children.count == 1 && grandparent.is(ClosureExprSyntax.self) } + + var isFunctionImplicitReturn: Bool { + guard let parent = parent?.as(CodeBlockItemListSyntax.self), + let functionDecl = parent.parent?.parent?.as(FunctionDeclSyntax.self) else { + return false + } + + return parent.children.count == 1 && functionDecl.signature.allowsImplicitReturns + } + + var isVariableImplicitReturn: Bool { + guard let parent = parent?.as(CodeBlockItemListSyntax.self) else { + return false + } + + let isVariableDecl = parent.parent?.parent?.as(PatternBindingSyntax.self) != nil + return parent.children.count == 1 && isVariableDecl + } + + var isSubscriptImplicitReturn: Bool { + guard let parent = parent?.as(CodeBlockItemListSyntax.self), + let subscriptDecl = parent.parent?.parent?.as(SubscriptDeclSyntax.self) else { + return false + } + + return parent.children.count == 1 && subscriptDecl.allowsImplicitReturns + } + + var isAcessorImplicitReturn: Bool { + guard let parent = parent?.as(CodeBlockItemListSyntax.self), + parent.parent?.parent?.as(AccessorDeclSyntax.self) != nil else { + return false + } + + return parent.children.count == 1 + } +} + +private extension FunctionSignatureSyntax { + var allowsImplicitReturns: Bool { + output?.allowsImplicitReturns ?? false + } +} + +private extension SubscriptDeclSyntax { + var allowsImplicitReturns: Bool { + result.allowsImplicitReturns + } +} + +private extension ReturnClauseSyntax { + var allowsImplicitReturns: Bool { + if let simpleType = returnType.as(SimpleTypeIdentifierSyntax.self) { + return simpleType.name.text != "Void" && simpleType.name.text != "Never" + } else if let tupleType = returnType.as(TupleTypeSyntax.self) { + return !tupleType.elements.isEmpty + } else { + return true + } + } } From 906bd866bdf8a56de39aa1b23cb7f159c830939e Mon Sep 17 00:00:00 2001 From: Marcelo Fabri Date: Sun, 8 May 2022 19:38:16 -0700 Subject: [PATCH 3/5] Fix changelog after rebase --- CHANGELOG.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c220ba608..dc8a19ae9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,8 @@ * Support arrays for the `included` and `excluded` options when defining a custom rule. +* Add back `void_function_in_ternary` opt-in rule to warn against using + a ternary operator to call `Void` functions. [Marcelo Fabri](https://github.com/marcelofabri) #### Bug Fixes @@ -90,10 +92,6 @@ [Juozas Valancius](https://github.com/juozasvalancius) [#3840](https://github.com/realm/SwiftLint/issues/3840) -* Add back `void_function_in_ternary` opt-in rule to warn against using - a ternary operator to call `Void` functions. - [Marcelo Fabri](https://github.com/marcelofabri) - * Add back `return_value_from_void_function` opt-in rule to warn against using `return ` in a function that returns `Void`. [Marcelo Fabri](https://github.com/marcelofabri) From 78891a96399b011a74bb0ea9b2756120ae2f9d93 Mon Sep 17 00:00:00 2001 From: Marcelo Fabri Date: Mon, 27 Jun 2022 01:21:45 -0700 Subject: [PATCH 4/5] Fix changelog after rebase --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc8a19ae9b..196d9279b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,8 @@ * Support arrays for the `included` and `excluded` options when defining a custom rule. + [Marcelo Fabri](https://github.com/marcelofabri) + * Add back `void_function_in_ternary` opt-in rule to warn against using a ternary operator to call `Void` functions. [Marcelo Fabri](https://github.com/marcelofabri) From 14c07d3b5a894e944dd8a282fc08cc209a6f0e58 Mon Sep 17 00:00:00 2001 From: Marcelo Fabri Date: Mon, 27 Jun 2022 01:23:44 -0700 Subject: [PATCH 5/5] Remove duplicated example --- .../Idiomatic/VoidFunctionInTernaryConditionRule.swift | 7 ------- 1 file changed, 7 deletions(-) diff --git a/Source/SwiftLintFramework/Rules/Idiomatic/VoidFunctionInTernaryConditionRule.swift b/Source/SwiftLintFramework/Rules/Idiomatic/VoidFunctionInTernaryConditionRule.swift index ca9a22e454..b30168c803 100644 --- a/Source/SwiftLintFramework/Rules/Idiomatic/VoidFunctionInTernaryConditionRule.swift +++ b/Source/SwiftLintFramework/Rules/Idiomatic/VoidFunctionInTernaryConditionRule.swift @@ -41,13 +41,6 @@ public struct VoidFunctionInTernaryConditionRule: ConfigurationProviderRule, Aut } """), Example(""" - func get() -> Int { 1 } - func get(from elements: [Int]) -> Int { elements[0] } - func f(elements: [Int]) -> Int { - elements.isEmpty ? get() : get(from: elements) - } - """), - Example(""" var value: Int { mode == .fast ? fastComputation() : expensiveComputation() }