Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unneeded_override rule #5139

Merged
merged 1 commit into from
Jul 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@
* Rewrite `control_statement` rule using SwiftSyntax.
[SimplyDanny](https://github.com/SimplyDanny)

* Add `unneeded_override` rule to remove function overrides that only
call super.
[keith](https://github.com/keith)
[5139](https://github.com/realm/SwiftLint/pull/5139)

#### Bug Fixes

* Fix false positive in `control_statement` rule that triggered on conditions
Expand Down
1 change: 1 addition & 0 deletions Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ public let builtInRules: [Rule.Type] = [
UnavailableFunctionRule.self,
UnhandledThrowingTaskRule.self,
UnneededBreakInSwitchRule.self,
UnneededOverrideRule.self,
UnneededParenthesesInClosureArgumentRule.self,
UnneededSynthesizedInitializerRule.self,
UnownedVariableCaptureRule.self,
Expand Down
130 changes: 130 additions & 0 deletions Source/SwiftLintBuiltInRules/Rules/Lint/UnneededOverrideRule.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
import SwiftSyntax
import SwiftSyntaxBuilder

struct UnneededOverrideRule: ConfigurationProviderRule, SwiftSyntaxCorrectableRule {
var configuration = SeverityConfiguration<Self>(.warning)

static let description = RuleDescription(
identifier: "unneeded_override",
name: "Unneeded Overridden Functions",
description: "Remove overridden functions that don't do anything except call their super",
kind: .lint,
nonTriggeringExamples: UnneededOverrideRuleExamples.nonTriggeringExamples,
triggeringExamples: UnneededOverrideRuleExamples.triggeringExamples,
corrections: UnneededOverrideRuleExamples.corrections
)

func makeVisitor(file: SwiftLintFile) -> ViolationsSyntaxVisitor {
Visitor(viewMode: .sourceAccurate)
}

func makeRewriter(file: SwiftLintFile) -> ViolationsSyntaxRewriter? {
Rewriter(
locationConverter: file.locationConverter,
disabledRegions: disabledRegions(file: file)
)
}
}

private func simplify(_ node: SyntaxProtocol) -> ExprSyntaxProtocol? {
if let expr = node.as(AwaitExprSyntax.self) {
return expr.expression
} else if let expr = node.as(TryExprSyntax.self) {
// Assume using try! / try? changes behavior
if expr.questionOrExclamationMark != nil {
return nil
}

return expr.expression
} else if let expr = node.as(FunctionCallExprSyntax.self) {
return expr
} else if let stmt = node.as(ReturnStmtSyntax.self) {
return stmt.expression
}

return nil
}

private func extractFunctionCallSyntax(_ node: SyntaxProtocol) -> FunctionCallExprSyntax? {
// Extract the function call from other expressions like try / await / return.
// If this returns a non-super calling function that 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 final class Visitor: ViolationsSyntaxVisitor {
override func visitPost(_ node: FunctionDeclSyntax) {
if isUnneededOverride(node) {
self.violations.append(node.positionAfterSkippingLeadingTrivia)
}
}
}

private final class Rewriter: SyntaxRewriter, ViolationsSyntaxRewriter {
var correctionPositions: [AbsolutePosition] = []
let locationConverter: SourceLocationConverter
let disabledRegions: [SourceRange]

init(locationConverter: SourceLocationConverter, disabledRegions: [SourceRange]) {
self.locationConverter = locationConverter
self.disabledRegions = disabledRegions
}

override func visit(_ node: FunctionDeclSyntax) -> DeclSyntax {
if isUnneededOverride(node) &&
!node.isContainedIn(regions: disabledRegions, locationConverter: locationConverter) {
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.containsOverride, 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.identifier.text
guard let call = extractFunctionCallSyntax(statement.item),
let member = call.calledExpression.as(MemberAccessExprSyntax.self),
member.base?.is(SuperRefExprSyntax.self) == true,
member.name.text == overridenFunctionName else {
return false
}

// Assume any change in arguments passed means behavior was changed
let expectedArguments = node.signature.input.parameterList.map {
($0.firstName.text == "_" ? "" : $0.firstName.text, $0.secondName?.text ?? $0.firstName.text)
}
let actualArguments = call.argumentList.map {
($0.label?.text ?? "", $0.expression.as(IdentifierExprSyntax.self)?.identifier.text ?? "")
}

guard expectedArguments.count == actualArguments.count else {
return false
}

for (lhs, rhs) in zip(expectedArguments, actualArguments) where lhs != rhs {
return false
}

return true
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
struct UnneededOverrideRuleExamples {
static let nonTriggeringExamples = [
Example("""
class Foo {
override func bar() {
super.bar()
print("hi")
}
}
"""),
Example("""
class Foo {
@available(*, unavailable)
override func bar() {
super.bar()
}
}
"""),
Example("""
class Foo {
override func bar() {
super.bar()
super.bar()
}
}
"""),
Example("""
class Foo {
override func bar() throws {
// Doing a different variation of 'try' changes behavior
try! super.bar()
}
}
"""),
Example("""
class Foo {
override func bar() throws {
// Doing a different variation of 'try' changes behavior
try? super.bar()
}
}
"""),
Example("""
class Foo {
override func bar() async throws {
// Doing a different variation of 'try' changes behavior
await try! super.bar()
}
}
"""),
Example("""
class Foo {
override func bar(arg: Bool) {
// Flipping the argument changes behavior
super.bar(arg: !arg)
}
}
"""),
Example("""
class Foo {
override func bar(_ arg: Int) {
// Changing the argument changes behavior
super.bar(arg + 1)
}
}
"""),
Example("""
class Foo {
override func bar(arg: Int) {
// Changing the argument changes behavior
super.bar(arg: arg.var)
}
}
"""),
Example("""
class Foo {
override func bar(_ arg: Int) {
// Not passing arguments because they have default values changes behavior
super.bar()
}
}
"""),
Example("""
class Foo {
override func bar(arg: Int, _ arg3: Bool) {
// Calling a super function with different argument labels changes behavior
super.bar(arg2: arg, arg3: arg3)
}
}
"""),
Example("""
class Foo {
override func bar(animated: Bool, completion: () -> Void) {
super.bar(animated: animated) {
// This likely changes behavior
}
}
}
"""),
Example("""
class Foo {
override func bar(animated: Bool, completion: () -> Void) {
super.bar(animated: animated, completion: {
// This likely changes behavior
})
}
}
""")
]

static let triggeringExamples = [
Example("""
class Foo {
↓override func bar() {
super.bar()
}
}
"""),
Example("""
class Foo {
↓override func bar() {
return super.bar()
}
}
"""),
Example("""
class Foo {
↓override func bar() {
super.bar()
// comments don't affect this
}
}
"""),
Example("""
class Foo {
↓override func bar() async {
await super.bar()
}
}
"""),
Example("""
class Foo {
↓override func bar() throws {
try super.bar()
// comments don't affect this
}
}
"""),
Example("""
class Foo {
↓override func bar(arg: Bool) throws {
try super.bar(arg: arg)
}
}
"""),
Example("""
class Foo {
↓override func bar(animated: Bool, completion: () -> Void) {
super.bar(animated: animated, completion: completion)
}
}
""")
]

static let corrections = [
Example("""
class Foo {
↓override func bar(animated: Bool, completion: () -> Void) {
super.bar(animated: animated, completion: completion)
}
}
"""): Example("""
class Foo {
}
"""),
Example("""
class Foo {
↓override func bar() {
super.bar()
}
}
"""): Example("""
class Foo {
}
"""),
Example("""
class Foo {
↓override func bar() {
super.bar()
}

// This is another function
func baz() {}
}
"""): Example("""
class Foo {

// This is another function
func baz() {}
}
""")
]
}
6 changes: 6 additions & 0 deletions Tests/GeneratedTests/GeneratedTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1232,6 +1232,12 @@ class UnneededBreakInSwitchRuleGeneratedTests: SwiftLintTestCase {
}
}

class UnneededOverrideRuleGeneratedTests: SwiftLintTestCase {
func testWithDefaultConfiguration() {
verifyRule(UnneededOverrideRule.description)
}
}

class UnneededParenthesesInClosureArgumentRuleGeneratedTests: SwiftLintTestCase {
func testWithDefaultConfiguration() {
verifyRule(UnneededParenthesesInClosureArgumentRule.description)
Expand Down