Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref ternary operator nullability suppressions #8379

Closed
jjonescz opened this issue Aug 22, 2024 · 3 comments
Closed

ref ternary operator nullability suppressions #8379

jjonescz opened this issue Aug 22, 2024 · 3 comments

Comments

@jjonescz
Copy link
Member

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
Copy link

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants