fix(emit): open-T nil compare / coalesce / unwrap emit verifiable IL (#839)

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

Co-authored-by: Copilot CLI <noreply@github.com>

Author: DavidObando

src/Core/CodeAnalysis/Emit/MethodBodyEmitter.Operators.cs

@@ -70,6 +70,49 @@ private void EmitUnary(BoundUnaryExpression u)
                 return;
             }
 
+            // Issue #831: `!!` on an open-type-parameter operand whose
+            // type is bare `T` or `T?` and where T is NOT
+            // struct-constrained. The bottom `dup; brtrue` path below
+            // is invalid IL — the verifier rejects `dup` / `brtrue`
+            // on an opaque `!!T` stack value because it cannot prove T
+            // is a reference type at the signature layer. Mirror the
+            // value-type spill: store the operand to a `T`-typed slot,
+            // probe its non-nullness via `box !!T; brtrue nonNull`,
+            // throw on the null path, and reload the slot on the
+            // non-null path. The JIT elides the box when T resolves to
+            // a reference type at runtime (ECMA-335 III.4.1). This
+            // catches both the un-narrowed `self!!` over `self T?` and
+            // the smart-cast `self!!` whose binder-typed operand has
+            // already collapsed to bare `T` after a null-check guard.
+            if (TryGetOpenTypeParameter(u.Operand.Type, out var tpOperand))
+            {
+                if (!this.receiverSpillSlots.TryGetValue(u, out var tpUnwrapSlot))
+                {
+                    throw new InvalidOperationException(
+                        "No scratch slot pre-allocated for class-constrained `T?` / open `T` '!!' operand — "
+                        + "check NullableValueTypeUnwrapCollector and the prepass in CollectLocalsAndLabels.");
+                }
+
+                var tpToken = this.outer.GetElementTypeToken(tpOperand);
+                var tpNonNull = this.il.DefineLabel();
+
+                this.EmitExpression(u.Operand);
+                this.il.StoreLocal(tpUnwrapSlot);
+                this.il.LoadLocal(tpUnwrapSlot);
+                this.il.OpCode(ILOpCode.Box);
+                this.il.Token(tpToken);
+                this.il.Branch(ILOpCode.Brtrue, tpNonNull);
+
+                var tpNreCtor = this.outer.wellKnown.GetNullReferenceExceptionCtorRef();
+                this.il.OpCode(ILOpCode.Newobj);
+                this.il.Token(tpNreCtor);
+                this.il.OpCode(ILOpCode.Throw);
+
+                this.il.MarkLabel(tpNonNull);
+                this.il.LoadLocal(tpUnwrapSlot);
+                return;
+            }
+
             this.EmitExpression(u.Operand);
             this.il.OpCode(ILOpCode.Dup);
             var nonNull = this.il.DefineLabel();
@@ -188,6 +231,52 @@ private void EmitBinary(BoundBinaryExpression b)
                 return;
             }
 
+            // Issue #831: NullCoalesce over a class-constrained (or
+            // unconstrained) `T?` whose underlying is an open type
+            // parameter. The underlying T has no static ClrType, so
+            // neither the value-type Nullable<T> spill branch above nor
+            // the bottom `dup; brtrue` shape is verifiable IL: the
+            // verifier rejects `dup` / `brtrue` on an opaque `!!T`
+            // stack value because it cannot prove T is a reference type
+            // at the signature layer (ECMA-335 III.1.8). Mirror the
+            // value-type spill: store the LHS to a `T`-typed slot, probe
+            // its non-nullness via `box !!T; brtrue/brfalse` (which the
+            // verifier accepts because `box` always produces an object
+            // reference), and either reload the slot or evaluate RHS.
+            // The JIT elides the box at runtime when T resolves to a
+            // reference type (ECMA-335 III.4.1), so the optimized native
+            // code is no worse than the original `dup; brtrue` shape.
+            if (b.Left.Type is NullableTypeSymbol tpNullable
+                && tpNullable.UnderlyingType is TypeParameterSymbol tpUnderlying
+                && !tpUnderlying.HasValueTypeConstraint)
+            {
+                if (!this.nullableCoalesceSpillSlots.TryGetValue(b, out var tpSlot))
+                {
+                    throw new InvalidOperationException(
+                        "No scratch slot pre-allocated for class-constrained `T?` '?:' LHS — "
+                        + "check NullableValueTypeCoalesceCollector and the prepass in CollectLocalsAndLabels.");
+                }
+
+                var tpToken = this.outer.GetElementTypeToken(tpUnderlying);
+                var fallback = this.il.DefineLabel();
+                var end = this.il.DefineLabel();
+
+                this.EmitExpression(b.Left);
+                this.il.StoreLocal(tpSlot);
+                this.il.LoadLocal(tpSlot);
+                this.il.OpCode(ILOpCode.Box);
+                this.il.Token(tpToken);
+                this.il.Branch(ILOpCode.Brfalse, fallback);
+
+                this.il.LoadLocal(tpSlot);
+                this.il.Branch(ILOpCode.Br, end);
+
+                this.il.MarkLabel(fallback);
+                this.EmitExpression(b.Right);
+                this.il.MarkLabel(end);
+                return;
+            }
+
             // Defensive: any other value-typed LHS (raw struct or enum)
             // remains unsupported by `?:`. Today the encoder rejects
             // nullable user-defined structs/enums, so this branch is
