Open LDM Issues for Nullable Reference Types

[cston]

Open LDM Issues for Nullable Reference Types

Open issues are on top. Resolved issues are at the bottom.

Relating to championed issue #36

There are also some issues marked as "Language Design" in roslyn repo.

---

Confirm interaction between MaybeNull and NotNull (open)

Which takes precedence? (currently it's MaybeNull)
See dotnet/roslyn#35955 (comment)

---

Should AllowNull and NotNull be allowed on value types? (open)

void M([AllowNull]int i, [DisallowNull]int i2)

Tracked by dotnet/roslyn#36009
See dotnet/roslyn#35955

---

Should ! change top level nullability when there was no warning? (open)

See dotnet/roslyn#33447 (comment)

Applying a null-suppression operator currently changes the top-level nullability of the expression it's applied to, regardless of whether that expression produced a warning in the first place. After dotnet/roslyn#33447 is merged, this will mean that applying the ! to a ref parameter that accepts a nullable object will mark the expression as non-null, regardless of the type of the ref. See the example below:

class C
{
    public void M()
    {
        string? s = string.Empty;
        M(ref s!);
        s.ToString(); // should we get a warning?

        string s2 = string.Empty;
        M(ref s2!);
        s2.ToString(); // No warning
    }
    void M(ref string? s) => throw null!;
}

Should we get a warning on the s.ToString() call?

---

Should ref arguments always be assumed to be assigned to? (open)

class C
{
    public void M(string? s)
    {
        M2(ref s);
        s.ToString(); // warn?
    }
    void M2(ref string s2) => throw null;
}

Two options:

• always assume an assignment
• only assume an assignment when the parameter is possibly null

Relates to dotnet/roslyn#33447 (comment)

---

Tracking state across conversions (open)

Which conversions should preserve the nullable state of members?

struct S { internal object? FS; }
class A { internal object? FA; }
class B : A { }

// Reference conversions
B b = new B() { FA = 1 };
A a = b;
a.FA.ToString();

b = (B)(object)a;
b.FA.ToString();

// Boxing conversions
S s = new S() { FS = 2 };
s = (S)(object)s;
s.FS.ToString();

// Nullable conversions
s = new S() { FS = 3 };
S? ns = s;
ns.Value.FS.ToString();

s = (S)ns;
s.FS.ToString();

// Tuple conversions
(A, S?) t = (new B() { FA = 1 }, new S() { FS = 2 });
t.Item1.FA.ToString();
t.Item2.Value.FS.ToString();

(B, S) u = ((B, S))t;
u.Item1.FA.ToString();
u.Item2.FS.ToString();

---

---

[MaybeNull] and other attributes (open)

See dotnet/roslyn#30953

Resolve unknowns with [MaybeNull] and other similar attributes that affect nullability, and the interaction with unspeakable types.

Using an attribute doesn't work well with locals or nested types (IEnumerable<T /*maybe-null*/>).

// need example

---

var? (open)

See dotnet/roslyn#31874

Should we support var?, and if so, what are the open issues?
Along those lines, would we allow var? (x, y) = ... deconstructions?
Some alternatives are: (1) the user can spell out the type or cast, (2) we could always take var to infer the nullable version of the type.
Note: var? suffers from similar issues as T? when applied to int?.

---

typeof(string?) (open)

Should we permit (or forbid) typeof(string?), given that we can never honor the nullable annotation?

---

When we compute a nullable annotation in flow analysis, should we use the context? (open)

From dotnet/roslyn#33639
The current LDM position is that we use the annotation context to compute the annotations (oblivious versus non-nullable). We don't currently do this. For example, when doing type inference, would we infer oblivious in a context where the nullability annotations are disabled, but non-null where nullability annotations are enabled?

#nonnull enable
T Copy<T>(T t) => t;
T M<T>(T t)
{
    // warning: possibly null dereference; the inferred type argument to Copy is unannotated
    Copy(t).ToString();

#nonnull disable
    // no warning; the inferred type argument to Copy is oblivious
    Copy(t).ToString();
}

---

Result of a dynamic invocation (open)

Should the result of a dynamic invocation be treated as non-null, as it is generally outside the scope of static analysis, or should we attempt to infer a nullability from the set of known candidates when the receiver is not dynamic?

See also dotnet/roslyn#29893

Resolved issues

Track assignments through ref with conditional expressions (closed)

cc @gafter

What is the nullability of ref variables when assigned through conditional expressions?

string? x = "";
string? y = "";
(b ? ref x : ref y) = null;
x.ToString(); // warning?
y.ToString(); // warning?

string? x = null;
string? y = null;
(b ? ref x : ref y) = "";

Resolved 2/13/2019:

Yes, those two should warn
Tracked by dotnet/roslyn#33365

---

Nullability of conditional access with unconstrained type parameters (closed)

cc @AlekseyTs

What is the nullability of x?.F()?

class C<T, U>
    where T : U
    where U : C<T, U>?
{
    static void M(U x)
    {
        U y = x?.F();
        T z = x?.F();
    }
    T F() => throw null;
}

Resolved 2/13/2019:

The nullability of a conditional invocation is always nullable.
Tracked by dotnet/roslyn#33430

---

! operator on L-values (closed)

See dotnet/roslyn#27522

! is currently allowed for certain out scenarios.

M(out x!);
M(out (x!));
M(out (RefReturning()!));

Should ! be allowed for the following?

M(out object x!);
object y! = x;
y! = x;

Resolved 2/13/2019:

We'll disallow those additional scenarios for now.

---

Expression is always (or never) null (closed)

See dotnet/roslyn#22743
See dotnet/roslyn#29868
cc @AlekseyTs, @gafter

Confirm we should (or not) report hidden diagnostics when an expression is always (or never) null. This might require nullable analysis that assumes expressions are nullable (rather than non-nullable) when in doubt.

static void F(object? x)
{
    if (x != null)
    {
        if (x is null) // hidden: comparison is always false
        {
        }
    }
}

Resolved 2019-02-20:

These were never part of the language specification. OK to drop them.

---

throw null (closed)

Should throw null produce a warning that the exception expression may be null?

throw null;      // warning?
throw maybeNull; // warning?

The current checked-in behavior (as of this PR) is to warn on throw null.

Resolved by email (2/12/2019):

throw null should warn.
Throw statements and throw expressions yield a warning when their operand may be null.
No special treatment of constant or literal null here, as everywhere else.
Relates to dotnet/roslyn#32877

---

is nullability in false case (closed)

See dotnet/roslyn#30297

Should is update the nullability in both branches or should the one branch be treated as unreachable?

string? s = "";
if (!(s is object))
    s.ToString(); // warning?

Note: this was discussed on 2/13/2019, but not resolved yet.

"Pure" null tests affect both branches:

1. x == null
2. x != null
3. (Type)x == null
4. (Type)x != null
5. x is null
6. s is string (when the type of s is string)
7. s is string s2 (when the type of s is string)
8. s is string _ (when the type of s is string)

Other null tests are not "pure" and so affect only one branch:

1. x is string
2. x is string s
3. x is C { Property = 3 }
4. TryGetValue ([NotNullWhenTrue])
5. string.IsNullOrEmpty(s) ([NotNullWhenFalse])
6. x?.ToString() != null

Resolved 2019-02-20:

We approved this proposal (some null tests are considered "pure" or "deliberate") with some adjustments. See the notes for details. Follow-up bug is dotnet/roslyn#33526

---

Discuss the default loophole (closed)

var y = default(SomeStructWithNonNullableFields); // we currently produce no warning

I understand that there are issues with ref assemblies, but maybe that means the guidelines for ref assemblies should be updated (if you have any private non-nullable field, then the reference assembly should preserve that).
Alternatively, would it be useful to be able to mark some struct types as not being instantiatable via default?

Resolved 2019-03-04:

Keep this loophole for now.

---

Nullable analysis of switch expressions and statements (closed)

In LDM 2/20, we decided on a list of "pure" null-tests, which affect both branches.
This gist documents a proposal whereby: A switch with a pure test sets the state of the tested variable to maybe-null at the entrance of the switch.

We need to review this proposal.
We need to confirm whether the same patterns which are considered "pure" null tests in an is (if (x is null) ... or if (x is {}) ... for instance) are also considered "pure" null tests in a switch.

Resolved 3/6/2019:

We're not considering is {} or {} => ... to be "pure tests". Neither o is object or s is string.

---

Nullable analysis of unreachable code (closed)

See dotnet/roslyn#28798
See dotnet/roslyn#32047
cc @gafter

How should nullability analysis treat unreachable code?

static void M(bool b, string s)
{
    var t = (b || true) ? s : null;
    t.ToString(); // warning: may be null
}

Resolved 3/6/2019:

No nullability warnings in unreachable code.

---

handling of type parameters that cannot be annotated (closed)

See also dotnet/roslyn#33436

A variable of a type parameter that cannot be annotated, when read, may yield a null value. But a (possibly) null value cannot be stored in it, because the type may be substituted with a type that is non-nullable. If we apply the usual rules, that would mean this causes a diagnostic:

void M<T>(T t)
{
    t = t; // warning: might be assigning null to T
}

To solve this, we propose the following rules:

1. The warning that a possibly-null value is converted to a type that doesn't accept null has an exception for a conversion to a value of a type that is a "type parameter that may possibly be a reference type and cannot be annotated" when the conversion is an implicit conversion (with two exceptions below). An implicit conversion to such an unannotatable type causes no warning. If the conversion is not implicit (e.g. a downcast between related type parameters) the warning is given.
2. We must introduce a safety warning whenever a possibly null value of such a type is introduced. Those contexts are:

• default and default(T) (the default conversion, though implicit, does introduce a warning)
• null (the null conversion, though implicit, does introduce a warning)
• e?.M() where the return type is a type parameter known to be a reference type
• dynamic conversion or cast to T when the dynamic value might be null
• any explicit conversion (cast) to T
• e as T when there is not an implicit conversion from the type of e to T
• Call to a method like e.GetFirstOrDefault() which is annotated with [MaybeNull].

How can these diagnostics be suppressed?

Resolved 3/6/2019:

Proposal was accepted.

---

Element-wise analysis of tuple conversions (closed)

See dotnet/roslyn#33035

Reasons to use element-wise analysis of tuple conversions:

• Avoid redundant safety warning
• Warn on deconstruction
• Generic type inference (?)

// current behavior:
static void M(string? x, string y)
{
    (string, string) t = (x, y); // warning: (string?, string) mismatch
    t.Item1.ToString();          // warning: may be null

    (string x2, string y2) = (x, y);  // warning

    Pair<string, string> p = Pair.Create(x, y); // warning: Pair<string?, string> mismatch
    p.Item1.ToString();                         // no warning
}

Resolved 3/8/2019 with team:

In short, we're ok with current behavior (similar to Pair<T1, T2> and anonymous types).

For the record, the alternative was clarified as (roughly): whenever you read a local of tuple type, we would give the result a type based on the state of the tuple.
But this involves complexity:

• We would do this for tuples in locals, but probably not those nested in other types.
• We’d have to do special checks for passing tuples as ref arguments.
  And the benefits aren’t clear:
• Users probably don’t expect to see the type of a tuple local change as they do for simple locals.
• Inferring (string?, string) in the scenario below doesn’t seem a major advantage.

---

Should anonymous type fields have top-level nullability? (closed)

Should the top-level nullability of anonymous type fields be part of the inferred signature in addition to being tracked?

static T Identity<T>(T t) => t;

static void F(string x, string? y)
{
    var a = new { x };
    a.x.ToString();           // ok

    a = new { x = y };        // warning?
    a.x.ToString();           // warning

    Identity(a).x.ToString(); // warning?
}

Resolved 3/8/2019:

Yes, we should warn on a = new { x = y };. But we should not warn on Identity(a).x.ToString(); (we inferred Identity<string!>).

[gafter]

As I understand it, there are two separate things:

1. The nullability of a type where the type is stated. This has three states: annotated, unannotated (in a context where nullable is enabled), and oblivious (unannotated where nullable is not enabled).
2. The nullability of an expression/value computed during flow analysis. This has two states: can be null, or cannot be null.

These should be separate things, and I’m working on a PR that separates them into two separate enums. An annotation (1) can affect flow analysis (2) when we for example read a field or property or get a value from an invocation. Flow analysis (2) can affect the inferred nullability of a type (1) due to type inference, but in that case we would always want to infer a speakable annotation. At least, that is how I understand the latest discussion in the LDM.

[YairHalberstadt]

@gafter

The nullability of a type where the type is stated. This has three states: annotated, unannotated (in a context where nullable is enabled), and oblivious (unannotated where nullable is not enabled).
The nullability of an expression/value computed during flow analysis. This has two states: can be null, or cannot be null.

I'm not sure I understand why something can only be oblivious in a context where nullability is disabled, and why an expression is either null or not null. What is the type of y here, and the what is the nullability of its initialiser expression:

#nullable disable
void M(string x)
{
#nullable enable
    var y = x;
}

[gafter]

The state of the initializing expression is “not null” (because it is the rvalue read from an oblivious symbol). From that we infer the type `string` (unannotated) for `y`.

[jcouv]

Updated OP to record the decision on throw null:

Resolved by email (2/12/2019):

throw null should warn.
Throw statements and throw expressions yield a warning when their operand may be null.
No special treatment of constant or literal null here, as everywhere else.

[gafter]

Added

---

handling of type parameters that cannot be annotated

A variable of a type parameter that cannot be annotated, when read, may yield a null value. But a (possibly) null value cannot be stored in it, because the type may be substituted with a type that is non-nullable. If we apply the usual rules, that would mean this causes a diagnostic:

void M<T>(T t)
{
    t = t; // warning: might be assigning null to T
}

To solve this, we propose the following rules:

1. The warning that a possibly-null value is converted to a type that doesn't accept null has an exception for a conversion to a value of a type that is a "type parameter that may possibly be a reference type and cannot be annotated" when the conversion is an implicit conversion. An implicit conversion to such an unannotatable type causes no warning. If the conversion is not implicit (e.g. a downcast between related type parameters) the warning is given.
2. We must introduce a safety warning whenever a possibly null value of such a type is introduced. Those contexts are:

• default and default(T)
• null
• e as T
• e.GetFirstOrDefault() - which would be annotated with [MaybeNull]

We should discuss this and make sure we agree.

[333fred]

Added

Should ! change top level nullability when there was no warning?

[jcouv]

Added

Should ref arguments always be assumed to be assigned to? (open)

class C
{
    public void M(string? s)
    {
        M2(ref s);
        s.ToString(); // warn?
    }
    void M2(ref string s2) => throw null;
}

Two options:

• always assume an assignment
• only assume an assignment when the parameter is possibly null

Relates to dotnet/roslyn#33447 (comment)

[gafter]

Added

---

When we compute a nullable annotation in flow analysis, should we use the context? (open)

From to dotnet/roslyn#33639
The current LDM position is that we use the annotation context to compute the annotations (oblivious versus non-nullable). We don't currently do this.

[jcouv]

Added:

---

Nullable analysis of switch expressions and statements (open)

In LDM 2/20, we decided on a list of "pure" null-tests, which affect both branches.
This gist documents a proposal whereby: A switch with a pure test sets the state of the tested variable to maybe-null at the entrance of the switch.

We need to review this proposal.
We need to confirm whether the same patterns which are considered "pure" null tests in an is (if (x is null) ... or if (x is {}) ... for instance) are also considered "pure" null tests in a switch.

[jcouv]

Added:

---

Discuss the default loophole (open)

var y = default(SomeStructWithNonNullableFields); // we currently produce no warning

I understand that there are issues with ref assemblies, but maybe that means the guidelines for ref assemblies should be updated (if you have any private non-nullable field, then the reference assembly should preserve that).
Alternatively, would it be useful to be able to mark some struct types as not being instantiatable via default?

[Joe4evr]

Alternatively, would it be useful to be able to mark some struct types as not being instantiatable via default?

Considering that this has been proposed more than once (#146, #2019), I'm certain that such an annotation will be well-received for many developers.

[jcouv]

Thanks for the references @Joe4evr. I'll read up on those issues and make one of them a championed issue.

[gafter]

Added

---

Result of a dynamic invocation (open)

Should the result of a dynamic invocation be treated as non-null, as it is generally outside the scope of static analysis, or should we attempt to infer a nullability from the set of known candidates when the receiver is not dynamic?

See also dotnet/roslyn#29893