dotnet/csharplang

ref ternary operator nullability suppressions

jjonescz opened this issue · 3 comments

jjonescz commented

How should suppressions in ref ternaries behave? Currently in Roslyn they don't work - all of these are warnings:

ref object M1(bool b, ref object? x, ref object y) => ref b ? ref x : ref y; // warns on the whole ternary
ref object M2(bool b, ref object? x, ref object y) => ref b ? ref x! : ref y; // warns on the whole ternary
ref object M3(bool b, ref object? x, ref object y) => ref b ? ref x : ref y!; // warns on the whole ternary
ref object M4(bool b, ref object? x, ref object y) => ref (b ? ref x : ref y)!; // warns on the whole ternary

Conditional operator spec

Suppressing the whole ternary should continue to do nothing. The spec says: "type of the result can be either type." I.e., the outer suppression works with the already-chosen type (ref object or ref object?) and all it can do is suppress any "assignment mismatch" warning, not the inner "operands mismatched" warning.

Which branch should a suppression work on? And where the warning should be reported?

  1. Reusing "best common type" and type inference.

    • Per the spec, only non-ref ternary should use "best common type". Ref ternary requires an identity conversion between the operands.
    • However, in Roslyn, binding (BindRefConditionalOperator) relies on type inference. On the other hand, nullability analysis (NullableWalker) uses type inference only for non-ref ternaries.

    Type inference picks one of the arguments/types as the winner and warns on the other.:

    ref T M<T>(ref T t1, ref T t2) => throw null!;  
ref object M1(bool b, ref object? x, ref object y) => ref M(ref x, ref y); // warns on x
ref object M2(bool b, ref object? x, ref object y) => ref M(ref x!, ref y); // suppressed
ref object M3(bool b, ref object? x, ref object y) => ref M(ref x, ref y!); // warns on x (suppression has no effect)

    Ref ternaries could behave similarly - reporting the warning on just one operand and allowing suppression only on that one, as well.

  2. Keep what the spec says - i.e., there can be only an identity conversion between the operands.

    • When the operands are ref object and ref object? and we consider nullability, they are not convertible to each other via an identity conversion.
    • Suppressing either of them should work - the suppressed one becomes convertible to the other - like it works in assignments: 
      void M(ref object x, ref object? y) { y = ref x!; x = ref y!; }
    • Both operands are mismatched with respect to each other, so the warning should be
      • on the whole ternary (but suppressing the whole ternary does nothing which is confusing), or
      • on both operands.
TahirAhmadov commented

My opinion is in comments:

ref object M1(bool b, ref object? x, ref object y) => ref b ? ref x : ref y; // should warn on the whole ternary
ref object M2(bool b, ref object? x, ref object y) => ref b ? ref x! : ref y; // should not warn at all
ref object M3(bool b, ref object? x, ref object y) => ref b ? ref x : ref y!; // should warn on the whole ternary
ref object M4(bool b, ref object? x, ref object y) => ref (b ? ref x : ref y)!; // should not warn at all

My thinking here is, (leaving aside the inner workings of Roslyn) the type of the ternary as a whole is determined depending on its branches, then analysis continues onward.
Line 1: ref x is object?, which makes the whole ternary object?, therefore warning.
Line 2: ref x! is object; the ternary is object; no warning.
Line 3: ref y! has no effect since it's already object; same as line 1.
Line 4: ternary is object?, then it's wholesale made to be object; no warning.

jjonescz commented

Discussed at LDM 2024-08-29. Going with option 1.