@@ -341,6 +430,45 @@ private void EmitBinary(BoundBinaryExpression b)
             return;
         }
 
+        // Issue #831: `T? == nil` / `T? != nil` where the underlying T
+        // is an open type parameter (class-constrained, struct-constrained
+        // or unconstrained). The concrete value-type Nullable<T> arm above
+        // (via liftedBinarySlots + EmitLiftedNullableBinary) only catches
+        // operands with a static `ClrType`, which open type parameters do
+        // NOT have — so without this guard the bottom of EmitBinary would
+        // emit `<T-value>; ldnull; ceq`. ilverify rejects that with
+        // `[StackUnexpected]: found Nullobjref ... expected value 'T'`
+        // (class/unconstrained) or `expected value 'Nullable`1<T>'`
+        // (struct-constrained), because `ceq` cannot match an opaque
+        // stack slot against a managed-null reference at the verifier
+        // layer. Box the operand first so the comparison runs against an
+        // `O` reference. The CLR's `box` opcode has the property that
+        // `box Nullable<T>` yields the managed-null reference when
+        // HasValue is false (ECMA-335 III.4.1) — so the same shape
+        // works uniformly for class-T (boxing a reference is a no-op the
+        // JIT elides) and struct-T (boxing a Nullable encodes HasValue
+        // into the reference). We use the LHS expression's full type
+        // token (the NullableTypeSymbol) so the `box` operand resolves to
+        // `!!T` for class/unconstrained-T (where T? stores as a bare
+        // reference slot) and to `Nullable<!!T>` for struct-T (where T?
+        // stores as a value-typed nullable).
+        if ((b.Op.Kind == BoundBinaryOperatorKind.Equals || b.Op.Kind == BoundBinaryOperatorKind.NotEquals)
+            && TryMatchTypeParameterNilCompare(b, out var tpNilOperand))
+        {
+            this.EmitExpression(tpNilOperand);
+            this.il.OpCode(ILOpCode.Box);
+            this.il.Token(this.outer.GetElementTypeToken(tpNilOperand.Type));
+            this.il.OpCode(ILOpCode.Ldnull);
+            this.il.OpCode(ILOpCode.Ceq);
+            if (b.Op.Kind == BoundBinaryOperatorKind.NotEquals)
+            {
+                this.il.LoadConstantI4(0);
+                this.il.OpCode(ILOpCode.Ceq);
+            }
+
+            return;
+        }
+
         // Short-circuit evaluation for logical `&&` and `||`: the right
         // operand must not be evaluated when the left operand already
         // determines the result. Emit a dup + conditional branch so the
@@ -442,6 +570,88 @@ private void EmitBinary(BoundBinaryExpression b)
         EmitNarrowingTruncationIfNeeded(b.Op.Kind, b.Type);
     }
 
