Fix InvalidProgramException for Nullable<T> tuple nil comparisons (#3626) - #3628
Merged
Merged
Conversation
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.
This fixes issue #3626. The reported symptom was
InvalidProgramExceptionin anasync, non-awaited
if letover an imported named tuple, but the actual rootcause is more general: any
tupleNullable != nil/== nilcomparison emitsinvalid IL (
ldfld <Nullable<Tuple>>; ldnull; ceq, verified by ilverify as aStackUnexpectederror) whenever the tuple'sClrTypeis symbolic/null —which happens whenever an element carries a nullable-reference-type
annotation (e.g.
string?), regardless of sync/async, same-compilation vs.imported, or named vs. unnamed tuple.
Root cause:
LiftedBinaryOperatorCollector.IsLiftedValueTypeBinaryinSlotPlanner.csdecided whether aT? == nilcomparison needs the liftedHasValue-based emission by probingUnderlyingType.ClrType.IsValueTypedirectly, instead of using the canonical
NullableLifting.IsValueTypeNullablepredicate (which already special-cases
TupleTypeSymbolfor exactly thisreason). When the probe missed, the comparison fell through to a generic
ldnull; ceqpath — invalid for a value-typeNullable<T>. The follow-onEmitLiftedNullableBinary/GetNullableHasValueRef/GetNullableValueRefemit helpers had the same class of bug: they dereferenced
UnderlyingType.ClrType(or gated on the narrowerIsUserValueTypeNullable) instead of the broaderNullableLifting.RequiresSymbolicNullableGetValue, which already routessymbolic tuples through the existing symbolic
get_HasValue/get_ValueMemberRef construction used elsewhere (null-conditional receiver probe,
(v!!)unwrap).Fix: swap the ad hoc
ClrType-based checks for the existing canonicalNullableLiftingpredicates in three places:SlotPlanner.cs:IsLiftedValueTypeBinarynow usesNullableLifting.IsValueTypeNullable.MethodBodyEmitter.Operators.cs:EmitLiftedNullableBinary's nil-compareform now resolves
get_HasValuethroughGetNullableHasValueRefinsteadof dereferencing
ClrTypedirectly;GetNullableHasValueRef/GetNullableValueRefnow gate onRequiresSymbolicNullableGetValueinstead of
IsUserValueTypeNullable.Tests: added
test/Compiler.Tests/Emit/Issue3626NullableTupleNilComparisonTests.cscovering (1) a same-compilation sync
if letwith both the match and nilbranches, (2) the original async/non-awaited/imported/named-tuple repro, and
(3) the same with an unnamed tuple (confirming names are not load-bearing).
All three compile+ilverify+run and fail without the fix. Also ran the
existing Nullable (600), Async (272), Tuple (89), and Lifted/IfLet (79)
scoped test suites — all pass with no regressions.