Added Relayed macro #9
Conversation
WalkthroughAdds the Relayed macro family (Relayed, RelayedProperty) and observation/publisher suppression macros (ObservationSuppressed, PublisherSuppressed); replaces PublisherIgnored with PublisherSuppressed; refactors publisher/observation builders and memoization to route through the registrar and unify IfConfig handling; and updates many tests and docs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tests/RelayTests/Combine/Publishable/SubclassedMainActorPublishableTests.swift (1)
312-312: Clarify whether@ObservationIgnoredshould be updated.The AI summary indicates that this PR replaces
ObservationIgnoredwithObservationSuppressed. However, these properties still use@ObservationIgnored. Should these be updated to@ObservationSuppressedfor consistency with the PR objectives, or is the old attribute intentionally maintained for backward compatibility?Based on the AI-generated summary, which describes the replacement of these macros.
Also applies to: 347-347
🧹 Nitpick comments (7)
Sources/Relay/Documentation.docc/ChoosingBetweenRelayedAndPublishableMacros.md (1)
36-37: Consider neutral phrasing for technical documentation.The first-person opinion "I would refrain from calling it a hack" is somewhat informal for API documentation. Consider more neutral phrasing such as: "This approach relies solely on well-understood behaviors of Swift and its public APIs, making it a valid technique rather than a workaround."
Tests/RelayMacrosTests/Combine/Relayed/SubclassedImplicitlyIsolatedRelayedMacroTests.swift (1)
117-119: Minor: Extra blank line before closing brace.There's an extra blank line at Line 118 before the closing brace of the
PropertyPublisherclass. This is a very minor formatting nit in the expected output string.final var isBulldog: some Publisher<Bool, Never> { _computedPropertyPublisher(for: \.isBulldog, object: object) } - }Tests/RelayMacrosTests/Memoized/RelayedObservationSuppressedMemoizedMacroTests.swift (1)
61-67: Inconsistent nullability handling pattern compared to RelayedMemoizedMacroTests.The
invalidateCachefunction here uses optional chaining (instance?.publisher...), whileRelayedMemoizedMacroTests.swiftusesguard let instance else { return }with non-optional access. Both work, but the guard pattern is slightly safer as it ensures all-or-nothing execution within the closure.Consider aligning the pattern across both test expectations if the underlying macro builder should produce consistent output.
Tests/RelayMacrosTests/Combine/Relayed/SubclassedRelayedMacroTests.swift (1)
117-118: Minor: Empty line in expanded source.There's an extra blank line (line 117) in the expected
expandedSourcebefore the closing brace ofPropertyPublisher. This is cosmetic but worth verifying if this matches the actual macro output.Tests/RelayTests/Combine/Relayed/SubclassedMainActorRelayedTests.swift (1)
312-316: Clarify the purpose of_ = oldValuein didSet.The override includes an empty-ish
didSetwith only_ = oldValue. If this is intentional to test that overriding a property with a custom observer still works with the macro-generated publishers, consider adding a brief comment explaining this pattern for future maintainers.override var age: Int { didSet { + // Forces property override to verify macro compatibility with custom observers _ = oldValue } }Also applies to: 346-350
Tests/RelayTests/Combine/Relayed/MainActorRelayedTests.swift (1)
104-151:willChangeanddidChangetests are nearly identical.Both tests have identical structure, assertions, and behavior. The only difference is which publisher is subscribed to (
personWillChangevspersonDidChange). Consider extracting common logic into a parameterized helper to reduce duplication.Example helper approach:
private func testChangeNotification( publisher: (Person.PropertyPublisher) -> some Publisher<Person, Never>, name: String ) { // shared test logic }Also applies to: 153-200
Macros/RelayMacros/Combine/Relayed/RelayedPropertyMacro.swift (1)
76-81: Consider simplifying bindings replacement.Using
replaceSubrangeto replace the entire collection with a single element works but is somewhat indirect. A more straightforward approach would be to construct a newPatternBindingListSyntaxdirectly.let pattern = PatternSyntax(IdentifierPatternSyntax(identifier: name)) let binding = property.binding.with(\.pattern, pattern) - var bindings = property.bindings - - bindings.replaceSubrange( - bindings.startIndex ..< bindings.endIndex, - with: CollectionOfOne(binding) - ) + let bindings = PatternBindingListSyntax([binding]) let storage = property.trimmed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (64)
Macros/Dependencies/PrincipleMacros(1 hunks)Macros/RelayMacros/Combine/Common/ObservationIgnoredMacro.swift(0 hunks)Macros/RelayMacros/Combine/Common/ObservationSuppressedMacro.swift(1 hunks)Macros/RelayMacros/Combine/Common/PropertyPublisherDeclBuilder.swift(4 hunks)Macros/RelayMacros/Combine/Common/PublisherSuppressedMacro.swift(3 hunks)Macros/RelayMacros/Combine/Publishable/ObservationRegistrarDeclBuilder.swift(1 hunks)Macros/RelayMacros/Combine/Publishable/PublishableMacro.swift(7 hunks)Macros/RelayMacros/Combine/Relayed/ObservableDeclBuilder.swift(1 hunks)Macros/RelayMacros/Combine/Relayed/RelayedMacro.swift(1 hunks)Macros/RelayMacros/Combine/Relayed/RelayedPropertyDeclAccessorBuilder.swift(1 hunks)Macros/RelayMacros/Combine/Relayed/RelayedPropertyMacro.swift(1 hunks)Macros/RelayMacros/Main/RelayPlugin.swift(1 hunks)Macros/RelayMacros/Memoized/MemoizedDeclBuilder.swift(4 hunks)Macros/RelayMacros/Memoized/MemoizedMacro.swift(6 hunks)README.md(2 hunks)Sources/Relay/Combine/Common/ObservationSuppressed.swift(1 hunks)Sources/Relay/Combine/Common/PublisherIgnored.swift(0 hunks)Sources/Relay/Combine/Common/PublisherSuppressed.swift(1 hunks)Sources/Relay/Combine/Publishable/Publishable.swift(4 hunks)Sources/Relay/Combine/Relayed/Relayed.swift(1 hunks)Sources/Relay/Combine/Relayed/RelayedProperty.swift(1 hunks)Sources/Relay/Documentation.docc/Changelog.md(1 hunks)Sources/Relay/Documentation.docc/ChoosingBetweenRelayedAndPublishableMacros.md(1 hunks)Sources/Relay/Documentation.docc/HowPublishableWorks.md(0 hunks)Sources/Relay/Documentation.docc/MemoizedMacros.md(1 hunks)Sources/Relay/Documentation.docc/Relay.md(1 hunks)Sources/Relay/Documentation.docc/RelayedAndPublishableMacros.md(3 hunks)Tests/RelayMacrosTests/Combine/Publishable/ExplicitlyIsolatedPublishableMacroTests.swift(6 hunks)Tests/RelayMacrosTests/Combine/Publishable/ImplicitlyIsolatedPublishableMacroTests.swift(6 hunks)Tests/RelayMacrosTests/Combine/Publishable/NonisolatedPublishableMacroTests.swift(6 hunks)Tests/RelayMacrosTests/Combine/Publishable/PublishableMacroTests.swift(6 hunks)Tests/RelayMacrosTests/Combine/Publishable/SubclassedImplicitlyIsolatedPublishableMacroTests.swift(2 hunks)Tests/RelayMacrosTests/Combine/Publishable/SubclassedPublishableMacroTests.swift(2 hunks)Tests/RelayMacrosTests/Combine/Relayed/ExplicitlyIsolatedRelayedMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Combine/Relayed/ImplicitlyIsolatedRelayedMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Combine/Relayed/NonisolatedRelayedMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Combine/Relayed/RelayedMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Combine/Relayed/RelayedPropertyMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Combine/Relayed/SubclassedImplicitlyIsolatedRelayedMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Combine/Relayed/SubclassedRelayedMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Memoized/ExplicitlyIsolatedMemoizedMacroTests.swift(4 hunks)Tests/RelayMacrosTests/Memoized/ImplicitlyIsolatedMemoizedMacroTests.swift(4 hunks)Tests/RelayMacrosTests/Memoized/MemoizedMacroTests.swift(4 hunks)Tests/RelayMacrosTests/Memoized/NonisolatedMemoizedMacroTests.swift(4 hunks)Tests/RelayMacrosTests/Memoized/ObservationSuppressedMemoizedMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Memoized/RelayedMemoizedMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Memoized/RelayedObservationSuppressedMemoizedMacroTests.swift(1 hunks)Tests/RelayMacrosTests/Memoized/RelayedPublisherSuppressedMemoizedMacroTests.swift(1 hunks)Tests/RelayTests/Combine/Publishable/AnyPropertyPublisherPublishableTests.swift(1 hunks)Tests/RelayTests/Combine/Publishable/MainActorPublishableTests.swift(2 hunks)Tests/RelayTests/Combine/Publishable/ObservablePublishableTests.swift(5 hunks)Tests/RelayTests/Combine/Publishable/SubclassedMainActorPublishableTests.swift(4 hunks)Tests/RelayTests/Combine/Publishable/SubclassedPublishableTests.swift(4 hunks)Tests/RelayTests/Combine/Relayed/AnyPropertyPublisherRelayedTests.swift(4 hunks)Tests/RelayTests/Combine/Relayed/MainActorRelayedTests.swift(1 hunks)Tests/RelayTests/Combine/Relayed/RelayedTests.swift(1 hunks)Tests/RelayTests/Combine/Relayed/SubclassedMainActorRelayedTests.swift(1 hunks)Tests/RelayTests/Combine/Relayed/SubclassedRelayedTests.swift(1 hunks)Tests/RelayTests/Helpers/_Observable.swift(1 hunks)Tests/RelayTests/Memoized/MainActorMemoizedTests.swift(1 hunks)Tests/RelayTests/Memoized/ObservationMemoizedTests.swift(1 hunks)Tests/RelayTests/Memoized/PublishableMemoizedTests.swift(2 hunks)Tests/RelayTests/Memoized/RelayedMemoizedTests.swift(1 hunks)Tests/RelayTests/Memoized/SwiftDataMemoizedTests.swift(1 hunks)
💤 Files with no reviewable changes (3)
- Sources/Relay/Documentation.docc/HowPublishableWorks.md
- Sources/Relay/Combine/Common/PublisherIgnored.swift
- Macros/RelayMacros/Combine/Common/ObservationIgnoredMacro.swift
🧰 Additional context used
🧬 Code graph analysis (14)
Macros/RelayMacros/Combine/Common/PropertyPublisherDeclBuilder.swift (1)
Macros/RelayMacros/Memoized/MemoizedMacro.swift (1)
extract(15-40)
Tests/RelayTests/Memoized/MainActorMemoizedTests.swift (4)
Tests/RelayTests/Memoized/PublishableMemoizedTests.swift (1)
calculateIgnoredValue(205-208)Tests/RelayTests/Memoized/ObservationMemoizedTests.swift (1)
calculateIgnoredValue(261-264)Tests/RelayTests/Memoized/RelayedMemoizedTests.swift (1)
calculateIgnoredValue(205-208)Tests/RelayTests/Memoized/SwiftDataMemoizedTests.swift (1)
calculateIgnoredValue(266-269)
Tests/RelayTests/Memoized/SwiftDataMemoizedTests.swift (4)
Tests/RelayTests/Memoized/PublishableMemoizedTests.swift (1)
calculateIgnoredValue(205-208)Tests/RelayTests/Memoized/MainActorMemoizedTests.swift (1)
calculateIgnoredValue(263-266)Tests/RelayTests/Memoized/ObservationMemoizedTests.swift (1)
calculateIgnoredValue(261-264)Tests/RelayTests/Memoized/RelayedMemoizedTests.swift (1)
calculateIgnoredValue(205-208)
Tests/RelayTests/Combine/Relayed/RelayedTests.swift (1)
Tests/RelayTests/Helpers/Array+PopFirst.swift (1)
popFirst(11-16)
Tests/RelayMacrosTests/Combine/Relayed/SubclassedImplicitlyIsolatedRelayedMacroTests.swift (3)
Tests/RelayMacrosTests/Combine/Relayed/SubclassedRelayedMacroTests.swift (1)
testExpansion(24-153)Tests/RelayMacrosTests/Combine/Relayed/ImplicitlyIsolatedRelayedMacroTests.swift (1)
testExpansion(24-293)Tests/RelayMacrosTests/Combine/Relayed/RelayedMacroTests.swift (1)
testExpansion(24-292)
Tests/RelayMacrosTests/Combine/Relayed/ImplicitlyIsolatedRelayedMacroTests.swift (4)
Tests/RelayMacrosTests/Combine/Relayed/SubclassedImplicitlyIsolatedRelayedMacroTests.swift (1)
testExpansion(24-154)Tests/RelayMacrosTests/Combine/Publishable/ImplicitlyIsolatedPublishableMacroTests.swift (1)
testExpansion(24-344)Tests/RelayMacrosTests/Combine/Relayed/NonisolatedRelayedMacroTests.swift (1)
testExpansion(24-292)Tests/RelayMacrosTests/Combine/Relayed/RelayedMacroTests.swift (1)
testExpansion(24-292)
Macros/RelayMacros/Combine/Relayed/RelayedMacro.swift (5)
Macros/RelayMacros/Combine/Relayed/RelayedPropertyMacro.swift (4)
validateNode(28-37)expansion(42-53)expansion(58-89)shouldAttach(15-23)Macros/RelayMacros/Combine/Publishable/PublishableMacro.swift (3)
validateNode(15-61)expansion(66-101)expansion(106-142)Macros/RelayMacros/Combine/Common/PropertyPublisherDeclBuilder.swift (2)
build(18-40)inheritanceClause(42-48)Macros/RelayMacros/Combine/Publishable/ObservationRegistrarDeclBuilder.swift (1)
build(30-50)Macros/RelayMacros/Combine/Relayed/ObservableDeclBuilder.swift (1)
build(24-32)
Tests/RelayTests/Combine/Publishable/ObservablePublishableTests.swift (3)
Tests/RelayTests/Combine/Publishable/MainActorPublishableTests.swift (3)
iOS(239-243)makeIgnoredMemoizedProperty(245-248)makePlatformMemoizedProperty(251-254)Tests/RelayTests/Combine/Relayed/MainActorRelayedTests.swift (3)
iOS(245-249)makeIgnoredMemoizedProperty(251-254)makePlatformMemoizedProperty(257-260)Tests/RelayTests/Combine/Relayed/RelayedTests.swift (3)
iOS(244-248)makeIgnoredMemoizedProperty(250-253)makePlatformMemoizedProperty(256-259)
Macros/RelayMacros/Combine/Publishable/PublishableMacro.swift (1)
Macros/RelayMacros/Combine/Relayed/RelayedMacro.swift (1)
validateNode(15-40)
Tests/RelayMacrosTests/Combine/Relayed/ExplicitlyIsolatedRelayedMacroTests.swift (4)
Tests/RelayMacrosTests/Combine/Relayed/SubclassedRelayedMacroTests.swift (1)
testExpansion(24-153)Tests/RelayMacrosTests/Combine/Relayed/SubclassedImplicitlyIsolatedRelayedMacroTests.swift (1)
testExpansion(24-154)Tests/RelayMacrosTests/Combine/Relayed/RelayedMacroTests.swift (1)
testExpansion(24-292)Tests/RelayMacrosTests/Combine/Relayed/RelayedPropertyMacroTests.swift (1)
testExpansion(21-86)
Tests/RelayTests/Memoized/ObservationMemoizedTests.swift (4)
Tests/RelayTests/Memoized/PublishableMemoizedTests.swift (1)
calculateIgnoredValue(205-208)Tests/RelayTests/Memoized/MainActorMemoizedTests.swift (1)
calculateIgnoredValue(263-266)Tests/RelayTests/Memoized/RelayedMemoizedTests.swift (1)
calculateIgnoredValue(205-208)Tests/RelayTests/Memoized/SwiftDataMemoizedTests.swift (1)
calculateIgnoredValue(266-269)
Tests/RelayTests/Combine/Relayed/MainActorRelayedTests.swift (1)
Tests/RelayTests/Helpers/Array+PopFirst.swift (1)
popFirst(11-16)
Macros/RelayMacros/Combine/Relayed/RelayedPropertyMacro.swift (3)
Macros/RelayMacros/Combine/Publishable/PublishableMacro.swift (3)
validateNode(15-61)expansion(66-101)expansion(106-142)Macros/RelayMacros/Combine/Relayed/RelayedMacro.swift (4)
validateNode(15-40)expansion(45-78)expansion(83-94)expansion(99-147)Macros/RelayMacros/Combine/Relayed/RelayedPropertyDeclAccessorBuilder.swift (1)
buildAccessors(15-22)
Tests/RelayTests/Memoized/RelayedMemoizedTests.swift (5)
Tests/RelayTests/Memoized/PublishableMemoizedTests.swift (7)
independent(14-51)dependent(53-124)share(126-161)calculateBaseArea(183-187)fileprivate(189-193)calculateIgnoredValue(205-208)macOS(211-215)Tests/RelayTests/Helpers/Array+PopFirst.swift (1)
popFirst(11-16)Tests/RelayTests/Memoized/MainActorMemoizedTests.swift (4)
calculateBaseArea(251-255)fileprivate(257-261)calculateIgnoredValue(263-266)macOS(269-273)Tests/RelayTests/Memoized/ObservationMemoizedTests.swift (4)
calculateBaseArea(249-253)fileprivate(255-259)calculateIgnoredValue(261-264)macOS(267-271)Tests/RelayTests/Memoized/SwiftDataMemoizedTests.swift (4)
calculateBaseArea(254-258)fileprivate(260-264)calculateIgnoredValue(266-269)macOS(272-276)
🪛 SwiftLint (0.57.0)
Tests/RelayMacrosTests/Memoized/ObservationSuppressedMemoizedMacroTests.swift
[Warning] 20-20: Function body should span 50 lines or less excluding comments and whitespace: currently spans 58 lines
(function_body_length)
[Warning] 81-81: Function body should span 50 lines or less excluding comments and whitespace: currently spans 63 lines
(function_body_length)
Tests/RelayMacrosTests/Combine/Relayed/SubclassedImplicitlyIsolatedRelayedMacroTests.swift
[Error] 24-24: Function body should span 100 lines or less excluding comments and whitespace: currently spans 129 lines
(function_body_length)
Tests/RelayMacrosTests/Memoized/RelayedMemoizedMacroTests.swift
[Warning] 20-20: Function body should span 50 lines or less excluding comments and whitespace: currently spans 64 lines
(function_body_length)
[Warning] 87-87: Function body should span 50 lines or less excluding comments and whitespace: currently spans 69 lines
(function_body_length)
Tests/RelayMacrosTests/Combine/Relayed/SubclassedRelayedMacroTests.swift
[Error] 24-24: Function body should span 100 lines or less excluding comments and whitespace: currently spans 128 lines
(function_body_length)
Tests/RelayMacrosTests/Combine/Relayed/ExplicitlyIsolatedRelayedMacroTests.swift
[Error] 24-24: Function body should span 100 lines or less excluding comments and whitespace: currently spans 268 lines
(function_body_length)
[Warning] 15-15: Type body should span 250 lines or less excluding comments and whitespace: currently spans 276 lines
(type_body_length)
Tests/RelayMacrosTests/Memoized/RelayedPublisherSuppressedMemoizedMacroTests.swift
[Warning] 20-20: Function body should span 50 lines or less excluding comments and whitespace: currently spans 64 lines
(function_body_length)
[Warning] 87-87: Function body should span 50 lines or less excluding comments and whitespace: currently spans 69 lines
(function_body_length)
Tests/RelayMacrosTests/Combine/Relayed/RelayedMacroTests.swift
[Error] 24-24: Function body should span 100 lines or less excluding comments and whitespace: currently spans 267 lines
(function_body_length)
[Warning] 15-15: Type body should span 250 lines or less excluding comments and whitespace: currently spans 275 lines
(type_body_length)
Tests/RelayMacrosTests/Combine/Relayed/NonisolatedRelayedMacroTests.swift
[Error] 24-24: Function body should span 100 lines or less excluding comments and whitespace: currently spans 267 lines
(function_body_length)
[Warning] 15-15: Type body should span 250 lines or less excluding comments and whitespace: currently spans 275 lines
(type_body_length)
Tests/RelayTests/Memoized/RelayedMemoizedTests.swift
[Warning] 54-54: Function body should span 50 lines or less excluding comments and whitespace: currently spans 58 lines
(function_body_length)
[Warning] 34-34: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 35-35: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 36-36: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 40-40: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 42-42: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 48-48: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 49-49: 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)
[Warning] 85-85: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 87-87: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 88-88: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 92-92: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 94-94: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 97-97: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 100-100: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 102-102: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 104-104: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 105-105: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 111-111: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 112-112: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 113-113: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 115-115: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 121-121: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 122-122: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 145-145: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 146-146: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 147-147: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 148-148: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 154-154: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 155-155: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 156-156: 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 (tvos)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (maccatalyst)
- GitHub Check: build-and-test (macos)
- GitHub Check: build-and-test (ios)
| } else { | ||
| """ | ||
| @Sendable nonisolated func invalidateCache() { | ||
| assumeIsolatedIfNeeded { | ||
| instance?.publisher._beginModifications() | ||
| instance?._\(raw: propertyName) = nil | ||
| instance?.publisher._endModifications() | ||
| } | ||
| } | ||
| """ |
There was a problem hiding this comment.
Inconsistent nil handling in publisher-only cache invalidation.
This branch uses optional chaining (instance?) throughout, which silently does nothing when instance is nil. However, if _beginModifications() succeeds but instance becomes nil before _endModifications(), modifications tracking becomes unbalanced. Other branches use guard let instance for consistent behavior.
Consider using guard for symmetry and safety:
@Sendable nonisolated func invalidateCache() {
assumeIsolatedIfNeeded {
+ guard let instance else { return }
- instance?.publisher._beginModifications()
- instance?._\(raw: propertyName) = nil
- instance?.publisher._endModifications()
+ instance.publisher._beginModifications()
+ instance._\(raw: propertyName) = nil
+ instance.publisher._endModifications()
}
}🤖 Prompt for AI Agents
In Macros/RelayMacros/Memoized/MemoizedDeclBuilder.swift around lines 98 to 107,
the publisher-only cache invalidation uses optional chaining (instance?) which
can leave _beginModifications() and _endModifications() unbalanced if instance
becomes nil mid-sequence; change this to a guarded unwrap (guard let instance =
instance else { return }) at the start of invalidateCache(), then call
instance.publisher._beginModifications(), instance._<propertyName> = nil and
instance.publisher._endModifications() using the non-optional instance to ensure
balanced begin/end and symmetry with the other branches.
| @MainActor @Publishable @_Observable | ||
| class PublishableAnimal { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@_Observable Swift attribute API status private
💡 Result:
Short answer: @_Observable (with a leading underscore) is an underscored/private compiler attribute and is not a supported public API — it’s an internal/implementation detail and may change or be removed. Use the public @observable macro (introduced in iOS 17 / Xcode 15) for stable behavior; avoid relying on underscored attributes. [1][2][3]
Sources:
- Overview and usage of the public @observable macro. [1]
- Swift community/Swift Forums discussion: leading-underscore attributes are private/unsupported and subject to change. [2]
- Doc/blog on underscored attributes and the “do not use” convention. [3]
🏁 Script executed:
# Search for all uses of @_Observable in the codebase
rg '@_Observable' --type swiftRepository: NSFatalError/Relay
Length of output: 1246
🏁 Script executed:
# Search for all uses of @Observable in the codebase to compare patterns
rg '@Observable' --type swift | head -50Repository: NSFatalError/Relay
Length of output: 3229
🏁 Script executed:
# Examine the PublishableMacro to understand the requirement
cat -n Macros/RelayMacros/Combine/Publishable/PublishableMacro.swiftRepository: NSFatalError/Relay
Length of output: 6549
🏁 Script executed:
# Look for documentation or comments explaining @_Observable vs @Observable
rg '@_Observable|@Observable' --type swift -B 2 -A 2 Macros/RelayMacros/Repository: NSFatalError/Relay
Length of output: 1405
Replace @_Observable with public @Observable or document the requirement for private API usage.
@_Observable is a private, unsupported Swift compiler attribute with no stability guarantees. Web search confirms it is explicitly discouraged by the Swift community and may change or be removed in future versions. Using it in tests creates unnecessary fragility.
The codebase shows this pattern: @Publishable macro warns against combining with the public @Observable macro because they both "supply their own Observable protocol conformance." However, relying on @_Observable as a workaround is not a sustainable solution.
Either:
- Update the
@Publishablemacro to support or properly handle the public@Observablemacro, eliminating the need for the private attribute - Add explicit documentation in code and tests explaining why
@_Observableis necessary and any plans to migrate away from it
Applies to lines 292-293, 303-304, and 338-339.
| extension SubclassedRelayedTests { | ||
|
|
||
| @Observable | ||
| class NonPublishableAnimal { | ||
|
|
||
| var name = "Unknown" | ||
| var age = 0 | ||
|
|
||
| var description: String { | ||
| "\(name), \(age)" | ||
| } | ||
| } | ||
|
|
||
| @Relayed | ||
| final class Cat: NonPublishableAnimal { | ||
|
|
||
| var breed: String? | ||
|
|
||
| var isSphynx: Bool { | ||
| breed == "Sphynx" | ||
| } | ||
|
|
||
| override var age: Int { | ||
| didSet { | ||
| _ = oldValue | ||
| } | ||
| } | ||
|
|
||
| override var description: String { | ||
| "\(breed ?? "-"), \(age)" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Cat class is defined but unused in tests.
The Cat class (subclass of NonPublishableAnimal with @Relayed) is declared but no test methods exercise it. This appears to be missing test coverage for the scenario where @Relayed is applied to a subclass of a non-publishable @Observable base class.
Consider adding tests for the Cat class similar to the Dog tests, or if this is intentional for a future PR, add a TODO comment.
Would you like me to generate test methods for the Cat class following the same patterns used for Dog?
🤖 Prompt for AI Agents
In Tests/RelayTests/Combine/Relayed/SubclassedRelayedTests.swift around lines
323 to 355, the @Relayed Cat subclass is declared but not exercised by any
tests; add test coverage mirroring the existing Dog tests to validate relayed
behavior on a subclass of a non-publishable @Observable base class: create tests
that instantiate Cat, subscribe to the relayed publisher(s)/observers used by
the Dog tests, mutate Cat properties (breed, age, name/description if relevant),
and assert the expected published events and state changes; alternatively, if
leaving Cat unused is intentional, add a clear TODO comment above the Cat
declaration explaining why and referencing the future test/PR that will cover
it.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Macros/RelayMacros/Combine/Publishable/PublishableMacro.swift (2)
15-42: Make the@Publishable+@Observablefix-it resilient to qualified usagesThe warning and fix-it toward
@Relayedare a nice touch. One subtlety: because the fix-it only replacesnode.attributeName, a usage like@Relay.Publishable(isolation: .main)would become@Relayed(isolation: .main), dropping theRelay.qualifier. If you expect clients to use the qualified form, consider rebuilding the replacementTypeSyntaxby preserving any existing base type fromnode.attributeName(e.g. turningRelay.PublishableintoRelay.Relayed) rather than always usingRelayedMacro.attribute.attributeNameas-is.
106-141: Optional: derive the inheritedPublishabletype fromprotocolsfor future-proofingThe extension expansion now hard-codes
Relay.Publishablein the inheritance clause, which aligns with the module-qualified protocol. If this macro ever needs to support alternative entry points or aliasing, you might consider deriving the inherited type from theprotocolsarray (after theguard !protocols.isEmpty) and only falling back to"Relay.Publishable"as a default. That would make the macro slightly more robust to future renames or re-exports while preserving the current behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Macros/RelayMacros/Combine/Publishable/PublishableMacro.swift(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Macros/RelayMacros/Combine/Publishable/PublishableMacro.swift (2)
Macros/RelayMacros/Combine/Relayed/RelayedMacro.swift (1)
validateNode(15-40)Macros/RelayMacros/Combine/Common/PropertyPublisherDeclBuilder.swift (2)
build(18-40)inheritanceClause(42-48)
⏰ 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 (tvos)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (maccatalyst)
- GitHub Check: build-and-test (ios)
🔇 Additional comments (2)
Macros/RelayMacros/Combine/Publishable/PublishableMacro.swift (2)
52-55: LGTM on switching the SwiftDataModel fix-it to.removeUsing
.removeon the@Publishableattribute now matches the diagnostic text (“Remove @Publishable macro”) and more accurately reflects the intent when applied to@Modelclasses whose internals are incompatible with a customObservationRegistrar. No issues here.
66-100: Good centralization viavalidateNodeand cleaner builder pipelineRouting the member expansion through
validateNodeand moving toPropertiesParser.parse(declarationGroup:)keeps validation and property discovery consistent with the rest of the macro suite. The renamedbuildersarray plusflatMap { try $0.build() }reads cleanly and is easy to extend. No functional concerns from this change.
Summary by CodeRabbit
New Features
Public API
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.