Add new redundant_sendable rule
#5902
Conversation
Generated by 🚫 Danger |
|
@mattmassicotte: I'd appreciate your opinion on the test cases/examples and the rule's description (which is also the message shown for a violation). Admittedly, I'm not super happy with the wording. |
SimplyDanny
force-pushed
the
redundant-sendable
branch
from
59dd9c1 to
c91baed
Compare
| Example("class C: Sendable {}"), | ||
| Example("actor A {}"), | ||
| Example("@MainActor struct S {}"), | ||
| Example("@MyActor enum E: Sendable { case a }"), |
There was a problem hiding this comment.
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"]]), |
There was a problem hiding this comment.
Oh I think I get it, the actor has to be defined first?
There was a problem hiding this comment.
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.
|
As for the description, I think you can simplify things a little if you want:
|
|
At the moment, this rule needs to be enabled explicitly. Feels like it's clear and safe enough to enable it by default though. |
SimplyDanny
force-pushed
the
redundant-sendable
branch
from
c91baed to
7992cdf
Compare
SimplyDanny
force-pushed
the
redundant-sendable
branch
from
7992cdf to
587a922
Compare
Inspired by https://www.massicotte.org/step-by-step-reading-from-storage.