Fix closure-capture resolution for lambdas, loop scopes and mixed field captures (#333) - #343
Open
DJGosnell wants to merge 6 commits into
Open
Fix closure-capture resolution for lambdas, loop scopes and mixed field captures (#333)#343DJGosnell wants to merge 6 commits into
DJGosnell wants to merge 6 commits into
Conversation
…ld captures (#333) A Quarry chain written inside a lambda emitted an interceptor that referenced the enclosing method's locals directly, failing to compile with CS0103 in generated code. Root-causing that surfaced two further capture-resolution defects that failed silently at runtime, so this fixes three related bugs and guards a fourth that cannot be fixed. 1. Sites inside a lambda were never enriched. DisplayClassEnricher unwrapped only MethodKind.LocalFunction, so an enclosing lambda's AnonymousFunction symbol was not unwrapped, ComputeMethodOrdinal returned -1 and the site was skipped entirely. No extraction plan was built, so the raw captured-local name was emitted. That is the CS0103. Contrary to the issue title the trigger is ANY enclosing lambda that is an invocation argument, at depth 1 or deeper; `new Func<>(lambda)` only appeared to work because it trips the QRY032 lambda-capture disqualifier first and masks the bug. 2. Declarations that own a scope were mis-scoped. FindDeclaringScope walked a variable up to the nearest enclosing block, so a lambda parameter, or a foreach/for/using/switch declaration, resolved to the enclosing method scope. Verified against emitted IL: those each get their OWN display class, and a lambda's parameters share one class with its body-block locals. The collision shifted every later closure ordinal, which broke chains capturing a loop variable and a method local in separate clauses. Single-scope chains passed only because ordinal 0 is correct by accident. 3. An instance field mixed with a local was read off the wrong object. With only a field captured the delegate target is the containing instance; add a local and the compiler interposes a display class holding <>4__this. Now emits a <>4__this accessor returning ref TContaining and reads the field from that. This hop IS expressible because <>4__this's type is the user's own class and needs no [return: UnsafeAccessorType]. 4. A clause capturing locals from two or more distinct closure scopes is now disqualified at build time with a QRY032 naming the shape and the workaround, instead of throwing MissingFieldException/InvalidCastException on first execution. Reaching the outer scope needs the CS$<>8__locals link field, whose type is another display class; a field accessor must return byref and a byref return cannot name an inaccessible type (dotnet/runtime#119664, open/Future). The Unsafe.As overlay alternative is undefined behaviour (dotnet/runtime discussion #111049) and its failure mode is silent wrong values, so it was rejected. The recommended workaround -- splitting into separate .Where(...) clauses -- works because of fix 2. The scope count deliberately ignores variables declared inside the clause: a nested subquery lambda contributes its own parameters to CapturedInside, and counting those made the guard reject working nested-subquery and set-operation chains. Tests: LambdaCaptureScopeTests asserts the emitted interceptor compiles and extracts; LambdaCaptureExecutionTests runs each shape against SQLite, because a wrong display-class prediction still compiles and only fails when executed.
…333) The named RunâWorkerAsync methods existed only to dodge the #333 build break. With capture resolution fixed they go back to inline lambdas, which also makes that suite exercise capture resolution inside a lambda under real concurrency rather than just the shared runtime state it was written for. Docs: - llm-testing.md: replace the "doubly-nested lambda does not compile" gotcha with the actual rule (a clause may capture from one closure scope), the QRY032 it now produces, and the split-into-separate-Where workaround. Adds the trap that a capture-probing test must use an invocation-argument lambda, since new Func<>(lambda) is disqualified before capture resolution runs and would silently prove nothing. - Generator llm.md: ground-truth table mapping each source shape to the display classes the compiler actually emits, the <>4__this rule, and why multi-scope is rejected rather than emitted, with the upstream references. Notes that a wrong prediction still compiles and only fails on execution, which is why LambdaCaptureExecutionTests exists.
#333) Code review found 26 items; the high-severity ones were each reproduced with a throwaway test before being accepted, and three were real breaks in this branch: - CS0111: the <>4__this hop accessor was named after the containing type, so two clauses on one chain that each mix an instance field with a local emitted it twice with identical signatures. Named per clause instead, matching the existing __ExtractVar_{name}_{clauseIndex} convention. - CS0122/CS0305: the hop emits the containing type as a real type name, which is impossible for a generic type (no type parameters in scope on a file-scoped carrier) or one not visible to generated code. The plan called for a fallback that was never implemented; such chains are now disqualified with a specific reason instead of emitting a build break. - catch-clause variables own a display class, like foreach/for/using. Omitting them mispredicted the ordinal AND made a catch-variable-plus-method-local clause look single-scope, so the multi-scope guard did not fire and the shape failed at execution. Confirmed by reproduction, then added to IsOwnScopeStatement. Also: LookupClosureOrdinal now shares the "declared outside the clause" filter with CountCaptureScopes via IsExtractableCapture, so the two cannot disagree about which scopes a clause reads from. Tests. The codegen suite asserted only "no CS0103", which is why it caught none of the above; it now asserts the generated code has no errors at all, excluding two documented harness-only diagnostic ids. That change immediately surfaced a genuinely missing assembly reference in the harness. The nested-subquery filter test never reached the filter it named (the inner lambda captured nothing), so it passed with or without the code it pinned. Added coverage for for/using/ switch-section/catch scopes, two field+local clauses on one chain, and both rejected containing-type shapes. Docs. analyzer-rules.md documents both capture limits under QRY032 with before/after examples; llm.md adds the catch row and flags switch-expression arms as a known unhandled form, since their field name is mangled and adding them to the scope rule alone would trade one wrong prediction for another. ConcurrencyTests records that its harness deconstruction is load-bearing (#338). Filed #341 (Update().Set computed-expression path, verified pre-existing) and #342 (switch-expression arms, guard input, unanalysed sentinel).
…pendent (#344) Inlining those worker bodies was planned, done, and green locally — then CI failed with TypeLoadException from the same commit. The prediction was identical on both machines; the compiler's answer was not: predicted <>c__DisplayClass5_3 (both) emitted <>c__DisplayClass5_3 SDK 10.0.110 (local) -> passes emitted <>c__DisplayClass5_1 SDK 10.0.302 (CI) -> TypeLoadException The method ordinal matched; only the closure ordinal diverged. The affected shape is an async lambda inside a loop whose clause captures a local. A sibling test with the same lambda nesting but a clause capturing nothing is unaffected, since no display class is named. The repo pins no SDK, so this can differ between a contributor's machine and CI with no code change. Restoring named worker methods makes the captures ordinary method locals, which predict stably, and the fixture now records why with the concrete evidence rather than the previous "inline these once #333 is fixed" note. This does not affect the rest of the branch: LambdaCaptureScopeTests and LambdaCaptureExecutionTests — chains inside single and nested lambdas, foreach/for/ using/switch-section/catch scopes, and instance fields mixed with locals — pass on both SDKs. Filed #344; llm.md and llm-testing.md record the fragility next to the ground-truth tables that were verified stable.
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
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
A Quarry chain written inside a lambda emitted an interceptor that referenced the enclosing method's
locals directly, so the generated file failed to compile with
CS0103— a build break inside generatedcode, with no Quarry diagnostic. Root-causing it surfaced three further capture-resolution defects that
failed silently at runtime, plus one shape that cannot be supported at all. This fixes four and rejects
the fifth at build time.
Reason for Change
The generator predicts compiler-generated display-class names in order to emit
[UnsafeAccessor]extractors for captured variables without reflection. Several of those predictions were wrong:
Sites inside a lambda were never enriched.
DisplayClassEnricherunwrapped onlyMethodKind.LocalFunction, so an enclosing lambda'sAnonymousFunctionsymbol was left in place,ComputeMethodOrdinalreturned-1, and the site was skipped entirely — no extraction plan, so theraw captured-local name was emitted. That is the CS0103.
The issue title says "doubly-nested", but the trigger is any enclosing lambda that is an
invocation argument, at depth 1 or deeper.
new Func<>(lambda)only appeared to work because it tripsthe QRY032 lambda-capture disqualifier first, which masks the bug.
Declarations that own a scope were mis-scoped. A lambda parameter, or a
foreach/for/using/switch-section/catchdeclaration, resolved to the enclosing block. Verifiedagainst emitted IL: each of those owns its own display class, and a lambda's parameters share one
class with its body-block locals. The collision shifted every later closure ordinal. Single-scope
chains passed only because ordinal 0 is correct by accident — the bug was invisible until a method had
two capture scopes.
An instance field mixed with a local was read off the wrong object. With only a field captured the
delegate target is the containing instance; add a local and the compiler interposes a display class
holding
<>4__this. The generator now emits a<>4__thisaccessor returningref TContainingandreads the field from that.
A clause capturing locals from two or more closure scopes cannot be emitted at all and is now
rejected at build time. The outer scope is reachable only through the compiler's
CS$<>8__localslinkfield, whose type is another display class — and a field accessor must return byref while a byref
return cannot name an inaccessible type
(dotnet/runtime#119664, open, milestone
Future,deliberately excluded as not memory safe).
The
Unsafe.Asshadow-overlay alternative was implemented and rejected: it is undefined behaviour(dotnet/runtime#111049 — display classes hold
reference fields, so they are non-blittable, get
Autolayout and have no guaranteed offsets), and itsfailure mode is silent. With two same-typed fields a mismatched overlay returns the values swapped,
binding
@p0to the wrong variable and returning wrong rows with no error at all.Impact
Shapes that previously threw at runtime now work:
One shape that previously compiled and then failed at runtime now fails at build time — see Breaking
Changes.
Plan items implemented as specified
AnonymousFunctionwhen resolving the enclosing method.foreach/for/using/switch-sectiondeclarations to their own scope.
<>4__thishop for instance fields captured alongside locals.src/Quarry.Generator/llm.md.Deviations from plan implemented
The original plan's steps 2–6 were abandoned. They assumed chained display-class access was
buildable. Step 1 was written as a gate specifically to test that, and it failed — four
[UnsafeAccessor]signature shapes were tried and all were rejected, a result that matches the upstream issue above. The
plan was rewritten against measured behaviour rather than continuing on the assumption.
The
ConcurrencyTestsworkaround was NOT reverted after all. Inlining those worker bodies wasplanned and done, and it passed locally — then failed in CI with
TypeLoadException. Cause: for thatspecific shape (an async lambda inside a loop whose clause captures a local) the predicted closure
ordinal is not stable across compiler versions —
<>c__DisplayClass5_3under SDK 10.0.110 versus<>c__DisplayClass5_1under 10.0.302, from identical source. The named worker methods are restored,with the evidence recorded in the fixture and in
llm-testing.md, and the fragility filed as Display-class closure ordinal prediction is not stable across C# compiler versions (TypeLoadException at runtime) #344.Everything else in this PR is unaffected: the
LambdaCapture*suites pass on both SDKs."Pick the innermost captured scope as the Target" was dropped in favour of just counting distinct
scopes. Since every genuinely multi-scope clause is now rejected, surviving clauses capture from exactly
one scope, where the existing first-match lookup is already correct. Smaller and lower-risk.
Gaps in original plan implemented
catch-clause variables. Not in the original plan; found in review. They own a display class, so acatch variable plus a method local looked single-scope and the guard did not fire — the safety
invariant was defeated for that shape and it failed at runtime.
<>4__thishop. The hop needs the containing type as areal type name, which is impossible for a generic type (CS0305) or one not visible to generated code
(CS0122). Both were reproduced; such chains are now disqualified instead.
chain each mixing a field with a local emitted it twice — CS0111 in generated code. Reproduced, then
named per clause.
docs/articles/analyzer-rules.mdnow documents both capture limits underQRY032 with before/after examples.
Migration Steps
Only for code hitting the new build error. Split a clause that captures across scopes:
Or copy the outer value into a local in the inner scope. For a field on a generic or inaccessible type,
copy it into a local before the chain.
Performance Considerations
No runtime cost change on the generated hot path — the same
[UnsafeAccessor]extraction, plus one extrafield read for the
<>4__thishop where it applies. Carriers that differ only in hop path no longer merge(
CapturedVariableExtractorequality feedsCarrierStructuralKey), so generated output grows slightlyfor the field-plus-local shape; deliberate, since merging them would reintroduce the #268 failure mode.
Security Considerations
None. Compile-time source generator with no new inputs or external surface. The
Unsafe.Asapproach wasrejected partly on memory-safety grounds, so no undefined behaviour is introduced.
Breaking Changes
(QRY032) instead of compiling and throwing
MissingFieldException/InvalidCastExceptionat execution.Likewise a clause capturing an instance field alongside a local when the containing type is generic or
inaccessible. Both previously produced silently broken builds, so this converts a runtime failure into a
build-time one — but code that "compiled" before may now stop compiling. Documented in
docs/articles/analyzer-rules.md.RawCallSitegainsCapturedScopeCountandThisIndirectionUnavailable(both excluded fromEquals/GetHashCode, consistent with the other enricher-set members);CapturedVariableExtractorgains
ThisIndirectionDisplayClassandThisHopMethodName, both included in equality.Follow-ups filed
t.Lite.Users()) is emitted against the wrong context(CS9144/CS0029). Pre-existing; reproduced with this branch's generator stashed.
UnsafeAccessorTypeAttributesupport for field accessors dotnet/runtime#119664.Update().Set(...)computed expression emits an invalid accessor.Pre-existing; reproduced with this branch's generator stashed.
an explicit "unanalysed" sentinel.
shapes. Found by this PR's own CI; pre-existing in the prediction approach, not introduced here.