Generator fixes: per-type Sendable, unlabeled-default thunking, erased init wrap#265
Generator fixes: per-type Sendable, unlabeled-default thunking, erased init wrap#265
Conversation
![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: 4578145474
ℹ️ 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 #265 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 41 41
Lines 6370 6512 +142
==========================================
+ Hits 6370 6512 +142
🚀 New features to boost your workflow:
|
`resolveBuilderArguments` was routing every non-dependency argument through override-storage paths (`nodePath.<label>` or the sendable extracted local), but the override struct only materializes slots for default-valued non-dependency parameters. For any required non-dependency argument — e.g. a runtime value threaded from an ancestor's forwarded binding into a customMock — the generated code referenced a field that doesn't exist. Restore the three-branch shape: dependency → bare label, default non-dependency → override storage, anything else → `argument.innerLabel` (lexical scope). Matches the pre-simplification logic. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Follow-up to 8116332 (cherry-picked from #265). Adds `mock_resolvesRequiredNonDependencyArgumentViaLexicalScope`, which feeds SafeDITool a macro-invalid customMock (required non-default non-dep arg) to exercise the restored lexical fallback in `resolveBuilderArguments`. `skipCompileVerification: true` because the generated `runtimeValue` reference has no binding in scope — the macro rejects this shape via `mockMethodNonDependencyMissingDefaultValue`, but the generator must still emit a sensible reference rather than silently producing a missing-field path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
dfed
changed the title
Follow-up to 8116332 (cherry-picked from #265). Adds `mock_resolvesRequiredNonDependencyArgumentViaLexicalScope`, which feeds SafeDITool a macro-invalid customMock (required non-default non-dep arg) to exercise the restored lexical fallback in `resolveBuilderArguments`. `skipCompileVerification: true` because the generated `runtimeValue` reference has no binding in scope — the macro rejects this shape via `mockMethodNonDependencyMissingDefaultValue`, but the generator must still emit a sensible reference rather than silently producing a missing-field path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Follow-up to 8116332 (cherry-picked from #265). Adds `mock_resolvesRequiredNonDependencyArgumentViaLexicalScope`, which feeds SafeDITool a macro-invalid customMock (required non-default non-dep arg) to exercise the restored lexical fallback in `resolveBuilderArguments`. `skipCompileVerification: true` because the generated `runtimeValue` reference has no binding in scope — the macro rejects this shape via `mockMethodNonDependencyMissingDefaultValue`, but the generator must still emit a sensible reference rather than silently producing a missing-field path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
dfed
force-pushed
the
dfed--codegen-fixes
branch
from
c7fdf60 to
837d7d6
Compare
dfed
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 837d7d6f3c
ℹ️ 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".
…hable inits
Eliding a non-dependency `_`-labeled defaulted parameter from the mock
override surface works only when no later parameter is also `_`-labeled.
For `init(_ enabled: Bool = false, _ leaf: Leaf)` we would emit
`{ Child($0) }` — but Swift still requires the positional slot for
`enabled`, so the generated call is uncallable. Exposing `enabled` on
`SafeDIMockConfiguration` isn't an option either: there's no external
label to use as the struct's field name.
Validate the shape in `DependencyTreeGenerator.generateMockCode` and
reject with a diagnostic that points to the offending parameter and
suggests labeling it, reordering so the defaulted unlabeled parameter is
last, or removing the default. Regression test:
`mock_throwsError_whenUnlabeledDefaultPrecedesUnlabeledRequiredParameter`.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Restrict initializer-shape validation to mock-reachable scopes
generateMockCode builds typeDescriptionToScopeMap with createMockTypeDescriptionToScopeMapping(), which includes all instantiables, and then immediately runs validateMockableInitializerShapes on that full map. This makes mock generation fail when an unrelated or non-generateMock type has the rejected _-default parameter shape, even if no generated mock ever references that type. The validation should be limited to the scopes actually reachable from selected mock roots (including additionalMocksToGenerate) so unrelated declarations do not become build-blocking.
ℹ️ 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".
- Add regression test for the production `hasGeneratedContent + erasedToConcreteExistential` branch (run_writesConvenienceExtensionOnRootOfTree_whenExistentialErasedPropertyResolvesConcreteTypeWithItsOwnDependencies). The change in 4578145 added a wrap at ScopeGenerator.swift:543, but no existing fixture reached that arm — all prior erased-existential tests have leaf concrete types with no sub-dependencies. - Add regression test for the cross-root @sendable union (mock_configurationStructIsSendableAnnotated_whenSharedTypeIsReachedSendablyInAnyRoot). Two roots, one non-sendable reference to Shared and one via SendableInstantiator<Shared> — the emitted config struct must carry @sendable closure fields regardless of root ordering, which was the entire point of computing the union across all roots. - Pin Child.SafeDIMockConfiguration shape in the unlabeled-default closure-wrap test (mock_emitsClosureWrappedDefaultBuilder_whenUnlabeledDependencyCoexistsWithUnlabeledNonDependencyDefault). The builder closure type `(Leaf) -> Child` — with hidden `enabled` absent — is the actual invariant; asserting only the root's mock file left that part of the contract unpinned. - Drop the structurally-unreachable third branch in `resolveBuilderArguments` (and its accompanying test, which fabricated a macro-invalid fixture to exercise it). The macro rejects the only shape that would reach it — a required non-dependency argument on an init or mock method — via `unexpectedArgument` / `mockMethodNonDependencyMissingDefaultValue`. Per CLAUDE.md: "If code can't be covered by a test with real parsed input, remove the code." - Fix `if { return }` fall-through in `defaultBuilderExpression` per CLAUDE.md style; extract the `_`-labeled-default predicate into a helper so `callSiteArguments` and `hasHiddenUnlabeledDefaults` can't drift. - Correct Documentation/Manual.md: the cross-sendable-boundary rule applies per-concrete-type across roots (not "anywhere in the dependency graph"), and a leaf type without its own config struct carries @sendable on its override slot inside the parent's struct rather than on a whole configuration struct. 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: 8a69973842
ℹ️ 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".
Per CLAUDE.md's validation-boundary guidance (macro for local-context checks with fix-its; plugin for cross-type), the init-shape check is intrinsically per-type. Move it out of `DependencyTreeGenerator` and into `InstantiableMacro` so every @Instantiable-decorated type gets the diagnostic at compile time via its own macro expansion. Two codex concerns addressed by the move: - Scope: the plugin validation ran on the full `typeDescriptionToScopeMap` pre-root-selection, so an unrelated non-mocked type could abort mock generation for every root. The macro fires per-decorated-type with no cross-type blast radius. - Predicate: broadened too much. `init(_ a: Int = 0, _ b: Int = 1)` was rejected, but both trailing `_` defaults are elided together and the call site becomes `Type()`. Tightened to only flag a `_`-labeled default when a later `_`-labeled parameter lacks a default. Makes the `Diagnostic` convenience initializer skip the fix-it wrapper when `changes` is empty — the SwiftDiagnostics `FixIt` requires at least one change, so a diagnostic without a mechanical fix shouldn't include one. Removed the now-dead `unlabeledDefaultBeforeUnlabeledParameter` case on `DependencyTreeGeneratorError` and the `validateMockableInitializerShapes` method on `DependencyTreeGenerator`. Removed the corresponding `mock_throwsError_…` tool test; the macro test covers it: `producesDiagnostic_whenUnlabeledDefaultPrecedesUnlabeledRequiredParameterOnInit` plus `producesNoDiagnostic_whenTrailingUnlabeledParametersAllHaveDefaults` pinning the tightened predicate. 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: dd0cb79f18
ℹ️ 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".
| Self.validateUnlabeledDefaultShape( | ||
| functionSyntax: mockFunction.signature, | ||
| arguments: mockFunctionInitializer.arguments, | ||
| dependencies: visitor.dependencies, | ||
| diagnosticNode: Syntax(mockFunction), |
There was a problem hiding this comment.
When validating extension mock/instantiate signatures, validateUnlabeledDefaultShape is called with visitor.dependencies, but for extension declarations that collection is empty (dependencies are derived from method parameters). This makes dependency parameters look like non-dependencies and can emit false errors for valid signatures such as an unlabeled defaulted dependency followed by another unlabeled required dependency, blocking compilation even though the generator can pass both dependency arguments explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already fixed — this was the bug addressed by c71ce00 Sources: fix-it + scope fix for unlabeled-default init-shape diagnostic. Current dfed--codegen-fixes HEAD passes extensionDependencies at InstantiableMacro.swift:897, not visitor.dependencies. Latest push also carries a regression test (producesNoDiagnostic_whenTrailingUnlabeledParametersAllHaveDefaults) that fails against the old shape.
- Fix-it: the diagnostic now carries a mechanical change that promotes the inner label to the external label (rewrites `_ x: T = …` as `x: T = …`). That's the cleanest of the three resolutions — the parameter becomes callable by keyword, so eliding it from the mock override surface is safe. Reordering or removing the default are still mentioned in the diagnostic for manual application. - Extension scope: validation on an extension's `instantiate()` method was reading `visitor.dependencies`, which is empty for extensions (extension deps come from the `instantiate` arguments themselves). Every `instantiate` argument IS a dependency, so the shape can't occur there — removed the extension `instantiate()` walk entirely. Extension mock methods now pass `extensionDependencies` (the flattened dependency set across the extension's instantiables) so `_`-labeled dependency parameters on a mock method aren't misclassified as non-dependencies. - Updated `validateUnlabeledDefaultShape` to align parameter syntaxes with argument indices so the fix-it can target the exact `FunctionParameterSyntax` for replacement. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Every caller passes at least one `.replace` change now that the unlabeled-default-before-unlabeled-parameter diagnostic carries a mechanical fix-it. No live site produces an empty `changes` array, so the `changes.isEmpty ? [] : …` branch was dead per CLAUDE.md'"'"'s rule against defensive fallbacks for structurally unreachable paths. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ix-it Codecov flagged 1 line missing on the patch: the description arm and fix-it arm for the new error case weren't exercised from `SafeDICoreTests`. `SafeDIMacros` already hits both transitively (the diagnostic fires during macro expansion), but the `SafeDICore` copy of `FixableInstantiableError.swift` is a separate build artifact whose coverage is driven by `FixableInstantiableErrorTests.swift`. Added the two symmetric test cases matching the pattern for `mockOnlyMissingMockMethod`, `mockOnlyWithIsRoot`, etc. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Most fixtures had never been typechecked as Swift; turning verification on surfaced a long tail of invalid inputs. Updates across the suite: - Rename types that shadowed `Swift.String`/`Swift.Int` so literal defaults compile against the fixture's types. - Add `skipCompileVerification: true` with explanatory comments to the ~14 cross-module fixtures, ~7 plugin-error tests (broken syntax, misconfigured stubs, stub-path coverage), the two `NonPublic` tests that intentionally exercise cross-module access control, and a few FIXME-tagged fixtures tracking known generator bugs (lazy self-cycle, erased-existential slot coercion, custom-mock additional-type return). The macro/generator fixes that these fixtures relied on are already in the base PR (#265); this PR only adds the verifier infrastructure and the fixture hygiene to make it pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 participant
Summary
Five generator/macro changes with the new tests landed alongside each, plus an internal refactor.
Changes vs. main
1. Strip non-dependency unlabeled-defaulted parameters from the mock override surface
For a callee declared like
public init(_ x: T = <expr>), SafeDI has two options on the override surface:SafeDIMockConfigurationis a struct with labeled fields, so we'd need an external label forx. The only candidate is the inner label (xhere), promoted from the parameter name. That's awkward — inner labels aren't part of the type's API contract, can collide with existing field names, and produce surprising renames.We pick (b). That's what
callSiteArgumentsfilters out of the override signature, and it's whydefaultBuilderExpressionfalls back to emitting{ Type(label: $0) }when the init reference's arity no longer matches — so the thunk path actually runs.Why (b) is also correct (not just convenient): Swift evaluates default expressions in the callee's lexical scope —
Selfbinds to the callee's type, unqualified names resolve against the callee's members. Any scheme that inlined<expr>at the caller would break those references.Tests:
mock_doesNotInlineUnderscoreDefaultExpression_whenDefaultReferencesTypeStaticMembermock_emitsClosureWrappedDefaultBuilder_whenUnlabeledDependencyCoexistsWithUnlabeledNonDependencyDefaultmock_emitsClosureWrappedDefaultBuilder_whenCustomMockHasUnlabeledDependencyAndUnlabeledNonDependencyDefault2. Reject unmockable init shapes in
InstantiableMacroThe strip in #1 doesn't work when a
_-labeled non-dep default is followed by a_-labeled required parameter — Swift still requires the positional slot to be filled. Instead of silently emitting uncallable code, the macro now diagnoses the shape on every@Instantiable-decorated concrete type init and mock method. The attached fix-it mechanically promotes the inner label to the external label (rewrites_ x: T = …asx: T = …), which makes the parameter callable by keyword so elision becomes safe. Reordering and removing-the-default are still mentioned in the diagnostic for manual application. Subsequent_-labeled parameters with defaults don't block elision — trailing runs likeinit(_ a = 0, _ b = 1)are allowed.Extension
instantiate()methods are skipped — every argument on aninstantiateis a dependency by construction, so the shape can't occur there. Extension mock methods use the extension's flattened dependency set (notvisitor.dependencies, which is empty in extension mode).Lives in the macro per CLAUDE.md's validation-boundary rule: the shape is purely local to the declared type, no cross-type context needed. Earlier iterations of this PR did it at the plugin level, which (a) aborted mock generation for unrelated roots on a single-type regression and (b) was too strict about trailing runs — both caught in review.
Tests:
producesDiagnostic_whenUnlabeledDefaultPrecedesUnlabeledRequiredParameterOnInitproducesNoDiagnostic_whenTrailingUnlabeledParametersAllHaveDefaults3. Preserve
_-labeled dependency args in mock call sitesThe filter from #1 could also drop a dependency shaped
_ dep: Dep = …, which silently replaced the SafeDI-resolved value with the syntactic default. The filter now keeps unlabeled args whose inner label matches a known dependency.Test:
mock_threadsResolvedBinding_whenDependencyParameterIsUnlabeledWithDefault4. Per-type
@SendableonSafeDIMockConfiguration(cross-root union)On main, each emission of
SafeDIMockConfigurationdecided@Sendableper-scope using the node's ownrequiresSendable. Because the struct is a nested type deduplicated by concrete type name across every root that references it, two different roots could emit different annotations for "the same" struct. Now the set of types needing@Sendableis computed once by unioningcollectSendableConfigurationTypeNames()across all mock roots (newDependencyTreeGeneratorpre-pass), then threaded intogenerateConfigurationStructas a singlerequiresSendable: Bool— so every root agrees on the shape.Documented in
Manual.md.Test:
mock_configurationStructIsSendableAnnotated_whenSharedTypeIsReachedSendablyInAnyRoot5. Keep leaf child builder closures
@Sendablewhen the child itself is sendably reachedBefore: a parent struct's leaf child slot used only
child.requiresSendable. When the parent was reached sendably (so the struct type is@Sendableper #4) but a leaf child wasn't, the slot lost the annotation; also, a non-sendable parent containing a leafSendableInstantiatorwasn't annotating the child slot even thoughgenerateInstantiatorBindinglater captured it inside a@Sendablelocal function.Now leaf child fields use
requiresSendable || child.requiresSendable.Test:
mock_leafSendableInstantiatorInsideNonSendableParentMarksChildFieldSendable6. Wrap erased-existential output in the production init path
When a property was both
erasedToConcreteExistentialandhasGeneratedContent, the production<Root>+SafeDI.swiftemittedfunctionName()directly without wrapping in the property's erased type. Added theproperty.typeDescription.asSource(functionName())wrap so the generated expression typechecks.Test:
run_writesConvenienceExtensionOnRootOfTree_whenExistentialErasedPropertyResolvesConcreteTypeWithItsOwnDependenciesInternal refactor:
resolveBuilderArgumentsRewritten against
callSiteArgumentsand.map(instead ofconstructionArguments/.compactMap) to align with the filter from #1 and #3.Diagnostic(…)convenience initializer now skips the fix-it wrapper whenchangesis empty (the underlying SwiftDiagnosticsFixItrequires at least one change).Relationship to #256
These are the generator/macro-visible subset of #256 (which also adds a
swiftc -typecheckpass over generated output). Landing here unblocks #256 from having to ship the generator fixes and the test infrastructure together.Test plan
swift build --traits sourceBuildswift test --traits sourceBuild— 840 tests pass./CLI/lint.sh🤖 Generated with Claude Code