fix: Fixed compiler error when using @PolymorphicCodableStrategyProviding macro on protocols defined inside nested types
#15
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughSwitches the macro attachment from extension to member, restructures macro implementations to use MemberMacro and PeerMacro, generates nested CodableStrategy and typealiases directly inside the host protocol, adds validation and access modifier handling, and updates tests to expect member-injected typealiases rather than extension-based ones. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer
participant Host as Host protocol
participant Macro as PolymorphicCodableStrategyProviding
participant Compiler as Swift Macro Engine
Dev->>Host: Annotate protocol with @PolymorphicCodableStrategyProviding(...)
Compiler->>Macro: Expand MemberMacro (providingMembersOf:)
Macro-->>Host: Inject 7 typealiases referencing <Protocol>CodableStrategy
Compiler->>Macro: Expand PeerMacro (providingPeersOf:)
Macro-->>Host: Generate nested <Protocol>CodableStrategy struct
Note over Host: Struct conforms to PolymorphicCodableStrategy<br/>includes coding key, static key, decode(from:)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Base automatically changed from
feature/elon/change-ResilientDecodingOutcome-logic
to
main
September 2, 2025 11:12
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/KarrotCodableKitMacros/PolymorphicCodableMacros/PolymorphicCodableStrategyMacro.swift (2)
59-71: Sanitize discriminator key to a valid enum case and validate non-empty matchingTypes.[HIGH] If
identifierCodingKeycontains characters not valid in Swift identifiers (e.g., “type-id”, “$type”), the generatedcase <raw>is a syntax error. Generate a String-backed CodingKey case with the original key as a raw value and a sanitized case name. Also, treat an empty array literal formatchingTypesas a validation error.Apply these diffs:
- Validate empty array literal and compute a sanitized enum case name:
guard let arguments = node.arguments?.as(LabeledExprListSyntax.self), let matchingTypes = SyntaxHelper.findArgument(named: "matchingTypes", in: arguments) else { throw CodableKitError.message("Missing required arguments") } + if let array = matchingTypes.expression.as(ArrayExprSyntax.self), array.elements.isEmpty { + throw CodableKitError.message("matchingTypes must contain at least one type.") + } + let identifierCodingKeyString = SyntaxHelper.findArgument( named: "identifierCodingKey", in: arguments ).flatMap { SyntaxHelper.extractString(from: $0) } ?? "type" if identifierCodingKeyString == "" { throw CodableKitError.message( "Invalid identifierCodingKey: expected a non-empty string." ) } let accessModifier = accessModifier(from: protocolDecl) let identifier = protocolDecl.name.text let strategyStructName = "\(identifier)CodableStrategy" + let codingKeyCase = sanitizedEnumCaseName(from: identifierCodingKeyString)
- Generate a String-backed CodingKey and use the sanitized case:
- return [ - DeclSyntax( - """ - \(raw: accessModifier)struct \(raw: strategyStructName): PolymorphicCodableStrategy { - enum PolymorphicMetaCodingKey: CodingKey { - case \(raw: identifierCodingKeyString) - } - - \(raw: accessModifier)static var polymorphicMetaCodingKey: CodingKey { - PolymorphicMetaCodingKey.\(raw: identifierCodingKeyString) - } + return [ + DeclSyntax( + """ + \(raw: accessModifier)struct \(raw: strategyStructName): PolymorphicCodableStrategy { + private enum PolymorphicMetaCodingKey: String, CodingKey { + case \(raw: codingKeyCase) = "\(raw: identifierCodingKeyString)" + } + + \(raw: accessModifier)static var polymorphicMetaCodingKey: CodingKey { + PolymorphicMetaCodingKey.\(raw: codingKeyCase) + } \(raw: accessModifier)static func decode(from decoder: Decoder) throws -> any \(raw: identifier) { try decoder.decode( codingKey: Self.polymorphicMetaCodingKey, matchingTypes: \(raw: formattedMatchingTypes), fallbackType: \(raw: fallbackType ?? "nil") ) } } """ ), ]Add this shared helper in the same file (outside the extensions):
// File-private helpers shared by both expansions. fileprivate extension PolymorphicCodableStrategyProvidingMacro { static func sanitizedEnumCaseName(from raw: String) -> String { let allowed = CharacterSet.alphanumerics.union(CharacterSet(charactersIn: "_")) var result = raw.unicodeScalars.map { allowed.contains($0) ? Character($0) : "_" }.reduce(into: "") { $0.append($1) } if result.first?.isNumber == true { result = "_" + result } if result.isEmpty { result = "_type" } return result } }This keeps expansions robust for arbitrary discriminator keys used in real-world APIs.
Also applies to: 83-102
107-111: Expose the helper to both macro extensions.[MEDIUM]
accessModifier(from:)isprivatein the PeerMacro extension, so the MemberMacro can’t reuse it. Make itfileprivateor move to a shared scope.Apply this change:
- private static func accessModifier(from protocolDecl: ProtocolDeclSyntax) -> String { + fileprivate static func accessModifier(from protocolDecl: ProtocolDeclSyntax) -> String { guard protocolDecl.accessLevel != .internal else { return "" } return protocolDecl.accessLevel.rawValue + " " }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
Sources/KarrotCodableKit/PolymorphicCodable/Interface/PolymorphicCodableStrategyProviding.swift(1 hunks)Sources/KarrotCodableKitMacros/PolymorphicCodableMacros/PolymorphicCodableStrategyMacro.swift(1 hunks)Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicCodableStrategyProvidingMacroTests.swift(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
Sources/KarrotCodableKit/**
⚙️ CodeRabbit configuration file
Sources/KarrotCodableKit/**: You are a senior Swift/iOS engineer reviewing runtime library code for KarrotCodableKit, a comprehensive Codable extension framework.1. Codable Performance [HIGH]
- Check encoding/decoding efficiency and memory usage
- Verify proper handling of large JSON structures
- Assess polymorphic type resolution performance
- Review property wrapper overhead and optimization
2. Type Safety & Polymorphism [HIGH]
- Validate PolymorphicCodable identifier-based type resolution
- Check AnyCodable type erasure implementation for edge cases
- Verify UnnestedPolymorphic macro integration with runtime components
- Assess error handling in polymorphic decoding scenarios
3. API Design [HIGH]
- Evaluate public interface consistency across modules
- Check property wrapper ergonomics (@DefaultFalse, @DATEvalue, etc.)
- Verify protocol design follows Swift API guidelines
- Assess extensibility for new Codable patterns
4. BetterCodable Integration [MEDIUM]
- Review property wrapper implementations for common patterns
- Check date strategy implementations (ISO8601, RFC3339, etc.)
- Verify default value and lossy conversion handling
- Assess data encoding strategies (Base64, etc.)
5. Error Handling [MEDIUM]
- Verify comprehensive DecodingError and EncodingError usage
- Check PolymorphicCodableError provides sufficient context
- Assess graceful fallback handling in polymorphic scenarios
Review Focus
- Prioritize runtime correctness and performance over style
- Focus on real-world JSON processing scenarios
- Mark comments with priority: [HIGH], [MEDIUM], or [LOW]
Files:
Sources/KarrotCodableKit/PolymorphicCodable/Interface/PolymorphicCodableStrategyProviding.swift
Tests/KarrotCodableMacrosTests/**
⚙️ CodeRabbit configuration file
Tests/KarrotCodableMacrosTests/**: You are a senior Swift/iOS engineer reviewing macro expansion tests for KarrotCodableKit's Swift macro implementations.1. Macro Expansion Testing [HIGH]
- Verify SwiftSyntaxMacrosTestSupport usage is comprehensive
- Check that all macro expansion scenarios are covered
- Assess edge case testing for invalid macro arguments
- Validate diagnostic message testing matches actual validation logic
2. Test Code Quality [HIGH]
- Review test case organization and naming clarity
- Check that test data represents realistic usage scenarios
- Verify test assertions are specific and meaningful
- Assess test maintainability and readability
3. Coverage Assessment [MEDIUM]
- Ensure all macro variants (Codable, Decodable, Encodable) are tested
- Check polymorphic macro test coverage for different identifier strategies
- Verify error conditions and validation failures are tested
- Assess macro registration and plugin functionality testing
Review Focus
- Prioritize test correctness and comprehensive coverage
- Focus on macro-specific testing patterns and SwiftSyntax integration
- Mark comments with priority: [HIGH], [MEDIUM], or [LOW]
Files:
Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicCodableStrategyProvidingMacroTests.swift
Sources/KarrotCodableKitMacros/**
⚙️ CodeRabbit configuration file
Sources/KarrotCodableKitMacros/**: You are a senior Swift/iOS engineer specializing in Swift macros and SwiftSyntax. This code implements macro functionality for KarrotCodableKit, a Codable extension library.1. Macro Implementation Quality [HIGH]
- Verify SwiftSyntax API usage follows current best practices
- Check macro expansion logic for correctness and edge cases
- Ensure diagnostic messages are clear and actionable
- Validate that generated code follows Swift conventions
2. Template Method Pattern [HIGH]
- Review BaseUnnestedPolymorphicMacro protocol extension usage
- Verify UnnestedPolymorphicMacroType implementations are consistent
- Check that shared validation logic in UnnestedPolymorphicValidation is properly utilized
- Assess Factory pattern usage for syntax generation
3. Macro Architecture [HIGH]
- Evaluate separation of concerns between validation, generation, and expansion
- Check PolymorphicMacroArgumentValidator for proper argument handling
- Verify UnnestedPolymorphicCodeGenerator orchestrates components correctly
- Assess error handling and propagation across macro layers
4. Code Generation Quality [MEDIUM]
- Review generated code for Swift idioms and performance
- Check CodingKeys generation follows naming conventions
- Verify property wrapper and protocol conformance generation
- Assess macro expansion output for unnecessary complexity
5. Testing and Validation [MEDIUM]
- Ensure macro tests cover edge cases and error conditions
- Verify diagnostic messages match actual validation logic
- Check that macro registration in KarrotCodableKitPlugin is complete
Review Focus
- Prioritize correctness of macro expansion over style
- Mark comments with priority: [HIGH], [MEDIUM], or [LOW]
- Suggest concrete improvements with SwiftSyntax examples when applicable
Files:
Sources/KarrotCodableKitMacros/PolymorphicCodableMacros/PolymorphicCodableStrategyMacro.swift
🧬 Code graph analysis (1)
Sources/KarrotCodableKitMacros/PolymorphicCodableMacros/PolymorphicCodableStrategyMacro.swift (3)
Sources/KarrotCodableKitMacros/PolymorphicCodableMacros/PolymorphicDecodableMacro.swift (2)
expansion(14-20)expansion(24-42)Sources/KarrotCodableKitMacros/PolymorphicCodableMacros/PolymorphicCodableMacro.swift (2)
expansion(14-20)expansion(24-42)Sources/KarrotCodableKitMacros/PolymorphicCodableMacros/PolymorphicEncodableMacro.swift (2)
expansion(14-20)expansion(24-42)
⏰ 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). (2)
- GitHub Check: Build and Test (16.4, release)
- GitHub Check: Check Macro Compatibility
🔇 Additional comments (1)
Sources/KarrotCodableKit/PolymorphicCodable/Interface/PolymorphicCodableStrategyProviding.swift (1)
28-28: Switch to member attachment is correct for nested protocols.[LOW] This change unblocks nested-type protocols. No further issues in this interface declaration.
Comment on lines
+17
to
+39
| extension PolymorphicCodableStrategyProvidingMacro: MemberMacro { | ||
| public static func expansion( | ||
| of node: AttributeSyntax, | ||
| providingPeersOf declaration: some DeclSyntaxProtocol, | ||
| providingMembersOf declaration: some DeclGroupSyntax, | ||
| in context: some MacroExpansionContext | ||
| ) throws -> [DeclSyntax] { | ||
| guard let protocolDecl = declaration.as(ProtocolDeclSyntax.self) else { | ||
| throw CodableKitError.message("Macro must be attached to a protocol.") | ||
| } | ||
|
|
||
| let identifier = protocolDecl.name.text | ||
| let strategyStructName = "\(identifier)CodableStrategy" | ||
|
|
||
| return [ | ||
| DeclSyntax("typealias Polymorphic = PolymorphicValue<\(raw: strategyStructName)>"), | ||
| DeclSyntax("typealias OptionalPolymorphic = OptionalPolymorphicValue<\(raw: strategyStructName)>"), | ||
| DeclSyntax("typealias LossyOptionalPolymorphic = LossyOptionalPolymorphicValue<\(raw: strategyStructName)>"), | ||
| DeclSyntax("typealias PolymorphicArray = PolymorphicArrayValue<\(raw: strategyStructName)>"), | ||
| DeclSyntax("typealias OptionalPolymorphicArray = OptionalPolymorphicArrayValue<\(raw: strategyStructName)>"), | ||
| DeclSyntax("typealias PolymorphicLossyArray = PolymorphicLossyArrayValue<\(raw: strategyStructName)>"), | ||
| DeclSyntax("typealias DefaultEmptyPolymorphicArray = DefaultEmptyPolymorphicArrayValue<\(raw: strategyStructName)>"), | ||
| ] | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make generated typealiases respect access level and guard on invalid args.
[HIGH] Currently the seven typealiases are emitted without access modifiers, which makes them internal even when the host protocol is public—this is a source-breaking visibility regression. Also, the MemberMacro should short-circuit on invalid arguments to avoid emitting aliases when the PeerMacro fails.
Apply this diff in MemberMacro expansion:
extension PolymorphicCodableStrategyProvidingMacro: MemberMacro {
public static func expansion(
of node: AttributeSyntax,
providingMembersOf declaration: some DeclGroupSyntax,
in context: some MacroExpansionContext
) throws -> [DeclSyntax] {
guard let protocolDecl = declaration.as(ProtocolDeclSyntax.self) else {
throw CodableKitError.message("Macro must be attached to a protocol.")
}
- let identifier = protocolDecl.name.text
- let strategyStructName = "\(identifier)CodableStrategy"
+ // Validate arguments here as well to avoid cascading errors when PeerMacro fails.
+ if let args = node.arguments?.as(LabeledExprListSyntax.self) {
+ if SyntaxHelper.findArgument(named: "identifierCodingKey", in: args)
+ .flatMap({ SyntaxHelper.extractString(from: $0) }) == "" {
+ throw CodableKitError.message("Invalid identifierCodingKey: expected a non-empty string.")
+ }
+ guard SyntaxHelper.findArgument(named: "matchingTypes", in: args) != nil else {
+ throw CodableKitError.message("Missing required arguments")
+ }
+ }
+
+ let identifier = protocolDecl.name.text
+ let strategyStructName = "\(identifier)CodableStrategy"
+ // Mirror host protocol's access level.
+ let accessModifier = (protocolDecl.accessLevel != .internal) ? protocolDecl.accessLevel.rawValue + " " : ""
- return [
- DeclSyntax("typealias Polymorphic = PolymorphicValue<\(raw: strategyStructName)>"),
- DeclSyntax("typealias OptionalPolymorphic = OptionalPolymorphicValue<\(raw: strategyStructName)>"),
- DeclSyntax("typealias LossyOptionalPolymorphic = LossyOptionalPolymorphicValue<\(raw: strategyStructName)>"),
- DeclSyntax("typealias PolymorphicArray = PolymorphicArrayValue<\(raw: strategyStructName)>"),
- DeclSyntax("typealias OptionalPolymorphicArray = OptionalPolymorphicArrayValue<\(raw: strategyStructName)>"),
- DeclSyntax("typealias PolymorphicLossyArray = PolymorphicLossyArrayValue<\(raw: strategyStructName)>"),
- DeclSyntax("typealias DefaultEmptyPolymorphicArray = DefaultEmptyPolymorphicArrayValue<\(raw: strategyStructName)>"),
- ]
+ return [
+ DeclSyntax("\(raw: accessModifier)typealias Polymorphic = PolymorphicValue<\(raw: strategyStructName)>"),
+ DeclSyntax("\(raw: accessModifier)typealias OptionalPolymorphic = OptionalPolymorphicValue<\(raw: strategyStructName)>"),
+ DeclSyntax("\(raw: accessModifier)typealias LossyOptionalPolymorphic = LossyOptionalPolymorphicValue<\(raw: strategyStructName)>"),
+ DeclSyntax("\(raw: accessModifier)typealias PolymorphicArray = PolymorphicArrayValue<\(raw: strategyStructName)>"),
+ DeclSyntax("\(raw: accessModifier)typealias OptionalPolymorphicArray = OptionalPolymorphicArrayValue<\(raw: strategyStructName)>"),
+ DeclSyntax("\(raw: accessModifier)typealias PolymorphicLossyArray = PolymorphicLossyArrayValue<\(raw: strategyStructName)>"),
+ DeclSyntax("\(raw: accessModifier)typealias DefaultEmptyPolymorphicArray = DefaultEmptyPolymorphicArrayValue<\(raw: strategyStructName)>"),
+ ]
}
}Additionally, make the shared helper visible across extensions (change shown below in a separate comment).
📝 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.
Suggested change
| extension PolymorphicCodableStrategyProvidingMacro: MemberMacro { | |
| public static func expansion( | |
| of node: AttributeSyntax, | |
| providingPeersOf declaration: some DeclSyntaxProtocol, | |
| providingMembersOf declaration: some DeclGroupSyntax, | |
| in context: some MacroExpansionContext | |
| ) throws -> [DeclSyntax] { | |
| guard let protocolDecl = declaration.as(ProtocolDeclSyntax.self) else { | |
| throw CodableKitError.message("Macro must be attached to a protocol.") | |
| } | |
| let identifier = protocolDecl.name.text | |
| let strategyStructName = "\(identifier)CodableStrategy" | |
| return [ | |
| DeclSyntax("typealias Polymorphic = PolymorphicValue<\(raw: strategyStructName)>"), | |
| DeclSyntax("typealias OptionalPolymorphic = OptionalPolymorphicValue<\(raw: strategyStructName)>"), | |
| DeclSyntax("typealias LossyOptionalPolymorphic = LossyOptionalPolymorphicValue<\(raw: strategyStructName)>"), | |
| DeclSyntax("typealias PolymorphicArray = PolymorphicArrayValue<\(raw: strategyStructName)>"), | |
| DeclSyntax("typealias OptionalPolymorphicArray = OptionalPolymorphicArrayValue<\(raw: strategyStructName)>"), | |
| DeclSyntax("typealias PolymorphicLossyArray = PolymorphicLossyArrayValue<\(raw: strategyStructName)>"), | |
| DeclSyntax("typealias DefaultEmptyPolymorphicArray = DefaultEmptyPolymorphicArrayValue<\(raw: strategyStructName)>"), | |
| ] | |
| } | |
| extension PolymorphicCodableStrategyProvidingMacro: MemberMacro { | |
| public static func expansion( | |
| of node: AttributeSyntax, | |
| providingMembersOf declaration: some DeclGroupSyntax, | |
| in context: some MacroExpansionContext | |
| ) throws -> [DeclSyntax] { | |
| guard let protocolDecl = declaration.as(ProtocolDeclSyntax.self) else { | |
| throw CodableKitError.message("Macro must be attached to a protocol.") | |
| } | |
| // Validate arguments here as well to avoid cascading errors when PeerMacro fails. | |
| if let args = node.arguments?.as(LabeledExprListSyntax.self) { | |
| if SyntaxHelper.findArgument(named: "identifierCodingKey", in: args) | |
| .flatMap({ SyntaxHelper.extractString(from: $0) }) == "" { | |
| throw CodableKitError.message("Invalid identifierCodingKey: expected a non-empty string.") | |
| } | |
| guard SyntaxHelper.findArgument(named: "matchingTypes", in: args) != nil else { | |
| throw CodableKitError.message("Missing required arguments") | |
| } | |
| } | |
| let identifier = protocolDecl.name.text | |
| let strategyStructName = "\(identifier)CodableStrategy" | |
| // Mirror host protocol's access level. | |
| let accessModifier = (protocolDecl.accessLevel != .internal) ? protocolDecl.accessLevel.rawValue + " " : "" | |
| return [ | |
| DeclSyntax("\(raw: accessModifier)typealias Polymorphic = PolymorphicValue<\(raw: strategyStructName)>"), | |
| DeclSyntax("\(raw: accessModifier)typealias OptionalPolymorphic = OptionalPolymorphicValue<\(raw: strategyStructName)>"), | |
| DeclSyntax("\(raw: accessModifier)typealias LossyOptionalPolymorphic = LossyOptionalPolymorphicValue<\(raw: strategyStructName)>"), | |
| DeclSyntax("\(raw: accessModifier)typealias PolymorphicArray = PolymorphicArrayValue<\(raw: strategyStructName)>"), | |
| DeclSyntax("\(raw: accessModifier)typealias OptionalPolymorphicArray = OptionalPolymorphicArrayValue<\(raw: strategyStructName)>"), | |
| DeclSyntax("\(raw: accessModifier)typealias PolymorphicLossyArray = PolymorphicLossyArrayValue<\(raw: strategyStructName)>"), | |
| DeclSyntax("\(raw: accessModifier)typealias DefaultEmptyPolymorphicArray = DefaultEmptyPolymorphicArrayValue<\(raw: strategyStructName)>"), | |
| ] | |
| } | |
| } |
🤖 Prompt for AI Agents
In
Sources/KarrotCodableKitMacros/PolymorphicCodableMacros/PolymorphicCodableStrategyMacro.swift
around lines 17–39, the generated typealias lines must preserve the host
protocol's access level and the macro must bail out when its attribute/peer
arguments are invalid: derive an accessPrefix string from protocolDecl.modifiers
(e.g., "public ", "internal ", etc.) and prepend it to each emitted "typealias
..." declaration (so the aliases have the same visibility as the protocol), and
before returning the aliases validate the attribute/peer arguments (inspect
node.arguments or the parsed peer info) and short-circuit by returning an empty
array if parsing/validation fails instead of emitting aliases; also ensure the
shared helper referenced elsewhere is made internal/public so it’s visible
across extensions as requested.
Comment on lines
+52
to
+65
| typealias Polymorphic = PolymorphicValue<NoticeCodableStrategy> | ||
| typealias OptionalPolymorphic = OptionalPolymorphicValue<NoticeCodableStrategy> | ||
| typealias LossyOptionalPolymorphic = LossyOptionalPolymorphicValue<NoticeCodableStrategy> | ||
| typealias PolymorphicArray = PolymorphicArrayValue<NoticeCodableStrategy> | ||
| typealias OptionalPolymorphicArray = OptionalPolymorphicArrayValue<NoticeCodableStrategy> | ||
| typealias PolymorphicLossyArray = PolymorphicLossyArrayValue<NoticeCodableStrategy> | ||
| typealias DefaultEmptyPolymorphicArray = DefaultEmptyPolymorphicArrayValue<NoticeCodableStrategy> |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert public access for generated typealiases.
[HIGH] With members injected into a public protocol, the default access of nested declarations is internal. Tests should assert public typealias to lock API and prevent accidental visibility regressions.
Apply this diff to the expected expansion:
- typealias Polymorphic = PolymorphicValue<NoticeCodableStrategy>
+ public typealias Polymorphic = PolymorphicValue<NoticeCodableStrategy>
- typealias OptionalPolymorphic = OptionalPolymorphicValue<NoticeCodableStrategy>
+ public typealias OptionalPolymorphic = OptionalPolymorphicValue<NoticeCodableStrategy>
- typealias LossyOptionalPolymorphic = LossyOptionalPolymorphicValue<NoticeCodableStrategy>
+ public typealias LossyOptionalPolymorphic = LossyOptionalPolymorphicValue<NoticeCodableStrategy>
- typealias PolymorphicArray = PolymorphicArrayValue<NoticeCodableStrategy>
+ public typealias PolymorphicArray = PolymorphicArrayValue<NoticeCodableStrategy>
- typealias OptionalPolymorphicArray = OptionalPolymorphicArrayValue<NoticeCodableStrategy>
+ public typealias OptionalPolymorphicArray = OptionalPolymorphicArrayValue<NoticeCodableStrategy>
- typealias PolymorphicLossyArray = PolymorphicLossyArrayValue<NoticeCodableStrategy>
+ public typealias PolymorphicLossyArray = PolymorphicLossyArrayValue<NoticeCodableStrategy>
- typealias DefaultEmptyPolymorphicArray = DefaultEmptyPolymorphicArrayValue<NoticeCodableStrategy>
+ public typealias DefaultEmptyPolymorphicArray = DefaultEmptyPolymorphicArrayValue<NoticeCodableStrategy>📝 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.
Suggested change
| typealias Polymorphic = PolymorphicValue<NoticeCodableStrategy> | |
| typealias OptionalPolymorphic = OptionalPolymorphicValue<NoticeCodableStrategy> | |
| typealias LossyOptionalPolymorphic = LossyOptionalPolymorphicValue<NoticeCodableStrategy> | |
| typealias PolymorphicArray = PolymorphicArrayValue<NoticeCodableStrategy> | |
| typealias OptionalPolymorphicArray = OptionalPolymorphicArrayValue<NoticeCodableStrategy> | |
| typealias PolymorphicLossyArray = PolymorphicLossyArrayValue<NoticeCodableStrategy> | |
| typealias DefaultEmptyPolymorphicArray = DefaultEmptyPolymorphicArrayValue<NoticeCodableStrategy> | |
| public typealias Polymorphic = PolymorphicValue<NoticeCodableStrategy> | |
| public typealias OptionalPolymorphic = OptionalPolymorphicValue<NoticeCodableStrategy> | |
| public typealias LossyOptionalPolymorphic = LossyOptionalPolymorphicValue<NoticeCodableStrategy> | |
| public typealias PolymorphicArray = PolymorphicArrayValue<NoticeCodableStrategy> | |
| public typealias OptionalPolymorphicArray = OptionalPolymorphicArrayValue<NoticeCodableStrategy> | |
| public typealias PolymorphicLossyArray = PolymorphicLossyArrayValue<NoticeCodableStrategy> | |
| public typealias DefaultEmptyPolymorphicArray = DefaultEmptyPolymorphicArrayValue<NoticeCodableStrategy> |
🤖 Prompt for AI Agents
In
Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicCodableStrategyProvidingMacroTests.swift
around lines 52 to 65, the generated nested typealiases are missing explicit
public visibility; update each declaration to be public typealias (e.g., public
typealias Polymorphic = ...) for all listed aliases (Polymorphic,
OptionalPolymorphic, LossyOptionalPolymorphic, PolymorphicArray,
OptionalPolymorphicArray, PolymorphicLossyArray, DefaultEmptyPolymorphicArray)
so the expected expansion asserts public access and prevents accidental
visibility regressions.
Comment on lines
+121
to
+133
| typealias Polymorphic = PolymorphicValue<NoticeCodableStrategy> | ||
| typealias OptionalPolymorphic = OptionalPolymorphicValue<NoticeCodableStrategy> | ||
| typealias LossyOptionalPolymorphic = LossyOptionalPolymorphicValue<NoticeCodableStrategy> | ||
| typealias PolymorphicArray = PolymorphicArrayValue<NoticeCodableStrategy> | ||
| typealias OptionalPolymorphicArray = OptionalPolymorphicArrayValue<NoticeCodableStrategy> | ||
| typealias PolymorphicLossyArray = PolymorphicLossyArrayValue<NoticeCodableStrategy> | ||
| typealias DefaultEmptyPolymorphicArray = DefaultEmptyPolymorphicArrayValue<NoticeCodableStrategy> |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror the public access assertion in the default-parameters case.
[HIGH] Same reasoning as above—ensure all seven aliases are public in the expected expansion.
Apply the same public typealias changes here.
🤖 Prompt for AI Agents
In
Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicCodableStrategyProvidingMacroTests.swift
around lines 121 to 133 the seven typealias declarations are missing the public
access modifier; update each declaration to be a public typealias (make
Polymorphic, OptionalPolymorphic, LossyOptionalPolymorphic, PolymorphicArray,
OptionalPolymorphicArray, PolymorphicLossyArray, and
DefaultEmptyPolymorphicArray all declared as public typealias) so the expected
expansion matches the public default-parameters case.
Comment on lines
227
to
239
| typealias Polymorphic = PolymorphicValue<NoticeCodableStrategy> | ||
| typealias OptionalPolymorphic = OptionalPolymorphicValue<NoticeCodableStrategy> | ||
| typealias LossyOptionalPolymorphic = LossyOptionalPolymorphicValue<NoticeCodableStrategy> | ||
| typealias PolymorphicArray = PolymorphicArrayValue<NoticeCodableStrategy> | ||
| typealias OptionalPolymorphicArray = OptionalPolymorphicArrayValue<NoticeCodableStrategy> | ||
| typealias PolymorphicLossyArray = PolymorphicLossyArrayValue<NoticeCodableStrategy> | ||
| typealias DefaultEmptyPolymorphicArray = DefaultEmptyPolymorphicArrayValue<NoticeCodableStrategy> |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid emitting members when arguments are invalid; update test accordingly.
[HIGH] When identifierCodingKey is invalid (empty), the PeerMacro errors but the MemberMacro still injects typealiases referencing a non-generated NoticeCodableStrategy, causing cascading compile errors. Prefer no member emission on validation failure and assert that in tests.
Update expected expansion to exclude the seven typealiases:
- protocol Notice: Codable {
- var type: String
- var title: String?
- var description: String
-
- typealias Polymorphic = PolymorphicValue<NoticeCodableStrategy>
- typealias OptionalPolymorphic = OptionalPolymorphicValue<NoticeCodableStrategy>
- typealias LossyOptionalPolymorphic = LossyOptionalPolymorphicValue<NoticeCodableStrategy>
- typealias PolymorphicArray = PolymorphicArrayValue<NoticeCodableStrategy>
- typealias OptionalPolymorphicArray = OptionalPolymorphicArrayValue<NoticeCodableStrategy>
- typealias PolymorphicLossyArray = PolymorphicLossyArrayValue<NoticeCodableStrategy>
- typealias DefaultEmptyPolymorphicArray = DefaultEmptyPolymorphicArrayValue<NoticeCodableStrategy>
- }
+ protocol Notice: Codable {
+ var type: String
+ var title: String?
+ var description: String
+ }Also consider adding tests for:
- A nested protocol inside a containing type to prove the fix.
- A discriminator like "type-id" to verify key sanitization (see macro comments).
- Empty
matchingTypesarray should produce a diagnostic.
🏁 Script executed:
#!/bin/bash
set -eux
file=$(fd --full-path PolymorphicCodableStrategyProvidingMacroTests.swift)
echo "Found file: $file"
sed -n '200,260p' "$file"Length of output: 2355
🏁 Script executed:
#!/bin/bash
set -eux
# Search for identifierCodingKey usage to locate validation logic
rg -n 'identifierCodingKey' -g '*.swift'Length of output: 8890
Exclude typealiases from expansion on invalid identifierCodingKey. Update testPolymorphicCodableStrategyProvidingMacroIdentifierCodingKeyValueError in Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicCodableStrategyProvidingMacroTests.swift to remove the seven typealias …<NoticeCodableStrategy> lines from the expected expandedSource. Also add tests for a nested protocol, for discriminator key sanitization (e.g. "type-id"), and for empty matchingTypes producing a diagnostic.
🤖 Prompt for AI Agents
In
Tests/KarrotCodableMacrosTests/PolymorphicCodableMacrosTests/PolymorphicCodableStrategyProvidingMacroTests.swift
around lines 227–239, the test
testPolymorphicCodableStrategyProvidingMacroIdentifierCodingKeyValueError
expects expandedSource to include seven typealias lines using
<NoticeCodableStrategy>, but per the change these typealiases must be excluded
when identifierCodingKey is invalid; remove those seven typealias lines from the
expected expandedSource in that test. Additionally add three new
assertions/tests: (1) a case for a nested protocol to ensure nested protocol
types are handled, (2) a case that verifies discriminator key sanitization (e.g.
input key "type-id" becomes a valid identifier) and the macro output uses the
sanitized key, and (3) a case where matchingTypes is empty that asserts the
macro emits the appropriate diagnostic; implement these tests by constructing
the input source and expected diagnostics/expandedSource consistent with
existing test patterns.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background (Required)
@PolymorphicCodableStrategyProvidingmacro on protocols defined inside nested typesChanges
@attached(extension)to@attached(member)forPolymorphicCodableStrategyProvidingTesting Methods
Review Notes
Summary by CodeRabbit
New Features
Refactor
Tests