Track nullability across reference and nullable conversions of tuple elements #34303

cston · 2019-03-20T22:18:22Z

Moves tracking of nullability across conversions into ApplyConversion and adds support for reference and nullable conversions of tuple elements.

There are a couple of cases that will be handled next (nested tuple conversions, and conversion groups where the top-most conversion is not the interesting conversion), but those are likely to involve more significant changes.

jcouv · 2019-03-20T23:01:16Z

nit: Please mark your PRs with Compilers label at a minimum, so it shows up in the "to review" query Jared had shared. Thanks

cston · 2019-03-21T15:16:22Z

@dotnet/roslyn-compiler please review.

RikkiGibson · 2019-03-21T19:21:16Z

Related to #32599

jcouv · 2019-03-21T21:01:42Z

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

@@ -1521,6 +1454,8 @@ public override BoundNode VisitObjectCreationExpression(BoundObjectCreationExpre
                            }
                        }
                    }
+
+                    this.State[slot] = resultState;


this.State[slot] = resultState; [](start = 20, length = 31)

~~Should this be pushed two lines lower even (outside the if ((object)type != null))?~~
Never mind, we would not have a slot #Closed

jcouv · 2019-03-21T21:02:34Z

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

@@ -1769,6 +1704,7 @@ private ArrayTypeSymbol VisitArrayInitializer(BoundArrayCreation node)
                }
            }

+            conversions.Free();


conversions.Free(); [](start = 12, length = 19)

Thanks #Closed

jcouv · 2019-03-21T21:23:06Z

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

@@ -3942,8 +3932,10 @@ private bool HasTopLevelNullabilityConversion(TypeWithAnnotations source, TypeWi
            ParameterSymbol target = null,
            bool reportTopLevelWarnings = true,
            bool reportRemainingWarnings = true,
-            bool extensionMethodThisArgument = false)
+            bool extensionMethodThisArgument = false,
+            bool trackNullability = false)


trackNullability [](start = 17, length = 16)

nit: consider trackState instead (here and elsewhere). Otherwise, please add an xml doc comment.
A comment is probably good anyways, as this flag seems to just be an optimization (don't assign some slots in return or other scenarios where we don't care about creating slots) #Resolved

Renamed to trackMembers and added comment.

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

jcouv · 2019-03-21T21:26:11Z

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

@@ -2190,7 +2126,7 @@ public override BoundNode VisitNullCoalescingAssignmentOperator(BoundNullCoalesc
            var leftState = this.State.Clone();
            LearnFromNonNullTest(leftOperand, ref leftState);
            LearnFromNullTest(leftOperand, ref this.State);
-            TypeWithState rightResult = VisitOptionalImplicitConversion(rightOperand, targetType, UseLegacyWarnings(leftOperand), AssignmentKind.Assignment);
+            TypeWithState rightResult = VisitOptionalImplicitConversion(rightOperand, targetType, useLegacyWarnings: UseLegacyWarnings(leftOperand), trackNullability: false, AssignmentKind.Assignment);


trackNullability: false, [](start = 149, length = 24)

~~I'm unsure whether this should be be trackNullability: true (I assume we'd remove the call to TrackNullableStateForAssignment on the next line)~~
From our discussion, we'll probably move in this direction, but for now ApplyConversion only invokes Track methods when it needs to create new slots (ie. for Nullable and tuples). #Closed

jcouv · 2019-03-21T22:22:53Z

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

+                trackConvertedValue(targetElements[i], conversions[i], valueElements[i]);
+            }
+
+            void trackConvertedValue(FieldSymbol targetField, Conversion conversion, FieldSymbol valueField)


trackConvertedValue [](start = 17, length = 19)

How does this function relate to ApplyConversion(..., trackNullability: true)? Could we just leverage ApplyConversion? #Closed

This function should probably be merged with ApplyConversion. I'm attempting to do that with the next PR.

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

jcouv · 2019-03-21T22:30:41Z

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