+    /// <summary>
+    /// Issue #831: matches `T? == nil` / `T? != nil` (and the symmetric
+    /// `nil == T?` / `nil != T?`) where the underlying T is an open
+    /// type parameter (class-constrained, struct-constrained, or
+    /// unconstrained). The concrete value-type Nullable&lt;T&gt; arm
+    /// (driven by <see cref="liftedBinarySlots"/>) only catches
+    /// operands with a static <c>ClrType</c>; open type parameters
+    /// have none, so this match fills the gap. The caller boxes the
+    /// matched operand using its full nullable type token, which
+    /// resolves to a bare reference slot for class/unconstrained T and
+    /// to <c>Nullable&lt;!!T&gt;</c> for struct T.
+    /// </summary>
+    private static bool TryMatchTypeParameterNilCompare(
+        BoundBinaryExpression node,
+        out BoundExpression operand)
+    {
+        if (node.Right.Type == TypeSymbol.Null
+            && IsOpenTypeParameterNullable(node.Left.Type))
+        {
+            operand = node.Left;
+            return true;
+        }
+
+        if (node.Left.Type == TypeSymbol.Null
+            && IsOpenTypeParameterNullable(node.Right.Type))
+        {
+            operand = node.Right;
+            return true;
+        }
+
+        operand = null;
+        return false;
+    }
+
+    /// <summary>
+    /// Issue #831: returns true when <paramref name="t"/> is a
+    /// <see cref="NullableTypeSymbol"/> wrapping an open type parameter
+    /// (regardless of constraint). The CLR storage of <c>T?</c> is
+    /// either a bare <c>!!T</c> reference slot (class/unconstrained T)
+    /// or a <c>Nullable&lt;!!T&gt;</c> value-typed slot (struct T) —
+    /// see <see cref="ReflectionMetadataEmitter.GetElementTypeToken"/>.
+    /// Both shapes need the same `box; ldnull; ceq` lowering for
+    /// nil-comparison to be verifier-clean: boxing a reference is a
+    /// JIT-elided no-op, while boxing <c>Nullable&lt;T&gt;</c> per
+    /// ECMA-335 III.4.1 yields a managed-null reference when
+    /// HasValue is false.
+    /// </summary>
+    private static bool IsOpenTypeParameterNullable(TypeSymbol t)
+    {
+        return t is NullableTypeSymbol nullable
+            && nullable.UnderlyingType is TypeParameterSymbol;
+    }
+
+    /// <summary>
+    /// Issue #831: returns true when <paramref name="t"/> resolves to an
+    /// open type parameter that is NOT struct-constrained, either
+    /// directly (e.g. after a smart-cast narrowing) or via a
+    /// <see cref="NullableTypeSymbol"/> wrapper. Used by the `!!`
+    /// (NullAssertion) emit path to recognise both `self T?` and the
+    /// narrowed bare-`T` operand shape produced after a preceding
+    /// nil-check guard.
+    /// </summary>
+    private static bool TryGetOpenTypeParameter(TypeSymbol t, out TypeParameterSymbol typeParameter)
+    {
+        if (t is TypeParameterSymbol bare && !bare.HasValueTypeConstraint)
+        {
+            typeParameter = bare;
+            return true;
+        }
+
+        if (t is NullableTypeSymbol nullable
+            && nullable.UnderlyingType is TypeParameterSymbol wrapped
+            && !wrapped.HasValueTypeConstraint)
+        {
+            typeParameter = wrapped;
+            return true;
+        }
+
+        typeParameter = null;
+        return false;
+    }
+
     private void EmitNarrowingTruncationIfNeeded(BoundBinaryOperatorKind kind, TypeSymbol resultType)
     {
         // IL evaluation-stack quirk: arithmetic, bitwise, and shift

src/Core/CodeAnalysis/Emit/SlotPlanner.cs

@@ -991,13 +991,27 @@ protected override void VisitIndirectAssignmentExpression(BoundIndirectAssignmen
         }
     }
 
