Added global actor preference extraction#12
Conversation
WalkthroughUpdates SwiftSyntax bounds and lockfile hash; refactors access-control utilities and modifiers; introduces GlobalActorIsolationPreference and plumbing for actor isolation; adds ActorDeclBuilder and BasicDeclSyntax change; changes ParameterExtractor API and tests to use optionals and AttributeSyntax initializer; makes InheritingDeclaration Hashable. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AttributeSyntax
participant ParameterExtractor
participant GlobalActorIsolationPreference
Caller->>ParameterExtractor: init(from: AttributeSyntax)
ParameterExtractor->>AttributeSyntax: read arguments (may be nil)
Caller->>ParameterExtractor: globalActorIsolationPreference(label)
ParameterExtractor->>ParameterExtractor: expression(withLabel:)
alt no expression found
ParameterExtractor-->>Caller: nil
else expression is NilLiteral
ParameterExtractor-->>Caller: .nonisolated
else expression is MemberAccess (type)
ParameterExtractor-->>Caller: .isolated(TypeSyntax)
else unexpected form
ParameterExtractor-->>Caller: throw ParameterExtractionError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. ✨ 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 (
|
![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
🧹 Nitpick comments (13)
Package.resolved (1)
2-2: Consider not committing Package.resolved for a library package.For libraries, committing Package.resolved can constrain your consumers’ dependency resolution unnecessarily. If you want reproducible CI, keep it; otherwise, consider removing it and letting dependents resolve within your declared range.
Sources/PrincipleMacros/Syntax/Helpers/GlobalActorIsolationPreference.swift (1)
11-15: Add brief docs to clarify semantics (nil vs. nonisolated).Consider documenting how this enum is intended to be used (e.g., nil at the call site meaning “unspecified,”
.nonisolatedmeaning “explicitly nonisolated,” and.isolated(T)meaning “isolate to T”). This reduces ambiguity at integration points.Suggested inline docs:
public enum GlobalActorIsolationPreference: Hashable { - - case nonisolated - case isolated(TypeSyntax) + /// Explicitly prefer nonisolated (e.g., the argument was present and `nil`) + case nonisolated + /// Explicitly isolate to the given global actor type (e.g., `SomeGlobalActor.self`) + case isolated(TypeSyntax) }Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilderSettings.swift (1)
16-22: Consider documenting the new setting to avoid misuse.A short doc comment on the property and initializer parameters will help future readers understand the nil/.nonisolated/.isolated distinctions.
Suggested diff:
- public var globalActorIsolationPreference: GlobalActorIsolationPreference? + /// Global actor isolation preference extracted from parameters (if any). + /// - nil: no preference specified by the user; builder may apply defaults. + /// - .nonisolated: user explicitly prefers `nonisolated`. + /// - .isolated(T): user explicitly prefers isolation to the given global actor type `T`. + public var globalActorIsolationPreference: GlobalActorIsolationPreference? @@ - accessControlLevel: AccessControlLevel, - globalActorIsolationPreference: GlobalActorIsolationPreference? = nil + accessControlLevel: AccessControlLevel, + globalActorIsolationPreference: GlobalActorIsolationPreference? = nil ) {Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax.swift (1)
36-41: Detail-based filtering is good; consider guarding against future tokenization changesThe
detail?.detail.tokenKindmatch is tight and correct today. If SwiftSyntax adjusts tokenization ofprivate(set)detail again, you’ll have a silent fallback to the primary ACL. The suggested fix above makes the resolution resilient without changing this helper. No code change required here beyond the setter fix.Sources/PrincipleMacros/Syntax/Custom/BasicDeclSyntax.swift (1)
12-13: Adding WithAttributesSyntax is sensible; verify all BasicDeclSyntax adopters complyExpanding
BasicDeclSyntaxto includeWithAttributesSyntaxis a good tightening of the abstraction. Ensure every site that providesany BasicDeclSyntaxactually uses decls that conform toWithAttributesSyntax; otherwise you’ll get type constraint errors.If you want, I can scan the repo for usages of
any BasicDeclSyntaxand list the concrete decl types to double-check conformance.Sources/PrincipleMacros/Builders/Declarations/Types/ActorDeclBuilder.swift (1)
16-21: Minor: add a brief doc comment for discoverabilityA short doc comment on
typeDeclarationimproves IDE discoverability and maintains parity with other builders.public var typeDeclaration: any TypeDeclSyntax { - declaration + // The concrete actor declaration treated as a general type declaration. + declaration }Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilder.swift (2)
35-43: Member inheritance shortcut is fine; consider naming clarity and small guard refactorLogic is sound. Two small readability tweaks:
- Avoid shadowing
settings(outer property) withlet settings = settings.accessControlLevel(rename toaclSettings).- Precompute
internalIndexonce to avoid repeatedly searchingKeyword.accessControlLevelsif this path expands.Optional refactor for naming clarity:
- let settings = settings.accessControlLevel + let aclSettings = settings.accessControlLevel guard let accessControlLevel = basicDeclaration.accessControlLevel, let index = TokenKind.accessControlLevels.firstIndex(of: accessControlLevel.tokenKind), - let maxAllowedIndex = Keyword.accessControlLevels.firstIndex(of: settings.maxAllowed) + let maxAllowedIndex = Keyword.accessControlLevels.firstIndex(of: aclSettings.maxAllowed) else { return nil } ... - switch settings.inheritingDeclaration { + switch aclSettings.inheritingDeclaration {
48-57: String-interpolated AttributeSyntax is convenient; verify SwiftSyntaxBuilder isn’t required
let globalActor: AttributeSyntax = "@\(globalActor)"relies on string interpolation builders. IfSwiftSyntaxBuilderis not imported in this target, this may fail to compile depending on SwiftSyntax version. If you hit that, either importSwiftSyntaxBuilderhere or construct the attribute using initializers.Otherwise, the overall isolation preference flow looks correct and the trailing space handling is consistent.
Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (3)
46-51: Drop unnecessarytryiftrailingClosurestops throwing
trailingClosure(withLabel:)currently has no throwing paths. If you accept the API cleanup suggested in the implementation, removetryhere.Apply this diff if you make
trailingClosurenon-throwing:- let extracted = try extractor.trailingClosure(withLabel: "operation") + let extracted = extractor.trailingClosure(withLabel: "operation")
54-59: Same here: removetryiftrailingClosurebecomes non-throwingKeeps call sites tidy and signals there are no error paths.
Apply this diff if you make
trailingClosurenon-throwing:- let extracted = try extractor.trailingClosure(withLabel: "operation") + let extracted = extractor.trailingClosure(withLabel: "operation")
80-121: Add tests for AttributeSyntax initializer pathYou’ve added
ParameterExtractor.init(from: AttributeSyntax)but tests only cover freestanding macro expansions. Consider mirroring a couple of these tests using attributes to exercise that initializer.Example to add (outside current hunks):
extension ParameterExtractorTests { private func makeExtractor(from attribute: AttributeSyntax) -> ParameterExtractor { ParameterExtractor(from: attribute) } @Test func testAttributeInit_GlobalActor_Nonisolated() throws { let attribute: AttributeSyntax = "@MyMacro(isolation: nil)" let extractor = makeExtractor(from: attribute) let extracted = try extractor.globalActorIsolationPreference(withLabel: "isolation") #expect(extracted == .nonisolated) } @Test func testAttributeInit_ExpressionExtraction() throws { let attribute: AttributeSyntax = "@MyMacro(value: Type.make())" let extractor = makeExtractor(from: attribute) let extracted = extractor.expression(withLabel: "value") let expected: ExprSyntax = "Type.make()" #expect(extracted?.description == expected.description) } }Would you like me to open a follow-up PR to add these?
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (2)
40-47: Remove unusedthrowsfromtrailingClosure(withLabel:)There are no throwing paths in this method. Dropping
throwsclarifies the API and avoids unnecessarytryat call sites.Apply this diff:
- public func trailingClosure( - withLabel label: TokenSyntax? - ) throws -> ExprSyntax? { + public func trailingClosure( + withLabel label: TokenSyntax? + ) -> ExprSyntax? { if let trailingClosure { return ExprSyntax(trailingClosure) } return expression(withLabel: label) }Note: Update test call sites accordingly (remove
try) as suggested in the tests review comments.
78-85: Minor simplification in.isolateddetectionYou can avoid re-wrapping
declNameand trim the base to prevent stray whitespace.Apply this diff:
- if let memberAccessExpression = MemberAccessExprSyntax(expression), - let referenceExpression = DeclReferenceExprSyntax(memberAccessExpression.declName), - referenceExpression.baseName.tokenKind == .keyword(.self), - let baseType = memberAccessExpression.base { - return .isolated("\(baseType)") - } + if let memberAccessExpression = MemberAccessExprSyntax(expression), + memberAccessExpression.declName.baseName.tokenKind == .keyword(.self), + let baseType = memberAccessExpression.base { + return .isolated("\(baseType.trimmed)") + }
📜 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 (12)
Package.resolved(1 hunks)Package.swift(1 hunks)Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilder.swift(1 hunks)Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilderSettings.swift(1 hunks)Sources/PrincipleMacros/Builders/Declarations/Types/ActorDeclBuilder.swift(1 hunks)Sources/PrincipleMacros/Parameters/ParameterExtractor.swift(2 hunks)Sources/PrincipleMacros/Syntax/Custom/BasicDeclSyntax.swift(1 hunks)Sources/PrincipleMacros/Syntax/Custom/TypeDeclSyntax.swift(1 hunks)Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax.swift(1 hunks)Sources/PrincipleMacros/Syntax/Helpers/GlobalActorIsolationPreference.swift(1 hunks)Sources/PrincipleMacros/Syntax/Helpers/InheritingDeclaration.swift(1 hunks)Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilderSettings.swift (1)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
globalActorIsolationPreference(67-86)
Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilder.swift (2)
Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax.swift (1)
accessControlLevel(36-41)Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
globalActorIsolationPreference(67-86)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (2)
Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (1)
arguments(96-112)Sources/PrincipleMacros/Builders/Expressions/AssociatedValuesListExprBuilder.swift (1)
expression(43-53)
Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (1)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (4)
expression(31-38)trailingClosure(40-47)rawString(49-65)globalActorIsolationPreference(67-86)
⏰ 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 (macos)
- GitHub Check: build-and-test (maccatalyst)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (ios)
🔇 Additional comments (18)
Package.swift (1)
23-26: Widened swift-syntax upper bound to < 604.0.0 — verify cross-minor compatibility.This broadens the allowed range from 600.x..<602.x to 600.x..<604.x. SwiftSyntax often has source-breaking changes across these minors. The current pin (Package.resolved) is 601.0.1, which is safe, but please ensure CI exercises at least one 602.x and 603.x toolchain to catch API drifts before consumers hit them.
Sources/PrincipleMacros/Syntax/Helpers/InheritingDeclaration.swift (1)
9-13: Hashable conformance looks good.Simple, no behavioral change, and useful for set/map usage.
Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilderSettings.swift (1)
14-15: Good addition: optional globalActorIsolationPreference captures absence vs. explicit choice.The optional distinguishes “unspecified” (nil) from “explicitly nonisolated,” which aligns with the extractor’s behavior.
Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax.swift (2)
46-51: LGTM: centralizing specifier and access-control token setsExposing
TokenKind.typeScopeSpecifiers,TokenKind.accessControlLevelsand theirKeywordcounterparts as internal constants is clean and works well with the index-based logic elsewhere.Also applies to: 55-68
32-34: Support both keyword and identifier for setter ACL detectionSwiftSyntax can treat the “set” in
private(set)as either a keyword or an identifier. To ensure we always detect the setter ACL, updatesetterAccessControlLevelto check both forms:public var setterAccessControlLevel: TokenSyntax? { - accessControlLevel(detail: .identifier("set")) ?? accessControlLevel + accessControlLevel(detail: .keyword(.set)) + ?? accessControlLevel(detail: .identifier("set")) + ?? accessControlLevel }Could you verify this against a declaration like
public private(set) var x: Intto confirm the setter ACL resolves to
private?Sources/PrincipleMacros/Syntax/Custom/TypeDeclSyntax.swift (1)
11-14: Conformance via BasicDeclSyntax is cleaner; final-ness semantics look correctSwitching to
BasicDeclSyntaxkeeps attributes and modifiers available without repeating constraints.isFinalfor class/protocol/actor/enum/struct is consistent with Swift semantics.Sources/PrincipleMacros/Builders/Declarations/Types/ActorDeclBuilder.swift (1)
11-14: Protocol shape looks goodNarrowing to
ActorDeclSyntaxviadeclarationis clear and consistent with existing builders.Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (7)
22-25: Solid migration to optional expression APIUsing optional chaining on the extracted expression matches the updated
expression(withLabel:) -> ExprSyntax?signature and keeps assertions concise.
30-33: Unlabeled argument extraction verifiedThis validates the nil-label path and ensures the first unlabeled argument is returned. Good coverage.
35-41: Great addition: missing label returns nilThis regression guard will prevent accidental exceptions when an argument is absent.
72-77: Error-path test for raw string extraction is correctThis asserts the right failure mode when a non-string expression is supplied.
83-87: Nil isolation preference when missingGood check ensuring absence of the parameter yields
nil.
90-94:nilisolation correctly treated as.nonisolatedThis test captures the intended semantics for an explicit
nilisolation argument.
114-120: Good negative test for unexpected global-actor syntaxEnsures metatype (
.Type) is rejected with the documented error.Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (4)
13-15: Makingargumentsoptional is the right callThis de-risks attribute forms without arguments and simplifies downstream checks.
21-29: AttributeSyntax initializer looks correctExtracting
.argumentListand explicitly settingtrailingClosuretonil(attributes can’t have trailing closures) is the right behavior.
31-38: Optional-returningexpression(withLabel:)is clean and predictableThe equality on
trimmedDescriptionhandles both labeled and unlabeled arguments (vianil == nil). This aligns with tests.
49-65: Raw-string extraction semantics are solidReturning
nilwhen absent and throwing on unexpected syntax type strikes a good balance between ergonomics and strictness.
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 (5)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (5)
21-29: AttributeSyntax initializer looks correct; consider broader argument support or explicit docsToday you only handle
.argumentListand treat everything else as “no arguments”. That’s fine for typical attribute invocations, but attributes can also carry other argument forms (e.g., balanced tokens, string-style). Either:
- Document that only labeled expression lists are supported, or
- Expand handling to cover other valid cases and throw a targeted error when present but unsupported.
Would you like me to draft an expansion that throws when unsupported argument forms are encountered so failures are explicit rather than silently treated as “no args”?
31-38:expression(withLabel:)is clean; consider a convenience overload for string labelsThe token-based label match is robust. For ergonomics at call sites (esp. in tests), consider adding an overload that takes
String?and converts it to aTokenSyntaxinternally.You can add this outside the changed segment:
// Convenience overload (optional) public func expression(withLabel label: String?) -> ExprSyntax? { expression(withLabel: label.map { .identifier($0) }) }
40-47:throwsis unnecessary here; drop it to signal semantics clearlyThis method never throws anymore. Keeping
throwsforces callers to usetrywith no benefit and obscures the new “nil means not found” semantics.Apply this diff:
- public func trailingClosure( - withLabel label: TokenSyntax? - ) throws -> ExprSyntax? { + public func trailingClosure( + withLabel label: TokenSyntax? + ) -> ExprSyntax? { if let trailingClosure { return ExprSyntax(trailingClosure) } return expression(withLabel: label) }If you’re intentionally preserving
throwsfor source-compatibility, a brief comment explaining that would help future readers.
49-65: String extraction semantics are sensible; interpolation intentionally rejectedReturning
nilwhen the argument is missing and throwing on non-literal strings (e.g., interpolations) is a good split that mirrors common macro expectations.Minor style nit: you mix
.as(T.self)and typed-view initializers elsewhere. Consider consistently using one style for readability.
67-85: Global-actor isolation preference: solid coverage fornilandType.self; trim base for robustnessThe detection for
nil→.nonisolatedandType.self→.isolated("Type")is correct and matches the tests. To avoid incidental whitespace in the captured type, consider trimming the base expression before interpolation.Apply this diff:
- if let memberAccessExpression = MemberAccessExprSyntax(expression), + if let memberAccessExpression = MemberAccessExprSyntax(expression), memberAccessExpression.declName.baseName.tokenKind == .keyword(.self), let baseType = memberAccessExpression.base { - return .isolated("\(baseType)") + return .isolated("\(baseType.trimmed)") }Optionally, if you plan to allow
MainActorwithout.selfin the future, you could add a path forDeclReferenceExprSyntaxor aTypeExprSyntaxbefore throwing.Do you want a follow-up to support
Typewithout.self?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilder.swift(1 hunks)Sources/PrincipleMacros/Parameters/ParameterExtractor.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilder.swift
🧰 Additional context used
🧬 Code Graph Analysis (1)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (2)
Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (1)
arguments(96-112)Sources/PrincipleMacros/Builders/Expressions/AssociatedValuesListExprBuilder.swift (1)
expression(43-53)
🔇 Additional comments (1)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
13-13: Good encapsulation: makingargumentsoptional and private is the right callThis aligns with the new optional-return API and keeps the surface area tight.
1 participant
Summary by CodeRabbit