From 45a03dec20ac1beb92382098564d3f8ed98dec90 Mon Sep 17 00:00:00 2001 From: Marcelo Fabri Date: Mon, 27 Jun 2022 09:41:52 -0700 Subject: [PATCH] Add back `void_function_in_ternary` rule (#3956) --- CHANGELOG.md | 4 + .../Models/PrimaryRuleList.swift | 1 + .../VoidFunctionInTernaryConditionRule.swift | 228 ++++++++++++++++++ .../AutomaticRuleTests.generated.swift | 6 + 4 files changed, 239 insertions(+) create mode 100644 Source/SwiftLintFramework/Rules/Idiomatic/VoidFunctionInTernaryConditionRule.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index fe6a0845c1..196d9279b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,10 @@ 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) + #### Bug Fixes * Ignore array types in `syntactic_sugar` rule if their associated `Index` is 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..b30168c803 --- /dev/null +++ b/Source/SwiftLintFramework/Rules/Idiomatic/VoidFunctionInTernaryConditionRule.swift @@ -0,0 +1,228 @@ +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) + } + """), + Example(""" + func compute(data: [Int]) -> Int { + data.isEmpty ? 0 : expensiveComputation(data) + } + """), + 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: [ + 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) + } + """), + 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] { + let visitor = VoidFunctionInTernaryConditionVisitor() + return visitor.walk(file: file) { visitor in + 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.isImplicitReturn 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 isImplicitReturn: Bool { + isClosureImplictReturn || isFunctionImplicitReturn || + isVariableImplicitReturn || isSubscriptImplicitReturn || + isAcessorImplicitReturn + } + + 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) + } + + 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 + } + } +} 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)