-
Notifications
You must be signed in to change notification settings - Fork 2
fix: Fixed compiler error when using @PolymorphicCodableStrategyProviding macro on protocols defined inside nested types
#15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -45,11 +45,24 @@ final class PolymorphicCodableStrategyProvidingMacroTests: XCTestCase { | |||||||||||||||||||||||||||||||||||||||||||
| """, | ||||||||||||||||||||||||||||||||||||||||||||
| // when | ||||||||||||||||||||||||||||||||||||||||||||
| expandedSource: """ | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| public protocol Notice: Codable { | ||||||||||||||||||||||||||||||||||||||||||||
| var type: String { get } | ||||||||||||||||||||||||||||||||||||||||||||
| var title: String? { get } | ||||||||||||||||||||||||||||||||||||||||||||
| var description: String { get } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| 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> | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+52
to
+65
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| public struct NoticeCodableStrategy: PolymorphicCodableStrategy { | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -72,16 +85,6 @@ final class PolymorphicCodableStrategyProvidingMacroTests: XCTestCase { | |||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| extension Notice { | ||||||||||||||||||||||||||||||||||||||||||||
| 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> | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| """, | ||||||||||||||||||||||||||||||||||||||||||||
| macros: testMacros, | ||||||||||||||||||||||||||||||||||||||||||||
| indentationWidth: .spaces(2) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -114,6 +117,20 @@ final class PolymorphicCodableStrategyProvidingMacroTests: XCTestCase { | |||||||||||||||||||||||||||||||||||||||||||
| var type: String { get } | ||||||||||||||||||||||||||||||||||||||||||||
| var title: String? { get } | ||||||||||||||||||||||||||||||||||||||||||||
| var description: String { get } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| 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> | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+121
to
+133
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Mirror the public access assertion in the default-parameters case. [HIGH] Same reasoning as above—ensure all seven aliases are Apply the same 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| public struct NoticeCodableStrategy: PolymorphicCodableStrategy { | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -136,16 +153,6 @@ final class PolymorphicCodableStrategyProvidingMacroTests: XCTestCase { | |||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| extension Notice { | ||||||||||||||||||||||||||||||||||||||||||||
| 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> | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| """, | ||||||||||||||||||||||||||||||||||||||||||||
| macros: testMacros, | ||||||||||||||||||||||||||||||||||||||||||||
| indentationWidth: .spaces(2) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -216,15 +223,19 @@ final class PolymorphicCodableStrategyProvidingMacroTests: XCTestCase { | |||||||||||||||||||||||||||||||||||||||||||
| var type: String | ||||||||||||||||||||||||||||||||||||||||||||
| var title: String? | ||||||||||||||||||||||||||||||||||||||||||||
| var description: String | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| extension Notice { | ||||||||||||||||||||||||||||||||||||||||||||
| 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> | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
227
to
239
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainAvoid emitting members when arguments are invalid; update test accordingly. [HIGH] When 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:
🏁 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 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| """, | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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
🤖 Prompt for AI Agents