Revise NullableWalker to use a two-state solution #33648

gafter · 2019-02-24T22:42:37Z

The NullableWalker is revised so that the inferred state of an expression is either NotNull or MaybeNull (represented by the new type NullableFlowState). There is no longer such as thing as an oblivious rvalue. There are four kinds of lvalues:

oblivious lvalues read as NotNull but null can be written to them
annotated lvalues read as MaybeNull and null can be written to them
unannotated lvalues read as NotNull and null may not be written to them
unconstrained type parameters read as MaybeNull but null may not be written to them

In order to preserve the safety in the face of such unconstrained type parameters, we warn immediately when a null value of such a type is introduced. This is a safety warning. The contexts in which such a warning are given are

default and default(T)
null conversion to T (when T is known to be ref type)
e?.M() when the return type is T
dynamic conversion or cast to T when the dynamic might be null
explicit conversion to T
e as T when there is not an implicit conversion from the type of e to T
a call to a method like FirstOrDefault() that is annotated with `[MaybeNull] (not yet implemented)

We remove the hidden diagnostics previously computed by the NullableWalker.

We add debugger display support for a number of types used by the NullableWalker. The display for NullableWalker summarizes the computed nullability of variables in a nice compact form.

We remove support for definite assignment in the NullableWalker. Some scenarios involving the use of not-definitely-assigned variables may cause cascaded diagnostics. We might remove them in the future by initializing all vaiables to the NotNull state when they enter scope.

We overhaul and simplify the inplementation of conversions, the null-conditional operator, and result inference for lifted operators.

We simplify the Meet and Join operations so that they form a lattice on both the NullableAnnotation and NullableFlowState.

Catch variables are now initialized to a non-null state on entry to the catch block.

In unreachable code, every expression produces a non-null rvalue. An erroneous expression produces a non-null rvalue (to suppress cascaded diagnostics).

The NullableWalker is revised so that the inferred state of an expression is either `NotNull` or `MaybeNull` (represented by the new type `NullableFlowState`). There is no longer such as thing as an oblivious rvalue. There are four kinds of lvalues: - oblivious lvalues read as NotNull but null can be written to them - annotated lvalues read as MaybeNull and null can be written to them - unannotated lvalues read as NotNull and null may not be written to them - unconstrained type parameters read as MaybeNull but null may not be written to them In order to preserve the safety in the face of such unconstrained type parameters, we warn immediately when a null value of such a type is introduced. This is a safety warning. The contexts in which such a warning are given are - `default` and `default(T)` - `null` conversion to `T` (when `T` is known to be ref type) - `e?.M()` when the return type is `T` - dynamic conversion or cast to `T` when the dynamic might be null - explicit conversion to `T` - `e as T` when there is not an implicit conversion from the type of `e` to `T` - a call to a method like `FirstOrDefault()` that is annotated with `[MaybeNull] (not yet implemented) We remove the hidden diagnostics previously computed by the NullableWalker. We add debugger display support for a number of types used by the NullableWalker. The display for `NullableWalker` summarizes the computed nullability of variables in a nice compact form. We remove support for definite assignment in the NullableWalker. Some scenarios involving the use of not-definitely-assigned variables may cause cascaded diagnostics. We might remove them in the future by initializing all vaiables to the `NotNull` state when they enter scope. We overhaul and simplify the inplementation of conversions, the null-conditional operator, and result inference for lifted operators. We simplify the Meet and Join operations so that they form a lattice on both the NullableAnnotation and NullableFlowState. Catch variables are now initialized to a non-null state on entry to the catch block. In unreachable code, every expression produces a non-null rvalue. An erroneous expression produces a non-null rvalue (to suppress cascaded diagnostics).

src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs

jcouv · 2019-02-25T21:30:20Z

        NullableFlowState getNullableState(BoundExpression e, TypeWithState t)

Consider extracting this method (same local function appears twice). #Closed

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:2086 in 6c329e3. [](commit_id = 6c329e3, deletion_comment = False)

src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs

jcouv · 2019-02-25T22:12:07Z

    public bool IsReferenceType => TypeSymbol.IsReferenceType;

All the properties that dereference TypeSymbol now seem dangerous, since TypeSymbol is now allowed to be null.
We may want to think about dereferences of _extensions as well. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs:599 in 6c329e3. [](commit_id = 6c329e3, deletion_comment = False)

src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs

jcouv · 2019-02-25T22:28:11Z

            return obj.TypeSymbol.GetHashCode();

Can TypeSymbol be null here? (same question for Equals below) #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs:731 in 6c329e3. [](commit_id = 6c329e3, deletion_comment = False)

jcouv · 2019-02-25T22:29:23Z

        return TypeSymbol.GetHashCode();

can TypeSymbol be null here? #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs:1054 in 6c329e3. [](commit_id = 6c329e3, deletion_comment = False)

jcouv · 2019-02-28T18:28:31Z

            // (8,9): warning CS8602: Possible dereference of a null reference.

Tracked by #33743 (issue with ??)

In reply to: 468053080 [](ancestors = 468053080)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:67259 in eb68fb8. [](commit_id = eb68fb8, deletion_comment = True)

jcouv

LGTM Thanks (iteration 9)
I'm going to merge latest bits from master into this branch to resolve conflicts and investigate the failing test (stack overflow on long fluent invocation)

333fred · 2019-02-28T19:32:17Z

        if (operatorKind.IsLifted())

Then this is incorrect for boolean logical operators: https://github.com/dotnet/csharplang/blob/master/spec/expressions.md#boolean-logical-operators

In reply to: 468145643 [](ancestors = 468145643,468073628)

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:1717 in eb68fb8. [](commit_id = eb68fb8, deletion_comment = False)

jcouv · 2019-02-28T20:21:54Z

        if (operatorKind.IsLifted())

Filed #33771

In reply to: 468406199 [](ancestors = 468406199,468145643,468073628)

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:1717 in eb68fb8. [](commit_id = eb68fb8, deletion_comment = False)

333fred · 2019-02-28T21:10:15Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs

+            var otherAnnotation = other.NullableAnnotation;
+            if (!HasType)
+            {
+                return thisAnnotation == otherAnnotation;


This is guaranteed to be true, as we'd have already returned above it wasn't.

333fred · 2019-02-28T21:11:09Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs

-            if (other.IsNull || !TypeSymbolEquals(other, comparison))
+            if (!HasType)
+            {
+                if (other.HasType || NullableAnnotation != other.NullableAnnotation)


I think we can just return !other.HasType && NullableAnnotation == other.NullableAnnotation, right?

333fred

LGTM (iteration 9) with filled followup issues.

…lablewalker-to-use-a-two-state-solution

jcouv · 2019-02-28T23:09:42Z

src/Compilers/CSharp/Test/Emit/Emit/EndToEndTests.cs

@@ -33,7 +33,7 @@ public void OverflowOnFluentCall()
                    numberFluentCalls = 460;
                    break;
                case 32 when !isDebug:
-                    numberFluentCalls = 1300;
+                    numberFluentCalls = 1100;


1100 [](start = 40, length = 4)

Filed #33775 to investigate further and restore our expectations

gafter added Area-Compilers New Language Feature - Nullable Reference Types Nullable Reference Types labels Feb 24, 2019

gafter added this to the 16.1.P1 milestone Feb 24, 2019

gafter self-assigned this Feb 24, 2019

gafter requested review from cston, jcouv and a team February 24, 2019 22:42