Fix #831: open-T == nil emits verifiable IL - #839
Merged
Conversation
Issue #831. The emitter naively produced `ldarg.0; ldnull; ceq` for `self == nil` when `self` had a binder-typed type parameter shape (e.g. `T?` over `[T class]` or unconstrained T). The runtime JIT tolerated it, but `ilverify` rejected the IL with `[StackUnexpected]: found Nullobjref ... expected value 'T'` because `ceq` cannot match an opaque `!!T` stack slot against a managed-null reference at the verifier layer. The same gap existed for `?:` (NullCoalesce) and `!!` (NullAssertion) over open-T operands, which both used `dup; brtrue` patterns the verifier also rejects on opaque type-parameter stack values. Eight `Gsharp.Extensions.Optional` helpers (`Map`, `FlatMap`, `OrElse`, `OrCompute`, `OrThrow`, `IfPresent`, `Filter`, plus a value-type `FlatMap` partner) tripped the gate. Root cause: the existing `liftedBinarySlots` / `NullableValueTypeUnwrapCollector` / `NullableValueTypeCoalesceCollector` paths gated on a static `ClrType`, which open type parameters never have, so every shape over an open T fell through to the bottom `ldnull; ceq` / `dup; brtrue` opcodes. Fix (emit-only — no new BoundNodeKind): * `MethodBodyEmitter.Operators.cs`: * For `T? == nil` / `T? != nil` over an open T (any constraint), emit `box <T?>; ldnull; ceq` and append `ldc.i4.0; ceq` for the `!=` lobe. ECMA-335 III.4.1 specifies that `box` of a `Nullable<T>` yields a managed-null reference when HasValue is false, so the same shape works uniformly for class-, struct-, and unconstrained-T (the JIT elides the box when T resolves to a reference type). * For `T? ?: rhs` over a class/unconstrained-T LHS, spill the LHS to a `T`-typed slot and probe via `box !!T; brfalse fallback`, reloading the slot on the non-null path. * For `!!` over a bare `T` or `T?` operand (class/unconstrained T), spill to a `T`-typed slot and probe via `box !!T; brtrue nonNull`, throwing NullReferenceException on null and reloading the slot on non-null. Handles both the un-narrowed `self T?` shape and the smart-cast narrowed bare-`T` shape produced after an `if self == nil { return }` guard. * `SlotPlanner.cs`: * Extend `NullableValueTypeUnwrapCollector` and `NullableValueTypeCoalesceCollector` to pre-allocate slots for the new open-T paths (typed as `!!T`). Tests (`Issue831OpenTNilCompareEmitTests`, 7 cases): * `OpenTNilCompare_ClassConstrained_RoundTrip_Verifiable` — the original Optional.Map repro. * `OpenTNilCompare_ClassConstrained_NotEquals_Verifiable` — exercises the `!= nil` negation lobe. * `OpenTNullCoalesce_ClassConstrained_Verifiable` — Optional.OrElse repro for `self ?: defaultValue`. * `OpenTNullAssertion_ClassConstrained_NonNull_Verifiable` — smart- cast unwrap after a nil-guard. * `OpenTNullAssertion_ClassConstrained_NullThrows_Verifiable` — confirms NRE still raised on null inputs. * `OpenTNilCompare_StructConstrained_PassesIlverify` — verifies the same box-and-compare guard handles struct-constrained T cleanly. * `OpenTNilCompare_BothOverloads_DispatchedCorrectly` — dual-overload dispatch end-to-end. Each test compiles with gsc, gates the PE through `IlVerifier.Verify`, then runs it via `dotnet exec` and asserts on stdout. `Gsharp.Extensions.Optional` helpers now emit zero ilverify errors (only the pre-existing unrelated `Sequences.Empty()` error remains, tracked separately). Closes #831 Related: #806, ADR-0084, parent #706
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
2 participants
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
Fixes the emitter to produce verifier-clean IL for nil-related operations on open type-parameter operands (
T?, bareTafter smart-cast narrowing). Pre-fix the JIT tolerated the IL at runtime butilverifyrejected it — eightGsharp.Extensions.Optionalhelpers (the SDK migration introduced by #806) tripped the gate.Root cause
The existing slot-allocation/emit paths for nullable-related ops (
liftedBinarySlots,NullableValueTypeUnwrapCollector,NullableValueTypeCoalesceCollector) gated on the operand's staticClrType. Open type parameters have noClrType, so every shape over an open T fell through to the bottom ofEmitBinary/EmitUnaryand produced:ldarg.0; ldnull; ceqforself == nil—[StackUnexpected]: found Nullobjref ... expected value 'T'dup; brtrueforself ?: rhs—Unexpected type on the stackfordupover an opaque!!Tdup; brtrueforself!!— same root causeFix (emit-only, no new BoundNodeKind)
src/Core/CodeAnalysis/Emit/MethodBodyEmitter.Operators.cs:T? == nil/!= nilover any open-T constraint: emitbox <T?>; ldnull; ceq(withldc.i4.0; ceqfor the!=lobe). Per ECMA-335 III.4.1,box Nullable<T>yields a managed-null reference whenHasValueisfalse, so the same shape works uniformly for class-, struct-, and unconstrained-T. The JIT elides the box for reference-typed T.T? ?: rhsover class/unconstrained T: spill the LHS to aT-typed slot, probe viabox !!T; brfalse fallback, reload the slot on the non-null path.!!over class/unconstrained T (both bareTandT?shapes): spill to aT-typed slot, probe viabox !!T; brtrue nonNull, throw NRE on null, reload slot on non-null.src/Core/CodeAnalysis/Emit/SlotPlanner.cs:NullableValueTypeUnwrapCollectorandNullableValueTypeCoalesceCollectorto pre-allocate!!T-typed slots for the new open-T paths.Tests
New file
test/Compiler.Tests/Emit/Issue831OpenTNilCompareEmitTests.cs(7 cases) — each compiles a sample withgsc, gates the PE throughIlVerifier.Verify, and runs it underdotnet execasserting stdout:OpenTNilCompare_ClassConstrained_RoundTrip_VerifiableOptional.MapreproOpenTNilCompare_ClassConstrained_NotEquals_Verifiable!= nilnegation lobeOpenTNullCoalesce_ClassConstrained_Verifiableself ?: defaultValue(OrElse)OpenTNullAssertion_ClassConstrained_NonNull_Verifiableself!!after guardOpenTNullAssertion_ClassConstrained_NullThrows_VerifiableOpenTNilCompare_StructConstrained_PassesIlverify[T struct]open-TNullable<T> == nilOpenTNilCompare_BothOverloads_DispatchedCorrectlyVerification
dotnet test GSharp.sln— all 5,286 tests pass (Core 3262, Compiler 1326, Interpreter 296, LanguageServer 266, Extensions 104, Sdk 32; 1RegenerateBaselineskipped).ilverifyonGsharp.Extensions.dll— 8 Optional-helper errors → 0. Only the pre-existing unrelatedSequences.Empty()error remains (out of scope for this PR).cd website && npm ci && npm run build— clean build, no broken links.Links
Closes #831
Related: #806 (SDK migration that surfaced the gap), ADR-0084, parent #706