-    // Issue #504: walks the bound tree collecting every `BoundUnaryExpression`
-    // whose operator is `NullAssertion` (`!!`) and whose operand is a
-    // value-type `Nullable<T>`. Each such site needs a `Nullable<T>`-typed
-    // temp slot so the emitter can spill the operand and call
-    // `Nullable<T>::get_Value` (which yields the underlying `T` or throws
-    // `InvalidOperationException`). The reference-type `!!` path uses the
-    // existing `dup; brtrue; throw NRE` pattern and needs no slot.
+    // Issue #504 + Issue #831: walks the bound tree collecting every
+    // `BoundUnaryExpression` whose operator is `NullAssertion` (`!!`)
+    // and whose operand requires an emit-time spill slot. Two shapes
+    // qualify:
+    //
+    //   * Value-type `Nullable<T>` operand — the emitter spills the
+    //     struct to a slot and calls `Nullable<T>::get_Value` (which
+    //     yields the underlying `T` or throws `InvalidOperationException`).
+    //
+    //   * Open-type-parameter operand whose binder-typed shape is `T?`
+    //     or bare `T` (after a smart-cast that narrowed `T?` to `T`),
+    //     where T is NOT struct-constrained. The bare `!!T` stack value
+    //     cannot be probed via `dup; brtrue` either — the verifier
+    //     rejects branching on an opaque type-parameter slot. The
+    //     emitter instead stores the operand to a `T`-typed slot,
+    //     probes its non-nullness via `box !!T; brtrue nonNull`, throws
+    //     on null, and reloads the slot on the non-null path.
+    //
+    // Reference-type `!!` over a concrete reference (string?, Func<T>?,
+    // sequence[T], ...) uses the existing `dup; brtrue; throw NRE`
+    // pattern and needs no slot.
     private sealed class NullableValueTypeUnwrapCollector : BoundTreeWalker
     {
         private readonly List<BoundUnaryExpression> sink;
@@ -1009,26 +1023,55 @@ public NullableValueTypeUnwrapCollector(List<BoundUnaryExpression> sink)
 
         protected override void VisitUnaryExpression(BoundUnaryExpression node)
         {
-            if (node.Op.Kind == BoundUnaryOperatorKind.NullAssertion
-                && node.Operand.Type is NullableTypeSymbol n
-                && n.UnderlyingType?.ClrType is { IsValueType: true })
+            if (node.Op.Kind == BoundUnaryOperatorKind.NullAssertion)
             {
-                this.sink.Add(node);
+                bool needsSlot = false;
+
+                if (node.Operand.Type is NullableTypeSymbol n)
+                {
+                    needsSlot = n.UnderlyingType?.ClrType is { IsValueType: true }
+                        || (n.UnderlyingType is TypeParameterSymbol tp && !tp.HasValueTypeConstraint);
+                }
+                else if (node.Operand.Type is TypeParameterSymbol bare && !bare.HasValueTypeConstraint)
+                {
+                    // Issue #831: smart-cast may narrow `self T?` to bare
+                    // `T` after a preceding `if self == nil { return }`
+                    // guard, but the runtime value still needs the
+                    // verifiable `box; brtrue` shape rather than the
+                    // direct `dup; brtrue` over an opaque `!!T` slot.
+                    needsSlot = true;
+                }
+
+                if (needsSlot)
+                {
+                    this.sink.Add(node);
+                }
             }
 
             base.VisitUnaryExpression(node);
         }
     }
 
     // Issue #519: walks the bound tree collecting every `BoundBinaryExpression`
-    // whose operator is `NullCoalesce` (`?:`) and whose LHS is a value-type
-    // `Nullable<T>`. Each such site needs a `Nullable<T>`-typed temp slot so
-    // the emitter can spill the LHS once, call `Nullable<T>::get_HasValue`
-    // off the slot's address, and either reload the slot (when the result
-    // type is `Nullable<T>`) or call `Nullable<T>::get_Value` off the slot's
-    // address (when the result type is the underlying `T`). The reference-
-    // type `?:` path uses the existing `dup; brtrue; pop; rhs` pattern and
-    // needs no slot.
+    // whose operator is `NullCoalesce` (`?:`) and whose LHS needs an emit-time
+    // spill slot. Two shapes qualify:
+    //
+    //   * Value-type `Nullable<T>` LHS — the emitter spills the LHS once and
+    //     calls `Nullable<T>::get_HasValue` / `get_Value` off the slot's
+    //     address (since `dup; brtrue` is invalid IL for struct stack values).
+    //
+    //   * Issue #831: open-type-parameter `T?` LHS where T is NOT
+    //     struct-constrained (class-constrained or unconstrained). The bare
+    //     `!!T` stack value cannot be probed with `dup; brtrue` either —
+    //     the verifier rejects branching on an opaque type-parameter slot
+    //     because it cannot prove T is a reference type at the signature
+    //     layer. The slot is typed as `T?` (which encodes as bare `!!T` per
+    //     `ReflectionMetadataEmitter.GetElementTypeToken`), and the emitter
+    //     probes its non-nullness via `box !!T; brfalse fallback`.
+    //
+    // Reference-type `?:` over a concrete reference type (string?, Func<T>?,
+    // sequence[T], etc.) uses the existing `dup; brtrue; pop; rhs` pattern
+    // and needs no slot.
     private sealed class NullableValueTypeCoalesceCollector : BoundTreeWalker
     {
         private readonly List<BoundBinaryExpression> sink;
@@ -1041,10 +1084,15 @@ public NullableValueTypeCoalesceCollector(List<BoundBinaryExpression> sink)
         protected override void VisitBinaryExpression(BoundBinaryExpression node)
         {
             if (node.Op.Kind == BoundBinaryOperatorKind.NullCoalesce
-                && node.Left.Type is NullableTypeSymbol n
-                && n.UnderlyingType?.ClrType is { IsValueType: true })
+                && node.Left.Type is NullableTypeSymbol n)
             {
-                this.sink.Add(node);
+                bool needsSlot = n.UnderlyingType?.ClrType is { IsValueType: true }
+                    || (n.UnderlyingType is TypeParameterSymbol tp && !tp.HasValueTypeConstraint);
+
+                if (needsSlot)
+                {
+                    this.sink.Add(node);
+                }
             }
 
             base.VisitBinaryExpression(node);