Track nullability of nullable value types #31499

cston · 2018-12-03T18:45:46Z

No description provided.

333fred · 2018-12-03T21:17:08Z

...Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs

@@ -835,9 +835,10 @@ private void CheckNewModifier(Symbol symbol, bool isNew, DiagnosticBag diagnosti
                            for (int i = 0; i < overridingParameters.Length; i++)


Nit: this whole loop can just be converted to a foreach, we don't use i anywhere except accessing overridingParameters. #ByDesign

Unfortunately i is used for overriddenParameters as well as overridingParameters.

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

Ah dang, and I thought I had gone through thoroughly here to avoid looking blind.

In reply to: 238500830 [](ancestors = 238500830,238440431)

333fred · 2018-12-03T21:17:29Z

...Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs

                                                                         TypeCompareKind.AllIgnoreOptions & ~(TypeCompareKind.IgnoreNullableModifiersForReferenceTypes | TypeCompareKind.IgnoreInsignificantNullableModifiersDifference)) &&
-                                    overridingParameters[i].Type.Equals(overridenParameterType, TypeCompareKind.AllIgnoreOptions))
+                                    overridingParameterType.Equals(overridenParameterType, TypeCompareKind.AllIgnoreOptions))
                                {
                                    diagnostics.Add(ErrorCode.WRN_NullabilityMismatchInParameterTypeOnOverride, overridingMemberLocation, new FormattedSymbol(overridingParameters[i], SymbolDisplayFormat.ShortFormat));


overridingParameters[i] [](start = 158, length = 23)

If you don't want to update the whole loop, please at least do this instance. #ByDesign

333fred · 2018-12-03T21:22:35Z

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

@@ -16498,9 +16498,15 @@ static void F6(A<object?>? x6, B<object>? y6)
                // (30,10): warning CS8619: Nullability of reference types in value of type 'A<object>' doesn't match target type 'B<object?>?'.
                //         (x5 ?? y5)/*T:B<object?>?*/.Value.F.ToString();
                Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "x5").WithArguments("A<object>", "B<object?>?").WithLocation(30, 10),
+                // (30,10): warning CS8629: Nullable value type may be null.
+                //         (x5 ?? y5)/*T:B<object?>?*/.Value.F.ToString();
+                Diagnostic(ErrorCode.WRN_NullableValueTypeMayBeNull, "x5 ?? y5").WithLocation(30, 10),


This feels like it's the wrong location. The chance for the null reference happens on the invocation of .Value, right? Unless this is reporting on the implicit conversion, in which case I feel like this isn't descriptive enough, and I would still have expected a warning on .Value. #Closed

This still feels wrong.

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

333fred · 2018-12-03T21:26:45Z

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

@@ -35026,6 +35042,12 @@ static void F(A<object>? x, A<object?>? y)
 }";
            var comp = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue());
            comp.VerifyDiagnostics(
+                // (14,24): warning CS8629: Nullable value type may be null.
+                //         B<object>? z = x;
+                Diagnostic(ErrorCode.WRN_NullableValueTypeMayBeNull, "x").WithLocation(14, 24),


Yeah, I think this example is more illustrative of wanting to have a clearer error message here. As a user, my first thought is "The nullabilities here don't differ, why are you warning me?" #Resolved

Actually, by my reading of the spec, this is an invalid warning.

Given a user-defined conversion operator that converts from a non-nullable value type S to a non-nullable value type T, a lifted conversion operator exists that converts from S? to T?.
https://github.com/dotnet/csharplang/blob/54df8973faabe32335fa087f9d32fd959366ff83/spec/conversions.md#lifted-conversion-operators

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

Good catch, thanks.

In reply to: 238445282 [](ancestors = 238445282,238443507)

333fred · 2018-12-03T22:00:27Z

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

+        _ = (T)x;
+        x = y;
+        _ = (T)x; // 2
+        _ = (T)x;


Perhaps add a dereference of y after this? #Closed

333fred · 2018-12-03T22:05:34Z

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

+        }
+
+        [Fact]
+        public void NullableT_12()


Is this substantially different from NullableT_06, other than also having the negative case? If not, consider consolidating. #Closed

333fred · 2018-12-03T22:07:21Z

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

