Fix #3824: fold a hoisted argument null-guard at the ILAst level#3852
Merged
Conversation
siegfriedpammer
force-pushed
the
fix-ctor-initializer-nullguard-ilast
branch
from
a9151c5 to
0ab8a20
Compare
When a chained constructor-call argument contains a throwing null-check (e.g. `value?.Length ?? throw ...`) and the argument is evaluated more than once, the compiler hoists the null-check in front of the chained call. The hoisted `if (value == null) throw ...;` then became the first body statement, so MoveConstructorInitializer could not recognize the chained call and left it as an illegal in-body `base..ctor(...)` / `this..ctor(...)` (a parse error). Fix it in the ILAst, where the `?? throw` shape already lives, rather than re-deriving it on the C# AST: NullCoalescingTransform folds a guard that directly precedes the chained call back into the first use of the parameter as `if.notnull(ldloc param, throw)`. Nothing in a constructor body can legally run before the chained call, so a statement preceding it is necessarily compiler-hoisted; matching is by ILVariable identity, not parameter name. The guard disappears before the AST transforms run, so they need no change. Reference types chain via a base/this..ctor CallInstruction; value types chain via `this = new TSelf(...)`, i.e. stobj(ldthis, newobj TSelf(...)), which ChainedConstructorCallILOffset does not report -- so the value-type chain (including the case where this is spilled to a stack slot because the guard sits between its load and the call) is matched directly. Assisted-by: Claude:claude-opus-4-8:Claude Code
A type's leading field assignments are extracted to field declarations only when every constructor that does not chain with this() agrees on them. When they disagree, the analyzer gave up on the whole type, so the remaining constructors' this()/base() calls were never lifted into initializers -- a struct with two divergent constructors plus a chaining one rendered the chain as `this = new TSelf(...)` instead of `: this(...)`. Chain lifting does not depend on the shared-initializer extraction, so a mismatch now just skips the extraction (the assignments stay in the bodies) and the transform continues. Primary constructors still bail, since their parameters drive the initializers that must be extracted. Assisted-by: Claude:claude-opus-4-8:Claude Code
siegfriedpammer
force-pushed
the
fix-ctor-initializer-nullguard-ilast
branch
from
b6b7e8a to
9a4f501
Compare
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.
Fixes #3824. Supersedes #3829 with an ILAst-level implementation.
When a chained constructor-call argument contains a throwing null-check (e.g.
value?.Length ?? throw ...) and the argument is evaluated more than once, the compiler hoists the null-check in front of the chained call. The hoistedif (value == null) throw ...;became the first body statement, soMoveConstructorInitializercould not recognize the chained call and left it as an illegal in-bodybase..ctor(...)/this..ctor(...)(a parse error).Instead of re-deriving the pattern on the C# AST, the fix folds the guard back at the ILAst level, where the
?? throwshape already lives:NullCoalescingTransformfolds a guard that directly precedes the chained call back into the first use of the parameter asif.notnull(ldloc param, throw). Nothing in a constructor body can legally run before the chained call, so a statement preceding it is necessarily compiler-hoisted; matching is byILVariableidentity, not parameter name. The guard disappears before the AST transforms run, so they need no change.Value types chain via
this = new TSelf(...)(stobj(ldthis, newobj ...)) rather than abase/this..ctorCallInstruction, whichChainedConstructorCallILOffsetdoes not report, so the value-type chain is matched directly -- including the case wherethisis spilled to a stack slot because the guard sits between its load and the call.The second commit fixes a related lifting bug this surfaced: when a type's non-chaining constructors disagree on their leading field assignments, the analyzer gave up on the whole type, so the remaining constructors'
this()/base()calls were never lifted into initializers. A field-initializer mismatch now only skips the shared-initializer extraction; chain lifting continues.Known limitation (unchanged from the status quo and shared with #3829): with throw expressions disabled (C# < 7) there is no compilable single-expression form for these constructors.
🤖 Generated with Claude Code