Add support for MainActor #4
Conversation
WalkthroughThe changes introduce support for main actor isolation in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Macro
participant Builders
participant GeneratedType
User->>Macro: Annotates class with @MainActor and @Publishable
Macro->>Builders: Detects @MainActor, sets mainActor = true
Builders->>GeneratedType: Generate PropertyPublisher & Registrar with @MainActor
GeneratedType-->>User: Provides main-actor-isolated publisher and registrar
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Sources/PublishableMacros/Builders/PropertyPublisherDeclBuilder.swift (1)
21-21: Remove unnecessary SwiftLint disable command.The
type_contents_orderdisable command is superfluous as indicated by the static analysis warning.- func build() -> [DeclSyntax] { // swiftlint:disable:this type_contents_order + func build() -> [DeclSyntax] {Sources/Publishable/Registrars/PublishableObservationRegistrar.swift (2)
77-123: Solid implementation with minor formatting improvements needed.The extension correctly handles MainActor isolation requirements. The
withMutationimplementation properly usesMainActor.assumeIsolatedwith a clear explanatory comment about the isolation context.Consider breaking up the long lines for better readability:
- try MainActor.assumeIsolated { [unchecked = UncheckedSendable(wrappedValue: (self, object, keyPath, mutation))] in - unchecked.wrappedValue.1.publisher.beginModifications() - let result = try unchecked.wrappedValue.0.underlying.withMutation(of: unchecked.wrappedValue.1, keyPath: unchecked.wrappedValue.2, unchecked.wrappedValue.3) - unchecked.wrappedValue.0.publish(unchecked.wrappedValue.1, keyPath: unchecked.wrappedValue.2) - unchecked.wrappedValue.1.publisher.endModifications() + try MainActor.assumeIsolated { [ + unchecked = UncheckedSendable( + wrappedValue: (self, object, keyPath, mutation) + ) + ] in + let (registrar, obj, kp, mut) = unchecked.wrappedValue + obj.publisher.beginModifications() + let result = try registrar.underlying.withMutation( + of: obj, + keyPath: kp, + mut + ) + registrar.publish(obj, keyPath: kp) + obj.publisher.endModifications()
125-131: Clean up the UncheckedSendable implementation.The struct serves its purpose well, but can be simplified and made more explicit.
Apply these improvements:
-struct UncheckedSendable<Value>: @unchecked Sendable { +internal struct UncheckedSendable<Value>: @unchecked Sendable { var wrappedValue: Value - - init(wrappedValue: Value) { - self.wrappedValue = wrappedValue - } }The memberwise initializer is automatically synthesized by Swift, and adding explicit access control improves clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Sources/Publishable/PropertyPublisher/Publishable.swift(2 hunks)Sources/Publishable/Registrars/PublishableObservationRegistrar.swift(1 hunks)Sources/PublishableMacros/Builders/ObservationRegistrarDeclBuilder.swift(2 hunks)Sources/PublishableMacros/Builders/PropertyPublisherDeclBuilder.swift(1 hunks)Sources/PublishableMacros/Main/PublishableMacro.swift(2 hunks)Tests/PublishableMacrosTests/PublishableMacroTests.swift(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
Sources/PublishableMacros/Builders/PropertyPublisherDeclBuilder.swift (2)
Sources/PublishableMacros/Builders/ObservationRegistrarDeclBuilder.swift (1)
build(25-60)Sources/PublishableMacros/Builders/PublisherDeclBuilder.swift (1)
build(20-26)
Sources/Publishable/Registrars/PublishableObservationRegistrar.swift (1)
Sources/Publishable/PropertyPublisher/AnyPropertyPublisher.swift (2)
beginModifications(49-54)endModifications(56-61)
🪛 SwiftLint (0.57.0)
Sources/PublishableMacros/Builders/PropertyPublisherDeclBuilder.swift
[Warning] 21-21: SwiftLint rule 'type_contents_order' did not trigger a violation in the disabled region; remove the disable command
(superfluous_disable_command)
Tests/PublishableMacrosTests/PublishableMacroTests.swift
[Warning] 161-161: Function body should span 50 lines or less excluding comments and whitespace: currently spans 71 lines
(function_body_length)
Sources/Publishable/Registrars/PublishableObservationRegistrar.swift
[Warning] 125-125: Type declaration should start with an empty line.
(empty_line_after_type_declaration)
[Warning] 125-125: Top-level declarations should specify Access Control Level keywords explicitly
(explicit_top_level_acl)
[Warning] 77-77: An 'extension' should not be placed amongst the file type(s) 'supporting_type'
(file_types_order)
[Warning] 114-114: Line should be 120 characters or less; currently it has 126 characters
(line_length)
[Warning] 116-116: Line should be 120 characters or less; currently it has 172 characters
(line_length)
[Warning] 128-128: This memberwise initializer would be synthesized automatically - you do not need to define it
(unneeded_synthesized_initializer)
🔇 Additional comments (12)
Sources/PublishableMacros/Builders/PropertyPublisherDeclBuilder.swift (2)
15-15: LGTM! Property addition supports MainActor isolation.The
mainActorproperty correctly enables conditional generation of@MainActor-annotatedPropertyPublisherclasses.
22-49: LGTM! Conditional MainActor generation is correctly implemented.The conditional logic properly generates
@MainActor-annotatedPropertyPublisherclasses when needed while preserving all existing functionality for non-MainActor types.Sources/PublishableMacros/Builders/ObservationRegistrarDeclBuilder.swift (2)
15-15: LGTM! Property addition supports MainActor isolation.The
mainActorproperty correctly enables conditional generation of@MainActor-annotatedObservationRegistrarstructs.
26-59: LGTM! Conditional MainActor generation and protocol conformance are correctly implemented.The conditional logic properly:
- Generates
@MainActor-annotatedObservationRegistrarstructs when needed- Conforms to
MainActorPublishableObservationRegistrarfor MainActor types- Conforms to
PublishableObservationRegistrarfor non-MainActor types- Preserves all existing functionality
The protocol conformance selection ensures proper actor isolation alignment.
Sources/PublishableMacros/Main/PublishableMacro.swift (4)
48-49: LGTM! MainActor detection is correctly implemented.The attribute detection logic properly identifies
@MainActorannotations on type declarations for propagation to code generation.
52-53: LGTM! Builder initialization correctly propagates MainActor flag.The
mainActorparameter is properly passed to bothPropertyPublisherDeclBuilderandObservationRegistrarDeclBuilder, ensuring consistent MainActor annotation generation.
75-75: LGTM! Consistent MainActor detection in ExtensionMacro.The same MainActor detection logic is correctly applied in the extension macro for consistency.
82-82: LGTM! Conditional protocol conformance is correctly implemented.The extension properly conforms to
MainActorPublishablefor MainActor types andPublishablefor non-MainActor types, ensuring proper actor isolation alignment.Tests/PublishableMacrosTests/PublishableMacroTests.swift (1)
161-233: LGTM! Comprehensive test coverage for MainActor expansion.The test thoroughly verifies:
@MainActorattribute preservation on the original class@MainActorannotation on generatedPropertyPublisherclass@MainActorannotation on generatedObservationRegistrarstruct- Conformance to
MainActorPublishableObservationRegistrar- Extension conformance to
MainActorPublishableThe test provides excellent coverage for the MainActor support functionality.
Note: The function body length warning (71 lines) is acceptable for a comprehensive macro expansion test that needs to verify detailed generated code.
Sources/Publishable/PropertyPublisher/Publishable.swift (2)
34-34: LGTM! Macro conformances correctly include MainActorPublishable.The conformances list properly includes both
PublishableandMainActorPublishableprotocols to support both actor-isolated and non-actor-isolated types.
61-80: LGTM! MainActorPublishable protocol is correctly designed.The protocol properly:
- Mirrors the structure of
Publishablefor consistency- Adds
@MainActorannotation to thepublisherproperty for actor isolation- Provides appropriate documentation matching the original protocol
- Maintains the same associated type and inheritance constraints
This design ensures proper actor isolation while maintaining API consistency.
Sources/Publishable/Registrars/PublishableObservationRegistrar.swift (1)
63-75: Well-designed protocol addition for MainActor support!The new
MainActorPublishableObservationRegistrarprotocol maintains architectural consistency with the existingPublishableObservationRegistrarwhile properly adding@MainActorisolation to thepublishmethod.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Sources/Publishable/Registrars/PublishableObservationRegistrar.swift (1)
77-77: Consider SwiftLint warning about file organization.SwiftLint suggests grouping all types together before extensions. However, the current organization (protocol followed immediately by its extension) enhances readability and maintainability. Consider if this trade-off is acceptable or if reorganizing would improve the codebase consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Sources/Publishable/Registrars/PublishableObservationRegistrar.swift(1 hunks)Sources/PublishableMacros/Builders/PropertyPublisherDeclBuilder.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/PublishableMacros/Builders/PropertyPublisherDeclBuilder.swift
🧰 Additional context used
🧬 Code Graph Analysis (1)
Sources/Publishable/Registrars/PublishableObservationRegistrar.swift (1)
Sources/Publishable/PropertyPublisher/AnyPropertyPublisher.swift (2)
beginModifications(49-54)endModifications(56-61)
🪛 SwiftLint (0.57.0)
Sources/Publishable/Registrars/PublishableObservationRegistrar.swift
[Warning] 77-77: An 'extension' should not be placed amongst the file type(s) 'supporting_type'
(file_types_order)
🔇 Additional comments (5)
Sources/Publishable/Registrars/PublishableObservationRegistrar.swift (5)
63-75: LGTM! Well-designed protocol for MainActor isolation.The protocol correctly mirrors
PublishableObservationRegistrarwhile adding appropriate MainActor constraints. The@MainActorannotation on thepublishmethod ensures thread safety when interacting with MainActor-isolated publishers.
79-96: LGTM! Correct MainActor isolation for mutation lifecycle methods.The
willSetanddidSetmethods are appropriately marked@MainActorand follow the same pattern as the original protocol, ensuring thread safety when managing publisher modifications.
98-103: LGTM! Correct isolation for read-only access.The
accessmethod correctly omits@MainActorannotation since it's a read-only operation that can safely delegate to the underlying registrar without actor isolation.
105-130: LGTM! Sophisticated concurrency handling for MainActor isolation.The
withMutationimplementation correctly handles the complexity of bridging between@Observable's nonisolated mutation methods and MainActor-isolated publishers. The use ofMainActor.assumeIsolatedwith the explanatory comment clearly documents the reasoning, and theUncheckedSendablewrapper appropriately facilitates cross-actor data transfer.
133-136: LGTM! Standard concurrency helper pattern.The
UncheckedSendablewrapper is a well-established pattern for safely transferring non-Sendable values across actor boundaries when the developer can guarantee thread safety. The private visibility and minimal implementation are appropriate.
NSFatalError
left a comment
NSFatalError
left a comment
There was a problem hiding this comment.
Thanks for contributing @erichoracek! My sincere apologies for taking so long to take a proper look at your PR.
I've extended the applicability of your changes in a separate PR to achieve two things:
- Support any global actor, not just the
MainActor - Not having to deal with specialized
MainActorPublishableprotocol - global actor isolated conformances are coming in Swift 6.2, so this one would become redundant quickly
One downside of my approach is that I've removed the publisher property from the Publishable protocol. Then again, as global actor isolated conformances are going to be released shortly with Xcode 26, this is a stopgap solution for Swift 6.1 specifically. I intend to reexpose the publisher again when the new release lands (besides, using Publishable as an existential type is of limited value anyway).
I'll appreciate if you'd find a moment to verify that my approach works for your use cases too. Thanks again for your work!
|
Thanks for working on a more general solution for this—it seems like a good solve given the constraints and agree it is more elegant and flexible than the separate In terms of whether it will work for our use case, it should work well, however we have additional changes we've made on top of this MainActor change to make publishing opt in via a new |
|
@erichoracek I finally found some time to work on this - sorry for keeping you waiting so long. I ended up using isolated conformances to preserve the existing behavior, and those changes have now been merged. Regarding your approach with I’ll close this PR since it has become redundant after my changes, but feel free to suggest other improvements or contribute further. Thanks again for your work! |
2 participants
When we have a
@Publishableon a@MainActortype (e.g. an@Observableview model), we conform to a specialMainActorPublishablewhich fixes build errors that occur otherwiseSummary by CodeRabbit
New Features
Tests