@@ -893,7 +827,7 @@ private void ReportNullabilityMismatchInAssignment(SyntaxNode syntaxNode, object
                    // https://github.com/dotnet/roslyn/issues/31395: We should copy all tracked state from `value` regardless of
                    // BoundNode type but we'll need to handle cycles (see NullableReferenceTypesTests.Members_FieldCycle_07).
                    // For now, we copy a limited set of BoundNode types that shouldn't contain cycles.
-                    if (((targetType.Type.IsReferenceType || targetType.TypeKind == TypeKind.TypeParameter) && (isSupportedReferenceTypeValue(value) || targetType.Type.IsAnonymousType)) ||
+                    if (((targetType.Type.IsReferenceType || targetType.TypeKind == TypeKind.TypeParameter) && (valueOpt is null || isSupportedReferenceTypeValue(valueOpt) || targetType.Type.IsAnonymousType)) ||


((targetType.Type.IsReferenceType || targetType.TypeKind == TypeKind.TypeParameter) && (valueOpt is null || isSupportedReferenceTypeValue(valueOpt) || targetType.Type.IsAnonymousType)) || [](start = 24, length = 187)

This is getting gnarly, but I don't see a good way to simplify :-( #Closed

jcouv

LGTM Thanks (iteration 1)

jcouv · 2019-03-21T22:31:34Z

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

@@ -518,6 +518,11 @@ protected override int MakeSlot(BoundExpression node)
                    }
                case BoundKind.Conversion:
                    {
+                        int slot = getPlaceholderSlot(node);
+                        if (slot > 0)


should this be >= 0? (I'm not sure if it's possible to observe) #Closed

The slot is never 0. This matches other tests in this class that are either > 0 or < 0.

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

cston · 2019-03-22T14:47:59Z

@dotnet/roslyn-compiler for a second review, thanks.

cston · 2019-03-25T15:03:05Z

@dotnet/roslyn-compiler please review.

…ions

jcouv · 2019-03-25T21:00:57Z

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

+    }
+}";
+            var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
+            // https://github.com/dotnet/roslyn/issues/32599: Track state across tuple element conversions.


// #32599: Track state across tuple element conversions. [](start = 8, length = 99)

Comment could be removed? #Pending

We should report warnings marked above. I have a fix for this case in the next PR.

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

jcouv · 2019-03-25T21:05:45Z

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

+}";
+            var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
+            // https://github.com/dotnet/roslyn/issues/32599: Track state across tuple element conversions.
+            comp.VerifyDiagnostics();
        }


} [](start = 8, length = 1)

