Added Publishable subclassing support #8
Conversation
WalkthroughRefactors the Publishable macro and its codegen to emit opaque per-property publishers, adds PublisherIgnored/ObservationIgnored markers, introduces AnyPropertyPublisher and Publishable protocol surface changes, reorganizes macro builders and tests, updates CI Xcode targets to 26.0, and adjusts SwiftLint rules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50-70 minutes Areas to focus during review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
Macros/RelayMacros/Combine/Relayed/RelayedMacro.swift (1)
11-14: Access level inconsistency with similar macro enums.This enum is declared
publicwhile similar enums (SwiftDataModelMacro,ObservableMacro) areinternal. Additionally, theattributeproperty lacks an explicit access modifier, defaulting tointernal, which may limit its utility if the enum is intended to be public.Consider either:
- Making the enum
internalfor consistency with other macro attribute holders:-public enum RelayedMacro { +internal enum RelayedMacro { static let attribute: AttributeSyntax = "@Relayed" }
- Or, if public access is intentional, explicitly mark the
attributeaspublic:public enum RelayedMacro { - static let attribute: AttributeSyntax = "@Relayed" + public static let attribute: AttributeSyntax = "@Relayed" }Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift (2)
325-358: Missing test coverage forCatclass.The
Catclass demonstrates an important scenario (a@Publishablesubclass of a non-@Publishablebase), but no tests exercise it. Consider adding tests to validate this scenario or document why it's intentionally untested.
312-317: Clarify the purpose of the emptydidSetwith_ = oldValue.The
_ = oldValuestatement is a no-op. If the intent is to demonstrate@ObservationIgnoredon an override, consider adding a brief comment explaining the purpose, or simplify if thedidSetbody isn't needed.Tests/RelayTests/Publishable/ObservationPublishableTests.swift (1)
220-236: Good coverage for platform-conditional and ignored properties.The additions test:
- Platform-specific properties with
#if os(macOS)and@available@PublisherIgnoredannotation on both stored and computed propertiesConsider adding explicit tests that verify
@PublisherIgnoredproperties do not emit publisher events when mutated, to confirm the macro correctly excludes them.Tests/RelayMacrosTests/Publishable/SubclassedPublishableMacroTests.swift (1)
117-118: Minor: Extra blank line in expected expansion output.There's a trailing blank line at line 117 in the
expandedSource. While this doesn't affect functionality, it may indicate whitespace handling in the macro output.final var isBulldog: some Publisher<Bool, Never> { _computedPropertyPublisher(for: \.isBulldog, object: object) } - }Tests/RelayMacrosTests/Memoized/NonisolatedMemoizedMacroTests.swift (1)
20-80: Good test coverage for nonisolated memoization on isolated classes.This test verifies that
@Memoized(isolation: nil)correctly generatesnonisolatedcache properties and accessors even when the enclosing class is@MainActorisolated. This is valuable for scenarios where memoization should work across isolation boundaries.The SwiftLint
function_body_lengthwarning is acceptable here - macro expansion tests inherently require complete expected output strings which inflate line counts. Consider adding a local disable comment if CI enforces this lint rule.Tests/RelayTests/Publishable/MainActorPublishableTests.swift (1)
221-237: Consider adding tests to verify@PublisherIgnoredbehavior.The new
ignoredStoredPropertyandignoredComputedPropertyare annotated with@PublisherIgnored, but there are no tests verifying that changes to these properties do not trigger publisher emissions. This would strengthen confidence in the macro's correctness.Do you want me to generate test cases that verify changes to
@PublisherIgnoredproperties do not emit values through the publisher?Tests/RelayMacrosTests/Publishable/ExplicitlyIsolatedPublishableMacroTests.swift (1)
15-16: SwiftLint function body length warning is acceptable here.The static analysis flags a 304-line function body, but this is inherent to macro expansion tests where the entire expanded source must be validated as a single assertion. The existing
swiftlint:disable:next type_body_lengthcomment handles the type-level warning; consider adding a similar disable comment forfunction_body_lengthif CI enforces this rule.Macros/RelayMacros/Combine/Common/PropertyPublisherDeclBuilder.swift (1)
80-86: Consider reordering deinitializer for consistency.The static analysis tool flagged that the deinitializer is placed after the initializer but amongst other methods. While this is functionally correct, Swift conventions typically group
init/deinittogether before other methods.Consider moving the
deinitializer()call inbuild()to be immediately afterinitializer()in the generated output, if the tool warning is a team concern. Otherwise, this is a minor stylistic note.Macros/RelayMacros/Memoized/MemoizedMacro.swift (1)
144-150: Potential ambiguity with dual subscript overloads.If
ValidationResultandParametersever have properties with the same name and type, the twodynamicMembersubscripts could cause ambiguity. Currently this isn't an issue, but consider documenting this constraint or adding a compile-time check if the types evolve.Sources/Relay/Combine/Common/AnyPropertyPublisher.swift (1)
51-63: Missing guard for unbalanced_endModificationscalls.If
_endModifications()is called without a matching_beginModifications(),pendingModificationswill go negative, causing subsequent modifications to behave incorrectly. While this is likely only called from macro-generated code which should be balanced, adding a defensive check would prevent silent corruption.Consider adding a guard:
public final func _endModifications() { + precondition(pendingModifications > 0, "Unbalanced _endModifications call") if pendingModifications == 1 { _didChange.send(()) } pendingModifications -= 1 }Tests/RelayMacrosTests/Publishable/ImplicitlyIsolatedPublishableMacroTests.swift (1)
25-329: Static analysis: function body exceeds 100 lines (304 lines).The
testExpansion()function is flagged by SwiftLint for exceeding the 100-line limit. However, this is a macro expansion test where the expected output is a single large string literal. Splitting this would reduce readability and maintainability.Consider adding a SwiftLint disable comment if the lint rule is enforced in CI:
// swiftlint:disable:next function_body_length func testExpansion() {Alternatively, this may be acceptable as-is given the nature of macro expansion tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
.github/workflows/pull-request.yml(2 hunks).github/workflows/release.yml(1 hunks).swiftlint.tests.yml(1 hunks).swiftlint.yml(6 hunks)Macros/Dependencies/PrincipleMacros(1 hunks)Macros/RelayMacros/Combine/Common/ObservableMacro.swift(1 hunks)Macros/RelayMacros/Combine/Common/ObservationIgnoredMacro.swift(1 hunks)Macros/RelayMacros/Combine/Common/PropertyPublisherDeclBuilder.swift(1 hunks)Macros/RelayMacros/Combine/Common/PublisherDeclBuilder.swift(2 hunks)Macros/RelayMacros/Combine/Common/PublisherIgnoredMacro.swift(1 hunks)Macros/RelayMacros/Combine/Common/SwiftDataModelMacro.swift(1 hunks)Macros/RelayMacros/Combine/Publishable/ObservationRegistrarDeclBuilder.swift(3 hunks)Macros/RelayMacros/Combine/Publishable/PublishableMacro.swift(3 hunks)Macros/RelayMacros/Combine/Relayed/RelayedMacro.swift(1 hunks)Macros/RelayMacros/Main/RelayPlugin.swift(1 hunks)Macros/RelayMacros/Memoized/MemoizedDeclBuilder.swift(2 hunks)Macros/RelayMacros/Memoized/MemoizedMacro.swift(4 hunks)Macros/RelayMacros/Publishable/PropertyPublisherDeclBuilder.swift(0 hunks)Sources/Relay/Combine/Common/AnyPropertyPublisher.swift(1 hunks)Sources/Relay/Combine/Common/PublishableProtocol.swift(1 hunks)Sources/Relay/Combine/Common/PublisherIgnored.swift(1 hunks)Sources/Relay/Combine/Publishable/Publishable.swift(3 hunks)Sources/Relay/Combine/Publishable/PublishableObservationRegistrar.swift(1 hunks)Sources/Relay/Publishable/AnyPropertyPublisher.swift(0 hunks)Tests/RelayMacrosTests/Memoized/ExplicitlyIsolatedMemoizedMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Memoized/ImplicitlyIsolatedMemoizedMacroTests.swift(5 hunks)Tests/RelayMacrosTests/Memoized/MemoizedMacroTests.swift(3 hunks)Tests/RelayMacrosTests/Memoized/NonisolatedMemoizedMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Publishable/ExplicitlyIsolatedPublishableMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Publishable/ImplicitlyIsolatedPublishableMacroTests.swift(6 hunks)Tests/RelayMacrosTests/Publishable/NonisolatedPublishableMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Publishable/PublishableMacroTests.swift(5 hunks)Tests/RelayMacrosTests/Publishable/SubclassedImplicitlyIsolatedPublishableMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Publishable/SubclassedPublishableMacroTests.swift(1 hunks)Tests/RelayTests/Memoized/MainActorMemoizedTests.swift(2 hunks)Tests/RelayTests/Memoized/ObservationMemoizedTests.swift(2 hunks)Tests/RelayTests/Memoized/PublishableMemoizedTests.swift(2 hunks)Tests/RelayTests/Memoized/SwiftDataMemoizedTests.swift(2 hunks)Tests/RelayTests/Publishable/AnyPropertyPublisherTests.swift(1 hunks)Tests/RelayTests/Publishable/MainActorPublishableTests.swift(3 hunks)Tests/RelayTests/Publishable/ObservationPublishableTests.swift(3 hunks)Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift(1 hunks)Tests/RelayTests/Publishable/SubclassedPublishableTests.swift(1 hunks)Tests/RelayTests/Publishable/SwiftDataPublishableTests.swift(0 hunks)
💤 Files with no reviewable changes (3)
- Tests/RelayTests/Publishable/SwiftDataPublishableTests.swift
- Sources/Relay/Publishable/AnyPropertyPublisher.swift
- Macros/RelayMacros/Publishable/PropertyPublisherDeclBuilder.swift
🧰 Additional context used
🧬 Code graph analysis (10)
Tests/RelayMacrosTests/Memoized/NonisolatedMemoizedMacroTests.swift (1)
Tests/RelayMacrosTests/Memoized/MemoizedMacroTests.swift (2)
testExpansion(20-80)testExpansionWithParameters(82-147)
Macros/RelayMacros/Combine/Common/PropertyPublisherDeclBuilder.swift (3)
Macros/RelayMacros/Memoized/MemoizedDeclBuilder.swift (1)
build(21-45)Macros/RelayMacros/Combine/Common/PublisherDeclBuilder.swift (1)
build(16-34)Macros/RelayMacros/Memoized/MemoizedMacro.swift (1)
extract(15-30)
Tests/RelayTests/Memoized/ObservationMemoizedTests.swift (3)
Tests/RelayTests/Memoized/MainActorMemoizedTests.swift (2)
fileprivate(257-261)macOS(264-268)Tests/RelayTests/Memoized/PublishableMemoizedTests.swift (2)
fileprivate(189-193)macOS(201-205)Tests/RelayTests/Memoized/SwiftDataMemoizedTests.swift (2)
fileprivate(260-264)macOS(267-271)
Macros/RelayMacros/Combine/Common/PublisherDeclBuilder.swift (1)
Macros/RelayMacros/Combine/Common/PropertyPublisherDeclBuilder.swift (1)
build(18-40)
Tests/RelayTests/Publishable/SubclassedPublishableTests.swift (2)
Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift (12)
storedProperty(15-53)observe(27-33)observe(67-73)observe(110-116)observe(155-161)observe(203-212)observe(252-261)storedOverridenProperty(55-93)computedProperty(98-141)computedOverridenProperty(143-186)willChange(191-238)didChange(240-287)Tests/RelayTests/Helpers/Array+PopFirst.swift (1)
popFirst(11-16)
Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift (2)
Tests/RelayTests/Publishable/SubclassedPublishableTests.swift (12)
storedProperty(14-52)observe(26-32)observe(66-72)observe(109-115)observe(154-160)observe(202-211)observe(251-260)storedOverridenProperty(54-92)computedProperty(97-140)computedOverridenProperty(142-185)willChange(190-237)didChange(239-286)Tests/RelayTests/Helpers/Array+PopFirst.swift (1)
popFirst(11-16)
Macros/RelayMacros/Combine/Publishable/ObservationRegistrarDeclBuilder.swift (3)
Macros/RelayMacros/Combine/Common/PropertyPublisherDeclBuilder.swift (1)
build(18-40)Macros/RelayMacros/Memoized/MemoizedDeclBuilder.swift (1)
build(21-45)Macros/RelayMacros/Combine/Common/PublisherDeclBuilder.swift (1)
build(16-34)
Tests/RelayTests/Publishable/MainActorPublishableTests.swift (4)
Tests/RelayTests/Memoized/MainActorMemoizedTests.swift (1)
macOS(264-268)Tests/RelayTests/Memoized/ObservationMemoizedTests.swift (1)
macOS(262-266)Tests/RelayTests/Memoized/PublishableMemoizedTests.swift (1)
macOS(201-205)Tests/RelayTests/Memoized/SwiftDataMemoizedTests.swift (1)
macOS(267-271)
Tests/RelayTests/Publishable/AnyPropertyPublisherTests.swift (2)
Tests/RelayTests/Publishable/ObservationPublishableTests.swift (2)
storedProperty(15-53)computedProperty(55-98)Tests/RelayTests/Helpers/Array+PopFirst.swift (1)
popFirst(11-16)
Tests/RelayTests/Memoized/SwiftDataMemoizedTests.swift (3)
Tests/RelayTests/Memoized/MainActorMemoizedTests.swift (2)
fileprivate(257-261)macOS(264-268)Tests/RelayTests/Memoized/ObservationMemoizedTests.swift (2)
fileprivate(255-259)macOS(262-266)Tests/RelayTests/Memoized/PublishableMemoizedTests.swift (2)
fileprivate(189-193)macOS(201-205)
🪛 SwiftLint (0.57.0)
Tests/RelayMacrosTests/Publishable/SubclassedPublishableMacroTests.swift
[Error] 24-24: Function body should span 100 lines or less excluding comments and whitespace: currently spans 177 lines
(function_body_length)
Tests/RelayMacrosTests/Memoized/NonisolatedMemoizedMacroTests.swift
[Warning] 20-20: Function body should span 50 lines or less excluding comments and whitespace: currently spans 59 lines
(function_body_length)
[Warning] 82-82: Function body should span 50 lines or less excluding comments and whitespace: currently spans 64 lines
(function_body_length)
Macros/RelayMacros/Combine/Common/PropertyPublisherDeclBuilder.swift
[Warning] 80-80: A 'deinitializer' should not be placed amongst the type content(s) 'other_method'
(type_contents_order)
Tests/RelayMacrosTests/Memoized/ExplicitlyIsolatedMemoizedMacroTests.swift
[Warning] 20-20: Function body should span 50 lines or less excluding comments and whitespace: currently spans 65 lines
(function_body_length)
[Warning] 88-88: Function body should span 50 lines or less excluding comments and whitespace: currently spans 70 lines
(function_body_length)
Tests/RelayTests/Publishable/SubclassedPublishableTests.swift
[Warning] 38-38: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 82-82: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 83-83: Magic numbers should be replaced by named constants
(no_magic_numbers)
Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift
[Warning] 39-39: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 83-83: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 84-84: Magic numbers should be replaced by named constants
(no_magic_numbers)
Tests/RelayMacrosTests/Publishable/ImplicitlyIsolatedPublishableMacroTests.swift
[Error] 25-25: Function body should span 100 lines or less excluding comments and whitespace: currently spans 304 lines
(function_body_length)
Tests/RelayMacrosTests/Publishable/ExplicitlyIsolatedPublishableMacroTests.swift
[Error] 25-25: Function body should span 100 lines or less excluding comments and whitespace: currently spans 304 lines
(function_body_length)
⏰ 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 (visionos)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (macos)
- GitHub Check: build-and-test (tvos)
- GitHub Check: build-and-test (ios)
🔇 Additional comments (84)
Tests/RelayTests/Publishable/AnyPropertyPublisherTests.swift (6)
12-34: Good use ofenumfor test namespace.Using
enuminstead ofstructfor the test container prevents accidental instantiation and is idiomatic Swift for organizing test fixtures. The nested type structure cleanly isolates non-equatable test types.
140-142: Verify this is the intended behavior for non-equatable value-type computed properties.This test expects the publisher to emit when
unrelatedPropertychanges, even thoughcomputedPropertyonly accessesstoredProperty. This differs fromreferenceTypeComputedProperty(line 181) which expects no emission.If intentional (e.g., non-equatable value types can't be deduplicated without observing which stored properties changed), consider adding a brief comment explaining why this behavior differs from reference types.
200-229: LGTM!The
EquatableTypestructure properly sets up test fixtures for verifying deduplication behavior. UsingIntfor value types and a customEquatableClassfor reference types provides comprehensive coverage of equatable scenarios.
259-266: Good test coverage for equatable deduplication.Lines 259-261 verify that setting the same value doesn't trigger emission, while lines 263-266 confirm that actual value changes do emit. This properly validates the deduplication behavior for
Equatabletypes.
303-310: Excellent test for reference type equality semantics.This correctly verifies that the publisher uses
Equatablecomparison (not identity) for reference types. Line 303-305 confirms that a new instance with the samevaluedoesn't emit, while lines 307-310 confirm that different values do emit.
319-405: LGTM!The computed property tests for
EquatableTypeproperly verify:
- Deduplication when backing store is set to equivalent values (lines 347-349, 391-393)
- Emission when backing store changes to different values
- Proper completion on object deallocation
Test patterns are consistent across all property types.
.swiftlint.tests.yml (1)
3-3: Good call disabling type_body_length in tests.Disabling this rule for test files is reasonable given the macro-generated test expansion mentioned in the PR summary.
.swiftlint.yml (4)
58-58: Verify the rationale for activating implicitly_unwrapped_optional.Line 58 uncomments the
implicitly_unwrapped_optionalrule, which enforces stricter optional handling. Please confirm this is intentional and aligns with the project's concurrency safety strategy, especially given the Publishable subclassing refactor.Also applies to: 58-58
192-200: Confirm removal of global_actor_attribute_order custom rule.The
global_actor_attribute_ordercustom rule has been removed (per AI summary). This rule enforces proper ordering of global actor attributes for concurrency safety. Please confirm this removal is intentional and that the project's concurrency guarantees are not compromised, particularly for the Publishable subclassing feature which may involve actor/isolation annotations.
11-11: Good alignment with swiftformat integration.The systematic annotation of SwiftLint rules with
(swiftformat)comments demonstrates a clear strategy to delegate formatting responsibilities to swiftformat, reducing tool overlap and simplifying maintenance. This is a sound architectural approach.Also applies to: 30-30, 60-60, 70-70, 147-147
176-176: Confirm ignores_multiline_strings addition.Line 176 adds
ignores_multiline_strings: trueto the line_length configuration. Please confirm this exemption is necessary for the project's documentation or generated code patterns (e.g., macro-generated multiline strings).Also applies to: 176-176
.github/workflows/release.yml (1)
9-9: Clarify the rationale for downgrading Xcode from 26.1 to 26.0.Xcode 26.1 was released November 3, 2025, and 26.0 was released September 15, 2025. The downgrade to the older version should include a clear explanation—whether it's to work around a specific issue, maintain compatibility, or for another reason. If this change is unintentional, revert to 26.1.
Macros/Dependencies/PrincipleMacros (1)
1-1: The review comment addresses the wrong module for Publishable implementation verification.The Publishable macro implementation is in
Macros/RelayMacros/Combine/Publishable/within this repository, not in the PrincipleMacros submodule. The submodule update is a dependency pointer change, unrelated to Publishable functionality.The Publishable macro itself does not restrict to final classes—it accepts any
ClassDeclSyntaxand includes logic to handle superclass types (line 60-61 in PublishableMacro.swift), suggesting inheritance support is already built in. The comment's verification request should target the actual Publishable implementation files in RelayMacros if specific concerns exist about non-final class or inheritance support.Likely an incorrect or invalid review comment.
Macros/RelayMacros/Combine/Common/SwiftDataModelMacro.swift (1)
11-14: LGTM!Simple and clean constant holder for the
@Modelattribute. Theinternalaccess level is appropriate for macro-internal usage.Macros/RelayMacros/Main/RelayPlugin.swift (1)
15-19: LGTM!The
PublisherIgnoredMacrois correctly registered with the compiler plugin alongside the existing macros.Macros/RelayMacros/Memoized/MemoizedDeclBuilder.swift (3)
22-26: LGTM! Thefinalkeyword supports subclassing.Adding
finalto the generated backing storage property prevents subclasses from accidentally overriding it, which aligns with the PR objective of supportingPublishablesubclassing. The availability comment appropriately explains the Swift limitation.
27-42: LGTM!The
finalkeyword on the generated computed property ensures consistent behavior with the backing storage and prevents unintended overrides in subclasses.
47-53: LGTM!The helper method clearly documents the Swift limitation for developers inspecting the generated code. The conditional logic correctly applies the comment only when availability attributes are present.
Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift (2)
1-94: Well-structured test coverage for MainActor-isolated Publishable subclassing.The test structure properly validates stored and overridden property publishers with observation tracking. The use of
nonisolated(unsafe)forobservationsQueueis appropriate for cross-isolation access in tests.
96-288: Comprehensive test coverage for computed properties and change publishers.Tests correctly validate computed property observation, overridden property behavior, and willChange/didChange publisher emissions. The identity comparison (
===) for object references is appropriate.Sources/Relay/Combine/Common/PublisherIgnored.swift (1)
9-13: Clean macro declaration following established patterns.The
@PublisherIgnoredmacro declaration correctly uses@attached(peer)and follows the same pattern as@ObservationIgnoredfrom Swift's Observation framework.Tests/RelayTests/Publishable/ObservationPublishableTests.swift (1)
110-110: Publisher naming updated to type-prefixed convention.The change from
willChangetopersonWillChangealigns with the macro-generated naming convention (e.g.,dogWillChangeinSubclassedPublishableMacroTests).Tests/RelayMacrosTests/Publishable/SubclassedPublishableMacroTests.swift (1)
15-202: Comprehensive macro expansion test for subclassed Publishable.The test validates critical aspects of subclassed Publishable macro expansion:
PropertyPublishercorrectly inherits fromAnimal.PropertyPublisher- Type-specific publishers (
dogWillChange,dogDidChange) are generatedObservationRegistrarproperly handles key-path publishing- Only non-overridden stored properties (
breed) get PassthroughSubject publishersThe SwiftLint function body length warning is expected for macro expansion tests with large expected output strings.
Sources/Relay/Combine/Publishable/PublishableObservationRegistrar.swift (1)
14-14: Constraint relaxation to support inheritable classes.Removing the
Publishableconstraint from theObjectassociatedtype is safe and necessary for supporting non-final classes. All eight implementations ofPublishableObservationRegistrar(in macro-generated code and tests) only requireObservableconformance; none depend onPublishablebeing present. This change enables the registrar to work with objects where Publishable behavior is macro-generated rather than explicitly protocol-conformant, aligning with the PR's goal of supporting subclassing scenarios.Macros/RelayMacros/Combine/Common/PublisherDeclBuilder.swift (2)
14-14: LGTM! Subclass type support added correctly.The
trimmedSuperclassTypeproperty enables the builder to detect when a class inherits from anotherPublishableclass, which is essential for the PR's objective of supporting non-final classes.
18-31: Clean implementation for publisher property generation.The lazy
_publisherpattern with computed accessor is appropriate. TheinheritedOverrideModifierwill correctly add theoverridekeyword when subclassing a Publishable class, fulfilling the PR requirements for subclass support.Tests/RelayTests/Memoized/MainActorMemoizedTests.swift (3)
243-249: Good addition of@ObservationIgnoredfor test instrumentation properties.Marking
calculateBaseAreaCallsCountandcalculateVolumeCallsCountas@ObservationIgnoredcorrectly excludes these test counters from observation tracking, preventing unintended side effects in the tests.
257-261: LGTM! Macro parameter usage demonstrates expanded API.The updated
@Memoized(.fileprivate, "volume")syntax properly tests the macro's visibility and custom name parameters.
263-269: Verify themacOS 26availability attribute is intentional.The availability annotation
@available(macOS 26, *)references a macOS version that doesn't exist yet (current is macOS 15 Sequoia). If this is intentional for testing availability attribute propagation through macro expansion, consider adding a brief comment to clarify. Otherwise, consider using a currently valid version.Macros/RelayMacros/Combine/Common/ObservationIgnoredMacro.swift (1)
11-24: Well-structured property tracking check.The
isStoredObservationTrackedimplementation correctly combines multiple conservative conditions:
- Filters to stored, mutable properties
- Excludes static/class members via
typeScopeSpecifier == nil- Excludes overridden properties (letting parent handle tracking)
- Respects explicit
@ObservationIgnoredannotationsThis mirrors the established pattern for
PublisherIgnoredMacroand maintains consistency across the macro infrastructure.Sources/Relay/Combine/Common/PublishableProtocol.swift (2)
1-1: Appropriate lint rule disable.The
file_namerule disable is reasonable here to avoid naming conflicts with the macro definition file while keeping the protocol name clear.
17-29: Clean protocol design supporting subclass-based Publishable.The protocol correctly:
- Constrains to
AnyObjectfor reference-type semantics- Requires
Observableconformance for dual-framework support- Uses an associated type for
PropertyPublisherallowing subclass-specific publisher types- Documents the critical safety consideration about not storing
publisherexternallyThis design enables the PR objective of supporting non-final classes with
@Publishable.Tests/RelayMacrosTests/Memoized/NonisolatedMemoizedMacroTests.swift (2)
16-18: LGTM! Standard test setup.The macros dictionary correctly maps to
MemoizedMacrofor expansion testing.
82-147: Comprehensive parameter test for nonisolated memoization.This test validates the combination of visibility (
.public), custom name ("customName"), availability (@available(macOS 26, *)), and explicit nonisolation (isolation: nil). The expected expansion correctly shows:
nonisolatedmodifiers propagated to both cache storage and computed property- Availability attribute applied to the computed property but not the storage (with explanatory comment)
- Custom access level and name applied
Same note about SwiftLint
function_body_lengthapplies here.Macros/RelayMacros/Combine/Common/PublisherIgnoredMacro.swift (3)
11-14: LGTM! Clean marker macro pattern.The static attribute constant provides a single source of truth for the
@PublisherIgnoredattribute syntax, consistent with theObservationIgnoredMacropattern in the codebase.
16-25: LGTM! Empty expansion is appropriate for marker macros.Returning an empty array is the correct implementation for a marker macro that only serves to annotate declarations for inspection by other macros.
27-43: LGTM! Publisher tracking predicates are well-defined.The tracking logic correctly:
- Requires stored properties to be mutable for publisher tracking
- Excludes static/class properties via
typeScopeSpecifier- Excludes overrides to prevent duplicate publishers in inheritance hierarchies
- Respects the
@PublisherIgnoredannotationThe distinction between stored (requires mutability) and computed (no mutability check) properties is appropriate.
Tests/RelayTests/Memoized/PublishableMemoizedTests.swift (3)
175-181: LGTM! Correct use of @ObservationIgnored for test instrumentation.Marking
calculateBaseAreaCallsCountandcalculateVolumeCallsCountwith@ObservationIgnoredcorrectly excludes these test-only counters from observation tracking, preventing them from interfering with the actual test assertions.
189-198: LGTM! Good test coverage for memoization with publisher exclusion.The changes add valuable test coverage:
@Memoized(.fileprivate, "volume")tests parameterized memoization with custom access level and namecalculateIgnoredValue()with@Memoized @PublisherIgnoredverifies that the publisher exclusion works correctly with memoized properties
200-206: LGTM! Platform-specific memoization test.This tests the interaction between
@availableattributes and@Memoizedmacro expansion on macOS, ensuring the macro correctly handles availability annotations.Tests/RelayMacrosTests/Memoized/ExplicitlyIsolatedMemoizedMacroTests.swift (2)
20-86: LGTM! Comprehensive macro expansion test.The test correctly validates the explicit isolation expansion including the
assumeIsolatedIfNeededwrapper pattern for actor-isolated cache invalidation.Regarding the SwiftLint
function_body_lengthwarning: this is expected for macro expansion tests where the expanded source string literals necessarily span many lines. Splitting the test would harm readability without improving maintainability.
88-159: LGTM! Tests parameterized memoization with availability.Good coverage of the
@availableattribute handling - the test verifies that:
- The stored cache property
_customNameis not marked with availability (as noted in the comment on line 115)- The computed property
customNamecorrectly inherits the@available(macOS 26, *)annotation- Custom access level (
.public) and name ("customName") are properly appliedTests/RelayMacrosTests/Publishable/NonisolatedPublishableMacroTests.swift (2)
17-22: LGTM! Correct MacroSpec configuration.The macroSpecs setup properly maps the
"Publishable"string toPublishableMacro.selfwith"Publishable"conformance, aligning with other Publishable macro tests in the repository.
24-316: LGTM! Comprehensive nonisolated Publishable test.This test provides excellent coverage of the
@Publishable(isolation: nil)case, validating:
- Nonisolated PropertyPublisher: The generated
PropertyPublisherclass is correctly markednonisolated(line 149)- Publisher exclusion: Properties annotated with
@PublisherIgnoredare correctly excluded from the generated publishers- Memoized + PublisherIgnored: The combination
@Memoized @PublisherIgnoredcorrectly excludes memoized properties from publishing- Platform-specific handling:
#if os(macOS)blocks are preserved in the expanded output- Nonisolated ObservationRegistrar: The
assumeIsolatedIfNeededimplementation correctly passes through for the nonisolated case (lines 302-306)The test complements the existing
ExplicitlyIsolatedPublishableMacroTestsandImplicitlyIsolatedPublishableMacroTeststo ensure full isolation mode coverage.Sources/Relay/Combine/Publishable/Publishable.swift (2)
26-41: LGTM! Conformance declaration added to member attachment.Adding
conformances: Publishableto the@attached(member)declaration properly advertises that this macro generatesPublishableconformance, enabling better tooling support and macro composition.
61-78: LGTM! Simplified isolation parameter API.The change from a generic parameter (
<Isolation: GlobalActor>) to an existential optional ((any GlobalActor.Type)?) improves usability:
- Cleaner call sites:
@Publishable(isolation: MainActor.self)or@Publishable(isolation: nil)- The
nilvalue elegantly represents the nonisolated case- No generic type inference complexity for callers
The documentation correctly explains that
nilproducesnonisolatedmembers.Tests/RelayTests/Publishable/MainActorPublishableTests.swift (1)
111-111: Publisher signal rename looks consistent.The rename from
willChange/didChangetopersonWillChange/personDidChangeis applied consistently across both test methods, aligning with the updated API surface.Also applies to: 160-160
Macros/RelayMacros/Combine/Publishable/ObservationRegistrarDeclBuilder.swift (3)
15-28: Clean refactoring to support tracked properties filtering.The introduction of
trackedPropertiesfiltered byisStoredPublisherTrackedand a uniquegenericParameterviacontext.makeUniqueName("T")is a solid approach for avoiding name collisions in macro-expanded code and properly scoping publisher emissions.
63-85: Key path lookup implementation is correct.The iteration over
trackedPropertieswith conditionalifConfigwrapping correctly handles platform-specific properties. The earlyreturnafter eachsendis efficient.
121-148: ThewithMutationgeneric signature correctly uses the unique parameter.Using
nonisolated(unsafe) var result: \(genericParameter)!with an implicitly unwrapped optional is intentional here—the value is guaranteed to be assigned before the return due to the synchronousassumeIsolatedIfNeededexecution pattern.Tests/RelayMacrosTests/Memoized/MemoizedMacroTests.swift (2)
44-46:finalmodifier additions align with macro evolution.The
finalmodifier on_areaandareais correct for memoized properties within observable classes, ensuring the backing storage and accessor cannot be overridden.
90-113: Availability handling is correctly tested.The test properly validates that:
- The
@available(macOS 26, *)annotation is preserved on the original function- The backing storage
_customNameincludes the explanatory comment about stored property availability limitations- The public accessor
customNameinherits the availability annotationThis matches the expected behavior for platform-gated memoized properties.
Tests/RelayMacrosTests/Publishable/ExplicitlyIsolatedPublishableMacroTests.swift (1)
25-330: Comprehensive test coverage for explicitly isolated Publishable macro.The test thoroughly validates the macro expansion including:
- Stored/computed property publishers with correct access levels
- Platform-specific
#if os(macOS)handling@PublisherIgnoredproperty exclusion@Memoizedproperty integration- Observation registrar with isolation-aware
assumeIsolatedIfNeeded- Proper
@MainActorconformance in the generated extensionThis aligns well with the PR objective of supporting Publishable subclassing.
Tests/RelayTests/Memoized/SwiftDataMemoizedTests.swift (2)
242-247:@Transientcorrectly excludes test counters from SwiftData persistence.Adding
@Transientto the call count properties ensures they're not persisted with the model, which is appropriate for test instrumentation.
260-272: Platform-specific memoized function follows established pattern.The
calculatePlatformValue()addition with#if os(macOS),@available(macOS 26, *), and@Memoizedis consistent with the same pattern inMainActorMemoizedTests,ObservationMemoizedTests, andPublishableMemoizedTests(as shown in relevant snippets), maintaining test consistency across the memoization test suite.Tests/RelayMacrosTests/Memoized/ImplicitlyIsolatedMemoizedMacroTests.swift (3)
14-14: LGTM on the class rename.The rename from
MainActorMemoizedMacroTeststoImplicitlyIsolatedMemoizedMacroTestsbetter reflects the test's purpose of verifying macro expansion in implicitly isolated contexts.
44-46: Good addition offinalmodifiers for subclassing support.Adding
finalto the generated_areaandareaproperties ensures they cannot be overridden in subclasses, which is correct behavior for macro-generated backing storage and computed properties. This aligns with the PR objective of supporting non-final classes with@Publishable.
115-119: Appropriate handling of availability attributes for stored properties.The comment on line 115-116 correctly documents that stored properties cannot have availability annotations in Swift. The macro correctly applies
@available(macOS 26, *)only to the computed property accessor (line 118) while leaving the backing storage without the availability attribute.Tests/RelayTests/Memoized/ObservationMemoizedTests.swift (3)
241-246: Good use of@ObservationIgnoredon test counters.Adding
@ObservationIgnoredtocalculateBaseAreaCallsCountandcalculateVolumeCallsCountprevents these test instrumentation properties from triggering observation callbacks, which would otherwise interfere with test assertions about the memoized properties' observation behavior.
255-259: Consistent with other test files.The
@Memoized(.fileprivate, "volume")annotation with explicit access control and property name matches the pattern used inMainActorMemoizedTests.swift,PublishableMemoizedTests.swift, andSwiftDataMemoizedTests.swiftas shown in the relevant code snippets.
261-267: Platform-specific test coverage for availability handling.This macOS-specific
calculatePlatformValue()method tests that@Memoizedcorrectly handles@availableattributes. The pattern is consistent with the other memoized test files in the codebase.Macros/RelayMacros/Combine/Common/PropertyPublisherDeclBuilder.swift (3)
11-16: Well-structured builder for PropertyPublisher generation.The struct clearly encapsulates the data needed for generating the nested
PropertyPublisherclass with support for both base classes (inheriting fromRelay.AnyPropertyPublisher) and subclasses (inheriting from superclass'sPropertyPublisher).
42-48: Correct inheritance clause for subclassing support.The conditional inheritance logic properly implements the PR objective: subclasses inherit from
SuperclassType.PropertyPublisherwhile base classes inherit fromRelay.AnyPropertyPublisher. This enables the nested publisher hierarchy to mirror the class hierarchy.
155-171: Good integration with MemoizedMacro extraction.The
memoizedPropertiesPublishersmethod correctly usesMemoizedMacro.extract(from:)to discover@Memoizedfunctions and generate corresponding publishers, while respecting@PublisherIgnoredto exclude marked properties.Tests/RelayTests/Publishable/SubclassedPublishableTests.swift (4)
12-52: Well-structured test for stored property publishing in subclasses.The test correctly validates that:
- Initial subscription receives the current value
- Changing unrelated properties doesn't trigger the publisher
- Changing the observed property triggers both publisher and observation tracking
- Deallocation sends completion signal
This mirrors the pattern in
SubclassedMainActorPublishableTests.swift.
291-321: Good test fixture design for Publishable subclassing.
PublishableAnimalas a non-final@Publishable @Observableclass withDogas a final subclass directly tests the PR objective. The@ObservationIgnoredon the overriddenageproperty prevents double observation registration through the inheritance chain.
311-316: Clarify the purpose of_ = oldValuein didSet.The
_ = oldValuepattern in thedidSetblock appears to be a workaround to prevent compiler warnings about unusedoldValue. However, the explicitdidSetwith@ObservationIgnoredoverride seems intentional for testing observation behavior.Is this pattern (
_ = oldValuein an otherwise emptydidSet) specifically required to trigger observation machinery, or is it just to satisfy the compiler? A brief comment would clarify intent for future readers.
324-355: Good coverage for non-publishable base class scenario.Testing
Catas a@Publishable @Observablesubclass of the plainNonPublishableAnimalbase class validates that the macro correctly handles the case where only the subclass is publishable, not the entire hierarchy.Macros/RelayMacros/Memoized/MemoizedMacro.swift (3)
13-30: Clean extraction API for discovering @memoized functions.The
extract(from:)method provides a well-designed entry point for other components (likePropertyPublisherDeclBuilder) to discover and process@Memoizedannotated functions. Usingtry?to silently fail on non-matching declarations is appropriate for a discovery/filtering use case.
35-65: Good refactor to throwing-based validation.The switch from context-based diagnostics to
throws-based error handling with optionalcontextparameter enables code reuse between the extraction path (wherecontextisnil) and the macro expansion path (wherecontextis provided). The validation logic correctly ensures:
- Function has return type
- No parameters
- No async/throws effects
- Declared in class body (when context available)
138-151: Elegant use of@dynamicMemberLookupfor result access.The
@dynamicMemberLookuppattern onExtractionResultallows callers to access properties from bothValidationResultandParametersseamlessly (e.g.,result.propertyNameandresult.preferredAccessControlLevel). This reduces boilerplate at call sites.Sources/Relay/Combine/Common/AnyPropertyPublisher.swift (2)
16-44: Well-structured base class design for publishable subclassing.The
open class AnyPropertyPublisherprovides a clean foundation for macro-generated subclasses. The use offinalon properties and methods ensures predictable behavior while allowing class inheritance. The lifecycle management indeinitproperly completes the subjects.
78-131: Computed property publisher overloads correctly handle type constraints.The four overloads provide appropriate deduplication semantics:
- Non-Equatable: emits all changes
- Equatable: value-based deduplication
- AnyObject: identity-based deduplication (
===)- AnyObject & Equatable: value-based deduplication (most specific, resolves ambiguity)
Tests/RelayMacrosTests/Publishable/SubclassedImplicitlyIsolatedPublishableMacroTests.swift (3)
17-22: MacroSpec with empty conformances correctly signals subclass scenario.The empty
conformances: []array indicates thatDogalready inheritsPublishableconformance fromAnimal, so the macro should generate overriding publisher infrastructure rather than the base conformance extension. This aligns with thehasPublishableSuperclass = protocols.isEmptylogic inPublishableMacro.swift.
79-118: Subclass macro expansion correctly generates overriding publisher infrastructure.The expected expansion properly demonstrates:
override var publisherreturningDog.PropertyPublisherPropertyPublisherinheriting fromAnimal.PropertyPublisher- Only Dog-specific properties (
breed,isBulldog) exposed as publishers- Proper initialization chain via
super.init(object: object)
120-210: ObservationRegistrar correctly handles isolation boundaries.The generated
ObservationRegistrarproperly:
- Uses
nonisolated(unsafe)for crossing isolation boundaries- Employs
assumeIsolatedIfNeededpattern for MainActor assertions- Maintains proper ordering:
_beginModificationsbefore mutation,_endModificationsafter publishMacros/RelayMacros/Combine/Publishable/PublishableMacro.swift (3)
15-45: Improved validation with throwing semantics and SwiftData warning.The refactored
validatefunction:
- Now throws
DiagnosticsErrorfor clearer error handling- Includes appropriate warning for
@Modelclasses with actionable fix-it- Takes
nodeparameter to enable precise fix-it targeting
109-125: Extension generation correctly propagates availability attributes.The extension inherits availability from the declaration (
declaration.availability ?? []), ensuring thePublishableconformance extension has matching platform requirements.
60-61: Superclass detection logic viaprotocols.isEmptyis correct.The logic
hasPublishableSuperclass = protocols.isEmptyaccurately leverages Swift's macro system behavior: when a type already conforms toPublishablevia its superclass, theconformingToarray is empty (as verified bySubclassedPublishableMacroTestswhereconformances: []), whereas fresh classes receiveconformances: ["Publishable"](as inPublishableMacroTests). The extension is correctly skipped whenprotocols.isEmpty(line 97-99), allowing the subclass to inherit the publisher infrastructure from its parent. This is a sound use of the macro system's conformance tracking.Tests/RelayMacrosTests/Publishable/ImplicitlyIsolatedPublishableMacroTests.swift (2)
53-79: Comprehensive coverage of platform-specific and ignored properties.The test correctly validates:
#if os(macOS)conditional compilation flows through to generated code@PublisherIgnoredproperties are excluded fromPropertyPublisher@Memoized @PublisherIgnoredcombination works correctly
150-216: PropertyPublisher generation with proper MainActor isolation.The generated
PropertyPublishercorrectly:
- Applies
@MainActorisolation at class level- Uses
@MainActor deinitfor cleanup- Includes platform-specific (
#if os(macOS)) property publishers- Propagates availability attributes to individual publishers
Tests/RelayMacrosTests/Publishable/PublishableMacroTests.swift (3)
17-22: MacroSpec configuration for base class Publishable conformance.The
conformances: ["Publishable"]indicates this test validates the base class scenario where thePublishableconformance extension should be generated.
149-215: Nonisolated PropertyPublisher generation.Unlike the
ImplicitlyIsolatedPublishableMacroTests, this test validates the nonisolated path where:
PropertyPublisherhas no actor isolationdeinitis not marked@MainActorassumeIsolatedIfNeededsimply invokes the operation directly
302-306: SimplifiedassumeIsolatedIfNeededfor nonisolated types.In the nonisolated case,
assumeIsolatedIfNeededis correctly simplified to a direct invocation, avoiding unnecessary actor isolation overhead.
|
|
||
| env: | ||
| XCODE_VERSION: "26.1" | ||
| XCODE_VERSION: "26.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub Actions macos-26 runner Xcode 26.0 simulator versions available
💡 Result:
On macos-26 the Xcode and simulator runtimes listed in the runner-images release notes are:
- Xcode: 26.1.1 (stable) and 26.2 (beta). [1]
- Installed simulator runtimes / SDKs:
- iOS Simulator: 26.1, 26.2. [1]
- tvOS Simulator: 26.1, 26.2. [1]
- watchOS Simulator: 26.1, 26.2. [1]
- visionOS Simulator: 26.1, 26.2. [1]
(See the GitHub Actions runner-images macOS-26 release notes for details and exact build paths.) [1][2]
Sources:
[1] actions/runner-images — macOS-26 release notes. [2] actions/runner-images README (Xcode simulator policy).
Critical: Specified Xcode and simulator versions do not exist on macos-26 runners.
The workflow specifies Xcode 26.0 and simulator OS versions 26.0/26.0.1, but GitHub Actions macos-26 runners only provide Xcode 26.1.1 (stable) and 26.2 (beta), with simulator runtimes limited to 26.1 and 26.2 for all platforms (iOS, tvOS, watchOS, visionOS). This configuration will cause CI failures. Update to an available version: either 26.1 or 26.2.
Also applies to: 91-91, 94-94, 97-97, 100-100
🤖 Prompt for AI Agents
.github/workflows/pull-request.yml around lines 15 (and also apply same change
at lines 91, 94, 97, 100): the workflow pins XCODE_VERSION and simulator OS
versions to 26.0/26.0.1 which do not exist on macos-26 runners; update those
values to a supported release (change XCODE_VERSION to "26.1" or "26.2") and
align any simulator runtime variables to "26.1" or "26.2" accordingly so the CI
uses available Xcode and simulator runtimes.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Sources/Relay/Combine/Publishable/Publishable.swift (1)
63-74: Clarify the Publishable conformance in macro attributes.Similar to the first macro overload, both
@attachedattributes declareconformances: Publishable. Please verify whether this is intentional given the reported removal of the Publishable protocol (see previous comments for verification script).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(2 hunks)Sources/Relay/Combine/Common/AnyPropertyPublisher.swift(1 hunks)Sources/Relay/Combine/Common/PublisherIgnored.swift(1 hunks)Sources/Relay/Combine/Publishable/Publishable.swift(5 hunks)Sources/Relay/Documentation.docc/Changelog.md(1 hunks)Sources/Relay/Documentation.docc/HowPublishableWorks.md(1 hunks)Sources/Relay/Documentation.docc/PublishableMacros.md(2 hunks)Sources/Relay/Documentation.docc/Relay.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Sources/Relay/Documentation.docc/HowPublishableWorks.md
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Relay/Combine/Common/PublisherIgnored.swift
🧰 Additional context used
🧬 Code graph analysis (1)
Sources/Relay/Combine/Common/AnyPropertyPublisher.swift (1)
Tests/RelayTests/Publishable/AnyPropertyPublisherTests.swift (1)
lhs(210-212)
🪛 GitHub Actions: Pull request
Sources/Relay/Combine/Common/AnyPropertyPublisher.swift
[error] 24-24: SwiftLint: Empty line after type declaration. Violation: Type declaration should start with an empty line. (empty_line_after_type_declaration)
[error] 33-33: SwiftLint: Empty line after type declaration. Violation: Type declaration should start with an empty line. (empty_line_after_type_declaration)
🪛 LanguageTool
Sources/Relay/Documentation.docc/Changelog.md
[grammar] ~7-~7: Ensure spelling is correct
Context: ...ue some Publisher<Output, Never> type instread of an erased `AnyPublisher<Output, Neve...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 SwiftLint (0.57.0)
Sources/Relay/Combine/Common/AnyPropertyPublisher.swift
[Warning] 24-24: Type declaration should start with an empty line.
(empty_line_after_type_declaration)
[Warning] 33-33: Type declaration should start with an empty line.
(empty_line_after_type_declaration)
🔇 Additional comments (14)
Sources/Relay/Documentation.docc/Relay.md (1)
9-12: Documentation index updated to include Changelog.The addition of the Changelog section is appropriate and follows standard documentation organization. This allows users to easily find breaking changes and version information.
Sources/Relay/Documentation.docc/PublishableMacros.md (2)
13-15: Documentation reflects updated macro integration model.The rewrite appropriately updates the description to emphasize compatibility with
@Observableand other macros built onObservation, aligning with the PR's objective of supporting non-final classes. Removing the@Modelreference is consistent with the Changelog entry (version 3.0) that indicates@Modelcompatibility limitations.
56-59: Topics section reorganization supports new macro surface.Adding
PublisherIgnored()and theHowPublishableWorksanchor reflects the expanded macro surface. These additions are appropriate for documenting helper macros and detailed behavioral documentation.README.md (2)
27-29: README updated to reflect macro integration scope.The text updates properly reflect that
@Publishableis designed for@Observablemacros and other Observation-based macros. This is consistent with the documentation changes in PublishableMacros.md and aligns with the PR's focus on non-final class support.
79-79: Clarified macro compatibility in verification section.Changing the reference from
@Modelto only@Observableis consistent with version 3.0 changelog entry and accurately reflects current macro compatibility without@Modelsupport.Sources/Relay/Documentation.docc/Changelog.md (2)
9-12: Version 3.0: AnyPropertyPublisher non-generic change clearly documented.This entry clearly explains the breaking change and how it enables subclassing support—the key objective of this PR. The explanation that generated subclasses still expose specialized publishers (e.g.,
personWillChange,personDidChange) properly sets expectations for migrating users.
14-18: Version 3.0: @model incompatibility clearly explained.The detailed justification for why
@Modelcompatibility was removed (SwiftData reflection and assertion issues) demonstrates thoughtful API design. This prevents users from silently encountering runtime failures and documents a known limitation upfront.Sources/Relay/Combine/Common/AnyPropertyPublisher.swift (5)
11-16: LGTM! Appropriate use ofopenclass.The
openclass declaration correctly enables subclassing, which aligns with the PR objective of supporting Publishable on non-final classes.
41-44: LGTM! Initializer designed for macro-generated subclasses.The unused
objectparameter and private documentation visibility are appropriate for this base class that will be extended by macro-generated code.
46-49: LGTM! Proper Combine publisher cleanup.The deinit correctly sends completion events to both subjects, ensuring proper cleanup.
74-81: LGTM! Clean stored property publisher implementation.The use of
prependensures subscribers immediately receive the current property value, which is the expected behavior for property publishers.
86-137: Theunownedcapture design is safe and intentional. Tests comprehensively verify that publishers complete when the object deallocates (confirmed by checkingcompletion == .finishedafterobject = nilin all test cases). The macro-generatedPropertyPublisherholds anunowned let objectreference and is tightly coupled to the object's lifecycle—it cannot outlive the object. The swiftlint disables forunowned_variable_captureare justified by this architecture.Likely an incorrect or invalid review comment.
Sources/Relay/Combine/Publishable/Publishable.swift (2)
27-38: Clarify the Publishable conformance in macro attributes.Both
@attachedattributes declareconformances: Publishable, but the AI summary indicates the Publishable protocol was removed. In Swift's macro system, theconformancesparameter should list actual protocols that the macro will add conformance to. Please verify:
- Is
Publishablea marker protocol defined elsewhere that's still in use?- If the protocol was removed, should these conformance declarations be updated or removed?
The verification script from the previous comment will help confirm whether the Publishable protocol exists in the codebase.
75-76: The macro implementation properly handles the existential parameter.The macro uses
ParameterExtractorto extract the(any GlobalActor.Type)?isolation parameter and wraps it in aGlobalActorIsolationtype for consistent handling throughout macro expansion. This approach abstracts away type erasure concerns at the macro implementation level.Backward compatibility is maintained through two overloads: the parameterless
@Publishable()macro (which infers isolation automatically) and the explicit@Publishable(isolation:)variant with the existential parameter. Both delegate to the same macro implementation, so existing code remains unaffected.The switch from generic to existential is intentional and aligns with supporting non-final classes and subclassing, as the existential type allows callers to pass any
GlobalActortype dynamically.
| private final let _willChange = PassthroughSubject<Void, Never>() | ||
| private final let _didChange = PassthroughSubject<Void, Never>() | ||
|
|
||
| /// Emits **before** any of the ``Publishable`` object's stored properties are assigned a new value. | ||
| /// | ||
| /// Generated subclasses also expose specialized publisher that emits the ``Publishable`` object itself, | ||
| /// named using the class name as a prefix. For example, a class named `Person` will provide a `personWillChange` publisher. | ||
| /// | ||
| public final var willChange: some Publisher<Void, Never> { | ||
| _willChange | ||
| } | ||
|
|
||
| /// Emits **after** any of the ``Publishable`` object's stored properties are assigned a new value. | ||
| /// | ||
| /// Generated subclasses also expose specialized publisher that emits the ``Publishable`` object itself, | ||
| /// named using the class name as a prefix. For example, a class named `Person` will provide a `personDidChange` publisher. | ||
| /// | ||
| public final var didChange: some Publisher<Void, Never> { | ||
| _didChange | ||
| } |
There was a problem hiding this comment.
Fix SwiftLint violations blocking CI.
Missing empty lines after type declarations at lines 24 and 33 are causing pipeline failures.
Apply this diff to fix the violations:
/// Generated subclasses also expose specialized publisher that emits the ``Publishable`` object itself,
/// named using the class name as a prefix. For example, a class named `Person` will provide a `personWillChange` publisher.
///
+
public final var willChange: some Publisher<Void, Never> {
_willChange
}
/// Emits **after** any of the ``Publishable`` object's stored properties are assigned a new value.
///
/// Generated subclasses also expose specialized publisher that emits the ``Publishable`` object itself,
/// named using the class name as a prefix. For example, a class named `Person` will provide a `personDidChange` publisher.
///
+
public final var didChange: some Publisher<Void, Never> {
_didChange
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private final let _willChange = PassthroughSubject<Void, Never>() | |
| private final let _didChange = PassthroughSubject<Void, Never>() | |
| /// Emits **before** any of the ``Publishable`` object's stored properties are assigned a new value. | |
| /// | |
| /// Generated subclasses also expose specialized publisher that emits the ``Publishable`` object itself, | |
| /// named using the class name as a prefix. For example, a class named `Person` will provide a `personWillChange` publisher. | |
| /// | |
| public final var willChange: some Publisher<Void, Never> { | |
| _willChange | |
| } | |
| /// Emits **after** any of the ``Publishable`` object's stored properties are assigned a new value. | |
| /// | |
| /// Generated subclasses also expose specialized publisher that emits the ``Publishable`` object itself, | |
| /// named using the class name as a prefix. For example, a class named `Person` will provide a `personDidChange` publisher. | |
| /// | |
| public final var didChange: some Publisher<Void, Never> { | |
| _didChange | |
| } | |
| private final let _willChange = PassthroughSubject<Void, Never>() | |
| private final let _didChange = PassthroughSubject<Void, Never>() | |
| /// Emits **before** any of the ``Publishable`` object's stored properties are assigned a new value. | |
| /// | |
| /// Generated subclasses also expose specialized publisher that emits the ``Publishable`` object itself, | |
| /// named using the class name as a prefix. For example, a class named `Person` will provide a `personWillChange` publisher. | |
| /// | |
| public final var willChange: some Publisher<Void, Never> { | |
| _willChange | |
| } | |
| /// Emits **after** any of the ``Publishable`` object's stored properties are assigned a new value. | |
| /// | |
| /// Generated subclasses also expose specialized publisher that emits the ``Publishable`` object itself, | |
| /// named using the class name as a prefix. For example, a class named `Person` will provide a `personDidChange` publisher. | |
| /// | |
| public final var didChange: some Publisher<Void, Never> { | |
| _didChange | |
| } |
🧰 Tools
🪛 GitHub Actions: Pull request
[error] 24-24: SwiftLint: Empty line after type declaration. Violation: Type declaration should start with an empty line. (empty_line_after_type_declaration)
[error] 33-33: SwiftLint: Empty line after type declaration. Violation: Type declaration should start with an empty line. (empty_line_after_type_declaration)
🪛 SwiftLint (0.57.0)
[Warning] 24-24: Type declaration should start with an empty line.
(empty_line_after_type_declaration)
[Warning] 33-33: Type declaration should start with an empty line.
(empty_line_after_type_declaration)
🤖 Prompt for AI Agents
In Sources/Relay/Combine/Common/AnyPropertyPublisher.swift around lines 18 to
37, SwiftLint reports missing empty lines after the type declarations for the
willChange and didChange properties at lines 24 and 33; add a single blank line
immediately after each of those property declarations (i.e., after the closing
brace of the willChange and didChange computed properties) to satisfy the
empty_line_after_type_declaration rule and re-run linting.
| public final func _beginModifications() { | ||
| pendingModifications += 1 | ||
| if pendingModifications == 1 { | ||
| _willChange.send(()) | ||
| } | ||
| } | ||
|
|
||
| public final func _endModifications() { | ||
| if pendingModifications == 1 { | ||
| _didChange.send(()) | ||
| } | ||
| pendingModifications -= 1 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add defensive guard against counter underflow.
If _endModifications() is called more times than _beginModifications(), the counter could become negative, preventing future didChange events from firing correctly.
Apply this diff to add a defensive check:
public final func _endModifications() {
+ guard pendingModifications > 0 else {
+ assertionFailure("_endModifications() called without matching _beginModifications()")
+ return
+ }
if pendingModifications == 1 {
_didChange.send(())
}
pendingModifications -= 1
}🤖 Prompt for AI Agents
In Sources/Relay/Combine/Common/AnyPropertyPublisher.swift around lines 57-69,
_endModifications() can underflow if called more times than
_beginModifications(); add a defensive guard that returns early (or
logs/asserts) when pendingModifications is already 0, otherwise decrement and
only send _didChange when the counter transitions from 1 to 0. Ensure the
function never decrements below 0 and preserves the existing behavior of sending
_didChange exactly when the outermost modification ends.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tests/RelayMacrosTests/Publishable/ImplicitlyIsolatedPublishableMacroTests.swift (1)
25-327: Address SwiftLintfunction_body_lengthfortestExpansion()As with the explicit‑isolation test, this
testExpansion()body is very long due to the embedded source strings, and SwiftLint reportsfunction_body_lengthdespite the file only disablingtype_body_length. Consider either extracting the large literals into helper constants or disabling the correct rule, e.g.:// swiftlint:disable function_body_length internal final class ImplicitlyIsolatedPublishableMacroTests: XCTestCase { // … }or
// swiftlint:disable:next function_body_lengthabovetestExpansion().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.swiftlint.yml(7 hunks)Tests/RelayMacrosTests/Publishable/ExplicitlyIsolatedPublishableMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Publishable/ImplicitlyIsolatedPublishableMacroTests.swift(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/RelayMacrosTests/Publishable/ImplicitlyIsolatedPublishableMacroTests.swift (3)
Tests/RelayMacrosTests/Publishable/ExplicitlyIsolatedPublishableMacroTests.swift (1)
testExpansion(25-330)Tests/RelayMacrosTests/Publishable/PublishableMacroTests.swift (1)
testExpansion(24-316)Tests/RelayMacrosTests/Publishable/SubclassedImplicitlyIsolatedPublishableMacroTests.swift (1)
testExpansion(24-215)
🪛 SwiftLint (0.57.0)
Tests/RelayMacrosTests/Publishable/ExplicitlyIsolatedPublishableMacroTests.swift
[Error] 25-25: Function body should span 100 lines or less excluding comments and whitespace: currently spans 304 lines
(function_body_length)
Tests/RelayMacrosTests/Publishable/ImplicitlyIsolatedPublishableMacroTests.swift
[Error] 25-25: Function body should span 100 lines or less excluding comments and whitespace: currently spans 304 lines
(function_body_length)
⏰ 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 (maccatalyst)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (ios)
- GitHub Check: build-and-test (visionos)
- GitHub Check: build-and-test (tvos)
🔇 Additional comments (6)
.swiftlint.yml (4)
11-11: Clarify the rationale for deferring style rules to SwiftFormat.Lines 11, 30, 60, 70, and 147 convert several rules (
attributes,discouraged_optional_boolean,indentation_width,modifier_order,yoda_condition) to swiftformat-commented variants, deferring their enforcement to SwiftFormat. While reducing tool duplication is sensible, this change trades SwiftLint's stricter checks for SwiftFormat's formatting.Confirm that:
- SwiftFormat is configured to enforce these style rules consistently.
- The team prefers delegating style enforcement to SwiftFormat over SwiftLint.
- These relaxations do not create gaps in code quality checks, especially for macro-generated code.
Also applies to: 30-30, 60-60, 70-70, 147-147
58-58: Enablingimplicitly_unwrapped_optionalis a good addition.This stricter check helps catch potentially unsafe IUO usage patterns, supporting safer Swift code practices.
176-176:ignores_multiline_strings: trueis a sensible adjustment.Prevents false positives on line length violations when multiline strings are present—a practical change for macro-generated code that may contain long embedded strings.
204-204: Verify theempty_line_after_type_declarationregex adjustment.The regex change to
(?!\\s*\\})adds a negative lookahead that excludes patterns followed by whitespace and a closing brace. Clarify:
- What was the old regex pattern, and how does the new pattern change the rule's behavior?
- Does this change support macro-generated type declarations, or is it a bug fix?
- Will the rule still catch intended violations (e.g., missing empty lines after type declarations)?
Tests/RelayMacrosTests/Publishable/ExplicitlyIsolatedPublishableMacroTests.swift (1)
18-327: MacroSpec wiring and explicit‑isolation expansion look consistentThe
macroSpecsconfiguration and the expected expansion for@Publishable(isolation: MainActor.self)match the structure used in the other Publishable tests (nestedPropertyPublisher, per‑property publishers, observation registrar, and explicit@MainActor Publishableconformance). I don’t see functional mismatches in the snapshot for the explicit‑isolation path.Tests/RelayMacrosTests/Publishable/ImplicitlyIsolatedPublishableMacroTests.swift (1)
16-327: Implicit‑isolation Publishable snapshot matches the new macro behaviorThe updated test class,
macroSpecsusage, and the expected expansion for@MainActor @Publishable @Observablealign with the other Publishable tests: correct removal of@Publishable, injected_publisher/PropertyPublisher, publishers for stored/computed/platform and memoized properties, and the@MainActor Publishableextension. This looks coherent with the intended implicitly isolated behavior.
| func testExpansion() { | ||
| assertMacroExpansion( | ||
| #""" | ||
| @available(iOS 26, macOS 26, *) | ||
| @CustomActor @Publishable(isolation: MainActor.self) @Observable | ||
| public final class Person { | ||
| static var user: Person? | ||
| let id: UUID | ||
| fileprivate(set) var age: Int | ||
| var name: String | ||
| public var surname: String { | ||
| didSet { | ||
| print(oldValue) | ||
| } | ||
| } | ||
| internal var fullName: String { | ||
| "\(name) \(surname)" | ||
| } | ||
| private var initials: String { | ||
| get { "\(name.prefix(1))\(surname.prefix(1))" } | ||
| set { _ = newValue } | ||
| } | ||
| #if os(macOS) | ||
| var platformStoredProperty = 123 | ||
| @available(macOS 26, *) | ||
| var platformComputedProperty: Int { | ||
| platformStoredProperty | ||
| } | ||
| #endif | ||
| @PublisherIgnored | ||
| var ignoredStoredProperty = 123 | ||
| @PublisherIgnored | ||
| var ignoredComputedProperty: Int { | ||
| ignoredStoredProperty | ||
| } | ||
| @available(iOS 26, *) | ||
| @Memoized(.private) | ||
| func makeLabel() -> String { | ||
| "\(fullName), \(age)" | ||
| } | ||
| @Memoized @PublisherIgnored | ||
| func makeIgnoredMemoizedProperty() -> Int { | ||
| ignoredStoredProperty | ||
| } | ||
| } | ||
| """#, | ||
| expandedSource: | ||
| #""" | ||
| @available(iOS 26, macOS 26, *) | ||
| @CustomActor @Observable | ||
| public final class Person { | ||
| static var user: Person? | ||
| let id: UUID | ||
| fileprivate(set) var age: Int | ||
| var name: String | ||
| public var surname: String { | ||
| didSet { | ||
| print(oldValue) | ||
| } | ||
| } | ||
| internal var fullName: String { | ||
| "\(name) \(surname)" | ||
| } | ||
| private var initials: String { | ||
| get { "\(name.prefix(1))\(surname.prefix(1))" } | ||
| set { _ = newValue } | ||
| } | ||
| #if os(macOS) | ||
| var platformStoredProperty = 123 | ||
| @available(macOS 26, *) | ||
| var platformComputedProperty: Int { | ||
| platformStoredProperty | ||
| } | ||
| #endif | ||
| @PublisherIgnored | ||
| var ignoredStoredProperty = 123 | ||
| @PublisherIgnored | ||
| var ignoredComputedProperty: Int { | ||
| ignoredStoredProperty | ||
| } | ||
| @available(iOS 26, *) | ||
| @Memoized(.private) | ||
| func makeLabel() -> String { | ||
| "\(fullName), \(age)" | ||
| } | ||
| @Memoized @PublisherIgnored | ||
| func makeIgnoredMemoizedProperty() -> Int { | ||
| ignoredStoredProperty | ||
| } | ||
| private final lazy var _publisher = PropertyPublisher(object: self) | ||
| /// A ``PropertyPublisher`` which exposes `Combine` publishers for all mutable | ||
| /// or computed instance properties of this object. | ||
| /// | ||
| /// - 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. | ||
| /// | ||
| public var publisher: PropertyPublisher { | ||
| _publisher | ||
| } | ||
| @MainActor public final class PropertyPublisher: Relay.AnyPropertyPublisher { | ||
| private final unowned let object: Person | ||
| public final var personWillChange: some Publisher<Person, Never> { | ||
| willChange.map { [unowned object] _ in | ||
| object | ||
| } | ||
| } | ||
| public final var personDidChange: some Publisher<Person, Never> { | ||
| didChange.map { [unowned object] _ in | ||
| object | ||
| } | ||
| } | ||
| public init(object: Person) { | ||
| self.object = object | ||
| super.init(object: object) | ||
| } | ||
| @MainActor deinit { | ||
| _age.send(completion: .finished) | ||
| _name.send(completion: .finished) | ||
| _surname.send(completion: .finished) | ||
| #if os(macOS) | ||
| _platformStoredProperty.send(completion: .finished) | ||
| #endif | ||
| } | ||
| fileprivate final let _age = PassthroughSubject<Int, Never>() | ||
| final var age: some Publisher<Int, Never> { | ||
| _storedPropertyPublisher(_age, for: \.age, object: object) | ||
| } | ||
| fileprivate final let _name = PassthroughSubject<String, Never>() | ||
| final var name: some Publisher<String, Never> { | ||
| _storedPropertyPublisher(_name, for: \.name, object: object) | ||
| } | ||
| fileprivate final let _surname = PassthroughSubject<String, Never>() | ||
| public final var surname: some Publisher<String, Never> { | ||
| _storedPropertyPublisher(_surname, for: \.surname, object: object) | ||
| } | ||
| #if os(macOS) | ||
| fileprivate final let _platformStoredProperty = PassthroughSubject<Int, Never>() | ||
| final var platformStoredProperty: some Publisher<Int, Never> { | ||
| _storedPropertyPublisher(_platformStoredProperty, for: \.platformStoredProperty, object: object) | ||
| } | ||
| #endif | ||
| internal final var fullName: some Publisher<String, Never> { | ||
| _computedPropertyPublisher(for: \.fullName, object: object) | ||
| } | ||
| fileprivate final var initials: some Publisher<String, Never> { | ||
| _computedPropertyPublisher(for: \.initials, object: object) | ||
| } | ||
| #if os(macOS) | ||
| @available(macOS 26, *) | ||
| final var platformComputedProperty: some Publisher<Int, Never> { | ||
| _computedPropertyPublisher(for: \.platformComputedProperty, object: object) | ||
| } | ||
| #endif | ||
| @available(iOS 26, *) | ||
| fileprivate final var label: some Publisher<String, Never> { | ||
| _computedPropertyPublisher(for: \.label, object: object) | ||
| } | ||
| } | ||
| private enum Observation { | ||
| nonisolated struct ObservationRegistrar: PublishableObservationRegistrar { | ||
| private let underlying = SwiftObservationRegistrar() | ||
| @MainActor private func publish( | ||
| _ object: Person, | ||
| keyPath: KeyPath<Person, some Any> | ||
| ) { | ||
| if keyPath == \.age { | ||
| object.publisher._age.send(object[keyPath: \.age]) | ||
| return | ||
| } | ||
| if keyPath == \.name { | ||
| object.publisher._name.send(object[keyPath: \.name]) | ||
| return | ||
| } | ||
| if keyPath == \.surname { | ||
| object.publisher._surname.send(object[keyPath: \.surname]) | ||
| return | ||
| } | ||
| #if os(macOS) | ||
| if keyPath == \.platformStoredProperty { | ||
| object.publisher._platformStoredProperty.send(object[keyPath: \.platformStoredProperty]) | ||
| return | ||
| } | ||
| #endif | ||
| } | ||
| nonisolated func willSet( | ||
| _ object: Person, | ||
| keyPath: KeyPath<Person, some Any> | ||
| ) { | ||
| nonisolated(unsafe) let keyPath = keyPath | ||
| assumeIsolatedIfNeeded { | ||
| object.publisher._beginModifications() | ||
| underlying.willSet(object, keyPath: keyPath) | ||
| } | ||
| } | ||
| nonisolated func didSet( | ||
| _ object: Person, | ||
| keyPath: KeyPath<Person, some Any> | ||
| ) { | ||
| nonisolated(unsafe) let keyPath = keyPath | ||
| assumeIsolatedIfNeeded { | ||
| underlying.didSet(object, keyPath: keyPath) | ||
| publish(object, keyPath: keyPath) | ||
| object.publisher._endModifications() | ||
| } | ||
| } | ||
| nonisolated func access( | ||
| _ object: Person, | ||
| keyPath: KeyPath<Person, some Any> | ||
| ) { | ||
| underlying.access(object, keyPath: keyPath) | ||
| } | ||
| nonisolated func withMutation<__macro_local_1TfMu_>( | ||
| of object: Person, | ||
| keyPath: KeyPath<Person, some Any>, | ||
| _ mutation: () throws -> __macro_local_1TfMu_ | ||
| ) rethrows -> __macro_local_1TfMu_ { | ||
| nonisolated(unsafe) let mutation = mutation | ||
| nonisolated(unsafe) let keyPath = keyPath | ||
| nonisolated(unsafe) var result: __macro_local_1TfMu_! | ||
| try assumeIsolatedIfNeeded { | ||
| object.publisher._beginModifications() | ||
| defer { | ||
| publish(object, keyPath: keyPath) | ||
| object.publisher._endModifications() | ||
| } | ||
| result = try underlying.withMutation( | ||
| of: object, | ||
| keyPath: keyPath, | ||
| mutation | ||
| ) | ||
| } | ||
| return result | ||
| } | ||
| private nonisolated func assumeIsolatedIfNeeded( | ||
| _ operation: @MainActor () throws -> Void, | ||
| file: StaticString = #fileID, | ||
| line: UInt = #line | ||
| ) rethrows { | ||
| try withoutActuallyEscaping(operation) { operation in | ||
| typealias Nonisolated = () throws -> Void | ||
| let rawOperation = unsafeBitCast(operation, to: Nonisolated.self) | ||
| try MainActor.shared.assumeIsolated( | ||
| { _ in | ||
| try rawOperation() | ||
| }, | ||
| file: file, | ||
| line: line | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| @available(iOS 26, macOS 26, *) extension Person: @MainActor Publishable { | ||
| } | ||
| """#, |
There was a problem hiding this comment.
Fix SwiftLint function_body_length violation in testExpansion()
SwiftLint is flagging testExpansion() for exceeding function_body_length, but the file only disables type_body_length. Either refactor the test to move the large input/expected strings into helpers/constants, or change the directive to target the correct rule, for example:
// swiftlint:disable function_body_length
internal final class ExplicitlyIsolatedPublishableMacroTests: XCTestCase {
// …
}or place a // swiftlint:disable:next function_body_length directly above testExpansion().
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 25-25: Function body should span 100 lines or less excluding comments and whitespace: currently spans 304 lines
(function_body_length)
🤖 Prompt for AI Agents
In
Tests/RelayMacrosTests/Publishable/ExplicitlyIsolatedPublishableMacroTests.swift
around lines 25 to 327, SwiftLint flags testExpansion() for exceeding
function_body_length because only type_body_length is disabled; fix by either
extracting the large multi-line input/expected strings into helper
constants/functions (reducing testExpansion() body length) or add a SwiftLint
disable for function_body_length (either file/class level or immediately above
testExpansion() with // swiftlint:disable:next function_body_length) so the
linter ignores this test's long body.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Sources/Relay/Combine/Publishable/Publishable.swift (1)
23-24: Optional: Call out final subclasses explicitly in the subclassing docsThe subclassing note already describes the right behavior (base classes annotated with
@Publishablecan be subclassed and subclasses must re‑apply the macro for new properties), which matches the non‑final support from Issue #7.If you want to tie it even more directly to the “base + final concrete subclasses” scenario from the issue description, you could mention final subclasses explicitly in the docs. For example:
-/// Classes to which the `@Publishable` macro has been attached can be subclassed. To generate publishers for any properties added in a subclass, -/// the macro must be applied again to the subclass definition. Subclasses should either be isolated to the same global actor as their superclass or remain nonisolated. +/// Classes to which the `@Publishable` macro has been attached can be subclassed, including final subclasses. +/// To generate publishers for any properties added in a subclass, the macro must be applied again to the subclass definition. +/// Subclasses should either be isolated to the same global actor as their superclass or remain nonisolated.(repeated in both doc blocks)
Purely a documentation polish; behavior is already correct.
Also applies to: 62-63
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Macros/RelayMacros/Combine/Common/ObservableMacro.swift(1 hunks)Sources/Relay/Combine/Publishable/Publishable.swift(3 hunks)Sources/Relay/Documentation.docc/Changelog.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Macros/RelayMacros/Combine/Common/ObservableMacro.swift
- Sources/Relay/Documentation.docc/Changelog.md
⏰ 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 (watchos)
- GitHub Check: build-and-test (macos)
- GitHub Check: build-and-test (maccatalyst)
- GitHub Check: build-and-test (tvos)
- GitHub Check: build-and-test (ios)
🔇 Additional comments (2)
Sources/Relay/Combine/Publishable/Publishable.swift (2)
14-16: Clarification that@Publishabledoesn’t synthesizeObservableis spot‑onThe new notes on Lines 14–16 and 53–55 make it explicit that
@Publishablemust be paired with something like@Observablerather than trying to infer/declareObservableitself. That lines up with how Swift macros interoperate with Observation and should prevent confusion about what this macro guarantees.Also applies to: 53-55
81-83: Existentialisolationparameter looks good; please confirm toolchain support and semanticsSwitching the overload to
public macro Publishable( isolation: (any GlobalActor.Type)? )nicely encodes three distinct cases: inferred isolation (
Publishable()), explicitly chosen global actor, andnonisolatedvianil, matching the doc comment.Two things to double‑check:
- That your minimum Swift/Xcode version for the library/CI supports using
(any GlobalActor.Type)?in macro parameter types.- That tests cover all three behaviors (
@Publishable(),@Publishable(isolation: SomeActor.self), and@Publishable(isolation: nil)) so the distinction is locked in.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift (1)
325-358: Consider adding tests forCatandNonPublishableAnimalor removing them.The
NonPublishableAnimalbase class andCatsubclass are defined but never used in any tests. Given that this PR adds subclassing support, these classes may represent an important test scenario: a@Publishablesubclass inheriting from a non-publishable base.Consider either adding tests that exercise these classes or removing them if they're not needed.
Do you want me to generate test methods for the
Catclass to verify the non-publishable base class scenario?Tests/RelayTests/Publishable/SubclassedPublishableTests.swift (3)
291-321: Helper typesPublishableAnimalandDogmatch the subclassing scenario; consider a base-typed testThe base
@Publishable @Observableclass plus finalDogsubclass is a good match for the non-final base/final subclass use case. To more directly exercise the “non-final base” support, consider adding at least one test where the variable is typed asPublishableAnimal(e.g.var animal: PublishableAnimal = Dog()) and you subscribe viaanimal.publisher/withObservationTrackingto ensure the generated API behaves correctly through the base type as well.
326-355:NonPublishableAnimal/Catpair nicely models a publishable subclass over a non-publishable base; consider adding explicit testsDefining a non-publishable base with a
@Publishable @ObservablefinalCatsubclass is exactly the other key scenario from the linked issue. If this pair isn’t already exercised from other test targets, consider adding at least one focused test here that:
- Holds a
Catinstance via aNonPublishableAnimalreference (and viaCatdirectly), and- Verifies property publishers and observation tracking behave as expected.
This would close the loop on the “publishable subclass of non-publishable base” case in this file.
38-38: Minor SwiftLint and naming nits (magic numbers, spelling of “overridden”)SwiftLint is flagging the literal
5at Line 38 and Lines 82–83 as magic numbers. In tests this is often acceptable, but if you want to stay aligned with theno_magic_numbersrule you could either:
- Extract them into a local constant (e.g.
let updatedAge = 5) or- Disable the rule for this file or for those specific lines if you consider the literals self-explanatory.
Also, the test names
overridenStoredProperty(Line 55) andoverridenComputedProperty(Line 143) have a small typo; consider renaming tooverriddenStoredProperty/overriddenComputedPropertyfor clarity.Also applies to: 82-83, 55-56, 143-143
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/Relay/Documentation.docc/Changelog.md(1 hunks)Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift(1 hunks)Tests/RelayTests/Publishable/SubclassedPublishableTests.swift(1 hunks)
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift
[Warning] 39-39: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 83-83: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 84-84: Magic numbers should be replaced by named constants
(no_magic_numbers)
Tests/RelayTests/Publishable/SubclassedPublishableTests.swift
[Warning] 38-38: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 82-82: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 83-83: Magic numbers should be replaced by named constants
(no_magic_numbers)
⏰ 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 (ios)
- GitHub Check: build-and-test (visionos)
- GitHub Check: build-and-test (tvos)
- GitHub Check: build-and-test (macos)
- GitHub Check: build-and-test (maccatalyst)
🔇 Additional comments (12)
Sources/Relay/Documentation.docc/Changelog.md (1)
1-23: Changelog documentation is clear and comprehensive.The typo fix ("instread" → "instead") has been successfully applied. The Version 3.0 changelog entries are well-structured and provide clear context for each breaking change, with concrete examples (e.g.,
personWillChangeandpersonDidChangefor aPersonclass) and detailed rationale for the @model compatibility warning. The documentation effectively communicates the subclassing support improvements and API shape changes aligned with the PR objectives.Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift (5)
15-53: Test structure looks solid.The test correctly verifies that the
nameproperty publisher emits values only whennamechanges, and properly tests observation tracking and cleanup behavior.
98-141: LGTM!The test correctly verifies that the computed property
isBulldogpublisher emits when its dependency (breed) changes, and doesn't emit for unrelated property changes.
143-186: LGTM!The test correctly verifies that the overridden computed property
descriptionemits when its dependencies (breedandage) change.
191-238: LGTM!The test correctly verifies that the
dogWillChangepublisher fires before any property modification, and properly uses identity comparison (===) for class instances.
240-287: LGTM!The test correctly verifies that the
dogDidChangepublisher fires after property modifications.Tests/RelayTests/Publishable/SubclassedPublishableTests.swift (6)
14-52:storedPropertytest correctly exercises base publishable property vs ignored overrideThe expectations around initial emission, ignoring
agechanges for anamesubscription, and completion ondog = nilall align with the intended semantics and nicely cover the subclass scenario with an overriddenagemarked@ObservationIgnored.
54-92:overridenStoredPropertytest gives good coverage of overridden stored property behaviorThis test complements
storedPropertywell by asserting thatagestill publishes and is observable despite the@ObservationIgnoredoverride, and that unrelatednamechanges are ignored for theagepublisher.
97-140:computedPropertytest cleanly validates publisher/observation for subclass-only computed stateSubscribing to
isBulldog, driving it viabreedmutations, and asserting no events on unrelatednamechanges gives clear coverage that computed properties in the subclass are tracked and published correctly.
142-185:overridenComputedPropertythoroughly covers overridden computed property notificationsUsing the overridden
descriptionto track changes driven bybreedandage, while confirmingnamechanges don’t emit, is a solid way to validate dependency tracking across an overridden computed property in the subclass.
190-237:willChangetest effectively validates object-level “will change” aggregationAsserting that
dogWillChangefires (with identity preserved via===) for mutations of all relevant properties, and that the observation tracking callback also fires, gives good confidence that the aggregated “will change” publisher is wired correctly on the subclass.
239-286:didChangetest mirrorswillChangeand gives symmetric coverageThis test mirrors the
willChangebehavior fordogDidChangeand looks consistent and correct, ensuring both phases of aggregation behave as expected for the subclass.
| @Test | ||
| func overridenStoredProperty() { | ||
| var dog: Dog? = .init() | ||
| var publishableQueue = [Int]() | ||
| nonisolated(unsafe) var observationsQueue = [Bool]() | ||
|
|
||
| var completion: Subscribers.Completion<Never>? | ||
| let cancellable = dog?.publisher.age.sink( | ||
| receiveCompletion: { completion = $0 }, | ||
| receiveValue: { publishableQueue.append($0) } | ||
| ) | ||
|
|
||
| func observe() { | ||
| withObservationTracking { | ||
| _ = dog?.age | ||
| } onChange: { | ||
| observationsQueue.append(true) | ||
| } | ||
| } | ||
|
|
||
| observe() | ||
| #expect(publishableQueue.popFirst() == 0) | ||
| #expect(observationsQueue.popFirst() == nil) | ||
|
|
||
| dog?.name = "Paco" | ||
| #expect(publishableQueue.popFirst() == nil) | ||
| #expect(observationsQueue.popFirst() == nil) | ||
|
|
||
| dog?.age = 5 | ||
| #expect(publishableQueue.popFirst() == 5) | ||
| #expect(observationsQueue.popFirst() == true) | ||
| observe() | ||
|
|
||
| dog = nil | ||
| #expect(publishableQueue.isEmpty) | ||
| #expect(observationsQueue.isEmpty) | ||
| #expect(completion == .finished) | ||
| cancellable?.cancel() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the Dog class definition and check the age property
rg -n -B5 -A15 "class Dog" Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift | head -100Repository: NSFatalError/Relay
Length of output: 552
🏁 Script executed:
# Search for @ObservationIgnored usage in the test file
rg -n -B3 -A3 '@ObservationIgnored' Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swiftRepository: NSFatalError/Relay
Length of output: 423
🏁 Script executed:
# Check the entire Dog class to see all property definitions
rg -n "var age" Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swiftRepository: NSFatalError/Relay
Length of output: 181
🏁 Script executed:
# Let's see the PublishableAnimal class definition to understand inheritance
rg -n -B10 -A30 "class PublishableAnimal" Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift | head -80Repository: NSFatalError/Relay
Length of output: 1057
🏁 Script executed:
# Let's look at the full test method to understand the flow
sed -n '55,93p' Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swiftRepository: NSFatalError/Relay
Length of output: 1237
🏁 Script executed:
# Check if there are other tests with @ObservationIgnored overrides to see expected behavior
rg -n -B10 -A10 "overridenStoredProperty\|@ObservationIgnored" Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift | grep -A10 -B10 "func\|@ObservationIgnored"Repository: NSFatalError/Relay
Length of output: 44
🏁 Script executed:
# Search for other tests with @ObservationIgnored to understand intended behavior
rg -n -B15 -A15 "\.popFirst().*true" Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift | grep -B20 -A5 "@ObservationIgnored\|observationsQueue"Repository: NSFatalError/Relay
Length of output: 6926
🏁 Script executed:
# Look for any comments or documentation about @ObservationIgnored behavior
rg -n "ObservationIgnored" Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift -B5 -A10Repository: NSFatalError/Relay
Length of output: 800
🏁 Script executed:
# Search for similar test patterns in other Publishable test files
fd ".*PublishableTests.*" Tests/RelayTests/ --type fRepository: NSFatalError/Relay
Length of output: 318
🏁 Script executed:
# Check if there's a test for the parent class behavior to compare
rg -n "func.*StoredProperty\|func.*override" Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swiftRepository: NSFatalError/Relay
Length of output: 44
🏁 Script executed:
# Let's look at tests in other Publishable test files to see if @ObservationIgnored behavior is documented
rg -n "@ObservationIgnored\|overridenStoredProperty\|override var" Tests/RelayTests/Publishable/SubclassedPublishableTests.swift -B3 -A8Repository: NSFatalError/Relay
Length of output: 44
🏁 Script executed:
# Check if there are any comments or documentation about what @ObservationIgnored does
rg -n "ObservationIgnored" . --type=swift -B5 -A5 | grep -i "comment\|//\|///\|purpose\|ignore" | head -40Repository: NSFatalError/Relay
Length of output: 4098
🏁 Script executed:
# Verify what @PublisherIgnored does and confirm the independent nature
cat Sources/Relay/Combine/Common/PublisherIgnored.swiftRepository: NSFatalError/Relay
Length of output: 887
🏁 Script executed:
# Let's check if there's any test showing @ObservationIgnored actually preventing something
# Look for tests where @ObservationIgnored properties are compared to non-ignored ones
rg -n "observationsQueue\|publishableQueue" Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift | grep -B20 "popFirst().*nil" | head -60Repository: NSFatalError/Relay
Length of output: 44
🏁 Script executed:
# Check the overridenComputedProperty test more carefully - it has computed property with @ObservationIgnored override
sed -n '143,186p' Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swiftRepository: NSFatalError/Relay
Length of output: 1440
🏁 Script executed:
# Let's check if there are tests that specifically verify @ObservationIgnored prevents observation
# Look for tests in Memoized that use @ObservationIgnored
sed -n '240,250p' Tests/RelayTests/Memoized/ObservationMemoizedTests.swiftRepository: NSFatalError/Relay
Length of output: 403
🏁 Script executed:
# Check if there's a pattern where @ObservationIgnored actually prevents something
rg -n "@ObservationIgnored" Tests/RelayTests/Memoized/ -A15 | head -80Repository: NSFatalError/Relay
Length of output: 5466
🏁 Script executed:
# Let's search for any tests that show @ObservationIgnored preventing observation tracking
rg -n "observationsQueue.*nil" Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift -B10 | grep -A10 "@ObservationIgnored\|age"Repository: NSFatalError/Relay
Length of output: 577
🏁 Script executed:
# Let's look at the first test (storedProperty) to understand the baseline behavior
sed -n '13,52p' Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swiftRepository: NSFatalError/Relay
Length of output: 1295
🏁 Script executed:
# Check the base class PublishableAnimal - does age have any annotation?
sed -n '290,302p' Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swiftRepository: NSFatalError/Relay
Length of output: 312
🏁 Script executed:
# Let's check if there's documentation or comments about how @ObservationIgnored should work with overrides
rg -n "@ObservationIgnored" Sources/ --type=swift -B5 -A10 | head -100Repository: NSFatalError/Relay
Length of output: 1447
🏁 Script executed:
# Check the macro implementation to see if it has special handling for overrides
cat Macros/RelayMacros/Combine/Common/ObservationIgnoredMacro.swiftRepository: NSFatalError/Relay
Length of output: 684
🏁 Script executed:
# Confirm that line 85 expects observation to fire for an overridden property
sed -n '55,93p' Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift | grep -n "observationsQueue"Repository: NSFatalError/Relay
Length of output: 389
🏁 Script executed:
# Check if there's any documentation explaining why overrides are excluded
rg -n "overrideSpecifier\|override" Macros/RelayMacros/ --type=swift -B3 -A3 | head -60Repository: NSFatalError/Relay
Length of output: 44
Line 85 expects observation tracking to fire for an overridden property, but the macro implementation explicitly excludes overridden properties.
The isStoredObservationTracked property in ObservationIgnoredMacro.swift requires underlying.overrideSpecifier == nil—meaning overridden properties never trigger observation tracking. Since Dog.age is an overridden property (line 313), the withObservationTracking block should not fire, and line 85 should expect nil instead of true.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 83-83: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 84-84: Magic numbers should be replaced by named constants
(no_magic_numbers)
🤖 Prompt for AI Agents
In Tests/RelayTests/Publishable/SubclassedMainActorPublishableTests.swift around
lines 55 to 93, the test expects observation tracking to fire for an overridden
stored property but the macro explicitly excludes overridden properties; update
the assertion at the point where observationsQueue.popFirst() is expected to be
true (around line 85) to assert nil instead (i.e., expect no observation), so
the test matches the macro behavior that overridden properties are not tracked.
Resolves #7
Summary by CodeRabbit
New Features
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.