Docs polish, test renames, codegen cleanup#255
Conversation
Both sections described the `scan` and `generate` subcommands; merging removes the duplication and keeps a single authoritative description. https://claude.ai/code/session_01LnijLUgwPSqxYBPomEuvVJ
Drop four full `@Instantiable` types that restated boilerplate; keep the parent/child pair that actually demonstrates `@Received(fulfilledByDependencyNamed:ofType:)`. https://claude.ai/code/session_01LnijLUgwPSqxYBPomEuvVJ
Drop the `LoggedOutView` / `LoggedInView` scaffolding; keep the `FeedView` snippet that actually demonstrates `onlyIfAvailable: true` and explain the two-parent motivation in a sentence. https://claude.ai/code/session_01LnijLUgwPSqxYBPomEuvVJ
Three subsections shared identical scaffolding around a single "how does dependency kind X appear in `mock()`" question. Fold them into one reference section with a table and a focused example per non-trivial case, reusing the already-declared `FeedView` for the `onlyIfAvailable` example. https://claude.ai/code/session_01LnijLUgwPSqxYBPomEuvVJ
The full `LoggedInView` declaration now lives only in the Macros intro. The `@Forwarded` section keeps the unique parent-side `NotesApp` snippet that shows how a runtime value is handed in; the `@Received` section keeps the unique `NoteStorage` snippet that shows the same value being received two levels down. Both reference back to the intro for the canonical declaration. https://claude.ai/code/session_01LnijLUgwPSqxYBPomEuvVJ
The claim that default-valued init parameters do not bubble through Instantiator / SendableInstantiator / ErasedInstantiator / SendableErasedInstantiator boundaries is no longer true: tests in SafeDIToolMockGenerationDefaultValueTests show the default surfaces in the parent mock nested inside the child's `SafeDIMockConfiguration` and is overridable (e.g. `.init(childBuilder: .init(flag: true))`). The "nested SafeDIMockConfiguration field when reached through a parent" row in the dependency-kinds table already describes the real behavior. https://claude.ai/code/session_01LnijLUgwPSqxYBPomEuvVJ
The five tests previously named `...DoesNotBubbleThroughInstantiator`, `...DoesNotBubbleThroughSendableInstantiator`, `...DoesNotBubbleThroughErasedInstantiator`, `...DoesNotBubbleThroughSendableErasedInstantiator`, and `...StopsAtInstantiatorBoundary` all assert the opposite of what their names suggest: the expected output shows the child's default-valued init parameter exposed on `Child.SafeDIMockConfiguration` (with a public init default) and accessed via `safeDIOverrides.childBuilder.flag` or `safeDIOverrides.child.grandchildBuilder.viewModel`. The caller can override through `.init(childBuilder: .init(flag: true))` across the Instantiator boundary — the default absolutely bubbles. Rename each test to describe the behavior its assertions actually verify: defaults are exposed on the (grand)child's `SafeDIMockConfiguration` when reached via Instantiator / SendableInstantiator / ErasedInstantiator / SendableErasedInstantiator. https://claude.ai/code/session_01LnijLUgwPSqxYBPomEuvVJ
…uments The fallback branch's comment claimed it fires for Instantiator children whose defaults don't bubble, but defaults do bubble through Instantiator boundaries via nested SafeDIMockConfiguration. The branch actually fires when a zero-arg mock initializer makes constructionInitializer nil while the production init still has default-valued non-dependency parameters. Update the comment accordingly. https://claude.ai/code/session_01LnijLUgwPSqxYBPomEuvVJ
The doc comment used `Instantiator<AnyView>`, but `Instantiator<T>` requires `T: Instantiable`, which AnyView is not. Switch to `ErasedInstantiator<(), AnyView>` to match the idiom used throughout the Manual and tests.
…dableInstantiator Both inits previously had identical \"A closure that returns an instance of \`Instantiable\`\" doc comments, which neither distinguished the two overloads nor named the right generic parameter. Update the forwarded-properties overload to describe the input it accepts and use T (not Instantiable) consistently.
\`validateMockRootScopeForCycles\` carried a stray first line describing a different function (collectReceivedProperties). \`validateReachableTypeDescriptions\` described mock-scope cycle validation, but the function actually checks that every reachable type description has an \`@Instantiable\`. Replace both with accurate one-line descriptions.
Two comments claimed positional parameter information that was irrelevant — the code looks up arguments by label, not by position — and one contained a typo (\"Instantiatable\") and misattributed \`conformsElsewhere\` to \`@Instantiated\` (it belongs to \`@Instantiable\`). Remove both.
The comment claimed extension mock methods must \"return the extended type or Self\", but the accepted-return-type set is the extended type or any of its \`fulfillingAdditionalTypes\`; \`Self\` is not accepted. Update to reflect the actual check at lines 903–904.
The \"first one only, matching current behavior\" phrasing was vague about where cardinality is enforced. Note that Generate validates the at-most-one invariant; Scan takes the first to drive directory/mocks expansion.
The prior doc claimed PluginScanner is \"used by the Xcode plugin\" and that over-matching is safe because \"SafeDITool creates empty files for declared outputs it doesn't need.\" Both claims are inaccurate: the scanner is used by both the XcodeProjectPlugin path and the SPM fallback when \`context.tool(named:)\` returns an unresolved path, and SafeDITool does not create files for over-declared paths. Replace with an accurate description.
The deleted comments each precede a block whose purpose is already obvious from the code immediately below (\"Parse --target argument.\", \"Find opening bracket.\", etc.). Per the project convention to only keep comments that explain non-obvious WHY, these add noise without adding signal.
Fifteen comment fixes, no behavior change. Grouped: Stale names: * \`_Configuration\` → \`SafeDIMockConfiguration\` in MockParameterNode's doc, \`requiresSendable\`, \`needsConfigurationStruct\`, and the leaf-skip comment. * \`build()\` → \`mock()\` in \`mockAttributes\` and the builder-parameter comment. Inaccurate claims: * \`receivedProperties\`: also orders child generation in dependency-tree mode. * \`MockContext\`: consumed at the mock root only, not threaded through the tree. * \`generatePropertyCode\`: mock mode only reaches this for .root; non-root mock code is produced via \`collectMockParameterTree\` + \`generateMockBodyBindings\`. * Forwarded dependencies: no longer \"bare required\" — defaults may bubble. * \`instantiatedTypeDescription\`: used for the builder's return type, disambiguation, and cycle detection; struct nesting uses \`concreteType\` instead. * \`needsConfigurationStruct\`: now describes the three conditions accurately. * \`generateSafeDIOverridesStruct\`: drop the false \"returns nil\" claim. * \`collectSendableExtractions\` / \`generateMockBodyBindings\`: the doc block was misattributed across the two functions. Split and corrected. * \`resolveBuilderArguments\`: add the two missing argument sources (inline default expression, sendable-extraction locals). * \`generateInstantiatorBinding\`: broaden from Instantiator<T> to the full family. \`mockAttributes\` on MockParameterNode is flagged as possibly dead — the field is only read via \`instantiable.mockAttributes\` at the root. Left in place for now; a follow-up could remove the field.
…edProperty Eight tests named \"run_onCodeWithUnfulfillableInstantiatedProperty*\" assert errors of the form \"@received property X is not @Instantiated or @forwarded\". The unfulfillable property is the @received side, not an @Instantiated one. Rename accordingly. Also fix an Avialable→Available typo and normalize prefix casing on one long-named test.
…it asserts The old name claimed the test verified comment emission, but the assertion compares full mock output with no comment present. What the test actually verifies is that Parent's mock emits a call to Child.mock(unrelated:shared:) — the signature SafeDI expects based on Child's dependencies — even though Child's hand-written mock takes no parameters. Rename to describe that.
* \`...whenEnableMockGenerationIsTrue\` — no \`enableMockGeneration\` flag exists. The setup uses \`generateMock: true\` with no #SafeDIConfiguration present. * \`...configurationFileGeneratedWithoutConditionalCompilation...\` — the primary assertion verifies the root mock file omits #if DEBUG; the configuration file stays empty. Name now reflects the root-file assertion.
* \`existingMockMethodSkipsGenerationForType...\` — Child does get a generated mock file with a call-through; name now matches. * \`userMockDependencyBecomesRequiredParameter...\` — expected output has the dep as \`(() -> ExternalService)? = nil\` (optional), and the dep IS fulfillable via .safedi. Both halves of the old name were wrong. * Three \`bubblesCustomMockDependencyDefault\` / \`nearestReceiverDefaultWins\` / \`forwardedPropertyDefaultBubblesAcrossThreeLevels\` tests — Root's mock keeps \`name\` required (no default); the customMock default does not bubble. Rename to \`doesNotBubble...\` to match inverted-intent drift.
* \`disambiguationFallsBackToFullSuffixWhenSimplifiedCollides\` — expected output shows zero flat-param disambiguation (only nested \`service\` in ChildA.SafeDIMockConfiguration plus a top-level \`service: LocalService? = nil\` from onlyIfAvailable). No fallback occurs. Rename to describe what does happen. * \`disambiguatedUncoveredDependencyInNestedInstantiator\` — the test source has no Instantiator type at all. Rename to describe the actual setup. * \`disambiguationFallsBackToFullSuffixWhenSimplifiedSuffixesCollide\` — expected output shows only nested SafeDIMockConfiguration fields; no \`value_ServiceA\`, \`value_ServiceA_Optional\`, or \`value_ServiceB\` top-level params appear. Several other tests in this file claim bindings that may not resolve (e.g., bare \`presenter\` with no enclosing binding). Flagged for separate investigation, not addressed here.
…ames The project forbids abbreviations. Two test names used \`Dep\` — rename to \`Dependency\`.
27 renames, grouped: Wrong prefix (declaration test mislabeled as extension): * \`extension_expandsWithoutIssueOnTypeDeclarationWhenInstantiableConformanceMissingAndConformsElsewhereIsTrue\` → \`declaration_expandsWithoutIssueWhenInstantiableConformanceMissingAndConformsElsewhereIsTrue\` (decorates a \`public final class\`, not an extension) * \`extension_expandsWithoutIssueOnTypeDeclarationWhenMockOnlyIsTrue\` → \`declaration_expandsWithoutIssueWhenMockOnlyIsTrue\` (decorates a \`public struct\`) Forbidden abbreviations: * \`Init\` → \`Initializer\` across ~24 fixit tests (e.g. \`updatesInitWhenExistingInitIsMissingAccessModifier\` → \`updatesInitializerWhenExistingInitializerIsMissingAccessModifier\`, \`MissingFromInit\` → \`MissingFromInitializer\`). * \`mockMethodWithPartialDepsProducesFixItPreservingExistingParams\` → \`mockMethodWithPartialDependenciesProducesFixItPreservingExistingParameters\`.
The field was assigned from \`childInstantiable.mockAttributes\` but never read. The three mock-attribute emission sites (mockAttributesPrefix, the two mock() signatures) all read from \`instantiable.mockAttributes\` on the root instead. Drop the field and the assignment.
…larations When siblings at the same mock-tree level share a propertyLabel with different types, flat `let <label> = ...` bindings for each sibling caused `invalid redeclaration` errors in the generated Swift. Fix by using the disambiguated label for each sibling's outer binding (and for its `__safeDI_<name>` wrapper), and by forcing consumers whose `.received` dep matches a disambiguated sibling label into a function wrapper that emits `let <label>: <Type> = <disambiguated>` via `emitReceiverBindings`. The root return statement now also consults the merged sibling map, so `Root(service: service)` resolves to `Root(service: service_TypeA)` when root's own `.instantiated` dep label collided with a promoted sibling. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ac102142d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The old comment claimed root-level flat-parameter disambiguation never applies to root return arguments. That stopped being true once tree siblings can collide with root's own `.instantiated` deps (e.g. `service: TypeA` vs a promoted sibling `service: TypeB`); the new `disambiguationMap` parameter exists exactly to remap those. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Emit receiver bindings whose fulfilling label matches a local child AFTER that child is bound, so the alias resolves to the same instance the sibling declares. Bindings that reference ancestor-scoped labels still emit BEFORE the child block (required for inner builder functions that close over them). Also skip promoting locally-fulfilled alias targets to root-level flat parameters in both `collectReceivedProperties` and `Scope.init` — the alias is already satisfied by the sibling @Instantiated, so promotion would emit an unused parameter and a stale warning. Regression test covers Root/Consumer/Service where Consumer declares `@Instantiated service` and `@Received alias` fulfilled by that same `service`: mock now binds a single Service instance and matches production behavior. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@codex-review |
Summary
No repository code changes were made, so I did not create a commit and did not call Testing
|
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Without the `propertiesToDeclare.contains(fulfillingProperty)` guard in `ScopeGenerator.init`, Consumer's `receivedProperties` spuriously included its alias's locally-fulfilled `fulfillingProperty`. The parent's `orderedPropertiesToGenerate` then ordered a sibling with the same label BEFORE Consumer — an incorrect topological edge since Consumer's alias actually binds to the local @Instantiated, not the sibling. The earlier tests didn't exercise this: without a sibling whose label matched the fulfilling property, the spurious edge had no observable effect. Replace them with tests where Root declares a matching-label sibling, so the emission order flips based on whether the guard is present. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 853833263b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| lines.append("\(innerIndent)if let safeDIBuilder = \(overridePath) {") | ||
| lines.append("\(innerIndent)\(standardIndent)return \(wrapperType)(safeDIBuilder(\(argumentList)))") |
There was a problem hiding this comment.
When a constant erased dependency is wrapped only to host receiver bindings (e.g., hasAliasedDependencies/disambiguation with needsConfigurationStruct == false), the override closure at overridePath already returns the erased property type, but this branch still emits return <WrapperType>(safeDIBuilder(...)). That introduces an extra wrap step and can fail to compile for erasers that do not accept their own erased type (or silently change behavior by nesting wrappers). This path should mirror the leaf erased logic and only wrap the default concrete-construction branch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex I think this is a false positive — could you verify the code path you're seeing?
The diff hunk you commented on uses an else if node.requiresFunctionWrapper || hasDisambiguatedReceiver(…) branch and an emitReceiverBindings helper that no longer exist in the current source. In the current ScopeGenerator.swift, the function-wrapper branch (lines 1542–1558) is reached only via the else { … } of if !node.needsConfigurationStruct at line 1504. So when the line you flagged (return \(wrapperType)(safeDIBuilder(\(argumentList))) at 1554) fires, needsConfigurationStruct == true is guaranteed — a SafeDIMockConfiguration struct exists and its safeDIBuilder returns the concrete type. Wrapping it with the wrapper type is correct, not a double-wrap.
The case you described (needsConfigurationStruct == false + erased) is the leaf branch at lines 1507–1520, which already does what you'd want: optionalBuilderPath?(args) ?? wrapper(default(…)) — only the default path is wrapped.
The existing test mock_erasedToConcreteExistentialWithMockInitializer (Tests/SafeDIToolTests/SafeDIToolMockGenerationTests.swift line 768) is the proof: DefaultMyService.init(tag: String = "default") triggers the configuration-struct path, the emitted safeDIBuilder has signature (String) -> DefaultMyService, and AnyMyService(safeDIBuilder(…)) is the correct wrap (test asserts this exact output at lines 826–830).
If you still think there's a real bug here, can you point at a specific call chain in the current HEAD of ScopeGenerator.swift where the wrap-both-paths branch fires with needsConfigurationStruct == false, and ideally a test input that would expose it? Otherwise I'm going to mark this resolved.
There was a problem hiding this comment.
You’re right to ask for a concrete path check — I re-verified against current HEAD, and I still see a reachable needsConfigurationStruct == false wrapper path.
What I verified
-
The branch you said was gone is still present in current source:
else if node.requiresFunctionWrapper || hasDisambiguatedReceiver(...)andemitReceiverBindings(...)are both ingenerateMockBodyBindings. Sources/SafeDICore/Generators/ScopeGenerator.swiftL1699-L1716
-
requiresFunctionWrapperis not equivalent toneedsConfigurationStruct:requiresFunctionWrapper = needsConfigurationStruct || hasAliasedDependencies. Sources/SafeDICore/Generators/ScopeGenerator.swiftL1023-L1042- So a node with aliased deps can enter the wrapper branch even when
needsConfigurationStruct == false.
-
In the
needsConfigurationStruct == falsesetup, override path is the flat node closure:optionalBuilderPath = nodePathwhenneedsConfigurationStructis false. Sources/SafeDICore/Generators/ScopeGenerator.swiftL1663-L1671
-
In erased + wrapper branch, override is always wrapped:
return \(wrapperType)(safeDIBuilder(...)). Sources/SafeDICore/Generators/ScopeGenerator.swiftL1729-L1746
-
But in erased + leaf branch, override is not wrapped (only default path is):
optionalBuilderPath?(...) ?? wrapper(default(...)). Sources/SafeDICore/Generators/ScopeGenerator.swiftL1759-L1767
So the specific path I’m pointing at is:
generateMockBodyBindings
→ node is erased existential
→ needsConfigurationStruct == false
→ hasAliasedDependencies == true (or disambiguated receiver)
→ enters wrapper branch
→ emits wrapped override return.
That said, your cited test (mock_erasedToConcreteExistentialWithMockInitializer) is valid evidence for the configuration-struct case specifically; it doesn’t cover the alias-only wrapper case above. Tests/SafeDIToolTests/SafeDIToolMockGenerationTests.swiftL768-L830
Checks run
- ✅
swift test --traits sourceBuild --filter SafeDIToolMockGenerationTests/mock_erasedToConcreteExistentialWithMockInitializer - ✅
swift test --traits sourceBuild --filter SafeDIToolMockGenerationTests/mock_erasedToConcreteExistentialWithExtensionBasedType - ✅
git status --short(confirmed no repo changes)
No repository changes were made, so I did not create a commit and did not call make_pr.
There was a problem hiding this comment.
You're right, my mistake — I had the wrong branch checked out and was reading a stale local view. Re-verified against current HEAD (8538332) and the call chain you described is real:
requiresFunctionWrapper = needsConfigurationStruct || hasAliasedDependencies(line 1040), so an aliased-only node enters the wrapper branch withneedsConfigurationStruct == false.- In that case
optionalBuilderPath = nodePath(line 1670), pointing at the inline() -> WrapperTypeclosure (perbuilderClosureTypeat line 1059 — "Uses the property type"), so wrapping its return is a double-wrap.
Going to write a failing regression test and fix the branch — most likely by gating the wrap on node.needsConfigurationStruct inside the erased path, mirroring lines 1731–1736 / 1770–1775. Will reply when pushed.
There was a problem hiding this comment.
Fixed in 0c4211e. Gated the wrap on node.needsConfigurationStruct inside the erased branch, matching the pattern already used at lines 1731-1736 and 1770-1775. Added a regression test (mock_erasedExistentialWithAliasedDependency_doesNotDoubleWrapOverride) that constructs an alias-only erased constant and asserts the override path emits return safeDIBuilder(provider) (no wrapper). Confirmed the test failed with the buggy double-wrap before the fix and passes with it. Full suite green.
Thanks for pushing back on this — good catch.
When a constant erased-existential mock node has aliased dependencies but no children/defaults/cycles, `requiresFunctionWrapper` was true (via `hasAliasedDependencies`) while `needsConfigurationStruct` was false. The function-wrapper branch unconditionally wrapped the override return value with the wrapper type, but in the no-config-struct case the override closure stored at `safeDIOverrides.<label>` already returns the property/wrapper type. The result was Swift like `return AnyMyService(safeDIBuilder(provider))`, which fails to compile for erasers whose initializer doesn't accept their own erased type. Mirror the existing instantiator branches at lines 1731-1736 and 1770-1775: gate the wrap on `node.needsConfigurationStruct`. The default-construction branch still wraps (its builder returns the concrete type). Reported by codex review on PR #255. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c4211ee98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A previous fix partitioned alias bindings into preChild/postChild lists and emitted the whole postChild list after all sibling children. That ordering breaks when a later sibling declares a nested function that captures a postChild alias: Swift rejects nested-function captures of locals declared later in the enclosing scope. Thread a per-fulfilling-label postChild map through the body-binding walk and emit each alias immediately after its fulfilling child is bound. preChild aliases at the root scope now emit before tree bindings so nested builder functions can capture them without forward-referencing. Production codegen already handles this case via orderedPropertiesToGenerate; an architectural unification is deferred.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79012d3158
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if localChildLabels.contains(fulfillingLabel) { | ||
| postChildByFulfilling[fulfillingLabel, default: []].append(line) | ||
| } else { |
There was a problem hiding this comment.
emitReceiverBindings stores postChildByFulfilling using only fulfillingLabel, but sibling disambiguation now allows multiple local children with the same label and different types. When an alias is fulfilled by one of those typed siblings, generateMockBodyBindings will emit that same alias after every sibling with that label, which can either redeclare the alias or reference a disambiguated binding before it exists (depending on sibling order). This produces non-compiling generated mocks for valid graphs that combine aliasing with same-label sibling collisions.
Useful? React with 👍 / 👎.
Sibling disambiguation lets multiple tree children share a propertyLabel when their types differ. Keying postChildByFulfilling only by label caused an alias fulfilled by one such sibling to re-emit after every sibling sharing that label — producing a redeclaration error and/or referencing a disambiguated binding before it was declared. Now keys the map by the specific (fulfillingLabel, fulfillingTypeSource) pair so each alias emits exactly once, immediately after its fulfilling sibling's binding. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90ab4b3972
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // than after all siblings) keeps the alias in scope for any | ||
| // subsequent sibling nested functions, which Swift forbids from | ||
| // forward-referencing later-declared locals. | ||
| if let postChild = postChildBindingsByFulfilling[node.propertyLabel]?[node.instantiatedTypeDescription.asSource] { |
There was a problem hiding this comment.
emitReceiverBindings stores deferred alias lines under postChildByFulfilling[fulfillingLabel][fulfillingProperty.typeDescription.asSource], but this lookup uses node.instantiatedTypeDescription.asSource. For Instantiator-family dependencies those types differ (for example Instantiator<Child> vs Child), so aliases fulfilled by local instantiator siblings are never emitted in the post-child slot. In that case generated mocks either forward-reference an undeclared symbol or capture an outer binding, changing which instance is injected for @Received(fulfilledByDependencyNamed:) aliases.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90ab4b3972
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let receiverBindings = emitReceiverBindings( | ||
| for: node.dependencies, | ||
| flatParameterDisambiguationMap: combinedDisambiguationMap, | ||
| localChildLabelAndTypes: Set(node.children.map { "\($0.propertyLabel):\($0.instantiatedTypeDescription.asSource)" }), |
There was a problem hiding this comment.
emitReceiverBindings indexes local fulfillers using fulfillingProperty.typeDescription.asSource, but this call builds localChildLabelAndTypes from instantiatedTypeDescription. For instantiator-family children (Instantiator<T>, SendableInstantiator<T>, erased variants), those keys differ (Instantiator<T> vs T), so aliases fulfilled by a local child are misclassified as pre-child bindings. In those cases the generated mock can reference the fulfiller before it is declared (or bind to the wrong instance) when aliasing local instantiator dependencies.
Useful? React with 👍 / 👎.
…d type Aliases are stored in postChildByFulfilling under the fulfilling property's `typeDescription.asSource`, but the lookup was using the child node's `instantiatedTypeDescription.asSource`. For Instantiator siblings these differ (`Instantiator<Child>` vs `Child`), so aliases fulfilled by a local Instantiator never matched their post-child slot — they fell through to preChild and forward-referenced their undeclared fulfilling binding. Align both storage and lookup on `typeDescription.asSource`, matching how `mergeSiblingDisambiguations` already keys disambiguated siblings. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a06d64726
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let typeSource = dependency.property.typeDescription.asSource | ||
| let resolved = disambiguationMap[argument.innerLabel]?[typeSource] ?? argument.innerLabel | ||
| parts.append("\(argument.label): \(resolved)") |
There was a problem hiding this comment.
This remap applies to every dependency argument, not just the sibling-disambiguation case it was added for. If a root has an uncovered @Instantiated dependency and a descendant also @Receiveds the same (label,type), flatParameterDisambiguationMap creates an entry for that label and this code rewrites the root return call to the disambiguated flat-received binding. The generated mock(...) then accepts both parameters (e.g. service and service_ExternalService) but always constructs Root from the flat-received one, silently ignoring the root-local parameter.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
custom-build-systemintoUnder the hood; tightenRenaming and retyping dependenciesandConditionally receiving dependenciesexamples; collapse dependency-kind-in-mocks into a single table; dedupe forwarded-Userexample across@Forwarded/@Received; differentiate the twoInstantiator/SendableInstantiatorinit doc comments; fix a non-compiling@Instantiatedexample.MockGenerationDisambiguationTests,MockGenerationConfigurationTests,MockGenerationCustomMockTests,MockGenerationTests,InstantiableMacroTests,SafeDIToolCodeGenerationTests,TypeDescriptionTests; expandDep→Dependency; fixAvialable/ASigletypos; fix driftedDoesNotBubble/StopsAtBoundarycases; tighten a reviewer-added alias-resolution test to a full==comparison.ScopeGenerator,DependencyTreeGenerator,AttributeSyntaxExtensions,Scope,PluginScanner, andperformScan; fix extension mock-method return-type description; remove code-restating comments in plugins.mockAttributesfield onMockParameterNode..forwardedbranch increateInitializerArgumentList.presenter,userVendor,userNetworkService,anyIterator, …) that had no binding in the enclosing scope — tests passed only because the expected output carried the same bug.let <alias>: <AliasType> = <fulfilling>bindings for aliased@Received(fulfilledByDependencyNamed:)dependencies at each scope boundary — the same model production uses. Replaces the alias-map-threading approach from an earlier commit on this branch. Aliased deps are referenced by their own labels via Swift lexical scoping, which correctly handles ancestor alias chains and local shadowing without any per-call substitution.propertyLabelwith different types (e.g.service: TypeAand a promotedservice: TypeB), the generator previously emittedlet service = …; let service = …in the same mock scope — invalid Swift. Sibling bindings now use the disambiguated name (service_TypeA,service_TypeB), and consumers whose.receiveddep matches a disambiguated label are wrapped in a function that rebinds the natural label viaemitReceiverBindings. The root's return call consults the merged sibling map soRoot(service: service)resolves toRoot(service: service_TypeA)when root's own.instantiateddep was disambiguated.emitReceiverBindingsnow returns(preChild, postChild). A receiver whose fulfilling label matches a local child binding emits AFTER that child (so the alias resolves to the same instance the sibling produces), while receivers that reference ancestor-scoped labels emit BEFORE the nested builder functions that close over them. Without this, the Root → Consumer (@Instantiated service,@Received aliasfulfilled byservice) scenario either produced a Swift forward-reference error or silently constructed twoServiceinstances (mock) while production shared one.collectReceivedPropertiesandScope.initnow omit the fulfilling property from root-promoted received parameters when it's already@Instantiatedat the local scope. This avoids emitting an unusedservice: (() -> Service)? = nilflat parameter (and an unusedlet service = ...binding) that would trigger warnings on consumers.All 824 tests pass;
./CLI/lint.shis clean; 100% line coverage onScopeGenerator.swift.Test plan
swift test --traits sourceBuild(824 tests, all green)swift test --traits sourceBuild --enable-code-coverage(100% lines onScopeGenerator.swift)./CLI/lint.sh(no diffs)🤖 Generated with Claude Code