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: 2bbd71b9e4
ℹ️ 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 #259 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 41 41
Lines 6515 6689 +174
==========================================
+ Hits 6515 6689 +174
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00cfa18073
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11fe93c873
ℹ️ 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".
dfed
force-pushed
the
claude/enable-compile-verification-easy-fixes
branch
from
11fe93c to
11f7dae
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11f7dae177
ℹ️ 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".
dfed
force-pushed
the
claude/enable-compile-verification-easy-fixes
branch
from
11f7dae to
453f8a1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 453f8a1b47
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c1877404b
ℹ️ 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".
dfed
force-pushed
the
claude/compile-verify-codegen-tests
branch
from
3d4c12a to
8970cec
Compare
dfed
force-pushed
the
claude/enable-compile-verification-easy-fixes
branch
from
091ff55 to
189cadf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 189cadfd27
ℹ️ 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".
dfed
force-pushed
the
claude/compile-verify-codegen-tests
branch
from
8970cec to
5ac9720
Compare
dfed
force-pushed
the
claude/enable-compile-verification-easy-fixes
branch
from
d66ab8c to
540d2fe
Compare
Three issues surfaced during self-review:
1. **P1 style violation** — `defaultBuilderExpression` in `ScopeGenerator.swift`
had `if hasHiddenUnlabeledDefaults { … return "{ … }" }` followed by a
bare `if callSiteArguments.isEmpty { return … } else { … }`. CLAUDE.md
forbids early returns from bare `if`s that fall through into another
`if`. Converted to `if/else if/else`.
2. **P2 test regression** — the generator test
`run_writesConvenienceExtensionOnRootOfTree_whenExistentialErasedPropertyResolvesConcreteTypeWithItsOwnDependencies`
was silently dropped when this PR rebased on main. It exercises a
distinct code path (nested `__safeDI_splashScreenView()` + `AnyView(…)`
wrap) that isn't redundant with any other erased-wrapper test. Restored.
3. **P2 coverage gap** — `mock_generatedForFullyLazySendableInstantiatorCycle`
asserted only on `Root+SafeDIMock.swift`. The test's own comment says
C's config struct must not expose an `aBuilder` field, but that
invariant was only observable on `C+SafeDIMock.swift`, which was never
checked. Added the full-string expectation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf5ef3541c
ℹ️ 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".
Three issues surfaced during self-review:
1. **P1 style violation** — `defaultBuilderExpression` in `ScopeGenerator.swift`
had `if hasHiddenUnlabeledDefaults { … return "{ … }" }` followed by a
bare `if callSiteArguments.isEmpty { return … } else { … }`. CLAUDE.md
forbids early returns from bare `if`s that fall through into another
`if`. Converted to `if/else if/else`.
2. **P2 test regression** — the generator test
`run_writesConvenienceExtensionOnRootOfTree_whenExistentialErasedPropertyResolvesConcreteTypeWithItsOwnDependencies`
was silently dropped when this PR rebased on main. It exercises a
distinct code path (nested `__safeDI_splashScreenView()` + `AnyView(…)`
wrap) that isn't redundant with any other erased-wrapper test. Restored.
3. **P2 coverage gap** — `mock_generatedForFullyLazySendableInstantiatorCycle`
asserted only on `Root+SafeDIMock.swift`. The test's own comment says
C's config struct must not expose an `aBuilder` field, but that
invariant was only observable on `C+SafeDIMock.swift`, which was never
checked. Added the full-string expectation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex P1 on #259: the `!overrideReachable` branch in `resolveBuilderArguments` inlined the raw default-expression text at the caller. For callee-scope references (`Self.*`, private static members, file-scoped helpers), Swift resolved the reference against the wrong type, producing code that silently built the wrong value or failed to compile. Emit a closure form in the pruned branch instead: - `MockParameterNode.callSiteArgumentsForPrunedOverride` filters labeled non-dependency defaults in addition to the unlabeled ones `callSiteArguments` already excludes. - `MockParameterNode.defaultBuilderExpressionForPrunedOverride` returns `{ Type(label: $0) }` (or `{ @sendable in Type(label: $0) }` when the node is reached sendably). The closure captures only deps + required non-deps, so every other default fires via Swift's default-argument thunk in the callee's lexical scope. - `resolveBuilderArguments` routes through `callSiteArgumentsForPrunedOverride` when `overrideReachable` is false, emitting only dep/required values. - The `!thisOverrideReachable` branch in `generateInstantiatorBinding` now uses `defaultBuilderExpressionForPrunedOverride` instead of the function reference, so Sendable captures stay valid inside `@Sendable` funcs. Updates `mock_generatedForCycleWithDefaultValuedNonDependencyParameter` expectation for the new closure form. Cycle and sendable-cycle regression tests continue to pass via compile verification. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dfed
force-pushed
the
claude/enable-compile-verification-easy-fixes
branch
from
bf5ef35 to
c50a7b5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c50a7b56c5
ℹ️ 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".
Codex P1 on #259: `isCycleNode` was checked before `!thisOverrideReachable`, so nodes that are both cycle-detected AND globally unreachable took the full-signature `defaultBuilder` path while `resolveBuilderArguments` elided their labeled non-dep args — producing arity-mismatched calls (`Type.init(dep:label:)` called with only `dep`). Reorder: check whether the pruned override would elide args (`callSiteArgumentsForPrunedOverride.count < callSiteArguments.count`) first and use `defaultBuilderExpressionForPrunedOverride` when so, regardless of cycle status. Keep `isCycleNode`'s full-signature path for reachable cycle nodes and for pruned cycle nodes with no labeled defaults (where the function reference's arity still matches). Existing `mock_generatedForCycleWithDefaultValuedNonDependencyParameter` exercises the cycle+pruned+labeled-default combo; its expected output (the inner `__safeDI_aBuilder` returning `{ A(bBuilder: $0) }(bBuilder)`) pins the fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e7faf7858
ℹ️ 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 onStack.contains(neighbor) { | ||
| edges.insert(ScopeGenerator.CycleEdge( | ||
| parent: node.asSource, | ||
| child: neighbor.asSource, | ||
| )) |
There was a problem hiding this comment.
This block records every DFS back-edge (onStack hit) as a pruned cycle edge, which over-prunes strongly connected components that have multiple ancestor edges. For example, in a graph like A→B, B→A, B→C, C→A, both B→A and C→A are added here, and generateConfigurationStruct then removes both override branches even though only one cut is needed to break cycles. That makes otherwise valid SafeDIMockConfiguration override paths unreachable and forces default construction on extra branches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid observation. The current DFS records all back-edges — that's a correct (acyclic-producing) feedback arc set, but not minimum. In your example the algorithm picks 2 edges {(B,A), (C,A)} when {(A,B)} alone would break every cycle.
Product impact: users lose granular override paths at intermediate scopes. They can still override at the root and cycles are broken; they just can't override aBuilder at B-level or C-level independently. Cost-wise, most SafeDI graphs have simple 2-node cycles where the DFS output is already minimum.
Fixing properly requires a real minimum-FAS heuristic (Tarjan's SCC → per-SCC cycle-breaking). That's a meaningful piece of work and I'd rather track it as a follow-up than bolt it onto this PR — which is already large. Would you be OK with deferring?
Three issues surfaced during self-review:
1. **P1 style violation** — `defaultBuilderExpression` in `ScopeGenerator.swift`
had `if hasHiddenUnlabeledDefaults { … return "{ … }" }` followed by a
bare `if callSiteArguments.isEmpty { return … } else { … }`. CLAUDE.md
forbids early returns from bare `if`s that fall through into another
`if`. Converted to `if/else if/else`.
2. **P2 test regression** — the generator test
`run_writesConvenienceExtensionOnRootOfTree_whenExistentialErasedPropertyResolvesConcreteTypeWithItsOwnDependencies`
was silently dropped when this PR rebased on main. It exercises a
distinct code path (nested `__safeDI_splashScreenView()` + `AnyView(…)`
wrap) that isn't redundant with any other erased-wrapper test. Restored.
3. **P2 coverage gap** — `mock_generatedForFullyLazySendableInstantiatorCycle`
asserted only on `Root+SafeDIMock.swift`. The test's own comment says
C's config struct must not expose an `aBuilder` field, but that
invariant was only observable on `C+SafeDIMock.swift`, which was never
checked. Added the full-string expectation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex P1 on #259: the `!overrideReachable` branch in `resolveBuilderArguments` inlined the raw default-expression text at the caller. For callee-scope references (`Self.*`, private static members, file-scoped helpers), Swift resolved the reference against the wrong type, producing code that silently built the wrong value or failed to compile. Emit a closure form in the pruned branch instead: - `MockParameterNode.callSiteArgumentsForPrunedOverride` filters labeled non-dependency defaults in addition to the unlabeled ones `callSiteArguments` already excludes. - `MockParameterNode.defaultBuilderExpressionForPrunedOverride` returns `{ Type(label: $0) }` (or `{ @sendable in Type(label: $0) }` when the node is reached sendably). The closure captures only deps + required non-deps, so every other default fires via Swift's default-argument thunk in the callee's lexical scope. - `resolveBuilderArguments` routes through `callSiteArgumentsForPrunedOverride` when `overrideReachable` is false, emitting only dep/required values. - The `!thisOverrideReachable` branch in `generateInstantiatorBinding` now uses `defaultBuilderExpressionForPrunedOverride` instead of the function reference, so Sendable captures stay valid inside `@Sendable` funcs. Updates `mock_generatedForCycleWithDefaultValuedNonDependencyParameter` expectation for the new closure form. Cycle and sendable-cycle regression tests continue to pass via compile verification. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex P1 on #259: `isCycleNode` was checked before `!thisOverrideReachable`, so nodes that are both cycle-detected AND globally unreachable took the full-signature `defaultBuilder` path while `resolveBuilderArguments` elided their labeled non-dep args — producing arity-mismatched calls (`Type.init(dep:label:)` called with only `dep`). Reorder: check whether the pruned override would elide args (`callSiteArgumentsForPrunedOverride.count < callSiteArguments.count`) first and use `defaultBuilderExpressionForPrunedOverride` when so, regardless of cycle status. Keep `isCycleNode`'s full-signature path for reachable cycle nodes and for pruned cycle nodes with no labeled defaults (where the function reference's arity still matches). Existing `mock_generatedForCycleWithDefaultValuedNonDependencyParameter` exercises the cycle+pruned+labeled-default combo; its expected output (the inner `__safeDI_aBuilder` returning `{ A(bBuilder: $0) }(bBuilder)`) pins the fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21ce155b37
ℹ️ 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 onStack.contains(neighbor) { | ||
| edges.insert(ScopeGenerator.CycleEdge( | ||
| parent: node.asSource, | ||
| child: neighbor.asSource, | ||
| )) |
There was a problem hiding this comment.
computeCycleEdges currently records every DFS edge that points to a node on the active stack, which over-prunes strongly connected components with multiple ancestor edges. For a graph like A→B, B→A, B→C, C→A, this logic marks both B→A and C→A as cycle cuts, and generateConfigurationStruct then removes both override branches even though cutting one edge is sufficient to break the cycle. The result is lost SafeDIMockConfiguration override paths and forced default construction on extra branches that are not required to prevent recursion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Duplicate of r3107884737 — see reply there.
Runs `swiftc -typecheck` on test inputs plus generated output to catch codegen regressions that were previously only exposed by integration tests. Fixtures that cannot be typechecked in isolation (cross-module placeholders, platform framework dependencies on Linux, etc.) opt out via `skipCompileVerification: true` with an in-place comment explaining the reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Self-review on #256 flagged that these two tests' opt-out comments described a cross-module access issue, but the verifier compiles everything in one synthetic module. The actual failure is Swift's same-module rule that a `public init`'s signature cannot reference an `internal` type. Updated comment to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three issues surfaced during self-review:
1. **P1 style violation** — `defaultBuilderExpression` in `ScopeGenerator.swift`
had `if hasHiddenUnlabeledDefaults { … return "{ … }" }` followed by a
bare `if callSiteArguments.isEmpty { return … } else { … }`. CLAUDE.md
forbids early returns from bare `if`s that fall through into another
`if`. Converted to `if/else if/else`.
2. **P2 test regression** — the generator test
`run_writesConvenienceExtensionOnRootOfTree_whenExistentialErasedPropertyResolvesConcreteTypeWithItsOwnDependencies`
was silently dropped when this PR rebased on main. It exercises a
distinct code path (nested `__safeDI_splashScreenView()` + `AnyView(…)`
wrap) that isn't redundant with any other erased-wrapper test. Restored.
3. **P2 coverage gap** — `mock_generatedForFullyLazySendableInstantiatorCycle`
asserted only on `Root+SafeDIMock.swift`. The test's own comment says
C's config struct must not expose an `aBuilder` field, but that
invariant was only observable on `C+SafeDIMock.swift`, which was never
checked. Added the full-string expectation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex P1 on #259: the `!overrideReachable` branch in `resolveBuilderArguments` inlined the raw default-expression text at the caller. For callee-scope references (`Self.*`, private static members, file-scoped helpers), Swift resolved the reference against the wrong type, producing code that silently built the wrong value or failed to compile. Emit a closure form in the pruned branch instead: - `MockParameterNode.callSiteArgumentsForPrunedOverride` filters labeled non-dependency defaults in addition to the unlabeled ones `callSiteArguments` already excludes. - `MockParameterNode.defaultBuilderExpressionForPrunedOverride` returns `{ Type(label: $0) }` (or `{ @sendable in Type(label: $0) }` when the node is reached sendably). The closure captures only deps + required non-deps, so every other default fires via Swift's default-argument thunk in the callee's lexical scope. - `resolveBuilderArguments` routes through `callSiteArgumentsForPrunedOverride` when `overrideReachable` is false, emitting only dep/required values. - The `!thisOverrideReachable` branch in `generateInstantiatorBinding` now uses `defaultBuilderExpressionForPrunedOverride` instead of the function reference, so Sendable captures stay valid inside `@Sendable` funcs. Updates `mock_generatedForCycleWithDefaultValuedNonDependencyParameter` expectation for the new closure form. Cycle and sendable-cycle regression tests continue to pass via compile verification. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex P1 on #259: `isCycleNode` was checked before `!thisOverrideReachable`, so nodes that are both cycle-detected AND globally unreachable took the full-signature `defaultBuilder` path while `resolveBuilderArguments` elided their labeled non-dep args — producing arity-mismatched calls (`Type.init(dep:label:)` called with only `dep`). Reorder: check whether the pruned override would elide args (`callSiteArgumentsForPrunedOverride.count < callSiteArguments.count`) first and use `defaultBuilderExpressionForPrunedOverride` when so, regardless of cycle status. Keep `isCycleNode`'s full-signature path for reachable cycle nodes and for pruned cycle nodes with no labeled defaults (where the function reference's arity still matches). Existing `mock_generatedForCycleWithDefaultValuedNonDependencyParameter` exercises the cycle+pruned+labeled-default combo; its expected output (the inner `__safeDI_aBuilder` returning `{ A(bBuilder: $0) }(bBuilder)`) pins the fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Exercise the cycle-specific paths in `defaultBuilderExpressionForPrunedOverride`
with two new tests:
1. `mock_generatedForCycleWithCustomMockAndLabeledDefaultOnCycleNode` —
A↔B cycle where A has customMock + labeled default. Exercises the
`customMockName` branch and the `.methodName(labeledArguments)`
call-expression branch.
2. `mock_generatedForSendableCycleWithLabeledDefaultOnCycleNode` —
SendableInstantiator cycle with labeled default. Exercises the
`{ @sendable in … }` closure form inside `@Sendable func` contexts.
Also removes the unreachable `isExtensionBased` branch in
`defaultBuilderExpressionForPrunedOverride`: extension `instantiate()`
args are all dependencies, so `callSiteArgumentsForPrunedOverride ==
callSiteArguments` and the helper is never invoked for
extensions-without-customMock. Extensions with customMock take the
`useMockInitializer` branch instead.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex P1 on #256 (r3107957694): when a back-edge made an `erasedToConcreteExistential` child unreachable, the generator set `optionalBuilderPath = nil` while `builderExpression` was assigned a non-optional default builder. Downstream emission still tried to `if let safeDIBuilder = \(overridePath)` (constant-wrapper path) or `\(builderExpression)?(\(argumentList)) ?? …` (leaf path) against that non-optional value — invalid Swift. Split the erased path into two cases: - Override unreachable (`!thisOverrideReachable`): emit a direct construction — `\(wrapperType)(\(node.defaultBuilderCall(arguments: arguments)))` — with no `if let` / optional-chain dance. - Override reachable but `optionalBuilderPath` is nil (sendable context — extracted local IS optional): keep the existing optional-binding behavior. Fixes the bug for cycles whose pruned edge targets an erased child. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dfed
force-pushed
the
claude/enable-compile-verification-easy-fixes
branch
from
21ce155 to
4320a97
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4320a97c80
ℹ️ 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".
| // `builderExpression` is a non-optional default builder, so | ||
| // emit a direct construction rather than an `if let` dance | ||
| // (which would fail to typecheck against a non-optional RHS). | ||
| lines.append("\(innerIndent)return \(wrapperType)(\(node.defaultBuilderCall(arguments: arguments)))") |
There was a problem hiding this comment.
When thisOverrideReachable is false, this branch still uses node.defaultBuilderCall(arguments: arguments) even though resolveBuilderArguments(... overrideReachable: false) may have removed labeled defaulted non-dependency parameters from arguments. defaultBuilderCall zips values against callSiteArguments (the full signature), so a valid initializer like init(dep1:, flag: = ..., dep2:) gets misaligned labels on pruned edges (dep2 is emitted at flag:), producing non-compiling generated mocks for erased-to-concrete cycle cuts.
Useful? React with 👍 / 👎.
Codex P1 on #256 (stale) / re-filed on #259: the closure literal was marked `@Sendable` only when `typeDescription.propertyType.isSendable` — i.e. when the node itself was a `SendableInstantiator` wrapper. This missed cases where a non-Sendable type is reached through a Sendable ancestor; the closure ends up captured inside an `@Sendable` helper func and Swift 6 rejects the non-Sendable capture. Move the decision to the caller, which knows whether the closure is about to land inside a sendable extraction / `@Sendable func`: `sendableExtractionPrefix != nil || propertyType.isSendable` now drives the attribute. The helper takes an `inSendableContext` bool. Over-annotating is also wrong — a `@Sendable` closure literal in a non-Sendable slot can fail to typecheck at the assignment site — so we don't unconditionally emit `@Sendable`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d30b20e62
ℹ️ 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".
Codex P1 on #259: `resolveBuilderArguments` now returns values based on `callSiteArgumentsForPrunedOverride` when the override path is pruned (fewer values — elided labeled non-dep defaults), but `defaultBuilderCall` kept zipping against the full `callSiteArguments` signature. For an init/customMock like `init(a: A, b: B = …, c: C)`, the emitted call would pair `C`'s value with label `b`, producing non-compiling mock code. Add an `overrideReachable` parameter (default `true` for existing callers) that routes the zip through the correct argument list. Caller sites inside the `!thisOverrideReachable` erased-pruned branches pass `false`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The erased-to-concrete-existential `!thisOverrideReachable` branches added earlier were defensive against a scenario I couldn't produce with valid input. On further analysis, it's structurally impossible: - Erased `@Instantiated` declarations are constant edges in the type graph. - `DependencyTreeGenerator` rejects any cycle containing a constant edge as "mix of constant and lazy" (line 450 error). - Therefore an erased child can't participate in a cycle, and its parent can't have a pruned back-edge targeting its concrete type. Removing the unreachable branches also lets `defaultBuilderCall` drop the `overrideReachable:` parameter — the only two call sites that set it to `false` are the branches being removed. All reachable paths for erased nodes have either a non-nil `optionalBuilderPath` (leaf/config- struct context) or an optional-typed `builderExpression` (sendable context), so the existing `optionalBuilderPath ?? builderExpression` fallback remains correct. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
|
@codex please construct a failing test that reaches the defensive branches removed in 5b8b9a4 ( Specifically, produce a test fixture where a My original argument for removal: Counter-claim to investigate: If you can produce a compiling test that exercises the removed branches, the removal was wrong and we should restore. If you can prove no valid SafeDI input reaches those branches (e.g., because ErasedInstantiator properties never become mock-tree children, or because erasedToConcreteExistential is dropped on the mock node in that shape), cite the code. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e038d7f95f
ℹ️ 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".
| } else if !thisOverrideReachable { | ||
| // Pruned override, no labeled defaults to elide — the full | ||
| // default builder and the (non-elided) argument list line up. | ||
| builderExpression = defaultBuilder | ||
| optionalBuilderPath = nil |
There was a problem hiding this comment.
When a back-edge override path is pruned, this branch falls back to defaultBuilder without adding sendable context. If the callee has hidden unlabeled defaults, defaultBuilderExpression is a closure literal (for example { A(bBuilder: $0) }), and in a SendableInstantiator cycle that closure is later extracted and captured by a nested @Sendable helper function. Because this literal has no @Sendable context here, Swift 6 strict concurrency rejects the generated code as a non-sendable closure capture. A reproducible case is a pruned sendable cycle where A has init(bBuilder: SendableInstantiator<B>, _ enabled: Bool = false).
Useful? React with 👍 / 👎.
|
Codex couldn't complete this request. Try again later. |
…torBinding Reverts e038d7f. The "structurally unreachable" argument was wrong: `@Instantiated(erasedToConcreteExistential: true) let x: ErasedInstantiator<(), AnyEraser>` resolves to `.erasedInstantiator` (non-constant per Property.swift:187), so a cycle through such a property passes `throwIfInvalidCycle` without tripping the "mix of constant and lazy" check. `computeCycleEdges` then records the erased edge as a back-edge when the DFS happens to reach the erased-child type first (alphabetical ordering). Adds the missing defensive branch to `generateInstantiatorBinding` as well — the `else if node.erasedToConcreteExistential` arm emitted `if let \(builderExpression)` where `builderExpression` is a non-optional default builder in the pruned-back-edge case, producing code that fails to typecheck. The Sendable-extracted path already handled optional locals; split the branch so `!overridePathReachable` emits direct construction. Adds `mock_generatedForErasedInstantiatorBackEdgeInLazyCycle` as a reproducer. Without these fixes, the compile verifier catches the bug: `if let ConcreteView.init(zBuilder:)` fails with "unwrap condition requires a valid identifier". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Reproduced and fixed in 1736972. My earlier "structurally unreachable" argument was wrong — the codex-style review was correct. Reproducer (
Without the defensive branches, the verifier catches: → Fixes:
All 849 tests pass. PR description will be updated on next push. Also: thanks to @codex (which I asked first) and my own follow-up investigation — the "best agent wins" experiment settled on the test-driven answer. The structural argument wasn't load-bearing enough. |
|
Codex couldn't complete this request. Try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 173697204c
ℹ️ 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".
…collectUniqueConfigurationTypes Two codex-flagged concerns resolve to dead code: 1. `generateMockBodyBindings` non-leaf + leaf erased `!thisOverrideReachable` branches: the non-Instantiator children handled in this function are constant-typed, and `validateMockRootScopeForCycles` rejects any cycle containing a constant edge via `throwIfInvalidCycle`'s constant / mixed-lazy checks. Pruned back-edges on erased children only occur on `ErasedInstantiator`-typed properties, which are handled by `generateInstantiatorBinding` (where the guard already landed in 1736972). 2. `collectUniqueConfigurationTypes`'s `guard !node.isPropertyCycle`: a cycle-closer (`isPropertyCycle == true`) is always reached at a deeper DFS level than its matching non-cycle occurrence — the property has to appear earlier in the stack for the flag to fire — so the `seen` dedup further down catches it after the real version has been recorded. Local coverage confirmed the guard was never hit. Also adds a regression test for the self-cycle root case codex called out (`mock_generatedForRootSelfInstantiatorCycle`), pinning that the config struct for a self-cycle root type IS emitted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dfed
changed the title
…nit (#270) ## Summary Capture convention/mechanism knowledge that wasn't load-bearing in the code but has tripped up recent work. All additions are docs + one small helper extraction — no behavior change. ## What's documented **Cycle mechanisms table** (CLAUDE.md). Four related things — `isPropertyCycle`, `cycleEdges`, `throwIfInvalidCycle`, `validateMockRootScopeForCycles` — each doing a different job with overlapping names. Table names them, says where each is set, and what it does. Also explicit: if `throwIfInvalidCycle` accepts a cycle, every hop is non-constant. **`erasedToConcreteExistential` × property-type matrix** (CLAUDE.md Common Pitfalls). The flag doesn't imply a constant property type — it's orthogonal. Four rows cover all combinations and whether each can appear in an accepted cycle. This mistake cost a round trip in #259 when defensive branches were removed on a wrong "erased = constant" assumption. **Two-layer mock return type** (CLAUDE.md Mock generation specifics). Inner closure type (`builderClosureType`, #251) vs outer `mock()` return type (#266) are distinct concerns when a `customMock` returns a type other than `Self`. Both layers have to agree. **"Selected construction initializer"** (CLAUDE.md). SafeDI picks a single canonical initializer per type. Validation that only matters for the path SafeDI uses (e.g., `Self.*`-in-default-expression check) should gate on this, not walk every initializer. Was #268's P1 bug class. **"Structurally unreachable" claims need a failing test** (CLAUDE.md Testing Philosophy). Codifies the discipline we're already trying to hold — structural arguments are easy to get wrong. **Inline /// comments**: - `MockParameterNode.isPropertyCycle` cross-references `cycleEdges` (per-root syntactic vs global feedback arc set). - `MockContext.cycleEdges` cross-references `isPropertyCycle` (opposite direction). - `throwIfInvalidCycle` docstring expanded with explicit rejection classes and acceptance condition. ## Code change Add `InstantiableVisitor.canonicalConstructionInitializer` computed property. Route the two existing `initializers.first(where: { $0.isValid(forFulfilling: dependencies) })` call sites through it: - `InstantiableVisitor.swift:377` (building `Instantiable.initializer`) - `InstantiableMacro.swift:259` (the `hasMemberwiseInitializerForInjectableProperties` guard) Removes drift risk — previously the macro recomputed the selector separately. ## Test plan - [x] `swift test --traits sourceBuild` — 885 tests pass - [x] `./CLI/lint.sh` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sts (#276) ## Summary Five mock-generation tests carried `skipCompileVerification: true` FIXMEs pointing at generator bugs that have since been fixed. Removing the opt-outs confirms the fixes hold end-to-end (fresh parse → generated code → actual swiftc typecheck). ## Changes Removed the FIXME comments and `skipCompileVerification: true` flag on: | Test | Bug | Fixed in | |------|-----|----------| | `mock_usesMockOnly_whenProductionHasNoMockAndBothFulfillSameAdditionalType` | mockOnly not coerced to fulfilled-slot type | #251 | | `mock_sendableClosureDependencyProducesValidIdentifier` | `@escaping` dropped on `@Sendable` closure on mock() | #267 | | `mock_generatedForFullyLazyInstantiationCycle` | Fully-lazy cycles produced self-referential `SafeDIMockConfiguration` defaults | #259 | | `mock_extensionUserMockReturningAdditionalTypeUsedForProtocolProperty` | customMock return type disagreement between root call site and type's own `mock()` | #266 | | `mock_extensionUserMockReturningAdditionalTypeNotUsedForConcreteProperty` | Same as above — sibling test | #266 | ## Test plan - [x] `swift test --traits sourceBuild` — 901 tests pass (up from 901; no tests added, but all five now exercise the compile-verification path) - [x] `./CLI/lint.sh` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Problem
Multi-hop lazy-instantiation cycles (e.g.
A → Instantiator<B> → B → Instantiator<C> → C → Instantiator<A>) producedSafeDIMockConfigurationstructs that referenced themselves through the cycle — Swift rejects those as infinite-size value types, so the generated mock code failed to compile.The root cause: the old generator relied on a per-tree
isPropertyCycleflag that breaks at whichever edge the walker happened to cross last. Different roots walking the same cycle produced different shapes for the same shared config struct.Fix
Compute cycle back-edges once, globally, in
DependencyTreeGenerator.computeCycleEdgesvia an alphabetical-DFS feedback arc set. Thread the chosencycleEdgesthrough both:SafeDIMockConfigurationstruct generation (so every root emits the same shape for a given type), andmock()bodies (so call sites agree on which slot is pruned).With the back-edge chosen globally, every root sees the same cycle break and the shared struct stops recursing into itself.
Secondary fixes required to make the above compile
resolveBuilderArgumentsprunes a back-edge, it used to inline raw default-expression text at the caller. It now emits a closure form ({ Type(label: $0) }, or{ @Sendable in … }in sendable contexts) that captures only deps + required non-deps, so Swift's default-argument thunk fires in the callee's lexical scope — preservingSelf.*, private members, and file-scoped helpers.ErasedInstantiatorpruned-back-edge branch.generateInstantiatorBinding's erased arm emittedif let \(builderExpression)even whenbuilderExpressionwas a non-optional default. Split the arm so pruned back-edges emit direct construction.@Sendableon config-struct closure fields. Only marks@Sendableon the specific override slot reached through aSendableInstantiator/SendableErasedInstantiator, not on the whole enclosing struct.Tests
Compile verification (from #256) typechecks every fixture's generated code. New regressions:
mock_generatedForFullyLazyInstantiationCycle— 3-node A↔B↔C↔A, now with compile verification enabled (previously skipped).mock_generatedForFourNodeFullyLazyInstantiationCycle— A→B→C→D→A, SCC handling scales past 3 nodes.mock_generatedForCycleWithNonCycleSibling— A↔B plus A→C, pruning operates per-edge not per-node.mock_generatedForLazyCycleBeneathConstantChild— cycle descendant under the constant-wrapper recursion path.mock_generatedForErasedInstantiatorBackEdgeInLazyCycle—ErasedInstantiatorproperty as the selected back-edge.mock_generatedForCycleWithDefaultValuedNonDependencyParameter— labeled non-dep default on a cycle node.mock_generatedForCycleWithCustomMockAndLabeledDefaultOnCycleNode— customMock + labeled default combined.mock_generatedForRootSelfInstantiatorCycle— self-cycle root, pins that the config struct is still emitted.Also fixes the
AnyServicefixture inmock_usesMockOnly_whenProductionHasNoMockAndBothFulfillSameAdditionalType(was a concrete class two structs claimed to fulfill — switched to a protocol).Test plan
swift test --traits sourceBuild— 850 tests pass./CLI/lint.sh🤖 Generated with Claude Code