+    {
+        if (t1.HasValue)
+        {
+            if (t1.HasValue) _ = t1.Value;


Shouldn't we have a warning here for something along the lines of "this will never be false"? #Closed

Logged #31516.

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

333fred · 2018-12-03T22:40:26Z

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

+{
+    static void F1(A? a)
+    {
+        B? b = a; // 1


Again, there should not be a arning here. Top-level nullabilities match. #Closed

333fred · 2018-12-03T22:49:52Z

Done review pass (commit 3) #Resolved

AlekseyTs · 2018-12-04T19:11:25Z

        builder.Add(getId(ErrorCode.WRN_ConvertingNullableToNonNullable));

Should WRN_NullableValueTypeMayBeNull be added to this list? #Closed

Refers to: src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs:23 in 5f14743. [](commit_id = 5f14743255e09c9143be2d7e10dd2eb49e25610a, deletion_comment = False)

AlekseyTs · 2018-12-04T19:19:10Z

                return TypeSymbolWithAnnotations.Create(resolvedType, customModifiers: customModifiers);

Possibly dropping "NotNullable" annotation? Perhaps annotations should be passed explicitly, or an assert should be added to verify that state is not going to be lost here. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs:1203 in 5f14743. [](commit_id = 5f14743255e09c9143be2d7e10dd2eb49e25610a, deletion_comment = False)

AlekseyTs · 2018-12-04T19:21:33Z

                return TypeSymbolWithAnnotations.Create(typeSymbol, customModifiers: customModifiers);

Here is possibly another place where state might be lost. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs:1213 in 5f14743. [](commit_id = 5f14743255e09c9143be2d7e10dd2eb49e25610a, deletion_comment = False)

AlekseyTs · 2018-12-04T19:30:23Z

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

-                // int?, T? where T : struct (add annotation)
-                nullableAnnotation = NullableAnnotation.Annotated;
+                case NullableAnnotation.Unknown:
+                case NullableAnnotation.NotAnnotated:


case NullableAnnotation.NotAnnotated: [](start = 16, length = 37)

Would it make sense to add an assert to TypeSymbolWithAnnotations's constructor that Unknown and NotAnnotated are never combined with a Nullable<T>? #Closed

AlekseyTs · 2018-12-04T19:49:20Z

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

@@ -380,7 +380,8 @@ protected override bool TryGetReceiverAndMember(BoundExpression expr, out BoundE
        // Remove this and populate struct members lazily to match classes.
        private Symbol GetBackingFieldIfStructProperty(Symbol symbol)
        {
-            if (symbol.Kind == SymbolKind.Property)
+            if (symbol.Kind == SymbolKind.Property &&
+                symbol.ContainingType.OriginalDefinition.SpecialType != SpecialType.System_Nullable_T)


.OriginalDefinition.SpecialType != SpecialType.System_Nullable_T) [](start = 37, length = 65)

We have a IsNullableType helper. #Closed

AlekseyTs · 2018-12-04T19:56:15Z

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

+                        {
+                            // Explicit conversion of Nullable<T> to T is equivalent to Nullable<T>.Value.
+                            int containingSlot = MakeSlot(operand);
+                            return containingSlot < 0 ? -1 : GetNullableOfTValueSlot(operand.Type, containingSlot);


return containingSlot < 0 ? -1 : GetNullableOfTValueSlot(operand.Type, containingSlot); [](start = 27, length = 88)

I would like to better understand why do we need to be able to get a slots for conversions. Could you provide some details? Offline would be fine. #Closed

As we discussed offline, in the following, need to track slots for a, a.Value, a.Value.B and a.Value.B.Value, and when evaluating ((A)a).B, we need to recognize the nullability of (A)a (not nullable) and the slot (a.Value).

struct A { B? B; } struct B { } class Program { static void F(A? a) { if (a?.B != null) _ = ((A)a).B.Value; } }

See NullableT_19.

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

AlekseyTs · 2018-12-04T20:01:55Z

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

@@ -814,6 +829,11 @@ private void InheritNullableStateOfTrackableType(int targetSlot, int valueSlot,
            }
        }

+        private TypeSymbol GetSlotType(int slot)
+        {
+            return variableBySlot[slot].Symbol.GetTypeOrReturnType().TypeSymbol;


GetTypeOrReturnType [](start = 47, length = 19)

It looks like we also have VariableType helper in the base class. Would it be more appropriate to use it here? #Closed

333fred · 2018-12-04T20:50:53Z

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

+        }
+
+        [Fact]
+        public void NullableT_IndexAndRange()


Are we expecting to get warnings here, or is this the actual final behavior? If the former, consider logging a bug. #ByDesign

There shouldn't be any warnings here. (There were warnings in an earlier iteration of this change.)

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

333fred

LGTM (commit 8)

AlekseyTs · 2018-12-04T21:48:24Z

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

+                        {
+                            // Explicit conversion of Nullable<T> to T is equivalent to Nullable<T>.Value.
+                            int containingSlot = MakeSlot(operand);
+                            return containingSlot < 0 ? -1 : GetNullableOfTValueSlot(operand.Type, containingSlot);


return containingSlot < 0 ? -1 : GetNullableOfTValueSlot(operand.Type, containingSlot); [](start = 28, length = 87)

Can we get here for a conversion from Nullable<Type1> to Type2? #Closed

Converting from Nullable<T1> to T2 would be either a Boxing conversion or an ExplicitUserDefined conversion.

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

Converting from Nullable to T2 would be either a Boxing conversion or an ExplicitUserDefined conversion.

Are you sure? What about int? -> long and the like?

In reply to: 239155510 [](ancestors = 239155510,238852151)

AlekseyTs · 2018-12-04T22:38:50Z

                    // getOperandSlots will only return slots for locations that are reference types.

Is this comment still accurate with respect to nullable value types? #Closed

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:1549 in 5de5cae. [](commit_id = 5de5caef98a5df5dcce9aedc904cf0e83a6b8107, deletion_comment = False)

AlekseyTs · 2018-12-04T22:58:08Z

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

+                        bool isLiftedConversion =
+                            operandType.IsNullableType() &&
+                            !parameterType.IsNullableType() &&
+                            parameterType.Equals(underlyingOperandType = operandType.GetNullableUnderlyingType(), TypeCompareKind.AllIgnoreOptions);


parameterType.Equals(underlyingOperandType = operandType.GetNullableUnderlyingType(), TypeCompareKind.AllIgnoreOptions) [](start = 28, length = 119)

It feels like this condition is too strict. For example, it is valid for a lifted conversion to have long as the parameter type and int? as the operand type. #Closed

The operand -> conversion "from" type conversion above should represent the conversion from int? to long?.

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

Do we have a test for a scenario like this? Also, the fact that we are "redefining" what operandType represents throughout this sequence, makes it very hard to reason about the code. Consider introducing locals with specific names.

In reply to: 239170622 [](ancestors = 239170622,238872741)

Added tests.

In reply to: 239174936 [](ancestors = 239174936,239170622,238872741)

AlekseyTs · 2018-12-04T23:13:05Z

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

+                        if (isLiftedConversion)
+                        {
+                            operandType = TypeSymbolWithAnnotations.Create(
+                                operandType.IsValueType ? compilation.GetSpecialType(SpecialType.System_Nullable_T).Construct(ImmutableArray.Create(operandType)) : operandType.TypeSymbol,


operandType.IsValueType ? compilation.GetSpecialType(SpecialType.System_Nullable_T).Construct(ImmutableArray.Create(operandType)) : operandType.TypeSymbol, [](start = 32, length = 155)

Unless I am misinterpreting something, I am not sure why do we need to wrap parameterType to nullable. What observable effect would that have on the analysis? #Closed

AlekseyTs · 2018-12-04T23:14:27Z

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

+                        {
+                            operandType = TypeSymbolWithAnnotations.Create(
+                                operandType.IsValueType ? compilation.GetSpecialType(SpecialType.System_Nullable_T).Construct(ImmutableArray.Create(operandType)) : operandType.TypeSymbol,
+                                operandAnnotation.IsAnyNullable() ? NullableAnnotation.Nullable : NullableAnnotation.NotNullable);


operandAnnotation.IsAnyNullable() ? NullableAnnotation.Nullable : NullableAnnotation.NotNullable); [](start = 32, length = 98)

It is not clear why would operandAnnotation matter, the lifted conversion method is always invoked with unwrapped (not nullable) value. #Closed

AlekseyTs · 2018-12-04T23:32:18Z

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

+                        {
+                            operandType = TypeSymbolWithAnnotations.Create(
+                                operandType.IsValueType ? compilation.GetSpecialType(SpecialType.System_Nullable_T).Construct(ImmutableArray.Create(operandType)) : operandType.TypeSymbol,
+                                operandAnnotation.IsAnyNullable() ? NullableAnnotation.Nullable : NullableAnnotation.NotNullable);


operandAnnotation.IsAnyNullable() ? NullableAnnotation.Nullable : NullableAnnotation.NotNullable [](start = 32, length = 96)

It feels like operandAnnotation shouldn't matter if methodOpt.ReturnType is nullable value type, or a reference type. #Closed

AlekseyTs · 2018-12-04T23:46:35Z

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

@@ -3377,9 +3427,7 @@ private bool HasTopLevelNullabilityConversion(TypeSymbolWithAnnotations source,
                case ConversionKind.Boxing:
                    if (!operandType.IsNull && operandType.IsValueType)
                    {
-                        // https://github.com/dotnet/roslyn/issues/29959 Should we worry about a pathological case of boxing nullable value known to be not null?
-                        //       For example, new int?(0)
-                        resultAnnotation = operandType.IsNullableType() ? NullableAnnotation.Nullable : NullableAnnotation.NotNullable;


operandType.IsNullableType() [](start = 43, length = 28)

I think we should keep the IsNullableType check. Malformed annotations in metadata can have nullable annotation of a non-nullable value type. #Closed

AlekseyTs · 2018-12-04T23:48:56Z

            case ConversionKind.Unboxing:

It feels like we should have a special handling for unboxing into a nullable value type. #Closed

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:3412 in 5de5cae. [](commit_id = 5de5caef98a5df5dcce9aedc904cf0e83a6b8107, deletion_comment = False)

cston · 2018-12-11T01:13:31Z

Thanks for the review feedback!

cston · 2018-12-11T05:37:42Z

cc @dotnet/roslyn-compiler

cston requested a review from a team as a code owner December 3, 2018 18:45

cston force-pushed the NVT branch from 6a7308a to 6b81a3b Compare December 3, 2018 19:51

333fred reviewed Dec 3, 2018

View reviewed changes

jinujoseph added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Dec 4, 2018

AlekseyTs reviewed Dec 4, 2018

View reviewed changes

333fred reviewed Dec 4, 2018

View reviewed changes

333fred approved these changes Dec 4, 2018

View reviewed changes

AlekseyTs reviewed Dec 4, 2018

View reviewed changes

cston added 21 commits December 10, 2018 13:38

Misc.

dc832a6

Fix tests

74a0742

Misc.

8602708

PR feedback

bb2d985

PR feedback

cc853f8

PR feedback

adad3e3

More tests

0e75ef3

Unboxing

76f4160

as

70b0e79

More tests

ee5b6fc

Even more tests

4d53a6a

Fix bug

89bdb18

Fix bug

0b8ddd5

Suppression operator

4f9fe33

PR feedback

5be7cff

PR feedback

9434b3a

Remove stale comment

257729e

PR feedback

ec48ae2

PR feedback

c370a80

Reference bug

3dc11f6

Fix test

4e82a55

cston force-pushed the NVT branch from 30f8a45 to 4e82a55 Compare December 10, 2018 21:48

Fix typos

5ff079d

cston merged commit 1f5f718 into dotnet:master Dec 11, 2018

cston deleted the NVT branch December 11, 2018 01:08

		@@ -835,9 +835,10 @@ private void CheckNewModifier(Symbol symbol, bool isNew, DiagnosticBag diagnosti
		for (int i = 0; i < overridingParameters.Length; i++)

Track nullability of nullable value types #31499

Track nullability of nullable value types #31499

Conversation

cston commented Dec 3, 2018

333fred Dec 3, 2018 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Dec 3, 2018 • edited by cston Loading

Choose a reason for hiding this comment

333fred Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Dec 3, 2018 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

333fred Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

333fred Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

333fred commented Dec 3, 2018 • edited by cston Loading

AlekseyTs commented Dec 4, 2018 • edited Loading

AlekseyTs commented Dec 4, 2018 • edited Loading

AlekseyTs commented Dec 4, 2018 • edited Loading

AlekseyTs Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

333fred Dec 4, 2018 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Dec 4, 2018 • edited Loading

AlekseyTs Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Dec 4, 2018 • edited Loading

cston commented Dec 11, 2018

cston commented Dec 11, 2018

333fred Dec 3, 2018 •

edited by cston

Loading

333fred Dec 3, 2018 •

edited by cston

Loading

333fred Dec 3, 2018 •

edited

Loading

333fred Dec 3, 2018 •

edited by cston

Loading

333fred Dec 3, 2018 •

edited

Loading

333fred Dec 3, 2018 •

edited

Loading

333fred Dec 3, 2018 •

edited

Loading

333fred Dec 3, 2018 •

edited

Loading

333fred commented Dec 3, 2018 •

edited by cston

Loading

AlekseyTs commented Dec 4, 2018 •

edited

Loading

AlekseyTs commented Dec 4, 2018 •

edited

Loading

AlekseyTs commented Dec 4, 2018 •

edited

Loading

AlekseyTs Dec 4, 2018 •

edited

Loading

AlekseyTs Dec 4, 2018 •

edited

Loading

AlekseyTs Dec 4, 2018 •

edited

Loading

AlekseyTs Dec 4, 2018 •

edited

Loading

333fred Dec 4, 2018 •

edited by cston

Loading

AlekseyTs Dec 4, 2018 •

edited

Loading

AlekseyTs commented Dec 4, 2018 •

edited

Loading

AlekseyTs Dec 4, 2018 •

edited

Loading

AlekseyTs Dec 4, 2018 •

edited

Loading

AlekseyTs Dec 4, 2018 •

edited

Loading

AlekseyTs Dec 4, 2018 •

edited

Loading

AlekseyTs Dec 4, 2018 •

edited

Loading

AlekseyTs commented Dec 4, 2018 •

edited

Loading