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 new redundant_sendable rule #5902

Merged
merged 1 commit into from
Dec 25, 2024
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 @@ -39,6 +39,11 @@
[dk-talks](https://github.com/dk-talks)
[SimplyDanny](https://github.com/SimplyDanny)

* Add new `redundant_sendable` rule that triggers on `Sendable` conformances of
types that are implicitly already `Sendable` due to being actor-isolated. It
is enabled by default.
[SimplyDanny](https://github.com/SimplyDanny)

#### Bug Fixes

* Ignore super calls with trailing closures in `unneeded_override` rule.
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 @@ -176,6 +176,7 @@ public let builtInRules: [any Rule.Type] = [
RedundantObjcAttributeRule.self,
RedundantOptionalInitializationRule.self,
RedundantSelfInClosureRule.self,
RedundantSendableRule.self,
RedundantSetAccessControlRule.self,
RedundantStringEnumValueRule.self,
RedundantTypeAnnotationRule.self,
Expand Down
130 changes: 130 additions & 0 deletions Source/SwiftLintBuiltInRules/Rules/Lint/RedundantSendableRule.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
import SwiftSyntax

@SwiftSyntaxRule(explicitRewriter: true)
struct RedundantSendableRule: Rule {
var configuration = RedundantSendableConfiguration()

static let description = RuleDescription(
identifier: "redundant_sendable",
name: "Redundant Sendable",
description: "Sendable conformance is redundant on an actor-isolated type",
kind: .lint,
nonTriggeringExamples: [
Example("struct S: Sendable {}"),
Example("class C: Sendable {}"),
Example("actor A {}"),
Example("@MainActor struct S {}"),
Example("@MyActor enum E: Sendable { case a }"),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should this last one not trigger?

],
triggeringExamples: [
Example("@MainActor struct ↓S: Sendable {}"),
Example("actor ↓A: Sendable {}"),
Example("@MyActor enum ↓E: Sendable { case a }", configuration: ["global_actors": ["MyActor"]]),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I think I get it, the actor has to be defined first?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the rule only operates on syntax-level, it needs to have global actors defined by name. There's no other way to know if the attribute is an actor or something else. @MainActor is clear and perhaps also @...Actor but that's too vague. Global actors aren't something that changes often, so people can just add their names to the configuration. It's always a trade-off.

],
corrections: [
Example("@MainActor struct S: Sendable {}"):
Example("@MainActor struct S {}"),
Example("actor A: Sendable {}"):
Example("actor A {}"),
Example("@MyActor enum E: Sendable { case a }", configuration: ["global_actors": ["MyActor"]]):
Example("@MyActor enum E { case a }"),
Example("actor A: B, Sendable, C {}"):
Example("actor A: B, C {}"),
]
)
}

private extension RedundantSendableRule {
final class Visitor: ViolationsSyntaxVisitor<ConfigurationType> {
override func visitPost(_ node: ActorDeclSyntax) {
if node.conformsToSendable {
violations.append(at: node.name.positionAfterSkippingLeadingTrivia)
}
}

override func visitPost(_ node: ClassDeclSyntax) {
collectViolations(in: node)
}

override func visitPost(_ node: EnumDeclSyntax) {
collectViolations(in: node)
}

override func visitPost(_ node: ProtocolDeclSyntax) {
collectViolations(in: node)
}

override func visitPost(_ node: StructDeclSyntax) {
collectViolations(in: node)
}

private func collectViolations(in decl: some DeclGroupSyntax & NamedDeclSyntax) {
if decl.conformsToSendable, decl.isIsolatedToActor(actors: configuration.globalActors) {
violations.append(at: decl.name.positionAfterSkippingLeadingTrivia)
}
}
}

final class Rewriter: ViolationsSyntaxRewriter<ConfigurationType> {
override func visit(_ node: ActorDeclSyntax) -> DeclSyntax {
if node.conformsToSendable {
correctionPositions.append(node.name.positionAfterSkippingLeadingTrivia)
return super.visit(node.withoutSendable)
}
return super.visit(node)
}

override func visit(_ node: ClassDeclSyntax) -> DeclSyntax {
super.visit(removeRedundantSendable(from: node))
}

override func visit(_ node: EnumDeclSyntax) -> DeclSyntax {
super.visit(removeRedundantSendable(from: node))
}

override func visit(_ node: ProtocolDeclSyntax) -> DeclSyntax {
super.visit(removeRedundantSendable(from: node))
}

override func visit(_ node: StructDeclSyntax) -> DeclSyntax {
super.visit(removeRedundantSendable(from: node))
}

private func removeRedundantSendable<T: DeclGroupSyntax & NamedDeclSyntax>(from decl: T) -> T {
if decl.conformsToSendable, decl.isIsolatedToActor(actors: configuration.globalActors) {
correctionPositions.append(decl.name.positionAfterSkippingLeadingTrivia)
return decl.withoutSendable
}
return decl
}
}
}

private extension DeclGroupSyntax where Self: NamedDeclSyntax {
var conformsToSendable: Bool {
inheritanceClause?.inheritedTypes.contains(where: \.isSendable) == true
}

func isIsolatedToActor(actors: Set<String>) -> Bool {
attributes.contains(attributeNamed: "MainActor") || actors.contains { attributes.contains(attributeNamed: $0) }
}

var withoutSendable: Self {
guard let inheritanceClause else {
return self
}
let inheritedTypes = inheritanceClause.inheritedTypes.filter { !$0.isSendable }
if inheritedTypes.isEmpty {
return with(\.inheritanceClause, nil)
.with(\.name.trailingTrivia, inheritanceClause.leadingTrivia + inheritanceClause.trailingTrivia)
}
return with(\.inheritanceClause, inheritanceClause
.with(\.inheritedTypes, inheritedTypes))
}
}

private extension InheritedTypeSyntax {
var isSendable: Bool {
type.as(IdentifierTypeSyntax.self)?.name.text == "Sendable"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import SwiftLintCore

@AutoConfigParser
struct RedundantSendableConfiguration: SeverityBasedRuleConfiguration {
typealias Parent = RedundantSendableRule

@ConfigurationElement(key: "severity")
private(set) var severityConfiguration = SeverityConfiguration<Parent>(.warning)
@ConfigurationElement(key: "global_actors")
private(set) var globalActors = Set<String>()
}
6 changes: 6 additions & 0 deletions Tests/GeneratedTests/GeneratedTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1045,6 +1045,12 @@ final class RedundantSelfInClosureRuleGeneratedTests: SwiftLintTestCase {
}
}

final class RedundantSendableRuleGeneratedTests: SwiftLintTestCase {
func testWithDefaultConfiguration() {
verifyRule(RedundantSendableRule.description)
}
}

final class RedundantSetAccessControlRuleGeneratedTests: SwiftLintTestCase {
func testWithDefaultConfiguration() {
verifyRule(RedundantSetAccessControlRule.description)
Expand Down
3 changes: 3 additions & 0 deletions Tests/IntegrationTests/default_rule_configurations.yml
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,9 @@ redundant_optional_initialization:
severity: warning
redundant_self_in_closure:
severity: warning
redundant_sendable:
severity: warning
global_actors: []
redundant_set_access_control:
severity: warning
redundant_string_enum_value:
Expand Down