not related to this PR: it may be good to add a test showing the user-defined conversions on tuples are ignored for analysis (we don't expect users to define their own ValueTuple types) #Pending

I'm including tests for user-defined conversions between tuples and custom types in the next PR.

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

jcouv

LGTM Thanks (iteration 4)

333fred · 2019-03-26T17:46:57Z

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

+            Debug.Assert(valueSlot > 0);
+
+            var valueTuple = operandType as TupleTypeSymbol;
+            if (valueTuple is null)


Consider using pattern matching if (operandType is TupleTypeSymbol valueTuple) #Pending

333fred · 2019-03-26T17:49:04Z

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

@@ -4164,6 +4171,19 @@ private bool HasTopLevelNullabilityConversion(TypeWithAnnotations source, TypeWi
                    break;

                case ConversionKind.ImplicitNullable:
+                    if (trackMembers && node.Kind == BoundKind.Conversion)


Consider using an is check instead of the kind and conversion. #Pending

These checks should be removed soon.

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

333fred · 2019-03-26T17:49:20Z

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

                case ConversionKind.ExplicitTupleLiteral:
                case ConversionKind.ExplicitTuple:
+                    if (trackMembers && node.Kind == BoundKind.Conversion)


Same comment. #Pending

333fred · 2019-03-26T17:50:00Z

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

@@ -4164,6 +4171,19 @@ private bool HasTopLevelNullabilityConversion(TypeWithAnnotations source, TypeWi
                    break;

                case ConversionKind.ImplicitNullable:
+                    if (trackMembers && node.Kind == BoundKind.Conversion)


Can this whole check be broken out into a local function? Seems to be duplicated here and for tuple conversions. #Pending

The check will be simplified in the next PR and removed later.

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

333fred

LGTM (commit 4). Couple of minor refactoring suggestions that can be addressed as a followup.

cston requested a review from a team as a code owner March 20, 2019 22:18

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Mar 20, 2019

jcouv modified the milestones: 16.2, 16.1.P1 Mar 20, 2019

jcouv reviewed Mar 21, 2019

View reviewed changes

jcouv approved these changes Mar 21, 2019

View reviewed changes

jcouv reviewed Mar 21, 2019

View reviewed changes

jcouv self-assigned this Mar 21, 2019

cston force-pushed the 32599 branch from 15055e6 to f0ccca8 Compare March 22, 2019 05:04

cston added 3 commits March 25, 2019 08:06

Track nullability across reference and nullable tuple element convers…

09384bb

…ions

PR feedback

7199a28

Add test

6c4bfc3

cston force-pushed the 32599 branch from f0ccca8 to 6c4bfc3 Compare March 25, 2019 15:19

Track state across nested tuple conversions

33dfe34

jcouv reviewed Mar 25, 2019

View reviewed changes

jcouv approved these changes Mar 25, 2019

View reviewed changes

333fred reviewed Mar 26, 2019

View reviewed changes

333fred approved these changes Mar 26, 2019

View reviewed changes

cston merged commit 7aac2e3 into dotnet:master Mar 26, 2019

cston deleted the 32599 branch March 26, 2019 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track nullability across reference and nullable conversions of tuple elements #34303

Track nullability across reference and nullable conversions of tuple elements #34303

cston commented Mar 20, 2019 •

edited

Loading

jcouv commented Mar 20, 2019

cston commented Mar 21, 2019

RikkiGibson commented Mar 21, 2019

jcouv Mar 21, 2019 •

edited

Loading

jcouv Mar 21, 2019 •

edited

Loading

jcouv Mar 21, 2019 •

edited by cston

Loading

cston Mar 21, 2019

jcouv Mar 21, 2019 •

edited

Loading

jcouv Mar 21, 2019 •

edited

Loading

cston Mar 21, 2019

jcouv Mar 21, 2019 •

edited

Loading

jcouv left a comment

jcouv Mar 21, 2019 •

edited

Loading

cston Mar 21, 2019 •

edited

Loading

cston commented Mar 22, 2019

cston commented Mar 25, 2019

jcouv Mar 25, 2019 •

edited by cston

Loading

cston Mar 25, 2019

jcouv Mar 25, 2019 •

edited by cston

Loading

cston Mar 25, 2019

jcouv left a comment

333fred Mar 26, 2019 •

edited by cston

Loading

333fred Mar 26, 2019 •

edited by cston

Loading

cston Mar 26, 2019

333fred Mar 26, 2019 •

edited by cston

Loading

333fred Mar 26, 2019 •

edited by cston

Loading

cston Mar 26, 2019

333fred left a comment

Track nullability across reference and nullable conversions of tuple elements #34303

Track nullability across reference and nullable conversions of tuple elements #34303

Conversation

cston commented Mar 20, 2019 • edited Loading

jcouv commented Mar 20, 2019

cston commented Mar 21, 2019

RikkiGibson commented Mar 21, 2019

jcouv Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv Mar 21, 2019 • edited by cston Loading

Choose a reason for hiding this comment

cston Mar 21, 2019

Choose a reason for hiding this comment

jcouv Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

cston Mar 21, 2019

Choose a reason for hiding this comment

jcouv Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

cston Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

cston commented Mar 22, 2019

cston commented Mar 25, 2019

jcouv Mar 25, 2019 • edited by cston Loading

Choose a reason for hiding this comment

cston Mar 25, 2019

Choose a reason for hiding this comment

jcouv Mar 25, 2019 • edited by cston Loading

Choose a reason for hiding this comment

cston Mar 25, 2019

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

333fred Mar 26, 2019 • edited by cston Loading

Choose a reason for hiding this comment

333fred Mar 26, 2019 • edited by cston Loading

Choose a reason for hiding this comment

cston Mar 26, 2019

Choose a reason for hiding this comment

333fred Mar 26, 2019 • edited by cston Loading

Choose a reason for hiding this comment

333fred Mar 26, 2019 • edited by cston Loading

Choose a reason for hiding this comment

cston Mar 26, 2019

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

cston commented Mar 20, 2019 •

edited

Loading

jcouv Mar 21, 2019 •

edited

Loading

jcouv Mar 21, 2019 •

edited

Loading

jcouv Mar 21, 2019 •

edited by cston

Loading

jcouv Mar 21, 2019 •

edited

Loading

jcouv Mar 21, 2019 •

edited

Loading

jcouv Mar 21, 2019 •

edited

Loading

jcouv Mar 21, 2019 •

edited

Loading

cston Mar 21, 2019 •

edited

Loading

jcouv Mar 25, 2019 •

edited by cston

Loading

jcouv Mar 25, 2019 •

edited by cston

Loading

333fred Mar 26, 2019 •

edited by cston

Loading

333fred Mar 26, 2019 •

edited by cston

Loading

333fred Mar 26, 2019 •

edited by cston

Loading

333fred Mar 26, 2019 •

edited by cston

Loading