Added global actor isolation support #5
Conversation
WalkthroughThis change updates lint rules, bumps dependencies, revises documentation, simplifies the Publishable protocol, adds a macro overload for explicit global-actor isolation, reworks the observation registrar API and generated code, threads isolation through macro builders, adds documentation comments, and introduces new tests covering MainActor-isolated macro expansion and publisher behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Person as Person (@MainActor, @Publishable, @Observable)
participant Publisher as PropertyPublisher
participant Registrar as ObservationRegistrar
participant SwiftObs as SwiftObservationRegistrar
participant Sub as Combine Subscriber
Note over Person,Registrar: Generated by Publishable macro
Client->>Person: mutate property
Person->>Registrar: willSet(object, keyPath)
Registrar->>SwiftObs: begin/access coordination
Registrar->>Publisher: route keyPath (assumeIsolatedIfNeeded)
Publisher-->>Sub: willChange emit (optional)
Person->>Registrar: withMutation(of:keyPath) { ... }
Registrar->>Registrar: assumeIsolatedIfNeeded(MainActor?)
Registrar->>Person: perform mutation
Registrar->>Publisher: publish(keyPath)
Publisher-->>Sub: value emit
Registrar->>SwiftObs: end coordination
Note over Registrar: access(object,keyPath) used for reads
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (16)
.swiftlint.yml (1)
186-190: Tighten custom rule regexes to reduce backtracking and avoid requiring a trailing whitespace characterThe current patterns rely on greedy
.+/.*and require a trailing\safter the attribute. This can be brittle and slower on large files. Also, module-qualified actors (e.g.,@MyModule.MainActor) aren’t explicitly handled.Suggested more specific, faster patterns that don’t require trailing whitespace and correctly match module-qualified global actors:
- regex: "(?-s)(@.+[^,\\s]\\s+@.*Actor\\s)" + regex: "(?-s)@[^,\\s]+\\s+@[\\w\\.]*Actor\\b"- regex: "(?-s)(@.+[^,\\s]\\s+@Sendable\\s)" + regex: "(?-s)@[^,\\s]+\\s+@Sendable\\b"Notes:
@[^,\s]+matches a single attribute token without overreaching.@[\\w\\.]*Actor\\bmatches@MainActorand@Module.MainActorwhile avoiding partials.\bmarks the end of the identifier (no need to require trailing whitespace).If you want, I can add quick unit-style fixtures to validate these patterns against common attribute layouts (single-line, multi-line, module-qualified).
Sources/Publishable/Documentation.docc/Publishable.md (1)
10-10: Good addition; consider clarifying isolation semantics (nil vs actor) with a short exampleThe symbol link is great. A one-liner here (or on the macro’s dedicated page) stating that
nilmeans nonisolated and a concrete example withMainActor.selfwould help readers adopt the new API faster.For example (DocC snippet to add near this topic list or on the macro page):
// @Publishable(isolation: MainActor.self) // MainActor-isolated // @Publishable(isolation: nil) // nonisolatedI can draft a small “Discussion” section if you prefer to keep the Topics list terse.
Sources/PublishableMacros/Builders/PublisherDeclBuilder.swift (1)
23-29: Nice doc addition; minor wording polish and actor-isolation noteThe warning is helpful. Suggested micro-edits for clarity and to acknowledge global-actor isolation of the enclosing type:
- /// A ``PropertyPublisher`` which exposes `Combine` publishers for all mutable - /// or computed instance properties of this object. + /// A ``PropertyPublisher`` that exposes `Combine` publishers for all mutable + /// or computed instance properties of this object. The property is isolated + /// to the same global actor as the enclosing type, if any. @@ - /// - Important: Don't store this instance in an external property. Accessing it after - /// the original object has been deallocated may result in a crash. Always access it directly - /// through the object that exposes it. + /// - Important: Don’t store this value outside the owning object. Accessing it + /// after the owner has been deinitialized will crash (it keeps an unowned reference). + /// Always access it directly through the object that exposes it.If
PropertyPublisheris type-aliased toAnyPropertyPublisher<…>elsewhere, consider cross-linking it withAnyPropertyPublisherin a See Also to aid doc navigation.Sources/Publishable/Registrars/PublishableObservationRegistrar.swift (2)
16-16: Initializer requirement looks fine; consider clarifying isolation expectations in docsAdding
init()enables macro-generated registrars to be value types with default initialization. Minor ask: please document whether conformers are expected to be actor-isolated ornonisolatedby default so downstream implementers don’t accidentally capture non-Sendable state across actors.
28-37: withMutation rethrows return pattern is good; consider makingaccesssemantics explicit
withMutationreturningTandrethrowsis appropriate.- For
access, please document that it records dependency tracking only (no publishing), matching the Observation framework’s read-tracking semantics. This avoids misuse where callers expect emissions.Happy to propose doc comment text if desired.
Tests/PublishableMacrosTests/PublishableMacroTests.swift (3)
75-83: Helpful publisher docs; consider adding a note on actor isolationNice docblock. Since global-actor isolation is now supported, consider appending a one-liner that the publisher’s accessors may be actor-isolated (e.g.,
@MainActor) depending on macro parameters or containing type isolation.
181-186: Routeaccessthrough the isolation shim for consistency
accesscurrently callsunderlying.accessdirectly. For consistency with mutation hooks and to prevent accidental cross-actor access when explicit isolation is used, consider:- nonisolated func access( + nonisolated func access( _ object: Person, keyPath: KeyPath<Person, some Any> ) { - underlying.access(object, keyPath: keyPath) + assumeIsolatedIfNeeded { + underlying.access(object, keyPath: keyPath) + } }This keeps all registrar interactions consistently isolated.
207-211: Isolation shim is a no-op here; consider an inline annotation to make that explicitThe non-actor variant’s
assumeIsolatedIfNeededis a straighttry operation(). That’s fine. A tiny readability tweak would be to add a comment explaining it’s intentionally a no-op when no global actor is configured.Sources/PublishableMacros/Builders/PropertyPublisherDeclBuilder.swift (2)
65-65: Prepending inherited global-actor attribute to stored property publishers: good; extend to thepublisherproperty tooAnnotating individual publisher accessors is correct. Consider also annotating the top-level
publisherproperty (emitted byPublisherDeclBuilder) with the sameinheritedGlobalActorAttributeso accessingobject.publisheritself is consistently actor-checked when explicit isolation is requested.I can wire
explicitGlobalActorIsolationintoPublisherDeclBuilderand add the attribute there.
24-24: Remove superfluous SwiftLint disableSwiftLint hints a superfluous disable for
type_contents_order. Since the rule didn’t trigger, drop the inline disable to keep the code clean.- func build() -> [DeclSyntax] { // swiftlint:disable:this type_contents_order + func build() -> [DeclSyntax] {Tests/PublishableMacrosTests/MainActorMacroTests.swift (3)
21-234: SwiftLint: function_body_length violation; move the multi-line strings out of the test methodThe test method spans 200+ lines due to inline multi-line strings. Extract the input and expected expansion into static helpers to satisfy the rule and improve readability.
Example refactor:
- func testExpansion() { - assertMacroExpansion( - #""" - @MainActor @Publishable @Observable - public final class Person { - ... - """#, - expandedSource: - #""" - @MainActor @Observable - public final class Person { - ... - """#, - macros: macros - ) - } + private static let inputSource: String = #""" + @MainActor @Publishable @Observable + public final class Person { + ... + } + """# + + private static let expectedExpansion: String = #""" + @MainActor @Observable + public final class Person { + ... + } + """# + + func testExpansion() { + assertMacroExpansion( + Self.inputSource, + expandedSource: Self.expectedExpansion, + macros: macros + ) + }
119-156: Actor-annotated helper methods: good shape; ensure Combine availabilityThe generated expansion references
PassthroughSubjectandAnyPublisher. In macro-only tests that’s fine (string compare), but consider adding an integration test in the runtime test target to type-check a sample expansion underimport Combineto catch drift early.I can outline a small type-check test that compiles an expanded sample.
207-224: Isolation shim implementation: solid, but consider adding a fast path when already on the correct actorYou could optionally guard with
MainActor.assumeIsolatedonly when needed. Not necessary, but if this path is hot, a fast-path could help:
- Attempt
MainActor.preconditionIsolated()in debug to catch misuse.- Or early-exit if already isolated (if you have a cheap check).
Given this is test expectation, treat this as a note for the builder implementation rather than the test text.
Sources/Publishable/PropertyPublisher/Publishable.swift (1)
13-14: Consider clarifying the error scenario in the documentation.The documentation mentions "If this causes compilation errors" but doesn't specify what types of errors users might encounter when isolation inference fails. It would be helpful to provide examples of specific compilation errors that would require using the explicit isolation overload.
-/// - Note: This macro infers the global actor isolation of the type and applies it to the generated declarations. -/// If this causes compilation errors, use ``Publishable(isolation:)`` instead. +/// - Note: This macro infers the global actor isolation of the type and applies it to the generated declarations. +/// If this causes compilation errors (e.g., "Call to main actor-isolated initializer 'init()' in a synchronous nonisolated context"), +/// use ``Publishable(isolation:)`` instead to explicitly specify the isolation.Tests/PublishableTests/Suites/MainActorTests.swift (1)
20-20: Consider adding a comment explaining the use ofnonisolated(unsafe).While the use of
nonisolated(unsafe)is correct here for tracking observations across actor boundaries, a brief comment would help future maintainers understand why this approach is necessary.- nonisolated(unsafe) var observationsQueue: [Void] = [] + // Using nonisolated(unsafe) to allow mutation from the onChange closure + // which may execute on a different isolation context + nonisolated(unsafe) var observationsQueue: [Void] = []Sources/PublishableMacros/Builders/ObservationRegistrarDeclBuilder.swift (1)
169-170: Consider documenting the unsafe cast rationale.While the
unsafeBitCastusage is correct and necessary here to work around Swift's type system limitations, adding a brief comment would help future maintainers understand why this approach is needed.+ // Strip the global actor annotation from the closure type to allow passing it to assumeIsolated typealias Nonisolated = () throws -> Void let rawOperation = unsafeBitCast(operation, to: Nonisolated.self)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
.swiftlint.yml(1 hunks)Package.resolved(1 hunks)Package.swift(1 hunks)Sources/Publishable/Documentation.docc/Publishable.md(1 hunks)Sources/Publishable/PropertyPublisher/Publishable.swift(2 hunks)Sources/Publishable/Registrars/PublishableObservationRegistrar.swift(1 hunks)Sources/PublishableMacros/Builders/ObservationRegistrarDeclBuilder.swift(6 hunks)Sources/PublishableMacros/Builders/PropertyPublisherDeclBuilder.swift(3 hunks)Sources/PublishableMacros/Builders/PublisherDeclBuilder.swift(1 hunks)Sources/PublishableMacros/Main/PublishableMacro.swift(2 hunks)Tests/PublishableMacrosTests/MainActorMacroTests.swift(1 hunks)Tests/PublishableMacrosTests/PublishableMacroTests.swift(3 hunks)Tests/PublishableTests/Suites/MainActorTests.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
Tests/PublishableMacrosTests/MainActorMacroTests.swift (1)
Tests/PublishableMacrosTests/PublishableMacroTests.swift (1)
testExpansion(21-221)
Sources/PublishableMacros/Builders/PropertyPublisherDeclBuilder.swift (1)
Sources/PublishableMacros/Builders/PublisherDeclBuilder.swift (1)
build(20-33)
Tests/PublishableTests/Suites/MainActorTests.swift (1)
Tests/PublishableTests/Helpers/Array+PopFirst.swift (1)
popFirst(11-16)
🪛 SwiftLint (0.57.0)
Tests/PublishableMacrosTests/MainActorMacroTests.swift
[Error] 21-21: Function body should span 100 lines or less excluding comments and whitespace: currently spans 212 lines
(function_body_length)
Sources/PublishableMacros/Builders/PropertyPublisherDeclBuilder.swift
[Warning] 24-24: SwiftLint rule 'type_contents_order' did not trigger a violation in the disabled region; remove the disable command
(superfluous_disable_command)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-and-test (macos)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (ios)
- GitHub Check: build-and-test (maccatalyst)
- GitHub Check: build-and-test (tvos)
🔇 Additional comments (18)
Package.resolved (1)
2-11: Lockfile aligns with Package.swift changes
- PrincipleMacros pinned to 2.0.4 (rev d55ddb9…).
- swift-syntax remains at 601.0.1, which is within the declared
"600.0.0" ..< "604.0.0"range.No concerns here.
Sources/Publishable/Registrars/PublishableObservationRegistrar.swift (1)
18-27: Symmetric willSet/didSet hooks are clear; verify ordering guarantees with underlying registrarThe contract implies:
willSetbefore mutation, then- actual mutation, then
didSet.If the underlying
SwiftObservationRegistrarcan throw or otherwise reentrantly notify duringdidSet, confirm that publish-side effects won’t be missed. If needed, specify ordering guarantees in doc comments (e.g., “didSetis invoked after mutation has completed and before publish events are dispatched”).Do you want me to draft concise doc comments for these hooks?
Tests/PublishableMacrosTests/PublishableMacroTests.swift (1)
117-117:underlyinglifetime and thread-safety
private let underlying = SwiftObservationRegistrar()is fine. Verify that every call tounderlyingthat mutates registrar state runs under the intended actor (or via an isolation shim) to avoid data races. In this non-actor test fixture, that’s handled by the wrappers aroundwillSet/didSet/withMutation; see follow-up onaccessbelow.Sources/PublishableMacros/Builders/PropertyPublisherDeclBuilder.swift (4)
15-22: Threading explicit global-actor isolation through settings: LGTMPassing
explicitGlobalActorIsolationintoDeclBuilderSettingslooks correct and aligns with the updated tests. No issues.
27-27: Generic argument switch totrimmedType: good catchMoving from
AnyPropertyPublisher<\(trimmedTypeName)>to<\(trimmedType)>preserves generic sugar (e.g., module-qualified names) and avoids name-only collisions. Looks good.
57-61: Use ofinlinableAccessControlLevelis appropriateThis will produce the expected access level for peer members and caps at
public. Nice improvement over the previous method.
75-86: Computed publishers also correctly inherit actor attributeLooks consistent with the stored publishers path. No issues.
Sources/PublishableMacros/Main/PublishableMacro.swift (1)
34-46: ExtractingexplicitGlobalActorIsolation: good placement and error handlingParsing the isolation parameter before property parsing is sensible. The throwing API is fine because
expansionalready throws. No changes requested.Sources/Publishable/PropertyPublisher/Publishable.swift (2)
44-77: LGTM! Well-designed macro overload for explicit isolation control.The new
Publishable<Isolation: GlobalActor>(isolation:)macro provides excellent flexibility for users who need explicit control over global actor isolation. The documentation clearly explains when to use this overload versus the inference-based version.
84-84: Clean protocol design after removing associated type requirements.The simplified
Publishableprotocol, without thePropertyPublisherassociated type andpublisherproperty, aligns well with the macro-driven approach where these are generated rather than protocol-required. This reduces complexity for implementers.Tests/PublishableTests/Suites/MainActorTests.swift (5)
16-54: Excellent test coverage for stored property publishers.The test thoroughly validates the behavior of stored property publishers, including:
- Initial value emission
- Property-specific change tracking
- Proper isolation of unrelated property changes
- Completion handling on deallocation
56-99: Well-structured test for computed property dependencies.The test effectively verifies that computed properties correctly track their dependencies (
nameandsurname) while ignoring unrelated changes (age). Good coverage of the reactive behavior.
104-151: Comprehensive willChange publisher testing.The test properly validates that
willChangeemits the object reference before mutations occur and tracks changes across multiple properties. Good use of identity comparison (===) to verify the same instance is emitted.
153-200: Good coverage of didChange publisher behavior.The test correctly verifies that
didChangeemits after mutations complete, maintaining consistency with the willChange test structure for easy comparison.
203-222: Well-designed test model with varied access levels.The
Personclass effectively tests the macro's handling of different property access levels (public, internal, package, fileprivate) and types (stored, computed). The@MainActorisolation properly exercises the new global actor isolation support.Sources/PublishableMacros/Builders/ObservationRegistrarDeclBuilder.swift (3)
15-22: Good addition of explicit isolation parameter.The
explicitGlobalActorIsolationparameter is properly integrated into the builder settings, allowing for flexible isolation control in the generated code.
106-157: Well-designed observation lifecycle methods with proper isolation handling.The implementation correctly handles the observation lifecycle with
willSet,didSet,access, andwithMutationmethods. The use ofnonisolated(unsafe)for capturing values before callingassumeIsolatedIfNeededis appropriate for maintaining actor isolation boundaries.
159-191: Clever isolation assumption implementation.The
assumeIsolatedIfNeededfunction elegantly handles both isolated and nonisolated cases. The use ofunsafeBitCastto strip the global actor annotation is a necessary workaround, and the implementation properly usesassumeIsolatedto maintain runtime safety.
|
Resolved in #6 |
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores
Style