Conversation
## New Features ### Expression Macros - #resolve - Throwing resolution with type inference - #resolved - Force unwrap resolution - #resolvedSafe - Optional resolution (returns nil if not found) All macros support: - key: parameter for keyed registrations - identifier: parameter for explicit DependencyIdentifier - in: parameter for custom Container instances ### Auto Registration - @AutoRegister macro for automatic protocol conformance - @AutoInjected macro for property-based injection - AutoRegistrable protocol for opt-in discovery ### Infrastructure - Added swift-syntax dependency for macro implementation - New DIContainerMacros target with macro implementations - Comprehensive E2E and unit tests (65 tests passing) ## Bug Fixes - Fixed parallel test execution conflicts by removing aggressive removeAllDependencies() calls from test init() - Fixed type inference bug in resolvedSafe by adding resolveSafe<T>() helper methods to Container ## Documentation - Updated README with expression macro examples - Added Custom Container section for in: parameter usage
- Replace (config,): (Config_Workflow,) = try c.resolveAll() with direct c.resolve() call for better Swift version compatibility - Change var to let for wrapperTest to fix compiler warnings
- Revert let to var in DIContainerTests to fix mutating getter error - Fix AutoRegisterMacro to generate valid code for single-parameter inits - Remove single-element tuple syntax (x,): (T,) in tests for compatibility
Adds support for using InjectIdentifier with the @AutoInjected macro: ```swift @AutoInjected(identifier: InjectIdentifier<MyProtocol>.by(key: "premium")) var service: MyProtocol ``` This enables typed identification instead of relying on string keys, providing better type safety and IDE support. Changes: - Sources/DIContainer/Macros.swift: Add new macro declaration - Sources/DIContainerMacros/AutoInjectedMacro.swift: Handle identifier argument - Tests/DIContainerMacrosTests/AutoMacrosTests.swift: Add macro expansion tests - Tests/DIContainerTests/AutoDI_MacroE2E_Tests.swift: Add E2E tests Co-Authored-By: Claude (kimi-k2.5:cloud) <noreply@anthropic.com>
![claudin-io[bot]](https://avatars.githubusercontent.com/in/4147354?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@claudin.io review |
| conformingTo protocols: [TypeSyntax], | ||
| in context: some MacroExpansionContext | ||
| ) throws -> [ExtensionDeclSyntax] { | ||
| // Extract the protocol type from macro arguments |
There was a problem hiding this comment.
@AutoRegister(key:as:) overload always reads first argument as protocol type; @AutoRegister(key: "premium", as: ServiceProtocol.self) gives protocolName="premium" → broken generated code
| // Extract the protocol type from macro arguments | |
| Check argument labels before reading: if firstArg.label?.text == "as", use that; else if firstArg.label?.text == "key", extract as: from the second argument |
| } | ||
| } | ||
|
|
||
| public enum MacroError: Error, CustomStringConvertible { |
There was a problem hiding this comment.
MacroError.notAStructOrClass and .noInitializerFound are defined but never thrown; macro applies to any decl group with zero validation
| public enum MacroError: Error, CustomStringConvertible { | |
| Add validation: guard declaration.is(StructDeclSyntax.self) || declaration.is(ClassDeclSyntax.self) else { throw .notAStructOrClass }; guard !initParams.isEmpty else { throw .noInitializerFound } |
| in context: some MacroExpansionContext | ||
| ) throws -> ExprSyntax { | ||
| let keyArg = extractArgument(from: node, label: "key") | ||
| let idArg = extractArgumentExpression(from: node, label: "identifier") |
There was a problem hiding this comment.
identifier silently takes priority over key when both are passed; caller has no feedback about the ambiguity
| let idArg = extractArgumentExpression(from: node, label: "identifier") | |
| Check key before identifier, or emit a diagnostic warning when both are provided |
| @@ -5,10 +5,6 @@ import Testing | |||
| @Suite("CircularDependencyTests", .serialized) | |||
| struct CircularDependencyTests { | |||
There was a problem hiding this comment.
init() calling removeAllDependencies() removed but tests still use Container.standard directly, risking cross-test state leakage (unlike DIContainerCoverageTests which was refactored to local containers)
| struct CircularDependencyTests { | |
| Either restore the cleanup init() or refactor tests to use local Container() instances |
| ``` | ||
|
|
||
| ## ✨ Auto DI — It Works Like Magic! (Swift 6+) | ||
|
|
There was a problem hiding this comment.
Documents @AutoRegister(key:as:) syntax that generates broken code per finding #1
| Fix macro implementation first, then document the working syntax, or remove the key:as: overload from docs until implemented |
|
@claudin.io review |
| let type = param.type.trimmedDescription | ||
| params.append((name: name, label: label, type: type)) | ||
| } | ||
| break // Use the first initializer found |
There was a problem hiding this comment.
break after first initializer silently ignores other init overloads. When a type has multiple init declarations (e.g., convenience + designated), only the first in source order is used. No diagnostic emitted to warn the user.
| break // Use the first initializer found | |
| Iterate all initializers and either generate a warning when multiple inits exist, or use a heuristic (e.g., prefer the one with most parameters). |
| /// ``` | ||
| /// | ||
| /// - Parameter types: The types to register (must conform to `AutoRegistrable`). | ||
| func bootstrap(_ types: AutoRegistrable.Type...) { |
There was a problem hiding this comment.
Overlapping bootstrap overloads: variadic AutoRegistrable.Type... (line 34) and parameter pack version (line 42) are functionally identical. The variadic array overload is fully redundant since the parameter pack version is more type-safe and covers all call sites.
| func bootstrap(_ types: AutoRegistrable.Type...) { | |
| Remove the variadic AutoRegistrable.Type... overload at line 34; the parameter pack version at line 42 handles all cases with better compile-time safety. |
|
|
||
| for arg in arguments { | ||
| if arg.label?.text == "key", | ||
| let stringLiteral = arg.expression.as(StringLiteralExprSyntax.self), |
There was a problem hiding this comment.
extractKeyArgument only matches StringLiteralExprSyntax. If a user passes a String variable (e.g., @AutoInjected(key: someVariable)), the key is silently ignored and falls through to default .by(type:) resolution. Same pattern as the already-flagged ResolveMacro extractArgument issue.
| let stringLiteral = arg.expression.as(StringLiteralExprSyntax.self), | |
| Use extractArgumentExpression (returning ExprSyntax) instead of extracting the string literal content, similar to how the identifier argument is handled. |
|
@claudin.io review |
|
|
||
| @Test func resolvedWithKeyMacroE2E() { | ||
| let container = Container() | ||
| Container.standard.register(type: Service_ResolvedKey.self, key: "vip_macro") { _ in |
There was a problem hiding this comment.
Unnecessary Container.standard.register pollutes shared container state while the test uses a local container. This leaks state across serialized tests.
| Container.standard.register(type: Service_ResolvedKey.self, key: "vip_macro") { _ in | |
| Remove the Container.standard.register call on line 42-43; the local container registration on line 48-50 is the one actually used by the assertion. |
| ) | ||
| } | ||
|
|
||
| @Test func autoInjectedInStruct() { |
There was a problem hiding this comment.
autoInjectedInStruct test validates @AutoInjected on a value type, but generated getter assigns to stored property _viewModel from a non-mutating getter. This code would not compile in a struct. assertMacroExpansion only checks text, not compilability.
| @Test func autoInjectedInStruct() { | |
| Remove the struct autoInjectedInStruct test or add a compile-time check. If struct support is desired, the getter must be marked mutating or the caching pattern must use a different approach (e.g., reference-type box) for value types. |
|
@claudin.io review |
1 similar comment
|
@claudin.io review |
| ? "let \(paramNames[0])" | ||
| : "let (\(paramNames.joined(separator: ", ")))" | ||
|
|
||
| let initCall = initParams.map { "\($0.label ?? $0.name): \($0.name)" }.joined(separator: ", ") |
There was a problem hiding this comment.
Underscore-labeled init parameters produce invalid Swift. For init(_ name: Type), the coalescing $0.label ?? $0.name resolves label=nil to name, generating Service(name: name) instead of Service(name). The init call string construction must drop the label prefix when label is nil.
| let initCall = initParams.map { "\($0.label ?? $0.name): \($0.name)" }.joined(separator: ", ") | |
| let initCall = initParams.map { p in p.label.map { "\($0): \(p.name)" } ?? p.name }.joined(separator: ", ") |
| let binding = varDecl.bindings.first, | ||
| let identifier = binding.pattern.as(IdentifierPatternSyntax.self), | ||
| let typeAnnotation = binding.typeAnnotation else { | ||
| return [] |
There was a problem hiding this comment.
AccessorMacro expansion silently returns empty array when guard fails (non-variable declaration, missing type annotation). Users get zero generated code with no feedback. Should emit a diagnostic via context.diagnose(...) instead of silent failure.
| let binding = varDecl.bindings.first, | ||
| let identifier = binding.pattern.as(IdentifierPatternSyntax.self), | ||
| let typeAnnotation = binding.typeAnnotation else { | ||
| return [] |
There was a problem hiding this comment.
PeerMacro expansion silently returns empty array on guard failure (same pattern as line 38). Should emit diagnostics for unsupported declarations.
|
@claudin.io review |
| let container = containerArg ?? "Container.standard" | ||
|
|
||
| if let idExpr = idArg { | ||
| return "\(container).resolve(\(idExpr))" |
There was a problem hiding this comment.
#resolve(in:), #resolved(in:), #resolvedSafe(in:) overloads accept in container: Resolvable (Macros.swift:158-210), but the generated code calls container.resolve(), container.resolve(key:), etc. — methods only defined on Container, not on Resolvable. Compile error for any non-Container Resolvable conformer.
| return "\(container).resolve(\(idExpr))" | |
| Generate `container.resolve(.by(type: T.self))` and `container.resolve(.by(type: T.self, key: "..."))` instead, which use the `Resolvable`-compliant `resolve(_ identifier:)` signature. Or change the macro declaration type from `Resolvable` to `Container`. |
|
@claudin.io review |
| ? "let \(paramNames[0])" | ||
| : "let (\(paramNames.joined(separator: ", ")))" | ||
|
|
||
| let initCall = initParams.map { "\($0.label ?? $0.name): \($0.name)" }.joined(separator: ", ") |
There was a problem hiding this comment.
initCall generates wrong Swift for unlabeled init params
| let initCall = initParams.map { "\($0.label ?? $0.name): \($0.name)" }.joined(separator: ", ") | |
| Change initCall generation: when label is nil, omit the 'label:' prefix entirely. Use initParams.map { $0.label.map { "\($0): \($0.name)" } ?? $0.name }.joined(separator: ", ") |
|
@claudin.io review |
| let extensionDecl: DeclSyntax = """ | ||
| extension \(raw: typeName): AutoRegistrable { | ||
| static func autoRegister(in container: Injectable) { | ||
| container.register(type: \(raw: protocolName).self) { c in |
There was a problem hiding this comment.
Hardcoded closure parameter c collides with init parameter named c. When an init has a parameter named c (e.g., init(a: A, c: C, d: D)), the destructuring let (a, b, c, d, e) shadows the closure's c, making c.resolveAll() call resolveAll() on the resolved protocol value rather than the container.
| container.register(type: \(raw: protocolName).self) { c in | |
| Use a unique/less-likely-to-collide name for the closure parameter, e.g. `container` or `resolver`, or generate a unique name per expansion. |
|
@claudin.io review |
| @Test func circularDependencyE2E() throws { | ||
| // Bootstrap services that reference each other | ||
| Container.standard.bootstrap( | ||
| NotificationServiceImpl.self, |
There was a problem hiding this comment.
Tests register on Container.standard without any isolation mechanism. Different suites (e.g. AutoDI_E2E_Tests, AutoDI_MacroE2E_Tests, CircularDependencyTests) all write to the same global singleton but run in parallel — .serialized only serializes within a suite. Registrations leak across tests, causing nondeterministic failures. Existing cleanup (removeAllDependencies() in init) was removed from CircularDependencyTests and DIContainerTests, compounding the problem.
| NotificationServiceImpl.self, | |
| Use a local `Container()` instance instead of `Container.standard` for these tests, or add a suite-level setUp/tearDown that resets the global container. |
| let getter: AccessorDeclSyntax = """ | ||
| get { | ||
| if let cached = \(raw: storageName) { return cached } | ||
| let resolved = try! Container.standard.resolve(\(raw: resolveCall)) |
There was a problem hiding this comment.
Accessor macro hardcodes Container.standard as the resolve target. If a user creates a custom Container() (e.g. for testing or scoped DI), any @AutoInjected property still resolves from the global singleton — silently pulling the wrong instance or crashing because the registration lives in the custom container.
| let resolved = try! Container.standard.resolve(\(raw: resolveCall)) | |
| Consider accepting an optional `container:` parameter on the macro, defaulting to `Container.standard` when absent. |
| /// let (repo, service): (RepoProtocol, ServiceProtocol) = try container.resolveAll() | ||
| /// ``` | ||
| public func resolveAll<each T>() throws -> (repeat each T) { | ||
| (repeat try self.resolve(.by(type: (each T).self))) |
There was a problem hiding this comment.
resolveAll always resolves by type only via .by(type: (each T).self). Dependencies registered with a key cannot be reached. This is a structural limitation of the parameter-pack approach — each element in the pack is a bare type with no mechanism to attach a key.
| (repeat try self.resolve(.by(type: (each T).self))) | |
| Document this limitation. If keyed resolution is needed via resolveAll, the caller must fall back to individual `resolve(.by(type:key:))` calls. |
|
@claudin.io review |
|
🤖 @claudin-io couldn't complete the review — an internal error or exhausted hourly budget. Try again after your budget resets. |
5 similar comments
|
🤖 @claudin-io couldn't complete the review — an internal error or exhausted hourly budget. Try again after your budget resets. |
|
🤖 @claudin-io couldn't complete the review — an internal error or exhausted hourly budget. Try again after your budget resets. |
|
🤖 @claudin-io couldn't complete the review — an internal error or exhausted hourly budget. Try again after your budget resets. |
|
🤖 @claudin-io couldn't complete the review — an internal error or exhausted hourly budget. Try again after your budget resets. |
|
🤖 @claudin-io couldn't complete the review — an internal error or exhausted hourly budget. Try again after your budget resets. |
|
|
||
| // MARK: - Overloads with Container Injection (in: Resolvable) | ||
|
|
||
| @freestanding(expression) |
There was a problem hiding this comment.
#resolve(in:), #resolved(in:), #resolvedSafe(in:) declare container as Resolvable but generated code calls resolve()/resolveSafe() which only exist on Container, not on Resolvable protocol. Compiles in practice because users pass Container instances, but the type is wrong in the signature.
| @freestanding(expression) | |
| Change the parameter type from Resolvable to Container in the macro declarations. |
|
@claudin.io testando o fix |
| @@ -0,0 +1,455 @@ | |||
| import Testing | |||
There was a problem hiding this comment.
No test exists for the @AutoRegister(key:as:) syntax. The broken overload (always reads arguments.first, ignoring key:/as: labels) at AutoRegisterMacro:34-44 has zero test coverage, so the bug would pass CI unnoticed.
| import Testing | |
| Add a test similar to autoRegisterGeneratesExtension but using @AutoRegister(key: "premium", as: ServiceProtocol.self) syntax, with the expected expansion. |
1 participant
New Features
Expression Macros
#resolve- Throwing resolution with type inference#resolved- Force unwrap resolution#resolvedSafe- Optional resolution (returns nil if not found)All macros support:
key:parameter for keyed registrationsidentifier:parameter for explicit DependencyIdentifierin:parameter for custom Container instancesAuto Registration
@AutoRegistermacro for automatic protocol conformance@AutoInjectedmacro for property-based injectionAutoRegistrableprotocol for opt-in discoveryInfrastructure
Bug Fixes
removeAllDependencies()calls from testinit()resolvedSafeby addingresolveSafe<T>()helper methods to ContainerDocumentation
in:parameter usage