Added superclass inference #18
Conversation
WalkthroughParser APIs were made throwable and gained if-config/member traversal helpers; several internal parser/result protocols/files were removed. Many focused SwiftSyntax extensions were added or split (availability, global-actor, access-control, if-config, trivia, superclass inference). Diagnostics and ParameterExtractor gained throwable/error APIs; builders and tests updated accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus during review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
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: 2
🧹 Nitpick comments (8)
Tests/PrincipleMacrosTests/Syntax/Extensions/ClosureExprSyntaxTests.swift (2)
23-23: Extract magic number to a named constant.The nesting level
2is repeated across all three test methods. Consider extracting it to a named constant to improve maintainability and clarify intent.Apply this diff to extract the magic number:
internal struct ClosureExprSyntaxTests { + + private let expectedNestingLevel = 2 @Test func withoutSignature() { let expr: ExprSyntax = """ @@ -20,7 +22,7 @@ let interpolation: ExprSyntax = """ .init( parameter: .init( - closure: \(expr.expanded(nestingLevel: 2)), + closure: \(expr.expanded(nestingLevel: expectedNestingLevel)), value: "Foo" ) ) @@ -49,7 +51,7 @@ let interpolation: ExprSyntax = """ .init( parameter: .init( - closure: \(expr.expanded(nestingLevel: 2)), + closure: \(expr.expanded(nestingLevel: expectedNestingLevel)), value: "Foo" ) ) @@ -85,7 +87,7 @@ let interpolation: ExprSyntax = """ .init( parameter: .init( - closure: \(expr.expanded(nestingLevel: 2)), + closure: \(expr.expanded(nestingLevel: expectedNestingLevel)), value: "Foo" ) )Based on static analysis.
Also applies to: 52-52, 88-88
14-111: Consider adding edge case tests.The current test coverage validates basic formatting scenarios well. To ensure robustness, consider adding tests for:
- Different nesting levels (0, 1, 3+)
- Closures with trailing closures
- Empty closures
- Closures with complex capture lists
This would help verify that the
expanded(nestingLevel:)method handles various scenarios correctly.Tests/PrincipleMacrosTests/Syntax/Extensions/ExprSyntaxTests.swift (2)
14-18: Clarify test naming: "optionalLiteral" is misleading.The test uses
"Int?"which is a type reference in an expression context, not an optional literal. The name "optionalLiteral" suggests testing something like optional chaining expressions (e.g.,value?.property). Consider renaming tooptionalTypeReferenceortypeReferenceAsOptionalfor clarity.
45-54: Clarify test naming and check for duplication with TypeSyntaxTests.The tests
arrayLiteralanddictionaryLiteraluse type syntax ([String],[String: Int]) rather than actual collection literals. This is confusing because:
- The naming suggests testing collection literals like
["hello"]or["key": 1]- Similar tests exist in
TypeSyntaxTests.swiftforTypeSyntax(see relevant snippets)- The distinction between testing type references as expressions vs. actual literals is unclear
Consider:
- Renaming to
arrayTypeReferenceanddictionaryTypeReference- Clarifying the intent of testing type syntax in expression contexts
- Verifying whether this duplicates coverage from
TypeSyntaxTests.swiftTests/PrincipleMacrosTests/Syntax/Extensions/TypeSyntaxTests.swift (1)
12-84: Solid coverage forTypeSyntax.standardizedwith minor naming nitThe suite gives good breadth over literals, collections, tuples, and composed cases and should guard future regressions. One small readability tweak would be to avoid shadowing the
typeparameter incomposition(e.g. rename the local tosyntaxorsut) so it’s immediately clear which is theStringargument vs. theTypeSyntax.Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+Availability.swift (1)
35-44: Consider making the visibility consistent across extensions.The
availabilityproperty onIfConfigClauseSyntaxis internal (no explicit access modifier), while the similar properties onIfConfigDeclSyntax(line 13) andIfConfigClauseSyntax.Elements(line 48) are markedpublic.If this is intended to be a helper property, this is fine. Otherwise, consider adding
publicfor API consistency.Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax+AccessControlLevel.swift (1)
36-41: Consider adding a clarifying comment for the detail check.The expression
detail?.detail.tokenKindon line 37 is checking for modifier details (e.g., distinguishingprivatefromprivate(set)), but the double "detail" might be initially confusing to readers.A brief comment explaining this would improve code clarity, though the implementation itself is correct.
private func accessControlLevel(detail detailTokenKind: TokenKind?) -> AccessControlLevel? { // Check if the modifier has a detail token (e.g., "set" in "private(set)") if detail?.detail.tokenKind == detailTokenKind { return AccessControlLevel(tokenSyntax: name) } return nil }Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift (1)
60-109: Member/decl-level helpers nicely expose enclosing/applying if-config behavior — consider adding focused testsThe
MemberBlockItemListSyntax,MemberBlockItemSyntax, andDeclSyntaxProtocolextensions provide a convenient way to (a) ask “what if-config am I under?” and (b) reapply that context to an arbitraryMemberBlockItemListSyntaxviaapplyingEnclosingIfConfig(to:). The replacement of sibling members and trimming/normalizing trivia make sense for isolating the active path.Given the amount of parent-chain and list-replacement logic here, it would be good to back this with a few targeted tests (e.g., single
#if, nested#ifchains, declarations without any#if, and branches with multiple siblings) to lock in the semantics and guard future refactors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
Sources/PrincipleMacros/Diagnostics/DiagnosticsError.swift(1 hunks)Sources/PrincipleMacros/Parsers/Common/Parser.swift(1 hunks)Sources/PrincipleMacros/Parsers/Common/ParserResultsCollection.swift(2 hunks)Sources/PrincipleMacros/Parsers/Common/_Parser.swift(0 hunks)Sources/PrincipleMacros/Parsers/Common/_ParserResultsCollection.swift(0 hunks)Sources/PrincipleMacros/Parsers/EnumCases/EnumCasesList.swift(1 hunks)Sources/PrincipleMacros/Parsers/EnumCases/EnumCasesParser.swift(1 hunks)Sources/PrincipleMacros/Parsers/Properties/PropertiesList.swift(1 hunks)Sources/PrincipleMacros/Parsers/Properties/PropertiesParser.swift(1 hunks)Sources/PrincipleMacros/Syntax/Concepts/GlobalActorIsolation.swift(1 hunks)Sources/PrincipleMacros/Syntax/Extensions/AttributeListSyntax.swift(0 hunks)Sources/PrincipleMacros/Syntax/Extensions/AttributeSyntax.swift(0 hunks)Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift(1 hunks)Sources/PrincipleMacros/Syntax/Extensions/ExprSyntax.swift(2 hunks)Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+Availability.swift(1 hunks)Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift(1 hunks)Sources/PrincipleMacros/Syntax/Extensions/WithAttributesSyntax+Availability.swift(1 hunks)Sources/PrincipleMacros/Syntax/Extensions/WithAttributesSyntax+GlobalActorIsolation.swift(1 hunks)Sources/PrincipleMacros/Syntax/Extensions/WithAttributesSyntax.swift(0 hunks)Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax+AccessControlLevel.swift(1 hunks)Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax+GlobalActorIsolation.swift(1 hunks)Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax+Specifiers.swift(1 hunks)Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax.swift(0 hunks)Tests/PrincipleMacrosTests/Builders/EnumCaseCallExprBuilderTests.swift(3 hunks)Tests/PrincipleMacrosTests/Builders/SwitchExprBuilderTests.swift(1 hunks)Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift(0 hunks)Tests/PrincipleMacrosTests/Parsers/EnumCasesParserTests.swift(3 hunks)Tests/PrincipleMacrosTests/Parsers/PropertiesListTests.swift(1 hunks)Tests/PrincipleMacrosTests/Parsers/PropertiesParserTests.swift(5 hunks)Tests/PrincipleMacrosTests/Syntax/ClosureExprSyntaxTests.swift(0 hunks)Tests/PrincipleMacrosTests/Syntax/Concepts/ClosureTypeSyntaxTests.swift(4 hunks)Tests/PrincipleMacrosTests/Syntax/Concepts/GlobalActorIsolationTests.swift(4 hunks)Tests/PrincipleMacrosTests/Syntax/ExprSyntaxTests.swift(0 hunks)Tests/PrincipleMacrosTests/Syntax/Extensions/AvailabilityTests.swift(1 hunks)Tests/PrincipleMacrosTests/Syntax/Extensions/ClassDeclSyntaxTests.swift(1 hunks)Tests/PrincipleMacrosTests/Syntax/Extensions/ClosureExprSyntaxTests.swift(1 hunks)Tests/PrincipleMacrosTests/Syntax/Extensions/ExprSyntaxTests.swift(1 hunks)Tests/PrincipleMacrosTests/Syntax/Extensions/IfConfigDeclSyntaxTests.swift(1 hunks)Tests/PrincipleMacrosTests/Syntax/Extensions/TypeSyntaxTests.swift(1 hunks)Tests/PrincipleMacrosTests/Syntax/TypeSyntaxTests.swift(0 hunks)
💤 Files with no reviewable changes (10)
- Sources/PrincipleMacros/Parsers/Common/_ParserResultsCollection.swift
- Sources/PrincipleMacros/Syntax/Extensions/WithAttributesSyntax.swift
- Sources/PrincipleMacros/Syntax/Extensions/AttributeListSyntax.swift
- Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift
- Sources/PrincipleMacros/Parsers/Common/_Parser.swift
- Tests/PrincipleMacrosTests/Syntax/TypeSyntaxTests.swift
- Sources/PrincipleMacros/Syntax/Extensions/AttributeSyntax.swift
- Tests/PrincipleMacrosTests/Syntax/ClosureExprSyntaxTests.swift
- Tests/PrincipleMacrosTests/Syntax/ExprSyntaxTests.swift
- Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax.swift
🧰 Additional context used
🧬 Code graph analysis (14)
Tests/PrincipleMacrosTests/Builders/SwitchExprBuilderTests.swift (1)
Tests/PrincipleMacrosTests/Builders/EnumCaseCallExprBuilderTests.swift (1)
makeEnumCase(14-18)
Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax+GlobalActorIsolation.swift (1)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
globalActorIsolation(156-175)
Tests/PrincipleMacrosTests/Syntax/Extensions/TypeSyntaxTests.swift (1)
Tests/PrincipleMacrosTests/Syntax/Extensions/ExprSyntaxTests.swift (4)
optionalLiteral(14-18)arrayLiteral(44-48)dictionaryLiteral(50-54)arguments(92-107)
Tests/PrincipleMacrosTests/Syntax/Extensions/ExprSyntaxTests.swift (1)
Tests/PrincipleMacrosTests/Syntax/Extensions/TypeSyntaxTests.swift (4)
optionalLiteral(14-18)arrayLiteral(26-30)dictionaryLiteral(32-36)arguments(68-83)
Tests/PrincipleMacrosTests/Syntax/Extensions/IfConfigDeclSyntaxTests.swift (1)
Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift (1)
applyingEnclosingIfConfig(95-108)
Tests/PrincipleMacrosTests/Parsers/PropertiesParserTests.swift (1)
Sources/PrincipleMacros/Parsers/Properties/PropertiesParser.swift (1)
parse(13-44)
Sources/PrincipleMacros/Parsers/Properties/PropertiesParser.swift (1)
Sources/PrincipleMacros/Parsers/Common/Parser.swift (3)
parse(22-35)parse(37-49)parse(51-55)
Sources/PrincipleMacros/Syntax/Extensions/WithAttributesSyntax+GlobalActorIsolation.swift (1)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
globalActorIsolation(156-175)
Sources/PrincipleMacros/Parsers/EnumCases/EnumCasesParser.swift (2)
Sources/PrincipleMacros/Parsers/Common/Parser.swift (3)
parse(22-35)parse(37-49)parse(51-55)Sources/PrincipleMacros/Parsers/Properties/PropertiesParser.swift (1)
parse(13-44)
Sources/PrincipleMacros/Parsers/Common/Parser.swift (2)
Sources/PrincipleMacros/Parsers/EnumCases/EnumCasesParser.swift (1)
parse(13-25)Sources/PrincipleMacros/Parsers/Properties/PropertiesParser.swift (1)
parse(13-44)
Tests/PrincipleMacrosTests/Parsers/PropertiesListTests.swift (2)
Sources/PrincipleMacros/Parsers/Common/Parser.swift (3)
parse(22-35)parse(37-49)parse(51-55)Sources/PrincipleMacros/Parsers/Properties/PropertiesParser.swift (1)
parse(13-44)
Tests/PrincipleMacrosTests/Parsers/EnumCasesParserTests.swift (1)
Sources/PrincipleMacros/Parsers/EnumCases/EnumCasesParser.swift (1)
parse(13-25)
Tests/PrincipleMacrosTests/Syntax/Extensions/ClassDeclSyntaxTests.swift (1)
Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift (1)
inferredSuperclass(13-16)
Tests/PrincipleMacrosTests/Builders/EnumCaseCallExprBuilderTests.swift (2)
Tests/PrincipleMacrosTests/Builders/SwitchExprBuilderTests.swift (1)
makeEnumCase(14-18)Tests/PrincipleMacrosTests/Parsers/EnumCasesParserTests.swift (2)
withUnnamedAssociatedValue(25-36)withMultipleAssociatedValues(51-66)
🪛 SwiftLint (0.57.0)
Tests/PrincipleMacrosTests/Syntax/Extensions/ClosureExprSyntaxTests.swift
[Warning] 23-23: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 52-52: 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)
⏰ 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 (macos)
- GitHub Check: build-and-test (maccatalyst)
- GitHub Check: build-and-test (ios)
🔇 Additional comments (40)
Tests/PrincipleMacrosTests/Syntax/Concepts/ClosureTypeSyntaxTests.swift (1)
15-15: LGTM! Improved test naming.The test method renames reduce redundancy by removing the "Closure" suffix, which is already clear from the struct name
ClosureTypeSyntaxTests. The new names are more concise while maintaining clarity.Also applies to: 33-33, 51-51, 69-69
Tests/PrincipleMacrosTests/Syntax/Extensions/ClosureExprSyntaxTests.swift (2)
9-12: LGTM! Clear test structure.The imports and test struct declaration are appropriate. Using Swift Testing framework with
@Testis a modern choice for test organization.
14-111: Well-structured tests with clear expectations.The test methods follow a consistent arrange-act-assert pattern and provide explicit expected outputs, making them easy to understand and maintain. The coverage of simple, signature-based, and multi-line closures provides good confidence in the formatting functionality.
Sources/PrincipleMacros/Syntax/Extensions/ExprSyntax.swift (2)
69-77: LGTM! Clean refactoring that improves API surface.The extraction of
inferredWrappedTypeinto a separate public property improves code organization and makes the wrapped type accessible to API consumers. The logic remains equivalent to the previous implementation.
151-153: LGTM! Improved code organization.Reordering the property improves readability with no functional impact.
Tests/PrincipleMacrosTests/Syntax/Extensions/ExprSyntaxTests.swift (3)
21-42: LGTM! Literal tests are clear and correct.The integer, float, boolean, and string literal tests appropriately validate the
inferredTypebehavior for actual literal expressions.
57-90: LGTM! Comprehensive expression type coverage.The tests for initializers, member access, function calls, and type references are well-structured and correctly validate the
inferredTypebehavior for various expression types.
92-107: LGTM! Excellent edge case coverage.The parameterized composition test effectively validates complex nested expressions with generic types and optional handling, ensuring the
inferredTypelogic works correctly for real-world scenarios.Sources/PrincipleMacros/Syntax/Concepts/GlobalActorIsolation.swift (1)
34-39: standardizedIsolationAttribute implementation looks correctThe if-let shorthand cleanly wraps
standardizedIsolationTypeinto anAttributeSyntaxand returnsnilotherwise; behavior is straightforward and consistent with the rest of the type.Sources/PrincipleMacros/Syntax/Extensions/WithAttributesSyntax+GlobalActorIsolation.swift (1)
11-40: Global-actor extraction via attributes is consistent and focusedThe lazy scan over
attributescombined withAttributeListSyntax.Element.attributecleanly returns the first applicable global-actor attribute while ignoring#ifelements, and theAttributeSyntax.globalActorIsolationheuristic (suffix “Actor” + standardized type) aligns with both@MainActorand custom actor types. The enum integration viaGlobalActorIsolation.isolated(standardizedType:)is coherent with the rest of the API.Tests/PrincipleMacrosTests/Syntax/Concepts/GlobalActorIsolationTests.swift (4)
14-38: Renaming toMemberDeclarationmatches the tested surface betterScoping these tests under
MemberDeclarationclarifies that they cover properties/functions rather than types, and the existing cases exercise isolated, nonisolated, and inherited isolation well.
40-47: Direct test fornonisolated(nonsending)on a functionThis addition ensures that the trimming logic preserves the full
nonisolated(nonsending)modifier on member declarations, which is important for correctly reflecting isolation in diagnostics or generated code.
78-86: Coverage fornonisolated(nonsending)overriding enclosing global actorThis test confirms that an explicit
nonisolated(nonsending)function inside a@MyActorcontext correctly prefers the modifier over the enclosing actor, matching Swift’s semantics and the behavior ofGlobalActorIsolation.resolved.
111-148: Type-level isolation tests (TypeDeclaration) align with resolution rulesThe renamed
TypeDeclarationblock clearly targets class declarations and verifies that:
- Direct
@MainActoron the type is discovered.- An unannotated class does not inherit isolation from an unrelated
@MyActorstruct passed as lexical context.- A directly annotated class in a
@MyActorcontext still resolves to its own@MainActor.This matches the intended behavior of
_resolved(in:preferred:)for type declarations.Tests/PrincipleMacrosTests/Parsers/PropertiesListTests.swift (1)
36-37: Tests updated correctly to the new throwing parser APIUsing
try #require(decl.as(ClassDeclSyntax.self))is clearer than the former initializer style, and callingPropertiesParser.parse(memberBlock: classDecl.memberBlock)matches the newParsersurface while allowing diagnostics to propagate via throws.Tests/PrincipleMacrosTests/Builders/SwitchExprBuilderTests.swift (1)
14-18: LGTM! Good encapsulation.The visibility restriction to
privateis appropriate sincemakeEnumCaseis only used within this test struct.Tests/PrincipleMacrosTests/Syntax/Extensions/ClassDeclSyntaxTests.swift (1)
22-73: Comprehensive test coverage.The test suite provides good coverage of the
inferredSuperclass()method across various scenarios:
- Protocol-only conformance (no superclass)
- Detection via override modifier
- Detection via super expression
- Correct scoping (nested classes don't affect parent)
Sources/PrincipleMacros/Parsers/Properties/PropertiesList.swift (1)
11-17: LGTM! API visibility correctly promoted.The changes align with the PR's goal of promoting protocol visibility and making parser results more accessible. The public initializer is required by the
ParserResultsCollectionprotocol.Tests/PrincipleMacrosTests/Parsers/PropertiesParserTests.swift (1)
19-19: LGTM! Test updates align with new Parser API.All test cases have been correctly updated to use the new
parse(declaration:)signature without the context parameter. The tests maintain coverage of various property types and scenarios.Also applies to: 36-36, 56-56, 76-76, 96-96
Sources/PrincipleMacros/Parsers/Common/ParserResultsCollection.swift (2)
15-17: LGTM! Protocol requirement enables direct construction.The required initializer
init(_ all: [Element])allows conforming types to be constructed directly from an array of elements, improving API usability.
29-31: LGTM! Convenience initializer for empty collections.The default empty initializer provides a convenient way to create empty result collections and correctly delegates to the protocol requirement.
Sources/PrincipleMacros/Parsers/EnumCases/EnumCasesParser.swift (1)
11-25: LGTM! Parser conformance updated correctly.The conformance change from
_ParsertoParserand removal of the context parameter align with the PR's refactoring goals. The non-throwing signature is appropriate since enum case parsing doesn't require validation that could fail.Sources/PrincipleMacros/Diagnostics/DiagnosticsError.swift (1)
13-20: LGTM! Clean convenience API for diagnostics.The convenience initializer simplifies error creation in parsers by eliminating boilerplate for the common case of creating a single diagnostic from a node and message string.
Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift (4)
13-16: LGTM! Clean public API design.The
inferredSuperclass()method provides a clear, well-named API for detecting a class's superclass. Delegation to the visitor pattern maintains good separation of concerns.
21-29: LGTM! Appropriate use of SyntaxVisitor.The
SuperclassFinderclass correctly extendsSyntaxVisitorwithsourceAccurateview mode to traverse the class syntax tree. Thefinalmodifier and private visibility are good practices.
31-40: LGTM! Correct superclass detection logic.The
find()method correctly identifies the superclass by:
- Extracting the first inherited type (which must be the superclass in Swift)
- Walking the class to detect override modifiers or super expressions
- Returning the inherited type only if evidence of a superclass is found
This correctly distinguishes superclasses from protocol conformances.
42-54: LGTM! Visitor methods correctly implement detection logic.The visitor methods properly:
- Skip nested classes to avoid false positives (line 43)
- Detect override modifiers via
overrideSpecifier(line 47)- Detect super expressions (line 52)
This ensures accurate superclass inference scoped to the target class only.
Tests/PrincipleMacrosTests/Syntax/Extensions/AvailabilityTests.swift (1)
12-59: LGTM! Comprehensive test coverage for availability handling.The test suite covers the key scenarios for availability extraction:
- Classes without availability attributes
- Classes with direct
@availableattributes- Classes with availability attributes within
#ifconfig blocksThe test expectations are clear and validate the correct behavior of the availability extraction utilities.
Tests/PrincipleMacrosTests/Syntax/Extensions/IfConfigDeclSyntaxTests.swift (1)
12-165: LGTM! Thorough test coverage for IfConfig enclosing behavior.The test suite comprehensively validates:
- Properties without enclosing
#ifconfigurations- Simple and
#elseifconfigurations- Nested
#ifconfigurations- Applying configurations to new member blocks
The test expectations correctly validate the description output of the IfConfig transformations, and the helper method
parseLastPropertyprovides a clean abstraction for test setup.Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax+GlobalActorIsolation.swift (1)
11-28: LGTM! Clean implementation of global actor isolation extraction.The extension correctly extracts the
nonisolatedglobal actor isolation from modifiers using an appropriate lazy evaluation pattern. The implementation aligns well with the modular extension approach introduced in this PR.Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+Availability.swift (1)
11-33: LGTM! Well-structured availability extraction logic.The implementation correctly:
- Aggregates availability from all clauses
- Replaces non-availability clauses with empty attribute lists
- Returns
nilwhen no availability is foundThe use of the
isEmptyflag to track whether any availability was found is a clean approach.Sources/PrincipleMacros/Parsers/EnumCases/EnumCasesList.swift (1)
11-17: LGTM! Conformance update aligns with public API refactor.The changes correctly update
EnumCasesListto conform to the new publicParserResultsCollectionprotocol and expose the public initializer, aligning with the broader parser infrastructure refactor in this PR.Sources/PrincipleMacros/Parsers/Properties/PropertiesParser.swift (1)
11-44: LGTM! Clean refactor to explicit error handling.The changes correctly:
- Update conformance from
_Parserto publicParserprotocol- Replace implicit error handling via
context.diagnosewith explicitthrowstatements- Provide clear, actionable error messages
The error messages are particularly well-crafted:
- "Property cannot be parsed" - clear and concise
- "Type of property cannot be inferred - provide it explicitly" - actionable guidance for users
Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax+AccessControlLevel.swift (1)
11-24: LGTM! Well-structured access control level extraction.The implementation correctly:
- Extracts both general and setter-specific access control levels
- Uses lazy evaluation for efficiency
- Provides a sensible fallback where setter access inherits general access level when not explicitly specified (line 22)
Sources/PrincipleMacros/Syntax/Extensions/WithAttributesSyntax+Availability.swift (1)
11-48: LGTM! Clean and consistent availability extraction API.The extension provides a well-designed availability extraction hierarchy:
WithAttributesSyntaxdelegates to its attributes collectionAttributeListSyntaxaggregates availability from all elementsAttributeListSyntax.Elementhandles both direct attributes and#ifconfig declarationsAttributeSyntaxvalidates the presence ofAvailabilityArgumentListSyntaxThe implementation follows a consistent pattern with other availability-related extensions in this PR and correctly returns
nilwhen no availability information is present.Tests/PrincipleMacrosTests/Builders/EnumCaseCallExprBuilderTests.swift (2)
14-18: Helper visibility and construction look solidMaking
makeEnumCase(from:)private while centralizing theEnumCaseDeclSyntax/ first-element extraction keeps the tests tidy and encapsulated. The helper matches the pattern used inSwitchExprBuilderTestsand looks correct.
21-50: Enum-case call builder tests align with intended behaviorThe three tests (
withoutAssociatedValues,withUnnamedAssociatedValue,withMultipleAssociatedValues) all exercise the builder output strings in a clear way. UsingIssue.record()plus a dummyExprSyntaxin the no-associated-values case correctly marks the closure as logically unreachable while keeping the closure total. The associated-value mapping and expected descriptions look consistent with the parser behavior.Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax+Specifiers.swift (1)
11-50: Specifier helpers are clean and efficientThe
overrideSpecifier,finalSpecifier, andtypeScopeSpecifieraccessors on bothWithModifiersSyntaxandDeclModifierSyntaxare straightforward, short-circuit on the first match, and cover the expected cases (override,final,class/static). This is a nice, focused replacement for the previous monolithic modifiers extension.Tests/PrincipleMacrosTests/Parsers/EnumCasesParserTests.swift (1)
16-66: EnumCasesParser tests give good coverage of associated-value shapesThe four tests nicely cover the main enum-case shapes (no associated values, unnamed optional, named array, and multiple mixed values). The expectations on
trimmedName,standardizedName, andstandardizedTypeline up with the inferred normalization rules, and dropping theMacroExpansionContextfromparse(declaration:)keeps the call sites clean.Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift (1)
11-58: If-config alignment and enclosing traversal read as correct for nested configurationsThe
alignedhelpers plus theenclosingIfConfigimplementations onIfConfigDeclSyntax,IfConfigClauseListSyntax, andIfConfigClauseSyntaxform a coherent traversal: they normalize trivia, clear out non-selected clauses’ elements, and walk up to any enclosing#ifwhile preserving the active path. This matches the goal of reconstructing a canonical if-config context for a given clause or nested if-config.
| static func parse( | ||
| declaration: some DeclSyntaxProtocol, | ||
| in context: some MacroExpansionContext | ||
| ) -> ResultsCollection | ||
| declaration: some DeclSyntaxProtocol | ||
| ) throws -> ResultsCollection | ||
| } | ||
|
|
||
| static func parse( | ||
| memberBlock: MemberBlockSyntax, | ||
| in context: some MacroExpansionContext | ||
| ) -> ResultsCollection | ||
| extension Parser { | ||
|
|
||
| public static func parse( | ||
| ifConfig: IfConfigDeclSyntax | ||
| ) throws -> ResultsCollection { | ||
| try ResultsCollection( | ||
| ifConfig.clauses.flatMap { clause in | ||
| switch clause.elements { | ||
| case let .decls(members): | ||
| try parse(members: members) | ||
| default: | ||
| ResultsCollection() | ||
| } | ||
| } | ||
| ) | ||
| } | ||
|
|
||
| public static func parse( | ||
| members: MemberBlockItemListSyntax | ||
| ) throws -> ResultsCollection { | ||
| try ResultsCollection( | ||
| members.flatMap { member in | ||
| if let ifConfig = member.decl.as(IfConfigDeclSyntax.self) { | ||
| try parse(ifConfig: ifConfig) | ||
| } else { | ||
| try parse(declaration: member.decl) | ||
| } | ||
| } | ||
| ) | ||
| } | ||
|
|
||
| public static func parse( | ||
| memberBlock: MemberBlockSyntax | ||
| ) throws -> ResultsCollection { | ||
| try parse(members: memberBlock.members) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Parser helpers for declarations, members, and if-configs are well factored — verify conformers’ signatures
The new throws-based Parser.parse(declaration:) requirement and the added parse(ifConfig:), parse(members:), and parse(memberBlock:) helpers give a clear, reusable traversal story for members and nested #if blocks. The flat-mapping of per-clause/per-member ResultsCollections into a single ResultsCollection matches the existing collection design.
One thing to double-check is that every Parser conformer’s parse(declaration:) implementation and all call sites have been updated to the new throws signature where appropriate.
🏁 Script executed:
#!/bin/bash
# Find non-throwing parse(declaration:) implementations that may need updating.
rg -nP 'static\s+func\s+parse\(\s*declaration:\s*some\s+DeclSyntaxProtocol\s*\)\s*->' SourcesLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find the Parser protocol definition and all conformers
echo "=== Finding Parser protocol definition ==="
rg -nA5 'protocol\s+Parser' Sources
echo ""
echo "=== Finding types that conform to Parser ==="
rg -nP '\b(struct|class|enum)\s+\w+.*Parser\b' Sources
echo ""
echo "=== Finding all parse\(declaration: signatures (simplified) ==="
rg -nP 'parse\s*\(\s*declaration:' SourcesLength of output: 2102
🏁 Script executed:
#!/bin/bash
# Find the Parser protocol definition and all conformers
echo "=== Finding Parser protocol definition ==="
rg -nA5 'protocol\s+Parser' Sources
echo ""
echo "=== Finding types that conform to Parser ==="
rg -nP '\b(struct|class|enum)\s+\w+.*Parser\b' Sources
echo ""
echo "=== Finding all parse\(declaration: signatures (simplified) ==="
rg -nP 'parse\s*\(\s*declaration:' SourcesLength of output: 2102
🏁 Script executed:
#!/bin/bash
# Check EnumCasesParser.parse(declaration:) signature
echo "=== EnumCasesParser.parse(declaration:) ==="
rg -nA3 'static\s+func\s+parse' Sources/PrincipleMacros/Parsers/EnumCases/EnumCasesParser.swift
echo ""
echo "=== PropertiesParser.parse(declaration:) ==="
rg -nA3 'static\s+func\s+parse' Sources/PrincipleMacros/Parsers/Properties/PropertiesParser.swiftLength of output: 463
EnumCasesParser.parse(declaration:) must be updated to match the new throws protocol requirement
The Parser protocol now requires parse(declaration:) to throw, but EnumCasesParser's implementation at Sources/PrincipleMacros/Parsers/EnumCases/EnumCasesParser.swift:13 is still missing the throws keyword. PropertiesParser was correctly updated, but EnumCasesParser was not. Update the signature to:
public static func parse(declaration: some DeclSyntaxProtocol) throws -> EnumCasesList🤖 Prompt for AI Agents
Sources/PrincipleMacros/Parsers/EnumCases/EnumCasesParser.swift around line 13:
the parse(declaration:) implementation is missing the throws keyword and should
match the protocol; change the function signature to be public static func
parse(declaration: some DeclSyntaxProtocol) throws -> EnumCasesList and update
its body to propagate errors (use try where calling other throwing parsers) so
the implementation compiles against the protocol.
Tests/PrincipleMacrosTests/Syntax/Extensions/ClassDeclSyntaxTests.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
189-215: Consider extracting common member access parsing logic.The
type(withLabel:)method (lines 198-202) duplicates logic fromglobalActorIsolation(withLabel:)(lines 168-172). Both methods:
- Extract an expression with a label
- Cast to
MemberAccessExprSyntax- Extract
baseTypeReference- Throw
unexpectedSyntaxTypeon failureYou could extract a shared helper method to reduce duplication:
private func extractTypeFromMemberAccess( withLabel label: TokenSyntax? ) throws -> TypeSyntax? { guard let expression = expression(withLabel: label) else { return nil } guard let memberAccessExpression = MemberAccessExprSyntax(expression), let type = memberAccessExpression.baseTypeReference else { throw ParameterExtractionError.unexpectedSyntaxType } return type }Then both methods could call this helper, reducing maintenance burden.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift(2 hunks)Sources/PrincipleMacros/Syntax/Extensions/ExprSyntax.swift(4 hunks)Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+Availability.swift(1 hunks)Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift(1 hunks)Tests/PrincipleMacrosTests/Syntax/Extensions/ClassDeclSyntaxTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Tests/PrincipleMacrosTests/Syntax/Extensions/ClassDeclSyntaxTests.swift
- Sources/PrincipleMacros/Syntax/Extensions/ExprSyntax.swift
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (1)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
type(191-205)
🔇 Additional comments (6)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
168-177: LGTM! Guard-based approach improves clarity.The refactor from conditional logic to guard statements makes the error path explicit and improves readability. The extraction of
baseTypeReferencefrom the member access expression is correct.Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (1)
147-168: LGTM! Well-structured tests following established patterns.The test extension provides good coverage:
- Parameterized test validates extraction for both simple and qualified type names
- Error case test ensures proper exception handling for invalid syntax
- Naming and structure are consistent with existing test extensions (e.g.,
globalActorExtraction)Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+Availability.swift (4)
1-10: LGTM!File header and imports are appropriate for the functionality being added.
35-44: LGTM!The delegation to
elements?.availabilityis clean and follows good patterns for optional propagation.
48-58: No issues found—extension dependency verified.The code correctly accesses
attributes.availabilityon line 51. Verification confirms thatAttributeListSyntaxhas a publicavailabilityproperty defined inWithAttributesSyntax+Availability.swift(line 20). The dependency exists and is properly implemented; the code is correct as written.
17-24: The line 22 behavior is intentional and validated by tests.The
withIfConfig()test (lines 40-57 in AvailabilityTests.swift) explicitly validates that the availability property extracts availability structure from #if config blocks by preserving conditional branches while replacing non-availability clause elements with empty attributes. For example, the#if os(macOS)clause without availability becomes an empty clause in the result.This is not corruption—it's the intended design: extract which branches have availability annotations while maintaining the conditional structure. The behavior you flagged at line 22 is correct for this use case.
| public func type( | ||
| withLabel label: TokenSyntax? | ||
| ) throws -> TypeSyntax? { | ||
| guard let expression = expression(withLabel: label) else { | ||
| return nil | ||
| } | ||
|
|
||
| guard let memberAccessExpression = MemberAccessExprSyntax(expression), | ||
| let type = memberAccessExpression.baseTypeReference | ||
| else { | ||
| throw ParameterExtractionError.unexpectedSyntaxType | ||
| } | ||
|
|
||
| return type | ||
| } | ||
|
|
||
| public func requiredType( | ||
| withLabel label: TokenSyntax? | ||
| ) throws -> TypeSyntax { | ||
| guard let type = try type(withLabel: label) else { | ||
| throw ParameterExtractionError.missingRequirement | ||
| } | ||
| return type | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add documentation for public API methods.
These new public methods lack doc comments explaining their purpose, parameters, return values, and error conditions. Since they're part of the public API, documentation would improve discoverability and usage.
Consider adding documentation like:
/// Extracts a type from a labeled expression of the form `Type.self`.
///
/// - Parameter label: The label of the parameter to extract, or `nil` for unlabeled parameters.
/// - Returns: The extracted type syntax, or `nil` if no expression with the given label exists.
/// - Throws: `ParameterExtractionError.unexpectedSyntaxType` if the expression is not a member access with a base type reference.
public func type(
withLabel label: TokenSyntax?
) throws -> TypeSyntax? {🤖 Prompt for AI Agents
In Sources/PrincipleMacros/Parameters/ParameterExtractor.swift around lines 191
to 214, the public methods `type(withLabel:)` and `requiredType(withLabel:)`
lack documentation; add Swift doc comments for both methods describing their
purpose, the `label` parameter (nil meaning unlabeled), the return value
(optional TypeSyntax for `type`, non-optional for `requiredType`), and the
thrown errors (`ParameterExtractionError.unexpectedSyntaxType` for `type` when
the expression isn't a member access with a base type reference, and
`ParameterExtractionError.missingRequirement` for `requiredType` when no type is
found); keep comments concise and follow the Swift /// style used across the
project so documentation is picked up by Xcode and swift-doc tools.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Sources/PrincipleMacros/Syntax/Extensions/TypeSyntax.swift (2)
25-36: LGTM! Clean and efficient implementation.The lazy evaluation in
first(like:)ensures the iteration stops at the first match, and the delegation pattern incontains(like:)is idiomatic. Consider adding documentation comments to these public API methods to describe the "likeness" comparison behavior.
18-36: Consider file organization for maintainability.The file is named
TypeSyntax.swiftbut now includes extensions forAttributeSyntaxandAttributeListSyntax. While consolidation reduces file count, a more descriptive filename (e.g.,SyntaxExtensions.swift) or separate files might improve discoverability for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Sources/PrincipleMacros/Syntax/Extensions/TypeSyntax.swift(1 hunks)
🔇 Additional comments (1)
Sources/PrincipleMacros/Syntax/Extensions/TypeSyntax.swift (1)
18-23: I need to search more specifically for the actual usage of the methods in the extension and see the complete file context.Based on my analysis of the codebase and the original review comment:
Verify that name-only comparison is the intended behavior.
The
isLikemethod implemented inAttributeSyntaxonly compares attribute names (by delegating toattributeName.isLike(other.attributeName)), ignoring all attribute arguments. For example,@available(iOS 13, *)would be considered "like"@available(macOS 10.15, *)since both have the same attribute name. Confirm this behavior aligns with your use cases for thePropertiesList.withInferredType(like:)method, or whether argument-level comparison is needed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
217-241: Address the documentation feedback from the previous review.The past review comment already noted that these public API methods lack documentation. Please add doc comments as suggested in the previous review.
🧹 Nitpick comments (4)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (2)
126-138: Consider the optional boolean return type.SwiftLint flags the
Bool?return type as discouraged. While optional booleans can lead to ambiguity (three states: true/false/nil), in this case the nil state serves a clear purpose—distinguishing "parameter not provided" from "parameter provided with value false."If you prefer to address the linter warning, consider returning a custom enum or result type that makes the tri-state explicit.
217-230: Consider extracting the common parsing logic.The
type(withLabel:)method shares nearly identical parsing logic withglobalActorIsolation(withLabel:)(lines 194-198). Both extract aMemberAccessExprSyntaxand access itsbaseTypeReference. Consider extracting this common pattern into a private helper method to reduce duplication.Example refactor:
private func extractBaseType( withLabel label: TokenSyntax? ) throws -> TypeSyntax? { guard let expression = expression(withLabel: label) else { return nil } guard let memberAccessExpression = MemberAccessExprSyntax(expression), let type = memberAccessExpression.baseTypeReference else { throw ParameterExtractionError.unexpectedSyntaxType } return type }Then both methods can use:
public func type(withLabel label: TokenSyntax?) throws -> TypeSyntax? { return try extractBaseType(withLabel: label) } public func globalActorIsolation(withLabel label: TokenSyntax?) throws -> GlobalActorIsolation? { // Handle nil literal case first if let expression = expression(withLabel: label), NilLiteralExprSyntax(expression) != nil { let isolation = DeclModifierSyntax(name: .keyword(.nonisolated)) return .nonisolated(trimmedModifer: isolation) } guard let globalActorType = try extractBaseType(withLabel: label) else { return nil } return .isolated(standardizedType: globalActorType.standardized) }Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift (2)
13-15: Clean implementation of firstInheritedType.The property correctly accesses the first inherited type from the inheritance clause. The implementation is straightforward and correct.
Consider adding documentation to clarify that this returns the first inherited type regardless of whether it's a class or protocol:
+ /// Returns the first type from the inheritance clause, which is typically + /// the superclass when present (Swift requires superclasses before protocols). public var firstInheritedType: TypeSyntax? { inheritanceClause?.inheritedTypes.first?.type }
17-20: Solid implementation of superclass inference.The method correctly delegates to the visitor pattern and applies
.trimmedfor clean type representation. The approach of detectingoverridemodifiers orsuperexpressions to infer superclass presence aligns well with Swift's inheritance semantics.Consider adding documentation to explain the inference logic:
+ /// Infers the superclass by detecting evidence of inheritance (override modifiers + /// or super expressions) and returns the first inherited type when found. + /// Returns nil if no superclass is detected. public func inferredSuperclass() -> TypeSyntax? { let superclassFinder = SuperclassFinder(for: self) return superclassFinder.find()?.trimmed }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift(3 hunks)Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift(1 hunks)Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (1)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (2)
rawBool(126-138)type(217-231)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (2)
Sources/PrincipleMacros/Builders/Expressions/AssociatedValuesListExprBuilder.swift (1)
expression(43-53)Tests/PrincipleMacrosTests/Parsers/PropertiesListTests.swift (1)
type(77-86)
🪛 SwiftLint (0.57.0)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift
[Warning] 128-128: Prefer non-optional booleans over optional booleans
(discouraged_optional_boolean)
[Warning] 142-142: Prefer non-optional booleans over optional booleans
(discouraged_optional_boolean)
🔇 Additional comments (5)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
194-202: LGTM! Stricter validation improves error handling.The guard-based approach with explicit validation of
MemberAccessExprSyntaxandbaseTypeReferenceis clearer and fails fast on invalid input.Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (2)
99-120: LGTM! Comprehensive test coverage for boolean extraction.The tests properly validate both the happy path (true/false extraction) and error handling (non-boolean syntax). The parameterized approach is clean and follows the established test patterns.
170-191: LGTM! Comprehensive test coverage for type extraction.The tests properly validate both simple and qualified type names, plus error handling for invalid syntax. The test structure mirrors the
globalActorExtractiontests, maintaining consistency across the test suite.Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift (2)
35-46: Excellent handling of nested classes and early exit.The
find()method efficiently returns early when no inheritance clause exists. The visitor correctly prevents descending into nested classes (line 45) by checking node identity, which avoids false positives from nested class overrides or super calls.
48-56: Correct detection of superclass indicators.Both visitor methods correctly identify evidence of superclass inheritance. The
overrideSpecifiercheck (line 49) andSuperExprSyntaxdetection (line 54) align with Swift's semantics where these features are only valid when a superclass exists.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
.swiftlint.yml (1)
30-31: Disabling discouraged_optional_boolean: confirm intent; prefer scoped relax over global.Optional Bool invites tri‑state ambiguity (nil/true/false). If this was to accommodate a few spots, consider re‑enabling the rule and:
- add localized disables (
// swiftlint:disable:next discouraged_optional_boolean) where strictly needed, or- add a nested .swiftlint.yml to disable it only in specific directories (e.g., generated/macro internals), or
- refactor to an enum (e.g., TriState { unset, enabled, disabled }) for clarity.
If the global relax is intentional, acknowledge the trade‑off to avoid accidental Optional in public APIs.
Would you like me to propose a scoped config patch (keeping the rule enabled but excluded for selected paths)?
Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift (2)
80-100: Clarify and test the semantics of the newinheritedBy*helpersThe three new helpers nicely centralize the propagation rules, but the
.privatebehavior is subtle:
inheritedByMemberreturnsnilfor.private(vs some concrete level).inheritedBySiblingpromotes.privateto.fileprivate.inheritedByPeerjust returnsself.Given how close this is to Swift’s access control rules, it would be good to either (a) add brief doccomments spelling out the intended invariants (e.g. “
nilhere means no constraint inherited from the enclosing type”), and/or (b) add a focused test matrix that asserts the mapping for each case (especially.privateand.package), so future refactors don’t accidentally drift.
141-155: Confirm_resolved’s new transform semantics (no re‑clamp, only applied toattached)The updated
_resolvedchanges behavior in a couple of ways that are worth double‑checking:
transformis now required and is applied only toattached, not topreferred.- The result of
transform(attached)is not re‑clamped bymaxAllowed, so a transform is allowed to widen the level (e.g..private→.fileprivate) as long asattacheditself passed themin(attached, maxAllowed)check.If the intent is:
preferred= a final, already context‑adjusted level that just needs clamping, andattached= raw modifier that still needs context‑specific adjustment viatransform,then this implementation matches that mental model. If, however, you want the final result (including after
transform) to obey<= maxAllowed, or you expectpreferredto also go through the sameinheritedBy*transform, you may want to:
- Re‑clamp the transformed value:
if let transformed = transform(attached) { return min(transformed, maxAllowed) }, and/or- Apply
transformtopreferredas well for symmetry.Might be worth scanning call sites with those invariants in mind to ensure there isn’t a silent behavioral change.
Sources/PrincipleMacros/Builders/Declarations/Types/ClassDeclBuilder.swift (2)
11-15: Clarify intent oftrimmedSuperclassTypeon the protocolThe new requirement is clear mechanically but its semantics (trimmed vs raw type, whether it must be a class vs protocol composition, etc.) are implicit. A brief doc comment on
ClassDeclBuilder.trimmedSuperclassTypewould make it easier for conformers to implement this consistently and avoid divergent interpretations.
26-38: Consider defaultingtrimmedSuperclassTypeto the inferred superclassRight now the default implementation always returns
nil, soinheritedOverrideModifieris only active if conformers overridetrimmedSuperclassType. If the goal is to hook into the newClassDeclSyntax.inferredSuperclass()logic by default, you could wire it through here and still allow conformers to override if needed:- public var trimmedSuperclassType: TypeSyntax? { - nil - } + public var trimmedSuperclassType: TypeSyntax? { + declaration.inferredSuperclass() + }That would make
inheritedOverrideModifier“just work” for common cases without extra boilerplate in each builder.Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift (2)
22-38: Double‑checkisExpected == truesemantics vs verified inferenceFor
inferredSuperclass(isExpected:), thetruebranch returnsunverifiedInferredSuperclasswithout consultingSubclassKeywordsVisitor, while thenilbranch uses the verifiedinferredSuperclass(). That means:
isExpected == truewill accept “any first inherited type” as a superclass.isExpected == nilmay returnnilfor the same class if nooverride/superevidence is found.If this divergence is intentional (e.g., “expected” means syntactically present, regardless of override usage), it’s fine, but it’s subtle. If you instead want the “expected” path to align with the verified heuristic, consider:
- case true: - if let superclass = unverifiedInferredSuperclass { - return superclass - } + case true: + if let superclass = inferredSuperclass() { + return superclass + }Either way, a short doc comment on the
isExpectedparameter explaining the tri‑state behavior (true/false/nil) would help call sites avoid misuse.
44-76: Visitor logic is sound; consider optional early‑exit / API tweaksThe
SubclassKeywordsVisitorcorrectly:
- Restricts traversal to the target
ClassDeclSyntaxvia thevisit(_ node: ClassDeclSyntax)override.- Treats either any
overridemodifier or anySuperExprSyntaxas sufficient evidence of subclass behavior.Two optional nits you might consider:
- Add an early‑exit guard to avoid unnecessary work once
didVerifyis true, e.g. short‑circuit invisit(_ node: DeclModifierSyntax)/visit(_: SuperExprSyntax)by returning.skipChildrenwhen already verified.- If this grows further, you might expose the verification logic as a standalone helper (e.g.,
hasSubclassIndicators) to make it reusable beyond this extension.These are purely micro/ergonomic improvements; the current implementation is correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.swiftlint.yml(1 hunks)Sources/PrincipleMacros/Builders/Declarations/Types/ClassDeclBuilder.swift(2 hunks)Sources/PrincipleMacros/Parameters/ParameterExtractor.swift(3 hunks)Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift(4 hunks)Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/PrincipleMacros/Parameters/ParameterExtractor.swift
🧰 Additional context used
🧬 Code graph analysis (2)
Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift (1)
Sources/PrincipleMacros/Syntax/Concepts/GlobalActorIsolation.swift (1)
_resolved(65-88)
Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift (2)
Sources/PrincipleMacros/Syntax/Extensions/TypeSyntax.swift (1)
first(31-35)Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (1)
type(217-231)
🔇 Additional comments (2)
Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift (1)
102-139: Key‑path basedtransformwiring looks good and keeps call sites conciseUsing
transform: \.inheritedByMember / \.inheritedBySibling / \.inheritedByPeermakes the three public helpers read clearly and avoids repeating transformation logic per call site. Assuming the newinheritedBy*semantics are what you want, these entry points look consistent and straightforward.Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift (1)
13-20: Superclass inference helpers look consistent with Swift inheritance rulesUsing
inheritanceClause?.inheritedTypes.first?.type.trimmedforunverifiedInferredSuperclassaligns with the “class first, then protocols” rule for class inheritance, and theinferredSuperclass()wrapper cleanly delegates verification to the visitor. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift (1)
80-107: Access-control inheritance helpers look coherent; check.privatebehavior and lint grouping.The new
inheritedByMember,inheritedBySibling, andinheritedByPeerhelpers read cleanly and themin(self, maxAllowed)use matches the enum’s case ordering, so the clamping logic itself looks sound. The one subtle semantic knob is treating.privatespecially:inheritedByMemberreturnsnilfor.private, while siblings get promoted toward.fileprivate(capped bymaxAllowed). That seems intentional for your macro model, but it’s worth double‑checking with tests that this matches how you want private containers to influence synthesized members/peers, especially around extensions and same‑file access.SwiftLint flags these three as
other_methodinterleaved withtype_method(type_contents_order); if you care about keeping the config green, you may want to move the instance helpers either above or below the staticfor*methods, or locally disable the rule around this extension.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilder.swift(0 hunks)Sources/PrincipleMacros/Builders/Declarations/Common/MemberBuilding.swift(1 hunks)Sources/PrincipleMacros/Builders/Declarations/Common/PeerBuilding.swift(1 hunks)Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift(4 hunks)
💤 Files with no reviewable changes (1)
- Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilder.swift
🧰 Additional context used
🧬 Code graph analysis (2)
Sources/PrincipleMacros/Builders/Declarations/Common/PeerBuilding.swift (1)
Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift (1)
forPeer(134-145)
Sources/PrincipleMacros/Builders/Declarations/Common/MemberBuilding.swift (1)
Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift (1)
forMember(108-119)
🪛 SwiftLint (0.57.0)
Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift
[Warning] 80-80: An 'other_method' should not be placed amongst the type content(s) 'type_method'
(type_contents_order)
[Warning] 91-91: An 'other_method' should not be placed amongst the type content(s) 'type_method'
(type_contents_order)
[Warning] 102-102: An 'other_method' should not be placed amongst the type content(s) 'type_method'
(type_contents_order)
🔇 Additional comments (3)
Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift (1)
108-160: Resolution pipeline (for*+_resolved) is straightforward; precedence rules look correct.The refactored
forMember/forSibling/forPeerall delegate through_resolvedwith a transform, which makes the inheritance strategy explicit and keepsmaxAllowedconsistently enforced. The precedence in_resolvedofpreferred(clamped bymaxAllowed) over any attached level, and then falling back to the transformed attached level, is clear and seems like the right API contract for builders.Given the change in behavior goes through a shared helper, I’d ensure you have tests that cover:
- preferred more/less visible than the attached level,
.privateattached levels for members vs siblings vs peers,- the
.open‑allowing variants in the builders.If those pass, this abstraction looks solid.
Sources/PrincipleMacros/Builders/Declarations/Common/PeerBuilding.swift (1)
13-26: Peer ACL inheritance properties align with the newAccessControlLevel.forPeerAPI.These computed properties are thin, readable adapters over
AccessControlLevel.forPeer, and the.open‑specific variant now clearly advertises that it can surfaceopenwhen allowed. The pattern matches the member‑building side and keeps all the clamping logic centralized inAccessControlLevel.I’d just make sure your tests cover both properties for combinations like private/public/open peers so regressions in the shared helper are caught quickly.
Sources/PrincipleMacros/Builders/Declarations/Common/MemberBuilding.swift (1)
13-26: Member ACL inheritance mirrors peer logic and keeps rules centralized.
inheritedAccessControlLeveland itsAllowingOpenvariant nicely route throughAccessControlLevel.forMember, so the special handling for.privatecontainers andmaxAllowedclamping all live in one place. The symmetry withPeerBuildingmakes the builder surface predictable.As with peers, it’d be good to have test coverage for private/internal/public/open types to confirm the synthesized member access levels stay within the language rules you expect.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.swiftlint.yml(2 hunks)Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift (1)
Sources/PrincipleMacros/Syntax/Concepts/GlobalActorIsolation.swift (1)
_resolved(65-88)
🔇 Additional comments (4)
.swiftlint.yml (2)
30-30: Clarify the rationale and permanence of disablingdiscouraged_optional_boolean.This opt-in rule is being disabled, likely due to new code patterns introduced in the refactoring. Please confirm:
- Which code locations require
Optional<Bool>and why alternatives (e.g., enum-based states) aren't viable?- Is this disablement temporary (for this refactoring phase) or permanent?
70-70: Clarify the rationale and permanence of disablingmodifier_order.This opt-in rule is being disabled, likely due to the split of "Attribute/modifier responsibilities into focused extensions" mentioned in the PR summary. Please confirm:
- Which specific modifier orderings in the refactored code conflict with this rule?
- Is this disablement temporary (during the refactoring) or a permanent adaptation to the new architecture?
- Could the code be structured to comply with the rule instead (e.g., by ensuring modifiers consistently appear in the expected order across extension boundaries)?
Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift (2)
116-148: Good refactoring using the new inheritance helpers.The transformation logic is now cleanly delegated to the dedicated
inheritedBy*methods via thetransformclosure parameter, making the code more maintainable and the inheritance semantics explicit.
150-163: LGTM: Clean abstraction with the transform parameter.Parameterizing the transformation strategy via the
transformclosure makes_resolvedmore flexible and reusable across different inheritance contexts (member, sibling, peer).
| extension AccessControlLevel { | ||
|
|
||
| public func inheritedByMember( | ||
| maxAllowed: Self = .public | ||
| ) -> Self? { | ||
| switch self { | ||
| case .private: | ||
| nil | ||
| default: | ||
| min(self, maxAllowed) | ||
| } | ||
| } | ||
|
|
||
| public func inheritedBySibling( | ||
| maxAllowed: Self = .public | ||
| ) -> Self { | ||
| switch self { | ||
| case .private: | ||
| min(.fileprivate, maxAllowed) | ||
| default: | ||
| min(self, maxAllowed) | ||
| } | ||
| } | ||
|
|
||
| public func inheritedByPeer( | ||
| maxAllowed: Self = .public | ||
| ) -> Self { | ||
| min(self, maxAllowed) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add documentation to clarify inheritance semantics and return type differences.
These public helper methods implement important access control inheritance logic, but lack documentation explaining:
- Why
inheritedByMemberreturnsSelf?(private members aren't inherited) - Why
inheritedBySiblingandinheritedByPeerreturn non-optionalSelf(siblings/peers always have some access level) - The semantic difference between "member," "sibling," and "peer" in this context
- Edge case behavior when
maxAllowedis more restrictive thanself
Consider adding doc comments to each method. For example:
/// Returns the access control level inherited by a member declaration, or `nil` if not inheritable.
///
/// - Parameter maxAllowed: The maximum access level allowed for the member.
/// - Returns: The inherited access level, or `nil` for `private` (which is not inherited by members).
public func inheritedByMember(maxAllowed: Self = .public) -> Self? {🤖 Prompt for AI Agents
In Sources/PrincipleMacros/Syntax/Concepts/AccessControlLevel.swift around lines
78 to 107, the three public helpers lack documentation explaining their
inheritance semantics, the nullable return of inheritedByMember, and edge-case
behavior with maxAllowed; add Swift doc comments to each method that briefly
state what "member", "sibling", and "peer" mean in this context, why
inheritedByMember returns an optional (private yields nil), why
inheritedBySibling and inheritedByPeer are non-optional (they always produce
some access), describe the maxAllowed parameter and its effect when more
restrictive than self, and document the return value and any nil cases and
examples where applicable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Sources/PrincipleMacros/Builders/Declarations/Members/PropertyDeclAccessorBuilder.swift (1)
11-14: LGTM! Consider adding documentation.The protocol design is clean and follows the builder pattern appropriately. The
throwsdeclaration properly communicates that building accessors may fail.Optionally, consider adding documentation to describe the protocol's purpose and the contract of
buildAccessors().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Sources/PrincipleMacros/Builders/Declarations/Members/PropertyDeclAccessorBuilder.swift(1 hunks)
⏰ 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 (maccatalyst)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (macos)
- GitHub Check: build-and-test (ios)
🔇 Additional comments (1)
Sources/PrincipleMacros/Builders/Declarations/Members/PropertyDeclAccessorBuilder.swift (1)
1-10: LGTM!File header and imports are properly structured.
| public func build() throws -> [DeclSyntax] { | ||
| try buildAccessors().map(DeclSyntax.init) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the PropertyDeclAccessorBuilder file
fd -n "PropertyDeclAccessorBuilder.swift" -x cat -n {}Repository: NSFatalError/PrincipleMacros
Length of output: 302
🏁 Script executed:
#!/bin/bash
# Search for the buildAccessors method definition
rg -n "buildAccessors" --type swift -B2 -A5Repository: NSFatalError/PrincipleMacros
Length of output: 1348
🏁 Script executed:
#!/bin/bash
# Search for protocol or extension definitions related to PropertyDeclAccessorBuilder
rg -n "PropertyDeclAccessorBuilder|protocol.*Accessor|extension.*Accessor" --type swift -B2 -A10Repository: NSFatalError/PrincipleMacros
Length of output: 3443
🏁 Script executed:
#!/bin/bash
# Look for usages of PropertyDeclAccessorBuilder to understand how it's used
rg -n "PropertyDeclAccessorBuilder" --type swift -C3Repository: NSFatalError/PrincipleMacros
Length of output: 2020
🏁 Script executed:
#!/bin/bash
# Find PropertyDeclBuilder protocol definition
rg -n "protocol PropertyDeclBuilder" --type swift -A10Repository: NSFatalError/PrincipleMacros
Length of output: 1210
🏁 Script executed:
#!/bin/bash
# Search for implementations or conformances to PropertyDeclAccessorBuilder
rg -n "PropertyDeclAccessorBuilder" --type swift -B3 -A15Repository: NSFatalError/PrincipleMacros
Length of output: 2450
🏁 Script executed:
#!/bin/bash
# Look at the project's Package.swift or similar to understand SwiftSyntax version
fd -type f -name "Package.swift" -o -name "Package.resolved" | head -5 | xargs catRepository: NSFatalError/PrincipleMacros
Length of output: 242
🏁 Script executed:
#!/bin/bash
# Find Package.swift
find . -name "Package.swift" -o -name "Package.resolved" | head -3 | xargs cat 2>/dev/null | head -100Repository: NSFatalError/PrincipleMacros
Length of output: 1978
🏁 Script executed:
#!/bin/bash
# Search for types that conform to PropertyDeclAccessorBuilder
rg -n "PropertyDeclAccessorBuilder" --type swift -C5Repository: NSFatalError/PrincipleMacros
Length of output: 2450
🏁 Script executed:
#!/bin/bash
# Search for struct/class implementing PropertyDeclAccessorBuilder
rg -n ": PropertyDeclAccessorBuilder" --type swift -A20Repository: NSFatalError/PrincipleMacros
Length of output: 54
🌐 Web query:
SwiftSyntax 602.0.0 AccessorDeclSyntax DeclSyntax type conversion
💡 Result:
Short answer: AccessorDeclSyntax is not a DeclSyntax (and cannot be implicitly converted to one). In SwiftSyntax 602 you must produce an AccessorDeclSyntax value when an AccessorMacro expects [AccessorDeclSyntax] — returning a DeclSyntax (or trying to cast an AccessorDeclSyntax to DeclSyntax) will fail.
Why: AccessorDeclSyntax represents accessor nodes (get/set/willSet/didSet) and conforms to SyntaxProtocol / AccessorDeclSyntaxProtocol, but it is not a DeclSyntax node (DeclSyntax represents top‑level/peer declarations). The macro APIs differentiate these types, so the compiler enforces returning the exact accessor type for AccessorMacro. Evidence and discussion in the Swift forums shows this exact error when a macro returned DeclSyntax instead of AccessorDeclSyntax. [1][3]
What to do:
- Build and return AccessorDeclSyntax values directly (use the provided initializers / stringLiteral or the builder closure forms). Examples and workarounds are discussed in the forums and tutorials. [1][3][5]
- Do not try to wrap or cast an AccessorDeclSyntax into DeclSyntax — that conversion is invalid for accessor nodes. If you actually need to produce a declaration instead, construct a DeclSyntax (e.g., VariableDeclSyntax / FunctionDeclSyntax) and return that from a Peer/Member macro that expects DeclSyntax. [4][6]
Sources:
- Swift Forums discussion (macro returned DeclSyntax vs expected AccessorDeclSyntax). [1].
- Example Accessor macro threads (showing how to construct AccessorDeclSyntax). [3][5].
- SwiftSyntax 602.0.0 release (context for API versions). [6]
If you want, paste the code that fails and I’ll show the minimal change to return a proper AccessorDeclSyntax.
AccessorDeclSyntax cannot be converted to DeclSyntax—this code will not compile.
In SwiftSyntax 602.0.0, AccessorDeclSyntax and DeclSyntax are distinct, non-convertible types. AccessorDeclSyntax represents accessor nodes (get, set, willSet, didSet) that conform to SyntaxProtocol, while DeclSyntax represents top-level declarations. The mapping at line 19 attempting to convert [AccessorDeclSyntax] to [DeclSyntax] via DeclSyntax.init will result in a compiler error.
The build() method signature declares a return type of [DeclSyntax], but buildAccessors() produces [AccessorDeclSyntax]. Either:
- Change the protocol's
build()return type to[AccessorDeclSyntax]if the intent is to return accessors, or - Implement
build()to construct actual declarations (e.g.,VariableDeclSyntax) containing the accessors in anAccessorBlockSyntax.
🤖 Prompt for AI Agents
In
Sources/PrincipleMacros/Builders/Declarations/Members/PropertyDeclAccessorBuilder.swift
around lines 18–20, the build() method returns [DeclSyntax] but buildAccessors()
yields [AccessorDeclSyntax] which cannot be converted to DeclSyntax; either
change the method signature to return [AccessorDeclSyntax] if callers expect
accessors, or construct a proper declaration that contains the accessors (for
example create an AccessorBlockSyntax from buildAccessors() and embed it in a
VariableDeclSyntax or other appropriate DeclSyntax) and return that; update any
protocol or caller types if you change the return type.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift(1 hunks)
⏰ 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 (macos)
- GitHub Check: build-and-test (ios)
- GitHub Check: build-and-test (watchos)
🔇 Additional comments (3)
Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift (3)
13-15: LGTM!The implementation correctly retrieves the first inherited type from the inheritance clause and trims it. The "unverified" naming appropriately signals that this might be a protocol rather than a class.
17-20: LGTM!The method correctly delegates to the SuperclassVerifier to return a verified superclass type.
44-77: Swift grammar assumption is correct;DeclModifierSyntax.overrideSpecifieris a valid custom extension in the codebase.The verification confirms:
Swift's type-inheritance rules enforce that a superclass (or enum raw type) must appear first in the inheritance clause, before any protocols. The compiler rejects declarations like
class Bad: ProtocolA, Super {}with the error "superclass 'X' must appear first in the inheritance clause." This validates the assumption in the code.
DeclModifierSyntax.overrideSpecifier(line 68) is a custom computed property extension defined inSources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax+Specifiers.swift(lines 34–36). It correctly returns the modifier token whenname.tokenKind == .keyword(.override). The property is properly implemented and available for use.The
SuperclassVerifierimplementation is sound.
| public func inferredSuperclassType( | ||
| isExpected: Bool? | ||
| ) throws -> TypeSyntax? { | ||
| switch isExpected { | ||
| case nil: | ||
| return inferredSuperclassType() | ||
| case true: | ||
| if let type = unverifiedInferredSuperclassType { | ||
| return type | ||
| } | ||
| throw DiagnosticsError( | ||
| node: self, | ||
| message: "\(name.trimmed) should have a superclass" | ||
| ) | ||
| case false: | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent behavior when isExpected is true.
When isExpected is true, the method returns the unverified inherited type (line 29-30), bypassing the verification logic used when isExpected is nil. This could return a protocol instead of a class, leading to incorrect assumptions by callers.
The method should likely return the verified superclass type when isExpected is true, and throw if verification fails (not just if the inherited type is missing).
Consider applying this approach:
) throws -> TypeSyntax? {
switch isExpected {
case nil:
return inferredSuperclassType()
case true:
- if let type = unverifiedInferredSuperclassType {
+ if let type = inferredSuperclassType() {
return type
}
throw DiagnosticsError(
node: self,
message: "\(name.trimmed) should have a superclass"
)
case false:
return nil
}
}📝 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.
| public func inferredSuperclassType( | |
| isExpected: Bool? | |
| ) throws -> TypeSyntax? { | |
| switch isExpected { | |
| case nil: | |
| return inferredSuperclassType() | |
| case true: | |
| if let type = unverifiedInferredSuperclassType { | |
| return type | |
| } | |
| throw DiagnosticsError( | |
| node: self, | |
| message: "\(name.trimmed) should have a superclass" | |
| ) | |
| case false: | |
| return nil | |
| } | |
| } | |
| public func inferredSuperclassType( | |
| isExpected: Bool? | |
| ) throws -> TypeSyntax? { | |
| switch isExpected { | |
| case nil: | |
| return inferredSuperclassType() | |
| case true: | |
| if let type = try inferredSuperclassType() { | |
| return type | |
| } | |
| throw DiagnosticsError( | |
| node: self, | |
| message: "\(name.trimmed) should have a superclass" | |
| ) | |
| case false: | |
| return nil | |
| } | |
| } |
🤖 Prompt for AI Agents
In Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift around lines
22 to 39, the isExpected == true branch currently returns
unverifiedInferredSuperclassType (bypassing verification) which can yield a
protocol; change it to call the verified method instead: call try
inferredSuperclassType() (same as the nil case), if it returns a TypeSyntax
return it, otherwise throw the DiagnosticsError that the class should have a
superclass; this ensures verification runs and verification errors propagate
rather than returning an unverified type.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Tests/PrincipleMacrosTests/Syntax/Extensions/ClosureExprSyntaxTests.swift (1)
25-25: Consider extracting the magic number to a named constant.The nesting level value
2is repeated across all three tests. Extracting it to a constant would improve maintainability if the nesting level needs to change in the future.Apply this diff to extract the constant:
struct Expansion { + + private let testNestingLevel = 2 @Test func withoutSignature() { let expr: ExprSyntax = """ { Date.now } """ let interpolation: ExprSyntax = """ .init( parameter: .init( - closure: \(expr.expanded(nestingLevel: 2)), + closure: \(expr.expanded(nestingLevel: testNestingLevel)), value: "Foo" ) ) """And similarly update lines 54 and 90 to use
testNestingLevel.Also applies to: 54-54, 90-90
Tests/PrincipleMacrosTests/Syntax/Extensions/IfConfigDeclSyntaxTests.swift (1)
119-126: Note the trailing space in expectation string.Line 121 has
#elsewith a trailing space. If this matches the actualIfConfigDeclSyntax.descriptionoutput, it's correct. However, relying on exact whitespace in description strings can make tests fragile if the SwiftSyntax library changes its formatting behavior.Consider whether trimming or a more flexible comparison would improve test robustness, or confirm this matches the expected output intentionally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Tests/PrincipleMacrosTests/Syntax/Extensions/ClassDeclSyntaxTests.swift(1 hunks)Tests/PrincipleMacrosTests/Syntax/Extensions/ClosureExprSyntaxTests.swift(1 hunks)Tests/PrincipleMacrosTests/Syntax/Extensions/ExprSyntaxTests.swift(1 hunks)Tests/PrincipleMacrosTests/Syntax/Extensions/IfConfigDeclSyntaxTests.swift(1 hunks)Tests/PrincipleMacrosTests/Syntax/Extensions/TypeSyntaxTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Tests/PrincipleMacrosTests/Syntax/Extensions/ClassDeclSyntaxTests.swift
- Tests/PrincipleMacrosTests/Syntax/Extensions/TypeSyntaxTests.swift
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/PrincipleMacrosTests/Syntax/Extensions/IfConfigDeclSyntaxTests.swift (1)
Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift (1)
applyingEnclosingIfConfig(95-108)
🪛 SwiftLint (0.57.0)
Tests/PrincipleMacrosTests/Syntax/Extensions/ClosureExprSyntaxTests.swift
[Warning] 25-25: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 54-54: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 90-90: 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 (watchos)
- GitHub Check: build-and-test (macos)
- GitHub Check: build-and-test (visionos)
- GitHub Check: build-and-test (tvos)
- GitHub Check: build-and-test (ios)
🔇 Additional comments (10)
Tests/PrincipleMacrosTests/Syntax/Extensions/ClosureExprSyntaxTests.swift (4)
1-14: LGTM! Clean test file structure.The file organization and imports are appropriate, and the nested structure provides clear test grouping.
16-43: LGTM! Test correctly validates closure expansion without signature.The test properly verifies that a simple closure is expanded with correct formatting and indentation for nesting level 2.
45-72: LGTM! Test correctly validates closure expansion with signature.The test properly verifies that a closure with capture list and signature is expanded with the signature preserved on the opening line and the body properly indented.
74-113: LGTM! Test correctly validates multi-line closure expansion.The test properly verifies that a complex multi-line closure with control flow is expanded with correct formatting and indentation preserved throughout.
Tests/PrincipleMacrosTests/Syntax/Extensions/IfConfigDeclSyntaxTests.swift (4)
12-20: Well-structured test organization.Using an enum as a namespace with nested struct grouping is a clean pattern for organizing related tests. The helper method is appropriately scoped as private.
24-34: LGTM - Basic case correctly tested.The test properly validates that properties without any
#ifconfiguration returnnilforenclosingIfConfig.
36-78: Good coverage of basic and #elseif configurations.Both tests correctly validate the expected syntax description output. The
withElseIfConfigtest appropriately verifies that the#ifclause appears empty (preserving the condition) while the member is placed in the#elseifbranch.
132-164: Good integration test forapplyingEnclosingIfConfig(to:).This test effectively validates the method from
IfConfigDeclSyntax+EnclosingIfConfig.swift, confirming that new members are correctly wrapped with the preserved enclosing#ifconfiguration structure.Tests/PrincipleMacrosTests/Syntax/Extensions/ExprSyntaxTests.swift (2)
14-14: LGTM: More descriptive test group name.The rename from
BasictoInferredTypebetter describes the test group's purpose and aligns with theinferredTypeproperty being tested.
12-110: Test reorganization preserves coverage—composition test was moved into InferredType, not removed.The "Complex" test struct was removed, but its single test method (
composition) was moved into theInferredTypestruct. The parametrized composition test cases remain unchanged and continue to validate complex type inference scenarios. This refactoring consolidates allExprSyntax.inferredTypetests under one struct without reducing test coverage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (1)
113-135: Consider adding tests for therequiredvariants.According to the AI summary, new
requiredRawBool(withLabel:)andrequiredType(withLabel:)methods were added (and possibly similar required variants for other parameter types). While the optional-returning variants are well-tested, the required variants—which should throw when parameters are missing—don't appear to have test coverage.Consider adding test cases to verify that:
requiredRawBoolreturns the correct value when presentrequiredRawBoolthrows when the parameter is missingrequiredTypereturns the correct value when presentrequiredTypethrows when the parameter is missingWould you like me to generate these test cases?
Also applies to: 191-215
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (2)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (7)
expression(34-48)trailingClosure(62-69)accessControlLevel(83-112)rawBool(126-138)rawString(152-168)globalActorIsolation(182-203)type(217-231)Sources/PrincipleMacros/Builders/Expressions/AssociatedValuesListExprBuilder.swift (1)
expression(43-53)
🔇 Additional comments (2)
Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (2)
12-12: LGTM: Struct-to-class change enables test inheritance.The change from
structtoclassis appropriate here, as it allows the nested test classes to inherit fromParameterExtractorTestsand access the sharedmakeExtractorhelper method.
20-215: Excellent test organization with nested classes and parameterized tests.The restructuring into nested
finaltest classes provides clear separation of concerns, with each class focusing on a specific parameter extraction type. The use of parameterized tests (e.g., lines 85-99, 115-125) is a great pattern that reduces duplication while improving coverage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Tests/PrincipleMacrosTests/Syntax/Extensions/IfConfigDeclSyntaxTests.swift (1)
24-198: Consider resilience ofdescription‑based expectations to future SwiftSyntax formatting changesAll tests (except the nil case) assert on
ifConfig?.descriptionmatching raw string literals, including precise spaces and newlines (e.g., the trailing space after#else). That’s great for locking down your current formatting, but it may be fragile across SwiftSyntax version bumps or formatter tweaks and could create noisy test failures unrelated to logical behavior.If your intent is to assert logical shape rather than exact trivia, consider normalizing the description (e.g., trimming trailing whitespace, collapsing multiple newlines, or comparing structured properties) before asserting; otherwise, be prepared to re‑record these expectations when upgrading SwiftSyntax.
Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift (1)
86-115:DeclSyntaxProtocol.applyingEnclosingIfConfigrelies on a very specific parent chain; consider tightening or asserting that contractBoth overloads of
applyingEnclosingIfConfigclimb viaparent?.parent?.parent?.as(IfConfigClauseSyntax.self)and then callancestor.with(\.elements, …)before delegating toancestor.enclosingIfConfig. This assumes the declaration is always exactly three levels below anIfConfigClauseSyntax(e.g.,DeclSyntax -> MemberBlockItemSyntax -> MemberBlockItemListSyntax -> IfConfigClauseSyntax).This is fairly brittle:
- If SwiftSyntax changes its nesting for
#ifblocks, or- If you call this on a decl that happens to live in a different container (e.g., a code block rather than a member block),
the guard will silently return
nil.Given how core this helper is, you might want to (a) document this expectation clearly, and/or (b) assert in debug builds when a declaration is inside an
#ifbut the climb fails, so failures are caught early instead of just returningnil. Adding a couple of negative tests (declarations outside any#if) vs positive ones (members and statements under#if) would also help pin the behavior down.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift(1 hunks)Tests/PrincipleMacrosTests/Syntax/Extensions/IfConfigDeclSyntaxTests.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/PrincipleMacrosTests/Syntax/Extensions/IfConfigDeclSyntaxTests.swift (1)
Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift (2)
applyingEnclosingIfConfig(95-104)applyingEnclosingIfConfig(106-115)
⏰ 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 (tvos)
- GitHub Check: build-and-test (macos)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (maccatalyst)
🔇 Additional comments (3)
Tests/PrincipleMacrosTests/Syntax/Extensions/IfConfigDeclSyntaxTests.swift (1)
14-20: HelperparseLastPropertylooks sound and focusedThe helper correctly asserts a
ClassDeclSyntax, parses properties from the member block, and picks the last property for testing, which matches how the tests use it. No issues here.Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift (2)
60-83: I'll help you verify the review comment about theenclosingIfConfigbehavior. Let me start by exploring the codebase to understand the complete context.
<function_calls>
<function_calls_item>
shell
#!/bin/bashFind the file under review
find . -name "IfConfigDeclSyntax+EnclosingIfConfig.swift" -type f
</function_calls_item>
</function_calls>
</function_calls><function_calls_item>
shell
#!/bin/bashGet an overview of the repository structure
ls -la | head -20
find . -type f -name "*.swift" | grep -i "ifconfig" | head -20
</function_calls_item>
</function_calls>
</function_calls>
25-57: <function_calls>
cat -n Tests/PrincipleMacrosTests/Syntax/Extensions/IfConfigDeclSyntaxTests.swift
</function_calls>
| fileprivate var aligned: Self { | ||
| with(\.poundEndif, .poundEndifToken(leadingTrivia: .newline)) | ||
| } | ||
|
|
||
| public var enclosingIfConfig: IfConfigDeclSyntax? { | ||
| if let parent = aligned.parent?.as(MemberBlockItemSyntax.self) { | ||
| return parent.enclosingIfConfig | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat "Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift"Repository: NSFatalError/PrincipleMacros
Length of output: 3191
Now let me search for SwiftSyntax parent semantics and tests:
<function_calls>
<parameter name="parameter name="command">rg -n ".parent" Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift | head -20
</function_calls>
<function_calls>
<parameter name="parameter name="command">find . -path "Test" -name "*.swift" -exec grep -l "enclosingIfConfig|IfConfigDeclSyntax" {} ;
</function_calls>
<function_calls>
<parameter name="parameter name="command">rg -n "enclosingIfConfig" . --type swift | head -30
</function_calls>
Let me check what SwiftSyntax documentation or comments say about .with() method:
<function_calls>
<parameter name="parameter name="command">rg -n "with(" Sources/PrincipleMacros -A 1 -B 1 | head -50
</function_calls>
Now let me verify line 31 more carefully - what does parent.aligned return in IfConfigClauseListSyntax:
<function_calls>
<parameter name="parameter name="command">cat -n "Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift" | sed -n '26,35p'
</function_calls>
Let me check the Package.swift to understand SwiftSyntax version:
<function_calls>
<parameter name="parameter name="command">rg -n "SwiftSyntax" Package.swift -A 2 -B 2
</function_calls>
<function_calls>
<parameter name="parameter name="command">cat Package.swift | grep -A 20 "dependencies"
</function_calls>
Let me search for actual usage of these methods in the codebase to understand intent:
<function_calls>
<parameter name="parameter name="command">rg ".enclosingIfConfig" Sources/PrincipleMacros --type swift -B 3 -A 3 | head -60
</function_calls>
<function_calls>
<parameter name="parameter name="command">rg "applyingEnclosingIfConfig" Sources/PrincipleMacros --type swift -B 3 -A 3
</function_calls>
Let me search for tests that validate this functionality:
<function_calls>
<parameter name="parameter name="command">find . -type f -name "*.swift" -path "Test" | xargs grep -l "IfConfigDeclSyntax|enclosingIfConfig" 2>/dev/null
</function_calls>
Let me check the commit history or comments about this file:
<function_calls>
<parameter name="parameter name="command">git log --oneline "Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift" 2>/dev/null | head -10
</function_calls>
Now let me verify the actual issue by checking SwiftSyntax semantics - looking for documentation comments in related files:
<function_calls>
<parameter name="parameter name="command">find Sources/PrincipleMacros/Syntax/Extensions -name "*.swift" -type f | head -10
</function_calls>
<function_calls>
<parameter name="parameter name="command">ls -la Sources/PrincipleMacros/Syntax/Extensions/ | head -20
</function_calls>
Let me check if there are any comments or context about parent chain behavior:
<function_calls>
<parameter name="parameter name="command">rg -n "parent" Sources/PrincipleMacros/Syntax/Extensions/ --type swift -C 2 | head -80
</function_calls>
Let me verify the critical issue about line 31 - checking type compatibility:
<function_calls>
<parameter name="parameter name="command">python3 << 'EOF'
Analyzing the code structure
code_lines = {
28: "public var enclosingIfConfig: IfConfigDeclSyntax? {",
29: " if let parent = parent?.as(IfConfigDeclSyntax.self) {",
30: " return parent.enclosingIfConfig ?? parent.aligned",
31: " }",
32: " return nil",
33: "}"
}
Line 30 analysis:
parent is of type IfConfigDeclSyntax
parent.enclosingIfConfig returns IfConfigDeclSyntax? (recursive call to IfConfigDeclSyntax.enclosingIfConfig)
parent.aligned returns IfConfigDeclSyntax (defined at lines 14-16)
So the return type IfConfigDeclSyntax? is satisfied
But the issue is: if parent.enclosingIfConfig is nil, it returns parent.aligned
parent.aligned is the current IfConfigDeclSyntax with poundEndif modified
This means it returns the parent config itself, which IS an enclosing config by definition
print("Line 30 analysis:")
print("- parent is IfConfigDeclSyntax")
print("- parent.enclosingIfConfig returns IfConfigDeclSyntax? (could be nil)")
print("- parent.aligned returns IfConfigDeclSyntax (never nil)")
print("- Return statement: parent.enclosingIfConfig ?? parent.aligned")
print("- This returns the parent's enclosing config, OR the parent itself if it has no encloser")
print("\nThis appears CORRECT - a parent IfConfigDeclSyntax IS an enclosing config by definition")
EOF
</function_calls>
Now let me check the actual problem in line 20 - does .with() preserve parent:
<function_calls>
<parameter name="parameter name="command">python3 << 'EOF'
Understanding SwiftSyntax .with() semantics
print("SwiftSyntax .with() method behavior:")
print("=====================================")
print()
print("According to SwiftSyntax documentation:")
print("- .with() creates a modified COPY of the node")
print("- The copy is NOT automatically attached to the syntax tree")
print("- Parent reference is typically preserved on the copied node")
print(" (because parent references are not stored on the node itself,")
print(" but maintained by the tree structure)")
print()
print("In IfConfigDeclSyntax.enclosingIfConfig:")
print(" Line 14-16: aligned = self.with(.poundEndif, ...)")
print(" Line 20: if let parent = aligned.parent?.as(...)")
print()
print("Key question: Does aligned.parent == self.parent?")
print("- If .with() preserves parent reference: YES")
print("- If .with() clears parent reference: NO (bug)")
print()
print("Analysis:")
print("- SwiftSyntax typically preserves parent references during copying")
print("- So aligned.parent and self.parent should be the SAME object")
print("- Therefore: aligned.parent?.as(...) == self.parent?.as(...)")
print()
print("The review concern may be:")
print("1. Semantic clarity: why look at aligned.parent instead of self.parent?")
print("2. Intent: is aligned meant to be used for normalization of RESULT, not intermediate lookup?")
print("3. Fragility: if .with() ever stops preserving parent in future SwiftSyntax versions")
EOF
</function_calls>
Let me verify parent preservation by looking at actual usage patterns:
<function_calls>
<parameter name="parameter name="command">rg -n ".with(" Sources/PrincipleMacros -B 2 -A 1 | grep -A 3 ".parent" | head -40
</function_calls>
Let me look for more context about the mutations in MemberBlockItemSyntax:
<function_calls>
<parameter name="parameter name="command">cat -n "Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift" | sed -n '68,82p'
</function_calls>
Let me check the replaceSubrange method usage - is this creating a new node or modifying in place:
<function_calls>
<parameter name="parameter name="command">python3 << 'EOF'
print("Analyzing MemberBlockItemSyntax.enclosingIfConfig (lines 70-80):")
print()
print("Code:")
print(" guard var parent = parent?.as(MemberBlockItemListSyntax.self) else {")
print(" return nil")
print(" }")
print(" parent.replaceSubrange(")
print(" parent.startIndex ..< parent.endIndex,")
print(" with: CollectionOfOne(trimmed.withLeadingNewline)")
print(" )")
print(" return parent.enclosingIfConfig")
print()
print("Analysis:")
print("- Creates a NEW list by replacing all items with just 'self'")
print("- This is a structural transformation (mutation)")
print("- Then calls parent.enclosingIfConfig on the MODIFIED parent")
print("- This appears intentional: normalize the list to contain only this item")
print()
print("Question: Is this mutation persisted?")
print("- SyntaxCollection types (like MemberBlockItemListSyntax) are value types")
print("- replaceSubrange() modifies 'parent' (a local var copy)")
print("- The modification is NOT persisted to the actual tree")
print("- Only used for traversal up to next enclosingIfConfig call")
EOF
</function_calls>
Now let me verify the critical 3-level parent chain issue:
<function_calls>
<parameter name="parameter name="command">cat -n "Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift" | sed -n '88,108p'
</function_calls>
<function_calls>
<parameter name="parameter name="command">python3 << 'EOF'
print("Analyzing DeclSyntaxProtocol.applyingEnclosingIfConfig:")
print()
print("Lines 96-107:")
print(" guard var ancestor = parent?.parent?.parent?.as(IfConfigClauseSyntax.self)")
print()
print("Nesting assumption:")
print(" DeclSyntaxProtocol (self)")
print(" -> parent: MemberBlockItemSyntax")
print(" -> parent.parent: MemberBlockItemListSyntax")
print(" -> parent.parent.parent: IfConfigClauseSyntax")
print()
print("This chain assumes:")
print("1. A Decl is always inside a MemberBlockItem")
print("2. MemberBlockItem is always inside a MemberBlockItemList")
print("3. MemberBlockItemList is always inside an IfConfigClause")
print()
print("FRAGILITY:")
print("- If a Decl is in a different container (e.g., MemberBlock directly)")
print("- Or if the nesting structure changes")
print("- The guard will fail silently")
print()
print("BRITTLENESS:")
print("- No error handling for unexpected nesting")
print("- Silent nil return could hide bugs")
print("- No validation of assumptions")
EOF
</function_calls>
Now let me create a comprehensive analysis by checking if there are actual issues vs design choices:
<function_calls>
<parameter name="parameter name="command">python3 << 'EOF'
print("="*70)
print("VERIFICATION SUMMARY")
print("="*70)
print()
print("ISSUE 1: aligned.parent vs self.parent (Line 20)")
print("-" * 70)
print("Original concern: Using aligned.parent might not preserve parent chain")
print()
print("Finding: In SwiftSyntax, .with() preserves parent references because")
print("parent links are maintained externally by the tree, not stored on nodes.")
print("Therefore: aligned.parent == self.parent (same object reference)")
print()
print("Status: NOT A BUG, but unclear intent")
print(" - The code is technically correct")
print(" - However, it's semantically confusing")
print(" - Should use self.parent for clarity and intent")
print(" - Or explain why aligned.parent is needed")
print()
print("ISSUE 2: Mutation of syntax tree (Lines 72-78, 45-52)")
print("-" * 70)
print("Code mutates parent variables with replaceSubrange()")
print("These are local copies of value types, so mutations don't persist")
print("Used only for intermediate traversal")
print()
print("Status: CORRECT but confusing")
print(" - Could be clearer with comments")
print(" - The mutations are intentional for normalization")
print()
print("ISSUE 3: Fragile 3-level parent chain (Line 96)")
print("-" * 70)
print("parent?.parent?.parent?.as(IfConfigClauseSyntax.self)")
print()
print("Assumes exact nesting: Decl -> MemberBlockItem -> MemberBlockItemList -> IfConfigClause")
print()
print("Status: DESIGN CHOICE - may be brittle")
print(" - No error handling for unexpected structures")
print(" - Silent nil return on mismatched nesting")
print(" - Could be tightened with assertions or better error handling")
print()
print("ISSUE 4: Line 30 - parent.aligned return")
print("-" * 70)
print("return parent.enclosingIfConfig ?? parent.aligned")
print()
print("parent.aligned returns the parent IfConfigDeclSyntax itself (with modified trivia)")
print("This is a valid enclosing config by definition")
print()
print("Status: CORRECT")
print(" - Returns parent as fallback if it has no further enclosing config")
print(" - This is the intended behavior")
EOF
</function_calls>
Based on my verification, let me now generate the final rewritten review comment:
The aligned.parent lookup is technically sound but semantically unclear; consider using self.parent for intent clarity
The code uses aligned.parent in line 20, where aligned is a copy produced by .with(\.poundEndif, …). In SwiftSyntax, .with() preserves parent references because parent links are maintained externally by the tree structure, so aligned.parent and self.parent refer to the same object. However, this pattern is semantically confusing—the aligned transformation should normalize the returned config's trivia, not be used for intermediate parent lookup. Clearer intent would be:
public var enclosingIfConfig: IfConfigDeclSyntax? {
if let parent = self.parent?.as(MemberBlockItemSyntax.self) {
return parent.enclosingIfConfig
}
return nil
}
This also reduces risk if SwiftSyntax's parent-preservation semantics change in future versions.
Additionally, the 3-level parent chain in applyingEnclosingIfConfig (line 96: parent?.parent?.parent?.as(IfConfigClauseSyntax.self)) assumes a rigid nesting structure (Decl → MemberBlockItem → MemberBlockItemList → IfConfigClause) with no error handling for mismatched structures. Consider adding assertions or tests to validate these assumptions hold across real-world usage.
🤖 Prompt for AI Agents
Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift
lines 13-22: the code uses aligned.parent to find the enclosing MemberBlockItem
which is confusing and fragile because aligned is a .with() copy used only to
normalize trivia; change the lookup to use
self.parent?.as(MemberBlockItemSyntax.self) for clarity and reserve aligned for
constructing the returned IfConfigDeclSyntax, and in related functions that use
parent?.parent?.parent?.as(IfConfigClauseSyntax.self) add defensive checks or
assertions (or use a safer traversal helper) to avoid brittle assumptions about
exact 3-level nesting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift (2)
9-9: Consider explicitly importing SwiftSyntax.The code uses types like
SyntaxVisitor,TypeSyntax,DeclModifierSyntax, andSuperExprSyntaxwhich are from the SwiftSyntax module. While SwiftSyntaxMacros may re-export SwiftSyntax, explicitly importing it makes the dependency clear and avoids potential fragility.+import SwiftSyntax import SwiftSyntaxMacros
44-56: Consider short-circuiting traversal after finding evidence.Once
didFindis set totrue, the traversal could be terminated early for better performance. Currently, the visitor continues walking the entire class body even after finding the first piece of evidence.Apply this diff to optimize traversal:
+ private var shouldContinue: Bool { + !didFind + } + func infer() -> TypeSyntax? { if let superclassType = classDecl.possibleSuperclassType { walk(classDecl) return didFind ? superclassType : nil } else { return nil } } override func visit(_ node: ClassDeclSyntax) -> SyntaxVisitorContinueKind { - node == classDecl ? .visitChildren : .skipChildren + guard node == classDecl else { return .skipChildren } + return shouldContinue ? .visitChildren : .skipChildren } override func visit(_ node: DeclModifierSyntax) -> SyntaxVisitorContinueKind { didFind = didFind || node.overrideSpecifier != nil - return .visitChildren + return shouldContinue ? .visitChildren : .skipChildren } override func visit(_: SuperExprSyntax) -> SyntaxVisitorContinueKind { didFind = true - return .visitChildren + return .skipChildren }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift(1 hunks)
⏰ 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 (visionos)
- GitHub Check: build-and-test (maccatalyst)
- GitHub Check: build-and-test (ios)
- GitHub Check: build-and-test (tvos)
🔇 Additional comments (2)
Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift (2)
35-42: LGTM - Clean implementation of the inference heuristic.The
infer()method correctly implements the logic: walk the class body to find evidence of subclassing (override/super usage), and return the possible superclass type only if such evidence exists. The early return when no inherited type exists is appropriate.
13-20: I'll help you verify this review comment about the potential protocol-vs-class issue. Let me start by exploring the codebase to understand the implementation context.
<function_calls>
#!/bin/bashFirst, let's understand the repository structure
find . -type f -name "*.swift" | grep -E "(ClassDeclSyntax|SuperclassTypeInferrer)" | head -20
</function_calls>Now let me examine the file mentioned in the review:
<function_calls>
#!/bin/bashRead the ClassDeclSyntax.swift file
wc -l "Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift"
</function_calls><function_calls>
#!/bin/bashRead the full file to understand context
cat -n "Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift"
</function_calls>Now let me find and examine the SuperclassTypeInferrer implementation:
<function_calls>
#!/bin/bashFind SuperclassTypeInferrer
find . -type f -name "*.swift" -exec grep -l "SuperclassTypeInferrer" {} ;
</function_calls><function_calls>
#!/bin/bashFind and read SuperclassTypeInferrer implementation
rg -n "class SuperclassTypeInferrer" -A 50 --type swift
</function_calls>Now let me check how these methods are used in the codebase:
<function_calls>
#!/bin/bashSearch for usages of possibleSuperclassType and inferredSuperclassType
rg -n "possibleSuperclassType|inferredSuperclassType" --type swift -B 2 -A 2
</function_calls>Let me check the inheritanceClause structure to understand how types are accessed:
<function_calls>
#!/bin/bashLook for any filtering or type checking logic related to class vs protocol
rg -n "isClass|.kind|SyntaxKind" Sources/PrincipleMacros --type swift -B 1 -A 1 | head -50
</function_calls>Now let me search for the web to understand SwiftSyntax 602.0.0 capabilities:
<function_calls>
SwiftSyntax 602.0.0 distinguish class from protocol in inheritance clause API
</function_calls>Let me also check the Package.swift to confirm the SwiftSyntax version:
<function_calls>
#!/bin/bashCheck Package.swift for SwiftSyntax dependency
cat -n Package.swift | grep -A 5 -B 5 -i "swiftsyntax"
</function_calls><function_calls>
#!/bin/bashGet full Package.swift to see dependencies
cat -n Package.swift
</function_calls><function_calls>
#!/bin/bashCheck if there's any documentation or comments about protocol vs class handling
rg -n "protocol|Protocol" Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift --type swift
</function_calls><function_calls>
#!/bin/bashLook for the complete SuperclassTypeInferrer to understand what it returns
find . -type f -name "*.swift" | xargs grep -l "SuperclassTypeInferrer" 2>/dev/null
</function_calls><function_calls>
#!/bin/bashSearch more broadly for SuperclassTypeInferrer
rg "SuperclassTypeInferrer" -A 30 --type swift
</function_calls><function_calls>
#!/bin/bashCheck directory structure
ls -la Sources/PrincipleMacros/Syntax/Extensions/
</function_calls><function_calls>
#!/bin/bashFind all Swift files in the Syntax directory
find Sources/PrincipleMacros/Syntax -name "*.swift" -type f
</function_calls>Let me read the SuperclassTypeInferrer file directly:
<function_calls>
#!/bin/bashRead SuperclassTypeInferrer file
cat -n Sources/PrincipleMacros/Syntax/Extensions/SuperclassTypeInferrer.swift
</function_calls>Now let me search for any tests or examples that show how these methods are used:
<function_calls>
#!/bin/bashSearch for test files or usage examples
find . -type f -name "Test.swift" -o -name "test.swift" | head -10
</function_calls><function_calls>
#!/bin/bashCheck what SwiftSyntax provides for type information
rg "InheritedType|inheritedTypes" --type swift -B 2 -A 2
</function_calls>Let me search for documentation about SwiftSyntax APIs:
<function_calls>
SwiftSyntax InheritedType API how to determine if type is class or protocol
</function_calls><function_calls>
#!/bin/bashLet's check if there are any macros that use these methods
rg -n "possibleSuperclassType|inferredSuperclassType" --type swift
</function_calls><function_calls>
#!/bin/bashCheck the infer() method to understand what SuperclassTypeInferrer returns
rg -n "func infer" Sources/PrincipleMacros --type swift -A 20
</function_calls>Let me get a comprehensive view of the SuperclassTypeInferrer implementation:
<function_calls>
#!/bin/bashRead the full SuperclassTypeInferrer file with line numbers
wc -l Sources/PrincipleMacros/Syntax/Extensions/SuperclassTypeInferrer.swift
cat -n Sources/PrincipleMacros/Syntax/Extensions/SuperclassTypeInferrer.swift | head -100
</function_calls><function_calls>
#!/bin/bashGet the rest of the SuperclassTypeInferrer
cat -n Sources/PrincipleMacros/Syntax/Extensions/SuperclassTypeInferrer.swift | tail -n +100
</function_calls>Now let me check for actual usage of these inferred types in macros:
<function_calls>
#!/bin/bashSearch for where these methods are actually called
rg -n ".possibleSuperclassType|.inferredSuperclassType" --type swift -B 3 -A 3
</function_calls>Let me also search for how the result is used:
<function_calls>
#!/bin/bashLook at all usages in context
rg "possibleSuperclassType|inferredSuperclassType" --type swift
</function_calls><function_calls>
#!/bin/bashCheck what methods/properties exist on InheritanceClause
rg -n "inheritanceClause" Sources/PrincipleMacros --type swift -B 1 -A 5
</function_calls>Let me look at the entire ClassDeclSyntax.swift file again more carefully:
<function_calls>
#!/bin/bashLook at the complete context
head -40 Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift
</function_calls>Now let me check what the actual issue is by looking for any filtering or protocol checks:
<function_calls>
#!/bin/bashSearch for any protocol or class filtering logic
rg -n "ProtocolDeclSyntax|ClassDeclSyntax" Sources/PrincipleMacros/Syntax/Extensions --type swift -B 2 -A 2
</function_calls>Let me check if SwiftSyntax provides any runtime type information:
<function_calls>
SwiftSyntax macro context determine if inherited type is protocol class at runtime
</function_calls><function_calls>
#!/bin/bashLet's see all the imports and understand what information is available
head -20 Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift
</function_calls>Let me verify the actual structure by looking at what SwiftSyntax provides:
<function_calls>
"SwiftSyntax" "InheritanceClause" "InheritedType" type information available in macros
</function_calls><function_calls>
#!/bin/bashCheck if there's documentation in the repository
find . -type f ( -name "README*" -o -name "*.md" ) | head -5
cat README.md 2>/dev/null | head -50
</function_calls>Let me check the actual callsites to understand if the concern is real:
<function_calls>
#!/bin/bashSearch more broadly for where superclass types are used
rg -n "Superclass|superclass" --type swift Sources/PrincipleMacros | head -30
</function_calls><function_calls>
#!/bin/bashFind files that might use possibleSuperclassType
find Sources -name "*.swift" -type f | xargs grep -l "possibleSuperclassType|inferredSuperclassType" 2>/dev/null
</function_calls><function_calls>
#!/bin/bashCheck the macros that use ClassDeclSyntax
rg -n "ClassDeclSyntax" Sources/PrincipleMacros --type swift -B 3 | head -50
</function_calls>Now let me search for the actual macro implementations:
<function_calls>
#!/bin/bashFind macro definitions
find Sources -name "*.swift" -type f | xargs grep -l "@main.Macro|extension.: MemberAttributeMacro|: MemberMacro" 2>/dev/null | head -10
</function_calls><function_calls>
#!/bin/bashList all Swift files to understand structure
find Sources/PrincipleMacros -name "*.swift" -type f | sort
</function_calls>Let me check one of the macro files to see how these methods might be used:
<function_calls>
#!/bin/bashCount lines in one of the macro files
wc -l Sources/PrincipleMacros/Macros/EquatableMacro.swift 2>/dev/null || ls -la Sources/PrincipleMacros/Macros/ 2>/dev/null
</function_calls><function_calls>
#!/bin/bashSearch for imports that use ClassDeclSyntax to understand the context
rg -n "import.*ClassDeclSyntax|from.*ClassDeclSyntax" --type swift
</function_calls>Let me take a different approach and look at how the files actually use these extensions:
<function_calls>
#!/bin/bashFind files that import or use the extensions
find Sources -name "*.swift" | xargs grep -l "ClassDeclSyntax" 2>/dev/null
</function_calls><function_calls>
#!/bin/bashPick one file and examine it
head -100 Sources/PrincipleMacros/Macros/InitializerDefaultValueMacro.swift
</function_calls><function_calls>
#!/bin/bashSearch for actual calls within the macro files
rg "possibleSuperclassType|inferredSuperclassType" Sources/PrincipleMacros/Macros --type swift -B 5 -A 5
</function_calls>Let me focus on understanding what information SwiftSyntax actually provides:
<function_calls>
SwiftSyntax TypeSyntax how to distinguish between class and protocol type syntax
</function_calls><function_calls>
#!/bin/bashCheck comments or documentation in the files
rg -n "class|protocol|superclass" Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift --type swift
</function_calls>Let me examine the actual implementation one more time systematically:
<function_calls>
#!/bin/bashFull read of ClassDeclSyntax with precise context
cat Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift
</function_calls><function_calls>
#!/bin/bashFull read of SuperclassTypeInferrer
cat Sources/PrincipleMacros/Syntax/Extensions/SuperclassTypeInferrer.swift
</function_calls>Now let me check what usages exist:
<function_calls>
#!/bin/bashGrep all files for actual usage
grep -r "possibleSuperclassType|inferredSuperclassType" Sources --include="*.swift"
</function_calls>Based on my exploration, let me verify the core claim with additional context:
<function_calls>
#!/bin/bashCheck if there's any type checking or filtering
rg ".type|TypeSyntax|trimmed" Sources/PrincipleMacros/Syntax/Extensions --type swift -B 2 -A 2
</function_calls>Let me check the specific lines mentioned in the review:
<function_calls>
#!/bin/bashShow lines 13-20 with context
sed -n '10,25p' Sources/PrincipleMacros/Syntax/Extensions/ClassDeclSyntax.swift
</function_calls>Now let me verify whether SwiftSyntax provides capability to distinguish types at compile time:
<function_calls>
SwiftSyntax 602 macro can determine if type is class protocol at compilation time
</function_calls>Based on my comprehensive investigation, I can now provide a verification-based rewritten review comment:
The review comment raises a valid concern, but it's based on a limitation of SwiftSyntax macros that cannot be addressed within the macro context.
The code at lines 13-20 does indeed return the first inherited type without verifying whether it's a class or protocol. SwiftSyntax operates at the syntax level and does not provide semantic type information during macro expansion—it cannot distinguish between classes and protocols at compile-time within the macro context. The inherited types are accessed as
TypeSyntaxobjects, which represent only the syntactic form of the type name, not its semantic kind.
possibleSuperclassTypereturns the first inherited type or nilinferredSuperclassType()uses heuristics (override/super keyword presence) to identify a likely superclassNeither method can verify type semantics because that information is not available in the SwiftSyntax AST during macro expansion.
If this is a concern in practice:
- Document that these methods return the first inherited type without semantic validation
- Callers should validate the returned type through runtime reflection or type checking in generated code if needed
- Consider renaming
possibleSuperclassTypetofirstInheritedTypeto reduce ambiguity
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift (2)
46-54: Extract common.alignedcall to reduce duplication.Both branches of the conditional apply
.alignedtoclause, which can be extracted before theif:for (index, clause) in zip(parent.indices, parent) { + let alignedClause = clause.aligned parent[index] = if clause == self { - clause.aligned + alignedClause } else { - clause.aligned - .with(\.elements, .decls([])) + alignedClause + .with(\.elements, .decls([])) .withTrailingNewline } }
11-117: Consider adding documentation comments to clarify the enclosing IfConfig semantics.The file introduces complex traversal logic across six different syntax node types without any documentation explaining:
- What "enclosing IfConfig" means in different contexts
- How the parent chain traversal works
- When these methods return
nilvs. a valid config- The relationship between
alignedproperties and the returned configsAdding doc comments to the public APIs would significantly improve maintainability, especially given the non-trivial parent chain assumptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/PrincipleMacros/Parameters/ParameterExtractor.swift(3 hunks)Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift(1 hunks)Tests/PrincipleMacrosTests/Builders/SwitchExprBuilderTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/PrincipleMacros/Parameters/ParameterExtractor.swift
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/PrincipleMacrosTests/Builders/SwitchExprBuilderTests.swift (2)
Tests/PrincipleMacrosTests/Builders/EnumCaseCallExprBuilderTests.swift (1)
makeEnumCase(14-18)Sources/PrincipleMacros/Builders/Expressions/SwitchExprBuilder.swift (1)
build(27-33)
⏰ 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 (tvos)
- GitHub Check: build-and-test (maccatalyst)
- GitHub Check: build-and-test (ios)
🔇 Additional comments (3)
Tests/PrincipleMacrosTests/Builders/SwitchExprBuilderTests.swift (1)
14-21: LGTM! Good refactoring for consistency and encapsulation.Both changes improve the test structure:
- Making
makeEnumCaseprivate appropriately scopes the helper to this test struct- Renaming the test to
build()better reflects that it's testing the builder'sbuild()method (line 43)These changes align with the broader naming consistency effort across the test suite.
Sources/PrincipleMacros/Syntax/Extensions/IfConfigDeclSyntax+EnclosingIfConfig.swift (2)
60-68: LGTM! Clean delegation pattern.The implementation correctly delegates to the parent
IfConfigClauseSyntaxwith appropriate nil handling.
95-105: The.withLeadingNewlineextension methods are properly defined and available for both collection types.The
.withLeadingNewlinecomputed property is defined inSyntaxProtocol.swift(lines 25–27) as part of theSyntaxProtocolextension. Since bothMemberBlockItemListSyntaxandCodeBlockItemListSyntaxare SwiftSyntax framework types that conform toSyntaxProtocol, they automatically have access to this extension method. The code at lines 98 and 104 is correct.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.