Track additional state for "maybe null even if not nullable" #39778

cston · 2019-11-12T16:54:37Z

Track additional flow state to improve analysis of [MaybeNull]T values.

The change includes:

Additional flow state NullableFlowState.MaybeDefault for values of type parameters that are not constrained to be not nullable (types that cannot be annotated).
[AllowNull] and [return: MaybeNull] on containing method signature now affect analysis within the method body.
No W-warnings for local assignment or explicit cast for type parameter types that cannot be annotated.
No warnings for expressions (of type parameter types that cannot be annotated) that may produce a null value. Instead, the value is simply tracked as NullableFlowState.MaybeDefault.

Examples:

[return: MaybeNull] static T Default<T>()
{
    return default(T); // ok
}

static T F<T>(IEnumerable<T> e)
{
    T t = e.FirstOrDefault(); // ok
    return t;                 // warning: maybe null
}

See dotnet/csharplang#2946.

Fixes #38638.
Fixes #37362.

cston · 2019-11-15T16:29:18Z

@dotnet/roslyn-compiler please review.

RikkiGibson · 2019-11-15T21:11:07Z

Does this PR partially enable #36039? #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:29466 in 2afc198. [](commit_id = 2afc198, deletion_comment = False)

RikkiGibson · 2019-11-15T21:32:25Z

                Diagnostic(ErrorCode.WRN_NullabilityMismatchInTypeParameterConstraint, "M1<object?>").WithArguments("Test1<object>.M1<S>(S)", "object", "S", "object?").WithLocation(7, 9),

Please delete the comments in source such as // 2, // 5 for the diagnostics which are no longer produced. (no need to renumber the existing ones IMO) #Closed

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

RikkiGibson · 2019-11-15T21:36:18Z

    T1 t1 = (T1)NullableObject(); // warn: T1 may be non-null

This comment doesn't seem accurate any more and probably others in this test, please update them #Closed

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

RikkiGibson · 2019-11-15T21:41:56Z

static U F17<T, U>(T t) where U : T => (U)t;        // W on cast

W on return? #Closed

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

RikkiGibson · 2019-11-15T21:41:57Z

static U F20<T, U>(T t) where U : T, new() => (U)t; // W on cast

W on return? #Closed

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

RikkiGibson · 2019-11-15T21:41:58Z

static U F21<T, U>(T t) => (U)(object)t;            // W on cast, W on cast

W on cast, W on return? #Closed

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

RikkiGibson · 2019-11-15T21:45:07Z

    T x1 = default; // 2

Please delete #Closed

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

RikkiGibson · 2019-11-15T21:47:01Z

    y = default; // 3

Please delete #Closed

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

RikkiGibson · 2019-11-15T21:51:01Z

    y = x; // 1

Please delete the markers for diagnostics that are no longer produced #Closed

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

RikkiGibson · 2019-11-15T22:08:50Z

        // PROTOTYPE: Should not report warning for F02 or F10. (We should treat [MaybeNull]T as T? when T is not nullable.)

Is this going to happen for all reference types, not just generics constrained to class? #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:117855 in e6474bf. [](commit_id = e6474bf, deletion_comment = False)

RikkiGibson · 2019-11-15T22:56:23Z

[MaybeNull]T F = default!;

Is this ! necessary? I thought we had some nice behavior fall out of MaybeNull returns, where we don't warn if the return value has a maybe-null flow state. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:118248 in b027ec9. [](commit_id = b027ec9, deletion_comment = False)

RikkiGibson · 2019-11-15T22:56:54Z

[MaybeNull]T F = default!;

Obviously it's not the same thing here, but I wondered if it might fall out in a similar way.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:118248 in b027ec9. [](commit_id = b027ec9, deletion_comment = False)

RikkiGibson · 2019-11-15T23:04:18Z

    return Identity(F<T>()); // 1

Does this Identity function provide the expected flow state for other combinations of constraints? e.g. M<T> where T : one of unconstrained, class, class?, struct. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:118444 in b027ec9. [](commit_id = b027ec9, deletion_comment = False)

RikkiGibson · 2019-11-15T23:05:29Z

[MaybeNull]T F = default!;

Seems like the answer is yes, it's needed (based on a later test)

In reply to: 554561397 [](ancestors = 554561397,554561256)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:118248 in b027ec9. [](commit_id = b027ec9, deletion_comment = False)

RikkiGibson · 2019-11-15T23:12:36Z

static T F1<T>(T t) => t ??= default(T);

It feels like we should also test ?? default. I would expect such a test to warn in the same places. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:118572 in b027ec9. [](commit_id = b027ec9, deletion_comment = False)

