Fix mock generation using init signature instead of hand-written mock#257
Merged
Fix mock generation using init signature instead of hand-written mock#257
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.
…mentList The \`.forwarded\` branch and the \`else\` branch produced identical output (\`label: innerLabel\`). Collapse into a single else.
Mock code gen emitted bare identifiers for disambiguated flat parameters (e.g. `presenter` instead of `presenter_PresenterA`) and for aliased dependencies (e.g. `userVendor` instead of the fulfilling `userManager` binding). Those identifiers had no binding in the generated scope, so the mock code referenced variables that did not exist — the tests passed only because expected output carried the same bug. Build a `flatParameterDisambiguationMap` once per mock root, thread it through `generateMockBodyBindings` / `generateInstantiatorBinding` / `resolveBuilderArguments`, and resolve each construction argument to the bound name a caller at that scope would actually reference. For aliased deps, fall back to the fulfilling property's label; for disambiguated flat parameters, substitute the `_TypeName`-suffixed form. Updated test expectations across the disambiguation and generation suites now reflect compilable output. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Fixes two codegen bugs in resolveBuilderArguments that surfaced while addressing PR #255 review: 1. Ancestor alias translations were dropped mid-tree. When an ancestor @INSTANTIABLE declared @received(fulfilledByDependencyNamed:ofType:), its scope bound the fulfilling label, but descendant @received references to the aliased receiver emitted the unbound receiver label. 2. Flat-parameter disambiguation was applied unconditionally, bypassing nearer lexical bindings that already satisfied the reference with a matching type. Introduces two per-scope context values threaded through mock body generation: MockAliasMap (ancestor-scope alias translations, with chain collapsing) and LocallyBoundLabels (label → typeSource for every let-bound identifier in the enclosing Swift scope). resolveBuilderArguments now consults both: a local binding with matching type wins over ancestor aliasing and root-level disambiguation; otherwise ancestor aliases translate first, then disambiguation applies. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ison Renamed to accurately reflect behavior (no root disambiguation exists in this setup — both `service` deps are subtree-scoped). Replaced .contains() assertions with a full expected-output comparison per CLAUDE.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Production code emits `let <alias>: <AliasType> = <fulfilling>` for aliased `@Received(fulfilledByDependencyNamed:)` dependencies. Mock generation was threading an alias map through recursion and substituting labels at each construction site — more code, more corner cases, and incorrect handling of ancestor alias chains and local shadowing. Replace the thread-alias-map strategy with a helper that emits alias bindings at each scope boundary (inside `__safeDI_<label>` wrappers for tree nodes, at the end of the mock body for root-level aliases). Aliased deps are then referenced by their own labels via Swift lexical scoping — the same model production uses. - Add `hasAliasedDependencies` / `requiresFunctionWrapper` on `MockParameterNode` so nodes with aliased deps force a wrapper function. - Add `emitAliasBindings` helper; threaded through `generateMockRootCode`, `generateMockBodyBindings`, and `generateInstantiatorBinding`. - `resolveBuilderArguments` now treats `.aliased` like `.instantiated` / `.forwarded` — uses its local label and skips disambiguation. - Simplify `generateReturnArgumentList`: validation guarantees that a root's own `@Received` label can't collide in flat params, so the disambiguation lookup at the return site was unreachable. Use the argument's `innerLabel` directly. - Update expected mock output in three affected tests; existing single-scope alias tests unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace the threaded flatParameterDisambiguationMap + enclosingBoundLabels lookup with per-scope `let <label>: <Type> = <source>` receiver bindings. Every dependency is now bound as a real Swift `let` at the scope that needs it — matching production's .alias case — so resolveBuilderArguments collapses to bare-label emission and descendants reach their values via Swift lexical scoping. Leaf constant consumers of disambiguated flat parameters gain a __safeDI_<label>() wrapper to host the alias binding. Net -50 lines in ScopeGenerator.swift; 100% line coverage preserved. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…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>
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>
`collectMockParameterTree` treated a hand-written mock with zero parameters as "no initializer to use" and fell back to the init's arguments for both `constructionArguments` and `defaultParameters`. That caused the generated call site to reference an init-only signature (`Service.customMock(onCancel:onSubmit:)`) and feed the init's default values as arguments — a shape that does not exist on the hand-written `customMock()`. The hand-written mock is responsible for its own construction, and its signature (even an empty one) must win. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![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: b95a211bbd
ℹ️ 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".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #257 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 41 41
Lines 6378 6369 -9
=========================================
- Hits 6378 6369 -9
🚀 New features to boost your workflow:
|
This branch was the defensive fallback for the bug just fixed: when constructionArguments contained init-only default parameters that weren't tracked in defaultParameters, the resolver fell back to emitting the default expression inline. Now that both lists are derived from the same initializer, every non-dependency default parameter is always tracked, and this branch is unreachable. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ture-reuse # Conflicts: # Documentation/Manual.md # Sources/SafeDICore/Generators/ScopeGenerator.swift
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ 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". |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
When a type declares a hand-written mock with fewer parameters than its
initializer,
collectMockParameterTreewas still treating the init asthe source of construction arguments. If the mock had zero arguments,
the codegen:
constructionInitializer(so default parameters wereskipped correctly)…
childInstantiable.initializer?.argumentsforconstructionArguments, re-introducing the init's parameters intothe generated call.
Result: the generated mock referenced a signature that did not exist on
the hand-written mock, e.g.
even though
Service.customMockwas declared ascustomMock() -> Self.Fix
collectMockParameterTreenow picks a singleconstructionInitializer(mock when
useMockInitializer && mockInitializer != nil, otherwiseinit) and derives both
defaultParametersandconstructionArgumentsfrom it. The empty-mock case produces empty args and no defaults,
correctly emitting
(safeDIOverrides.service ?? Service.customMock)().Test plan
mock_referencesHandWrittenMockSignature_whenInitHasAdditionalDefaultedParamsregressionmock_typeWithUserMockAndOnlyDefaultValuedParamsGeneratesCallThrough_whenCustomMockNameIsSetexpected output (previously encoded the bug)mock_referencesExpectedMockSignature_whenCustomMockMethodMissingDependencyParametersexpected output (same bug, missing-deps error-path variant)swift test --traits sourceBuild— all 825 tests pass./CLI/lint.sh🤖 Generated with Claude Code