Refactored GlobalActorIsolation type
#14
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughPublic API refactor replaces GlobalActorIsolationPreference with GlobalActorIsolation, updates settings and extractor APIs and DeclBuilder to use the new type, adds a GlobalActorIsolation helper, removes the old preference helper, and updates tests to the new API and value shape. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DeclBuilder
participant Settings
participant Declaration
Client->>DeclBuilder: request inheritedGlobalActorIsolation
DeclBuilder->>Settings: read preferredGlobalActorIsolation?
alt preferred provided
Settings-->>DeclBuilder: GlobalActorIsolation
DeclBuilder-->>Client: return preferred isolation
else not provided
DeclBuilder->>Declaration: read basicDeclaration.globalActor?
Declaration-->>DeclBuilder: Attribute/TypeSyntax?
alt inferred type present
DeclBuilder-->>Client: .isolated(trimmedType)
else none
DeclBuilder-->>Client: .nonisolated
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Sources/PrincipleMacros/Syntax/Helpers/GlobalActorIsolation.swift (1)
25-30: Confirm AttributeSyntax convenience initializer availability across your SwiftSyntax versionUsing AttributeSyntax(attributeName:) is concise, but availability varies across SwiftSyntax versions. If you target multiple toolchains, consider the explicit initializer to guarantee the “@” is present.
If you run into availability issues, this explicit form is robust:
- return AttributeSyntax(attributeName: type) + return AttributeSyntax( + atSign: .atSignToken(), + attributeName: type + )Sanity-check that the printed form matches expectations (e.g., “@mainactor”), which your tests already assert against.
Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilder.swift (1)
29-37: Effective isolation is never nil; consider making the type non-optionalThe property always returns a value (.nonisolated at minimum). Exposing it as GlobalActorIsolation instead of GlobalActorIsolation? would simplify call sites and avoid optional chaining. If keeping optional for API continuity is intentional, no action needed.
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
67-85: Extraction logic looks solid; minor robustness note on TypeSyntax creationThe .self gate and base extraction are correct. Building TypeSyntax via string interpolation works well in SwiftSyntax’s DSL, but it silently depends on description round-tripping. If you ever need stricter guarantees, consider parsing the base into a TypeSyntax via TypeSyntax("(raw: ...)") (explicit raw interpolation) to avoid accidental trivia changes.
Example (no behavior change, just explicit intent):
- return .isolated("\(globalActor)") + return .isolated("\(raw: globalActor)")Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (1)
100-104: Attribute-based verification for isolated cases is appropriateAsserting on attribute?.description matches the new API surface neatly. Consider adding a case for a nested type referencing Self.SomeActor.self to guard against potential base-resolution regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilder.swift(1 hunks)Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilderSettings.swift(1 hunks)Sources/PrincipleMacros/Parameters/ParameterExtractor.swift(2 hunks)Sources/PrincipleMacros/Syntax/Helpers/GlobalActorIsolation.swift(1 hunks)Sources/PrincipleMacros/Syntax/Helpers/GlobalActorIsolationPreference.swift(0 hunks)Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift(3 hunks)
💤 Files with no reviewable changes (1)
- Sources/PrincipleMacros/Syntax/Helpers/GlobalActorIsolationPreference.swift
🧰 Additional context used
🧬 Code Graph Analysis (4)
Sources/PrincipleMacros/Syntax/Helpers/GlobalActorIsolation.swift (4)
Sources/PrincipleMacros/Syntax/Helpers/GlobalActorIsolationPreference.swift (1)
nonisolated(11-15)Sources/PrincipleMacros/Syntax/Extensions/WithAttributesSyntax.swift (1)
globalActor(11-18)Sources/PrincipleMacros/Builders/Declarations/Types/ActorDeclBuilder.swift (1)
declaration(11-14)Sources/PrincipleMacros/Syntax/Custom/TypeDeclSyntax.swift (1)
isFinal(30-35)
Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilder.swift (3)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
preferredGlobalActorIsolation(67-85)Sources/PrincipleMacros/Syntax/Helpers/GlobalActorIsolationPreference.swift (1)
nonisolated(11-15)Sources/PrincipleMacros/Syntax/Extensions/WithAttributesSyntax.swift (1)
globalActor(11-18)
Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilderSettings.swift (2)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
preferredGlobalActorIsolation(67-85)Sources/PrincipleMacros/Syntax/Helpers/GlobalActorIsolationPreference.swift (1)
nonisolated(11-15)
Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (1)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (2)
trailingClosure(40-47)preferredGlobalActorIsolation(67-85)
🔇 Additional comments (6)
Sources/PrincipleMacros/Syntax/Helpers/GlobalActorIsolation.swift (1)
16-23: No Action Needed: Swift 6 Mode Is GuaranteedThe package’s
Package.swiftspecifies// swift-tools-version: 6.0, so SE-0380 (switch-as-expression) is fully supported. You can safely keep the current implementation without adding explicitreturnstatements.Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilderSettings.swift (1)
14-22: API rename and integration are coherentThe rename to preferredGlobalActorIsolation and the new type wiring are consistent with the rest of the refactor. Initializer defaults and property exposure look good.
Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (4)
49-50: Trailing-closure assertions simplified—LGTMComparing descriptions directly is clear and keeps the tests focused on extracted shape rather than syntax node identity.
Also applies to: 56-57
81-85: Covers missing isolation case wellNil return when parameter is absent is validated. Good baseline.
88-92: Good check for explicit nonisolated preferenceThe equality against .nonisolated ensures the enum case is surfaced as intended.
107-111: Error path test remains meaningfulThe unexpected syntax path is preserved; good coverage to prevent silent acceptance of “.Type”.
This reverts commit 9c7315d.
Summary by CodeRabbit
New Features
Refactor
Tests