RikkiGibson · 2019-11-15T23:16:38Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -1043,46 +1041,70 @@ private enum AssignmentKind
            Conversion conversion = default,
            Location location = null)
        {
+            // PROTOTYPE: Fix callers so we don't have cases where valueType and targetType differ.


In which scenarios do they differ? #Closed

The types may differ in binary logical operators and deconstruction. See #39867.

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

gafter

I think there are three substantive comments remaining:

Discards should allow null without warning. Possibly just file a separate issue for that.
TypeWithState.Create should handle a type parameter constrained by int?, which can occur by substitution. Currently it is treated the same as an unconstrained type parameter, which doesn't know if it can contain null or not. Such type parameters do have null in their domain.
SetStateAndTrackForFinally should use Join when incorporating new state information.

RikkiGibson · 2019-11-19T23:35:08Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+        }
+        finally
+        {
+            F0(t2); // 1


I'm a little confused by this-- is it expected that we could throw out of the 'try' block at any point and then run the 'finally' block, so we have to be as pessimistic as possible here? #Closed

Yes, we assume an exception could have been thrown between the assignments above, although perhaps we could do better for those simple assignments.

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

RikkiGibson · 2019-11-19T23:40:11Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/NullablePublicAPITests.cs

+            verify(exprs[0], PublicNullableAnnotation.NotAnnotated, PublicNullableFlowState.MaybeNull);
+            verify(exprs[1], PublicNullableAnnotation.Annotated, PublicNullableFlowState.MaybeNull);
+
+            void verify(DefaultExpressionSyntax expr, PublicNullableAnnotation expectedAnnotation, PublicNullableFlowState expectedState)


consider putting expected values first or labeling in order to match the conventions of the xunit Assert methods. #ByDesign

I'll leave as is since this matches similar local functions in this class.

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

cston · 2019-11-19T23:45:08Z

    return r2; // 2

Ideally we wouldn't warn for this case but we do not handle this case currently. I'll add a comment.

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:117749 in bb8e3a2. [](commit_id = bb8e3a2, deletion_comment = False)

RikkiGibson · 2019-11-19T23:54:56Z

    return Identity(F<T>()); // 1

Just curious, was this ever addressed by additional tests or comments elsewhere?

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:118444 in b027ec9. [](commit_id = b027ec9, deletion_comment = False)

RikkiGibson · 2019-11-19T23:58:52Z

static T F1<T>(T t) => t ??= default(T);

It doesn't look like any test was added in response to this, did I miss it by accident, or maybe miss a relevant existing test?

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:118572 in b027ec9. [](commit_id = b027ec9, deletion_comment = False)

cston · 2019-11-20T00:11:35Z

static T F1<T>(T t) => t ??= default(T);

Yes, F0 in MaybeNullT_16.

In reply to: 555771498 [](ancestors = 555771498,554565234)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:118572 in b027ec9. [](commit_id = b027ec9, deletion_comment = False)

cston · 2019-11-20T02:26:14Z

Yes, for [AllowNull] and [MaybeNull].

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:29466 in 2afc198. [](commit_id = 2afc198, deletion_comment = False)

cston · 2019-11-20T02:28:02Z

@RikkiGibson, @333fred, @gafter, all feedback has been addressed, thanks.

gafter

RikkiGibson · 2019-11-20T20:46:09Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+using System.Diagnostics.CodeAnalysis;
+class Program
+{
+    static T F0<T>(T t) => t ??= default;


This appears to still be using ??= #Closed

RikkiGibson

LGTM modulo a minor bit of test coverage

333fred · 2019-11-20T21:15:21Z

static T F1(T t)

Nit: unused parameter #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:117647 in e6474bf. [](commit_id = e6474bf, deletion_comment = False)

333fred

LGTM (commit 13)

jaredpar · 2019-11-20T21:51:12Z

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

+        /// <summary>
+        /// Maybe null (type may be not nullable).
+        /// </summary>
+        MaybeDefault = 0b11,


Why 0b11 and not 0b10? #ByDesign

So we can (continue to) use bit-wise | and & for Join() and Meet().

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

jaredpar · 2019-11-20T21:59:48Z

    public void MaybeNullT_09()

Is there a version of this test where the method is invoked, stored into a local and then the local is returned? Essentially verifying that the state transfers through locals as well. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:118649 in e6474bf. [](commit_id = e6474bf, deletion_comment = False)

cston force-pushed the MaybeNullT branch 7 times, most recently from d045f46 to 9588a81 Compare November 14, 2019 22:07

cston marked this pull request as ready for review November 14, 2019 22:15

cston requested a review from a team as a code owner November 14, 2019 22:15

RikkiGibson added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Nov 14, 2019

Track additional state for "maybe null even if not nullable"

b027ec9

cston force-pushed the MaybeNullT branch from 704338a to b027ec9 Compare November 15, 2019 04:30

RikkiGibson reviewed Nov 15, 2019

View reviewed changes

Remove return value

88ef98e

gafter suggested changes Nov 19, 2019

View reviewed changes

PR feedback

bb8e3a2

RikkiGibson reviewed Nov 19, 2019

View reviewed changes

PR feedback

2afc198

Misc.

bb293f9

gafter approved these changes Nov 20, 2019

View reviewed changes

RikkiGibson reviewed Nov 20, 2019

View reviewed changes

RikkiGibson approved these changes Nov 20, 2019

View reviewed changes

Add test

e6474bf

333fred approved these changes Nov 20, 2019

View reviewed changes

jaredpar reviewed Nov 20, 2019

View reviewed changes

PR feedback

bf34fb8

cston merged commit 3915513 into dotnet:master Nov 21, 2019

cston deleted the MaybeNullT branch November 21, 2019 16:28

stephentoub mentioned this pull request Nov 21, 2019

Address new nullable warnings from compiler [MaybeNull] updates dotnet/runtime#179

Merged

This was referenced Nov 21, 2019

Report a warning when calling a property or method that returns [MaybeNull]T #39650

Closed

Consider adding MaybeDefault to Microsoft.CodeAnalysis.NullableFlowState #39981

Closed

jcouv added this to the 16.5 milestone Jan 29, 2020

This was referenced Jan 29, 2020

[MaybeNull]: Cannot implement a generic method with a "default" return value with nullable references #30953

Closed

Roslyn should warn about possible mismatching nullability when assigning dependent type parameters #36901

Closed

Track additional state for "maybe null even if not nullable" #39778

Track additional state for "maybe null even if not nullable" #39778

Conversation

cston commented Nov 12, 2019 • edited Loading

cston commented Nov 15, 2019

RikkiGibson commented Nov 15, 2019 • edited Loading

RikkiGibson commented Nov 15, 2019 • edited by 333fred Loading

RikkiGibson commented Nov 15, 2019 • edited Loading

RikkiGibson commented Nov 15, 2019 • edited Loading

RikkiGibson commented Nov 15, 2019 • edited Loading

RikkiGibson commented Nov 15, 2019 • edited Loading

RikkiGibson commented Nov 15, 2019 • edited Loading

RikkiGibson commented Nov 15, 2019 • edited Loading

RikkiGibson commented Nov 15, 2019 • edited by 333fred Loading

RikkiGibson commented Nov 15, 2019 • edited Loading

RikkiGibson commented Nov 15, 2019 • edited Loading

RikkiGibson commented Nov 15, 2019

RikkiGibson commented Nov 15, 2019 • edited Loading

RikkiGibson commented Nov 15, 2019

RikkiGibson commented Nov 15, 2019 • edited Loading

RikkiGibson Nov 15, 2019 • edited Loading

Choose a reason for hiding this comment

cston Nov 18, 2019

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

RikkiGibson Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

cston Nov 19, 2019

Choose a reason for hiding this comment

RikkiGibson Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

cston Nov 19, 2019

Choose a reason for hiding this comment

cston commented Nov 19, 2019

RikkiGibson commented Nov 19, 2019

RikkiGibson commented Nov 19, 2019

cston commented Nov 20, 2019

cston commented Nov 20, 2019

cston commented Nov 20, 2019

gafter left a comment

Choose a reason for hiding this comment

RikkiGibson Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

RikkiGibson left a comment

Choose a reason for hiding this comment

333fred commented Nov 20, 2019 • edited by cston Loading

333fred left a comment

Choose a reason for hiding this comment

jaredpar Nov 20, 2019 • edited by cston Loading

Choose a reason for hiding this comment

cston Nov 20, 2019

Choose a reason for hiding this comment

jaredpar commented Nov 20, 2019 • edited by cston Loading

cston commented Nov 12, 2019 •

edited

Loading

RikkiGibson commented Nov 15, 2019 •

edited

Loading

RikkiGibson commented Nov 15, 2019 •

edited by 333fred

Loading

RikkiGibson commented Nov 15, 2019 •

edited

Loading

RikkiGibson commented Nov 15, 2019 •

edited

Loading

RikkiGibson commented Nov 15, 2019 •

edited

Loading

RikkiGibson commented Nov 15, 2019 •

edited

Loading

RikkiGibson commented Nov 15, 2019 •

edited

Loading

RikkiGibson commented Nov 15, 2019 •

edited

Loading

RikkiGibson commented Nov 15, 2019 •

edited by 333fred

Loading

RikkiGibson commented Nov 15, 2019 •

edited

Loading

RikkiGibson commented Nov 15, 2019 •

edited

Loading

RikkiGibson commented Nov 15, 2019 •

edited

Loading

RikkiGibson commented Nov 15, 2019 •

edited

Loading

RikkiGibson Nov 15, 2019 •

edited

Loading

RikkiGibson Nov 19, 2019 •

edited

Loading

RikkiGibson Nov 19, 2019 •

edited

Loading

RikkiGibson Nov 20, 2019 •

edited

Loading

333fred commented Nov 20, 2019 •

edited by cston

Loading

jaredpar Nov 20, 2019 •

edited by cston

Loading

jaredpar commented Nov 20, 2019 •

edited by cston

Loading