From eecb44db6ed5bc60bb79faa254d65547f06034e7 Mon Sep 17 00:00:00 2001 From: Leonardo de Sousa Rodrigues <63623604+leonardosrodrigues0@users.noreply.github.com> Date: Thu, 30 Nov 2023 17:35:53 -0300 Subject: [PATCH] Add option to make `unneeded_override` rule affect initializers (#5270) --- CHANGELOG.md | 5 + .../Rules/Lint/UnneededOverrideRule.swift | 215 +++++++++++------- .../UnneededOverrideRuleConfiguration.swift | 11 + .../UnneededOverrideRuleTests.swift | 65 ++++++ 4 files changed, 213 insertions(+), 83 deletions(-) create mode 100644 Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/UnneededOverrideRuleConfiguration.swift create mode 100644 Tests/SwiftLintFrameworkTests/UnneededOverrideRuleTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 505efc28c..8b89d1f0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,11 @@ [SimplyDanny](https://github.com/SimplyDanny) [#4801](https://github.com/realm/SwiftLint/pull/4801) +* Add `affect_initializers` option to allow `unneeded_override` rule + to affect initializers. + [leonardosrodrigues0](https://github.com/leonardosrodrigues0) + [#5265](https://github.com/realm/SwiftLint/issues/5265) + #### Bug Fixes * Ignore overridden functions with default parameters in the `unneeded_override` diff --git a/Source/SwiftLintBuiltInRules/Rules/Lint/UnneededOverrideRule.swift b/Source/SwiftLintBuiltInRules/Rules/Lint/UnneededOverrideRule.swift index 2af243167..5b3e9316a 100644 --- a/Source/SwiftLintBuiltInRules/Rules/Lint/UnneededOverrideRule.swift +++ b/Source/SwiftLintBuiltInRules/Rules/Lint/UnneededOverrideRule.swift @@ -3,7 +3,7 @@ import SwiftSyntaxBuilder @SwiftSyntaxRule(explicitRewriter: true) struct UnneededOverrideRule: Rule { - var configuration = SeverityConfiguration(.warning) + var configuration = UnneededOverrideRuleConfiguration() static let description = RuleDescription( identifier: "unneeded_override", @@ -16,6 +16,137 @@ struct UnneededOverrideRule: Rule { ) } +private extension UnneededOverrideRule { + final class Visitor: ViolationsSyntaxVisitor { + override func visitPost(_ node: FunctionDeclSyntax) { + if node.isUnneededOverride { + self.violations.append(node.positionAfterSkippingLeadingTrivia) + } + } + + override func visitPost(_ node: InitializerDeclSyntax) { + if configuration.affectInits && node.isUnneededOverride { + self.violations.append(node.positionAfterSkippingLeadingTrivia) + } + } + } + + final class Rewriter: ViolationsSyntaxRewriter { + override func visit(_ node: FunctionDeclSyntax) -> DeclSyntax { + guard node.isUnneededOverride else { + return super.visit(node) + } + + return visitUnneededOverride(node) + } + + override func visit(_ node: InitializerDeclSyntax) -> DeclSyntax { + guard node.isUnneededOverride else { + return super.visit(node) + } + + return visitUnneededOverride(node) + } + + private func visitUnneededOverride(_ node: some DeclSyntaxProtocol) -> DeclSyntax { + correctionPositions.append(node.positionAfterSkippingLeadingTrivia) + let expr: DeclSyntax = "" + return expr + .with(\.leadingTrivia, node.leadingTrivia) + .with(\.trailingTrivia, node.trailingTrivia) + } + } +} + +private extension FunctionDeclSyntax { + var isUnneededOverride: Bool { + mayBeUnneededOverride(name: name.text) + } +} + +private extension InitializerDeclSyntax { + var isUnneededOverride: Bool { + guard + optionalMark == nil, // init? can be overridden with init! and vice versa. + !modifiers.contains(keyword: .private) // An initializer can be hidden by overriding it. + else { + return false + } + + return mayBeUnneededOverride(name: "init") + } +} + +private protocol OverridableDecl: WithAttributesSyntax, WithModifiersSyntax { + var signature: FunctionSignatureSyntax { get } + var body: CodeBlockSyntax? { get } +} + +private extension OverridableDecl { + /// Perform checks common to all overridable types of declarations. + func mayBeUnneededOverride(name: String) -> Bool { + guard modifiers.contains(keyword: .override), let statement = body?.statements.onlyElement else { + return false + } + + // Assume having @available changes behavior. + if attributes.contains(attributeNamed: "available") { + return false + } + + guard let call = extractFunctionCallSyntax(statement.item), + let member = call.calledExpression.as(MemberAccessExprSyntax.self), + member.base?.is(SuperExprSyntax.self) == true, + member.declName.baseName.text == name else { + return false + } + + let declParameters = signature.parameterClause.parameters + if declParameters.contains(where: { $0.defaultValue != nil }) { + // Any default parameter might be a change to the super. + return false + } + + // Assume any change in arguments passed means behavior was changed. + let expectedArguments = declParameters.map { + ($0.firstName.text == "_" ? "" : $0.firstName.text, $0.secondName?.text ?? $0.firstName.text) + } + let actualArguments = call.arguments.map { + ($0.label?.text ?? "", $0.expression.as(DeclReferenceExprSyntax.self)?.baseName.text ?? "") + } + + guard expectedArguments.count == actualArguments.count else { + return false + } + + for (lhs, rhs) in zip(expectedArguments, actualArguments) where lhs != rhs { + return false + } + + return true + } +} + +extension FunctionDeclSyntax: OverridableDecl {} + +extension InitializerDeclSyntax: OverridableDecl {} + +/// Extract the function call from other expressions like try / await / return. +/// +/// If this returns a non-super calling function, it will get filtered out later. +private func extractFunctionCallSyntax(_ node: some SyntaxProtocol) -> FunctionCallExprSyntax? { + var syntax = simplify(node) + while let nestedSyntax = syntax { + if nestedSyntax.as(FunctionCallExprSyntax.self) != nil { + break + } + + syntax = simplify(nestedSyntax) + } + + return syntax?.as(FunctionCallExprSyntax.self) +} + private func simplify(_ node: some SyntaxProtocol) -> (any ExprSyntaxProtocol)? { if let expr = node.as(AwaitExprSyntax.self) { return expr.expression @@ -34,85 +165,3 @@ private func simplify(_ node: some SyntaxProtocol) -> (any ExprSyntaxProtocol)? return nil } - -private func extractFunctionCallSyntax(_ node: some SyntaxProtocol) -> FunctionCallExprSyntax? { - // Extract the function call from other expressions like try / await / return. - // If this returns a non-super calling function, it will get filtered out later. - var syntax = simplify(node) - while let nestedSyntax = syntax { - if nestedSyntax.as(FunctionCallExprSyntax.self) != nil { - break - } - - syntax = simplify(nestedSyntax) - } - - return syntax?.as(FunctionCallExprSyntax.self) -} - -private extension UnneededOverrideRule { - final class Visitor: ViolationsSyntaxVisitor { - override func visitPost(_ node: FunctionDeclSyntax) { - if isUnneededOverride(node) { - self.violations.append(node.positionAfterSkippingLeadingTrivia) - } - } - } - - final class Rewriter: ViolationsSyntaxRewriter { - override func visit(_ node: FunctionDeclSyntax) -> DeclSyntax { - if isUnneededOverride(node) { - correctionPositions.append(node.positionAfterSkippingLeadingTrivia) - let expr: DeclSyntax = "" - return expr - .with(\.leadingTrivia, node.leadingTrivia) - .with(\.trailingTrivia, node.trailingTrivia) - } - - return super.visit(node) - } - } -} - -private func isUnneededOverride(_ node: FunctionDeclSyntax) -> Bool { - guard node.modifiers.contains(keyword: .override), let statement = node.body?.statements.onlyElement else { - return false - } - - // Assume having @available changes behavior. - if node.attributes.contains(attributeNamed: "available") { - return false - } - - let overridenFunctionName = node.name.text - guard let call = extractFunctionCallSyntax(statement.item), - let member = call.calledExpression.as(MemberAccessExprSyntax.self), - member.base?.is(SuperExprSyntax.self) == true, - member.declName.baseName.text == overridenFunctionName else { - return false - } - - let declParameters = node.signature.parameterClause.parameters - if declParameters.contains(where: { $0.defaultValue != nil }) { - // Any default parameter might be a change to the super function. - return false - } - - // Assume any change in arguments passed means behavior was changed. - let expectedArguments = declParameters.map { - ($0.firstName.text == "_" ? "" : $0.firstName.text, $0.secondName?.text ?? $0.firstName.text) - } - let actualArguments = call.arguments.map { - ($0.label?.text ?? "", $0.expression.as(DeclReferenceExprSyntax.self)?.baseName.text ?? "") - } - - guard expectedArguments.count == actualArguments.count else { - return false - } - - for (lhs, rhs) in zip(expectedArguments, actualArguments) where lhs != rhs { - return false - } - - return true -} diff --git a/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/UnneededOverrideRuleConfiguration.swift b/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/UnneededOverrideRuleConfiguration.swift new file mode 100644 index 000000000..14d496815 --- /dev/null +++ b/Source/SwiftLintBuiltInRules/Rules/RuleConfigurations/UnneededOverrideRuleConfiguration.swift @@ -0,0 +1,11 @@ +import SwiftLintCore + +@AutoApply +struct UnneededOverrideRuleConfiguration: SeverityBasedRuleConfiguration { + typealias Parent = UnneededOverrideRule + + @ConfigurationElement(key: "severity") + private(set) var severityConfiguration = SeverityConfiguration(.warning) + @ConfigurationElement(key: "affect_initializers") + private(set) var affectInits = false +} diff --git a/Tests/SwiftLintFrameworkTests/UnneededOverrideRuleTests.swift b/Tests/SwiftLintFrameworkTests/UnneededOverrideRuleTests.swift new file mode 100644 index 000000000..0d731cf32 --- /dev/null +++ b/Tests/SwiftLintFrameworkTests/UnneededOverrideRuleTests.swift @@ -0,0 +1,65 @@ +@testable import SwiftLintBuiltInRules + +class UnneededOverrideRuleTests: SwiftLintTestCase { + func testIncludeAffectInits() { + let nonTriggeringExamples = [ + Example(""" + override init() { + super.init(frame: .zero) + } + """), + Example(""" + override init?() { + super.init() + } + """), + Example(""" + override init!() { + super.init() + } + """), + Example(""" + private override init() { + super.init() + } + """) + ] + UnneededOverrideRuleExamples.nonTriggeringExamples + + let triggeringExamples = [ + Example(""" + class Foo { + ↓override init() { + super.init() + } + } + """), + Example(""" + class Foo { + ↓public override init(frame: CGRect) { + super.init(frame: frame) + } + } + """) + ] + + let corrections = [ + Example(""" + class Foo { + ↓override init(frame: CGRect) { + super.init(frame: frame) + } + } + """): Example(""" + class Foo { + } + """) + ] + + let description = UnneededOverrideRule.description + .with(nonTriggeringExamples: nonTriggeringExamples) + .with(triggeringExamples: triggeringExamples) + .with(corrections: corrections) + + verifyRule(description, ruleConfiguration: ["affect_initializers": true]) + } +}