Macro: diagnose Self.* refs in default expressions#268
Merged
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: 3b86f8e186
ℹ️ 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 #268 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 41 41
Lines 6515 6635 +120
==========================================
+ Hits 6515 6635 +120
🚀 New features to boost your workflow:
|
dfed
added a commit
that referenced
this pull request
Apr 19, 2026
SafeDI threads dependency arguments explicitly at every mock call site (see `generateReturnArgumentList` and the `.dependency` branch in `resolveBuilderArguments`), so a dependency parameter's default expression never reaches a caller's override struct — `Self.*` inside a dep default resolves correctly in the callee. Filter dependency labels out of `validateDefaultExpressions`, matching the pattern `validateUnlabeledDefaultShape` already uses. Adds `producesNoDiagnostic_whenDefaultExpressionReferencesSelfOnDependencyParameter` to pin the exemption. Addresses codex review on #268. 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: fb5b0979b3
ℹ️ 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
added a commit
that referenced
this pull request
Apr 19, 2026
Two refinements to the callee-scope default-expression check: 1. `_`-labeled non-dependency defaults are exempt. `ScopeGenerator` already elides them from call-site arguments (`isElidedInCallSite`) and excludes them from promoted mock defaults (`rootDefaultParameters` skips `label == "_"`), so Swift's default-argument thunk fires in the callee's scope and `Self.*` resolves correctly. The diagnostic was a false-positive for these shapes. 2. Drop the `self.` check. Swift rejects `self.` in default expressions on every parameter shape SafeDI encounters: `init` (self isn't constructed yet) and static functions (no instance). The `self.` branch in `SelfReferenceFinder` was unreachable. Adds `producesNoDiagnostic_whenDefaultExpressionReferencesSelfOnUnlabeledNonDependency` to pin the `_`-labeled exemption. Addresses codex review on #268. 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: e6dae4cbee
ℹ️ 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
changed the title
dfed
added a commit
that referenced
this pull request
Apr 19, 2026
`validateDefaultExpressions` was fed `extensionDependencies` — the union of dependencies across every `Instantiable` produced by the extension. When an extension produces multiple Instantiables with different `concreteInstantiable` values (possible when `instantiate` overloads have different return types, even though the macro also flags `incorrectReturnType` on the non-matching overload), a label that is a non-dependency for one overload's customMock but a dependency for another's instantiate was misclassified as a dep, skipping the Self-ref check even though the default would still be promoted on the mock surface. Resolve each mock function's deps by matching its return type against the corresponding Instantiable (same pattern already used at the `mockMethodMissingArguments` check), with `extensionDependencies` as a safe fallback for return types that don't match any Instantiable. Addresses codex review on #268. Constructing a clean multi-overload regression test requires fixtures that also trip `incorrectReturnType`, so no new test; the existing extension-macro suite covers the single-overload path this PR was originally exercising. 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: 54708e1cda
ℹ️ 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
added a commit
that referenced
this pull request
Apr 19, 2026
When a type has a `customMock` method (`visitor.mockInitializer != nil`), SafeDI uses the mock method — not the init — for mock construction (see `generateMockRootCode`). Init defaults never reach the override surface in that case, so `init(name: String = Self.defaultName)` is a safe pattern and the diagnostic was a false-positive. Gate `validateDefaultExpressions` on the concrete init path to run only when no custom mock is declared. The custom mock's own defaults are still validated on the separate mock-syntax path. Adds `producesNoDiagnostic_whenInitDefaultReferencesSelfButCustomMockExists`. Addresses codex review on #268. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ 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". |
dfed
added a commit
that referenced
this pull request
Apr 20, 2026
Three issues surfaced during self-review:
1. **P1** — `SelfReferenceFinder` only caught `Self.member` access.
Bare `Self` in other positions — `Self()` constructor calls, `Self`
as a function argument, `Self[…]` subscripts — was missed, silently
permitting the same override-surface bug. Added a
`DeclReferenceExprSyntax` visitor that reports any `Self` not
already captured by the member-access visitor (which now returns
`.skipChildren` to prevent double-reporting).
2. **P2** — the per-overload dep lookup on extension mocks used
`mockFunction.signature.returnClause?.type.typeDescription`
directly. For `Self`-returning methods this never matched any
`Instantiable`'s `concreteInstantiable` (the visitor records the
actual extended type), so it fell back to the union across
overloads. Map `Self` to the extended type before the lookup.
3. **P2** — added two new tests:
- `producesDiagnostic_whenDefaultExpressionCallsSelfAsConstructor`
covers the bare-`Self` case (`Self()`).
- `producesDiagnostic_whenExtensionCustomMockDefaultReferencesSelf`
covers the extension-path validation branch, which was previously
exercised only indirectly by the fixes in 54708e1 / 66f788c.
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: 499d892e2c
ℹ️ 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".
Default expressions on `@Instantiable` init and customMock parameters are evaluated in the callee's lexical scope during production construction, but SafeDI's generated mock surface may surface the default on an override struct in another type's extension — where `Self` binds to the caller's type, producing the wrong value or failing to typecheck. Walk each default-value expression's syntax tree and diagnose explicit `Self.member` and `self.member` member access. The diagnostic is emitted on the parameter so the IDE points users at the exact site; users can move the referenced value to a file-scoped or module-scoped symbol, or drop the default entirely. Unqualified references to callee-scope members can't be distinguished from module-scoped symbols without type resolution, so the macro does not cover them. The existing stripping workaround in `ScopeGenerator.callSiteArguments` / `hasHiddenUnlabeledDefaults` stays in place to handle those cases. Hooked into all three validation sites: concrete init, concrete custom mock, and extension custom mock methods. Extension `instantiate()` arguments are always dependencies, so their defaults never reach the mock surface — no check needed there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SafeDI threads dependency arguments explicitly at every mock call site (see `generateReturnArgumentList` and the `.dependency` branch in `resolveBuilderArguments`), so a dependency parameter's default expression never reaches a caller's override struct — `Self.*` inside a dep default resolves correctly in the callee. Filter dependency labels out of `validateDefaultExpressions`, matching the pattern `validateUnlabeledDefaultShape` already uses. Adds `producesNoDiagnostic_whenDefaultExpressionReferencesSelfOnDependencyParameter` to pin the exemption. Addresses codex review on #268. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two refinements to the callee-scope default-expression check: 1. `_`-labeled non-dependency defaults are exempt. `ScopeGenerator` already elides them from call-site arguments (`isElidedInCallSite`) and excludes them from promoted mock defaults (`rootDefaultParameters` skips `label == "_"`), so Swift's default-argument thunk fires in the callee's scope and `Self.*` resolves correctly. The diagnostic was a false-positive for these shapes. 2. Drop the `self.` check. Swift rejects `self.` in default expressions on every parameter shape SafeDI encounters: `init` (self isn't constructed yet) and static functions (no instance). The `self.` branch in `SelfReferenceFinder` was unreachable. Adds `producesNoDiagnostic_whenDefaultExpressionReferencesSelfOnUnlabeledNonDependency` to pin the `_`-labeled exemption. Addresses codex review on #268. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`validateDefaultExpressions` was fed `extensionDependencies` — the union of dependencies across every `Instantiable` produced by the extension. When an extension produces multiple Instantiables with different `concreteInstantiable` values (possible when `instantiate` overloads have different return types, even though the macro also flags `incorrectReturnType` on the non-matching overload), a label that is a non-dependency for one overload's customMock but a dependency for another's instantiate was misclassified as a dep, skipping the Self-ref check even though the default would still be promoted on the mock surface. Resolve each mock function's deps by matching its return type against the corresponding Instantiable (same pattern already used at the `mockMethodMissingArguments` check), with `extensionDependencies` as a safe fallback for return types that don't match any Instantiable. Addresses codex review on #268. Constructing a clean multi-overload regression test requires fixtures that also trip `incorrectReturnType`, so no new test; the existing extension-macro suite covers the single-overload path this PR was originally exercising. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When a type has a `customMock` method (`visitor.mockInitializer != nil`), SafeDI uses the mock method — not the init — for mock construction (see `generateMockRootCode`). Init defaults never reach the override surface in that case, so `init(name: String = Self.defaultName)` is a safe pattern and the diagnostic was a false-positive. Gate `validateDefaultExpressions` on the concrete init path to run only when no custom mock is declared. The custom mock's own defaults are still validated on the separate mock-syntax path. Adds `producesNoDiagnostic_whenInitDefaultReferencesSelfButCustomMockExists`. Addresses codex review on #268. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three issues surfaced during self-review:
1. **P1** — `SelfReferenceFinder` only caught `Self.member` access.
Bare `Self` in other positions — `Self()` constructor calls, `Self`
as a function argument, `Self[…]` subscripts — was missed, silently
permitting the same override-surface bug. Added a
`DeclReferenceExprSyntax` visitor that reports any `Self` not
already captured by the member-access visitor (which now returns
`.skipChildren` to prevent double-reporting).
2. **P2** — the per-overload dep lookup on extension mocks used
`mockFunction.signature.returnClause?.type.typeDescription`
directly. For `Self`-returning methods this never matched any
`Instantiable`'s `concreteInstantiable` (the visitor records the
actual extended type), so it fell back to the union across
overloads. Map `Self` to the extended type before the lookup.
3. **P2** — added two new tests:
- `producesDiagnostic_whenDefaultExpressionCallsSelfAsConstructor`
covers the bare-`Self` case (`Self()`).
- `producesDiagnostic_whenExtensionCustomMockDefaultReferencesSelf`
covers the extension-path validation branch, which was previously
exercised only indirectly by the fixes in 54708e1 / 66f788c.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`SelfReferenceFinder.visit(MemberAccessExprSyntax)` has two paths: `Self.member` (reported and returns `.skipChildren`) and the non-Self fallback (`return .visitChildren`). All existing tests exercised only `Self.*` defaults, leaving the fallback uncovered — codex project coverage fell below 100%. Add `producesNoDiagnostic_whenDefaultExpressionReferencesMemberOfModuleType` with a `Config.fallback` default. The default must pass through the visitor (walking the MemberAccessExpr) without firing the diagnostic, hitting the `.visitChildren` branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex P2 on #268: `SelfReferenceFinder` only saw expression-level `Self` (MemberAccess and DeclReference). Defaults whose types embedded `Self` — `Optional<Self>.none`, `Result<Self, E>.failure(...)`, `[Self]` — slipped through, so the override-struct's caller-scope `Self` binding could still surface the wrong type. Adds `TypeDescription.containsSelf` — walks recursively through generics, wrappers, composed types, closures, tuples — and wires it into `SelfReferenceFinder.visit(IdentifierTypeSyntax)`. Using TypeDescription keeps the detection in one structural check rather than maintaining a syntax-kind-specific visitor per wrapper. Adds `producesDiagnostic_whenDefaultExpressionReferencesSelfInTypeExpression` to pin the fix against `Optional<Self>.none`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dfed
force-pushed
the
claude/reject-callee-scope-defaults
branch
from
499d892 to
e897502
Compare
Follows the existing pattern in `TypeDescriptionTests` — each case is exercised by parsing a real Swift source snippet and walking to the relevant TypeSyntax via the corresponding visitor, then asserting on the resulting `TypeDescription`'s `containsSelf` result. No enum-literal constructed fixtures. Covers every branch: `.simple`, `.nested` (Self-as-parent + Self-in-generics), `.optional`, `.implicitlyUnwrappedOptional`, `.some`, `.any`, `.array`, `.dictionary` (both key and value), `.tuple`, `.composition`, `.closure` (argument and return), `.metatype`, `.attributed`, `.unknown`, `.void`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the three-visit-method finder (MemberAccessExprSyntax, DeclReferenceExprSyntax, IdentifierTypeSyntax) with two visits keyed off a single signal: `containsSelfReference: Bool`. - Expression-level `Self` (bare `Self`, `Self()`, and `Self.foo`'s inner `Self`) is caught by matching `DeclReferenceExprSyntax` with name `Self`. - Type-level `Self` (`Optional<Self>`, `[Self]`, `(Self, T)`, etc.) is caught by parsing the `IdentifierTypeSyntax` into a `TypeDescription` and asking `containsSelf`, which already recurses through generics, wrappers, tuples, closures, and composed types. Dropping the `MemberAccessExprSyntax` visit means the diagnostic no longer spells the specific member (`Self.defaultName`) — it just says `Self` — but the user-facing fix-it is unchanged, and the visitor becomes substantially simpler to reason about. Removes the `reference` associated value from `calleeScopeReferenceInDefaultExpression` and updates all test expectations. Coverage on the finder is now 100% (the previous `return .visitChildren` fallthrough was uncovered because no test fixture had a non-Self `IdentifierTypeSyntax`). 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: 94d5ca58df
ℹ️ 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".
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94d5ca58df
ℹ️ 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 flagged that `validateDefaultExpressions` ran inside the `for initializer in visitor.initializers` loop, so every initializer was diagnosed — including secondary/convenience inits that SafeDI never uses for mock construction. `InstantiableVisitor` selects the first init matching `isValid(forFulfilling: dependencies)` (see `InstantiableVisitor:377`), and `generateMockRootCode` reads that same init. Defaults on other inits never reach the override surface. Gate the Self-reference check to the selected init. `validateUnlabeledDefaultShape` still runs on every init — that shape is awkward regardless of use. Also pin two more type-expression forms against regression: - `[Self]()` — array generic argument - `Self.self` — bare Self plus `.self` metatype access Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
dfed
added a commit
that referenced
this pull request
Apr 20, 2026
…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>
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
1 participant
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
FixableInstantiableError.calleeScopeReferenceInDefaultExpressionand a syntax visitor inInstantiableMacrothat walks each default-value expression oninit/customMockparameters and diagnosesSelfreferences — bothSelf.membermember access and bareSelfin constructor calls / argument positions / subscripts._-labeled non-dependency parameters (elided from the call site byScopeGenerator).initdefaults when acustomMockmethod exists (the mock method takes over construction).Instantiables, each mock overload's deps are resolved from the matching return type (mappingSelfto the extended type) rather than the union across overloads.Self-review pass (commit
aab0c76)SelfReferenceFinderto catch bareSelf(constructor calls, argument positions, subscripts) via aDeclReferenceExprSyntaxvisitor. The member-access visitor returns.skipChildrento avoid double-reporting the base.Selfreturn types to the extended type (previously fell back to the union).producesDiagnostic_whenDefaultExpressionCallsSelfAsConstructorandproducesDiagnostic_whenExtensionCustomMockDefaultReferencesSelfto pin the new coverage.Why
Default expressions are evaluated in the callee's lexical scope in the production init, so
Self.defaultName/Self()binds to the decorated type. But SafeDI's generated mock code promotes labeled non-dependency defaults onto the caller'sSafeDIOverridesstruct — whereSelfbinds to the caller's type — producing the wrong value or failing to typecheck. Silent override-surface mismatch becomes a compile error instead.Scope limitation
Unqualified references (
foowherefoois a callee-scope member) cannot be distinguished from module-scoped symbols without type resolution, so the macro only catches explicitSelfaccess. The existing elision inScopeGenerator.callSiteArguments/hasHiddenUnlabeledDefaultsstays in place for_-labeled defaults — users who wantSelf.*defaults can opt in by making the parameter unlabeled or by supplying acustomMockmethod.Test plan
swift test --traits sourceBuild— 852 tests pass./CLI/lint.sh🤖 Generated with Claude Code