Added support for setting null-conditional access receiver state. #28722

333fred · 2018-07-20T01:07:59Z

For something like

if (c?.d != null)
{
    c.d.ToString();
}

we will now track nullability of conditionally accessed expressions.

@dotnet/roslyn-compiler @jcouv @cston for review.

cston · 2018-07-20T05:54:15Z

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

+                }
+            }
+
+            ImmutableArray<int> getOperandSlots(BoundExpression operand)


Consider having the caller pass an ArrayBuilder argument rather than creating an ImmutableArray result. #Resolved

cston · 2018-07-20T05:55:51Z

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

+                {
+                    // Due to the nature of binding, if there are conditional access they will be at the top of the bound tree,
+                    // potentially with a conversion on top of it. We go through any conditional accesses, adding slots for the
+                    // condtional receivers if they have them. If we ever get to a receiver that MakeSlot doesn't return a slot


condtional [](start = 23, length = 10)

Typo. #Resolved

cston · 2018-07-20T05:56:14Z

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

+                    //     a?.GetB()?.C // a is a field, GetB is a method, and C is a property
+                    //
+                    // The top of the tree is the a?.GetB() conditional call. We'll ask for a slot for a, and we'll get one because
+                    // locals have slots. The AccessExpression of the BoundCondtionalAccess is another BoundCondtionalAccess, this time


Condtional [](start = 75, length = 10)

Typo, here and later in the line. #Resolved

cston · 2018-07-20T05:56:35Z

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

+                    //
+                    // The top of the tree is the a?.GetB() conditional call. We'll ask for a slot for a, and we'll get one because
+                    // locals have slots. The AccessExpression of the BoundCondtionalAccess is another BoundCondtionalAccess, this time
+                    // with a reciever of the GetB() BoundCall. Attempting to get a slot for this receiver will fail, and we'll


reciever [](start = 30, length = 8)

Typo. #Resolved

cston · 2018-07-20T06:02:22Z

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

+                                // If we got a slot we must have processed the previous conditional receiver.
+                                Debug.Assert(_lastConditionalAccessSlot == -1);
+
+                                if (slot >= this.State.Capacity) Normalize(ref this.State);


Normalize(ref this.State); [](start = 65, length = 26)

Consider calling Normalize once, unconditionally, in the caller of getOperandSlots. #Closed

cston · 2018-07-20T06:12:47Z

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

+                                slotListBuilder.Add(slot);
+
+                                // When MakeSlot is called on the nested AccessExpression, it will recurse through receivers
+                                // until it gets to the BoundConditionalRecevier associated with this node. In our override


In our override [](start = 108, length = 15)

This sentence stops short. #Resolved

cston · 2018-07-20T06:13:26Z

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

+                                slotListBuilder.Add(slot);
+
+                                // When MakeSlot is called on the nested AccessExpression, it will recurse through receivers
+                                // until it gets to the BoundConditionalRecevier associated with this node. In our override


Recevier [](start = 72, length = 8)

Typo. #Closed

cston · 2018-07-20T14:08:25Z

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

+                            if (!slots.IsEmpty)
+                            {
+                                Split();
+                                foreach (int slot in slots)


Perhaps:

ref LocalState state = ref (op == BinaryOperatorKind.Equal) ? ref this.StateWhenFalse : ref this.StateWhenTrue; foreach (var slot in slots) { state[slot] = true; } ``` #Resolved

cston · 2018-07-20T17:36:51Z

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


-                                if (op == BinaryOperatorKind.Equal)
+                                if (slot >= this.State.Capacity) Normalize(ref this.State);
+                                if (slotListBuilder == null)


Let's avoid creating the builder in several places. Consider creating upfront in the caller. #Resolved

cston · 2018-07-20T17:38:56Z

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

+                            {
+                                // If we didn't get a slot, it's possible that the current _lastConditionalSlot was never processed,
+                                // so we reset before leaving the function.
+                                _lastConditionalAccessSlot = -1;


Can we move this comment and assignment so it only appears once? Perhaps break; out of the switch statements when done, and have _lastConditionalAccessSlot = -1; return; after the switch. #Resolved

333fred · 2018-07-20T22:15:31Z

@cston addressed feedback.

cston · 2018-07-20T22:49:12Z

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

@@ -1461,50 +1461,46 @@ protected override void AfterLeftChildHasBeenVisited(BoundBinaryOperator binary)

                        if (operandComparedToNull.Type?.IsReferenceType == true)
                        {
-                            ImmutableArray<int> slots = getOperandSlots(operandComparedToNull);
-                            if (!slots.IsEmpty)
+                            Normalize(ref this.State);


Normalize should be called after getOperandSlots. #Resolved

cston · 2018-07-20T22:49:39Z

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

-                                    {
-                                        this.StateWhenTrue[slot] = true;
-                                    }
+                                    state[slot] = true;
                                }
                            }


slotBuilder.Free(); #Resolved

cston · 2018-07-20T22:50:46Z

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

                                }
                            }
                        }
                    }
                }
            }

-            ImmutableArray<int> getOperandSlots(BoundExpression operand)
+            void getOperandSlots(BoundExpression operand, ArrayBuilder<int> slotListBuilder)


Minor point: perhaps rename to slotBuilder or simply builder. #Resolved

cston · 2018-07-20T22:54:42Z

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

+
+                                // When MakeSlot is called on the nested AccessExpression, it will recurse through receivers
+                                // until it gets to the BoundConditionalReceiver associated with this node. In our override,
+                                // we substitute this slot when we encounter a BoundConditionalAccessReceiver, and reset the


Should that be BoundConditionalReceiver? #Resolved

I keep getting if confused with IConditionalAccessInstanceOperation.

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

cston · 2018-07-20T22:57:40Z

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

+                            if (slot > 0)
+                            {
+                                // If we got a slot we must have processed the previous conditional receiver.
+                                Debug.Assert(_lastConditionalAccessSlot == -1);


Is this default case handling BoundConditionalAccess / BoundConditionalReceiver or all other operands except those? If the latter, consider moving the Debug.Assert up before MakeSlot. #ByDesign

The latter.

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

@cston after thinking though the state changes, I'm moving this back into the conditional. In the case of a?.ToString(), we won't have processed the BoundConditionalReceiver from the ToString BoundCall by this point. It is correct as originally written.

In reply to: 204188950 [](ancestors = 204188950,204187214)

cston · 2018-07-20T22:59:11Z

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

+            {
+                Debug.Assert(operand != null);
+                Debug.Assert(_lastConditionalAccessSlot == -1);
+                int slot;


Consider moving slot declaration closer to the use - either inside each case statement or at least inside the do loop. #Resolved

cston · 2018-07-21T00:27:40Z

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

+                //             c6._cField.GetC().ToString(); // warn
+                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c6._cField.GetC()").WithLocation(79, 13)
+            );
+        }


Please add a test for unconstrained T.

static void F<T>(T t) { if (t?.ToString() != null) t.ToString(); } ``` #Resolved

jcouv · 2018-07-21T18:32:17Z

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

-                            if (slot > 0)
+                            var slotBuilder = ArrayBuilder<int>.GetInstance();
+                            getOperandSlots(operandComparedToNull, slotBuilder);
+                            Normalize(ref this.State);


Can we move the Normalize call down into the conditional (only normalize if slots were returned/created)? #Resolved

I just cannot win: #28722 (comment) #Closed

I think @jcouv is just suggesting moving Normalize down two lines, into the if. Seems reasonable.

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

Yes, that's what I meant. #Closed

jcouv · 2018-07-21T18:37:35Z

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

-
-                            if (slot > 0)
+                            var slotBuilder = ArrayBuilder<int>.GetInstance();
+                            getOperandSlots(operandComparedToNull, slotBuilder);


May be good to add a comment like // ex: in 'a?.b?.c' we'll update the states for 'a', 'b' and 'c'. #Closed

jcouv · 2018-07-21T18:40:13Z

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

+        else if (c3?._cField != null)
+        {
+            c3._cField.ToString();
+            c3._cField._cField.ToString(); // warn


Nit: to maximize readability, I suggest using unique labels // warn 1, // warn 2 and 3, etc so it's really easy to match the diagnostics to the code. #Resolved

jcouv · 2018-07-21T18:43:36Z

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

+                //             c1.ToString(); // warn
+                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c1").WithLocation(13, 13)
+            );
+        }


Could you add a test with a nested null test with an null-conditional operator?

if (a?.GetB(c?.d == null).e == null) ... ``` #Resolved

333fred · 2018-07-23T20:59:19Z

@jcouv addressed feedback.

jcouv · 2018-07-23T21:07:13Z

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

                                Split();
-
-                                if (op == BinaryOperatorKind.Equal)
+                                ref LocalState state = ref (op == BinaryOperatorKind.Equal) ? ref this.StateWhenFalse : ref this.StateWhenTrue;


= ref [](start = 53, length = 5)

Random thought: this may be the first ref-conditional in our codebase.

This one already existed: https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs#L2458 #Closed

jcouv

LGTM Thanks

cston · 2018-07-23T21:31:24Z

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

+            c2.Prop.ToString(); // warn 1 2
+        }
+    }
+}", parseOptions: TestOptions.Regular8);


Is the following case interesting?

static void M(object? x, object? y) { if ((x = y)?.GetHashCode() != null) x.ToString(); } ``` #Resolved

It is, but because GetHashCode() returns a nullable int, and not a reference type. Should be a simple fix.

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

…operators * features/NullableReferenceTypes: `is` operator informs nullability analysis (dotnet#28686) Make module-level NonNullTypes explicit in tests (dotnet#28769) Lazily evaluate NonNullTypes (dotnet#28736) Calculate TypeSymbolWithAnnotations.IsNullable lazily (dotnet#28687)

cston · 2018-07-23T22:05:47Z

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

-                                slotBuilder.Add(slot);
+                                // We need to continue the walk regardless of whether the receiver is a value
+                                // type, but we only want to update the slots of reference types
+                                if (conditional.Receiver.Type?.IsReferenceType == true)


Are we handling unconstrained type parameter types? #Resolved

I'll verify this when I address #28792, as I'm not changing our existing behavior much here.

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

Turns out I need to address this now, as it's breaking a test.

In reply to: 204573537 [](ancestors = 204573537,204568700)

…receivers.

cston · 2018-07-23T23:32:39Z

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

+
+                bool shouldUpdateType(TypeSymbol operandType) =>
+                    operandType != null &&
+                    (operandType.IsReferenceType == true || operandType.IsUnconstrainedTypeParameter());


(object)operandType != null &&

jcouv · 2018-07-25T18:22:17Z

I forgot to ask: could you update the speclet to reflect this new behavior? There is a section on "null tests".

333fred requested review from cston, jcouv and a team July 20, 2018 01:07

Added support for settings null-conditional access receiver state.

5b1d18a

333fred force-pushed the null-conditional-operators branch from 7e63107 to 5b1d18a Compare July 20, 2018 01:11

cston reviewed Jul 20, 2018

View reviewed changes

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jul 20, 2018

jcouv added this to the 16.0 milestone Jul 20, 2018

cston reviewed Jul 20, 2018

View reviewed changes

Address PR feedback.

f0c590a

cston reviewed Jul 20, 2018

View reviewed changes

Address more PR feedback.

631e421

cston reviewed Jul 21, 2018

View reviewed changes

cston approved these changes Jul 21, 2018

View reviewed changes

jcouv reviewed Jul 21, 2018

View reviewed changes

Addressed more PR feedback.

179d459

jcouv reviewed Jul 23, 2018

View reviewed changes

jcouv approved these changes Jul 23, 2018

View reviewed changes

cston reviewed Jul 23, 2018

View reviewed changes

333fred added 2 commits July 23, 2018 14:35

Cover additional case of comparing to a non-reference type.

08a7d11

cston reviewed Jul 23, 2018

View reviewed changes

Address unconstrained type parameter variables as conditional access …

d8fba1f

…receivers.

cston reviewed Jul 23, 2018

View reviewed changes

Correct null check to check for reference identity.

4ca10c2

333fred force-pushed the null-conditional-operators branch from de4c697 to 4ca10c2 Compare July 24, 2018 00:03

333fred merged commit a818e66 into dotnet:features/NullableReferenceTypes Jul 24, 2018

333fred deleted the null-conditional-operators branch July 24, 2018 17:48

jcouv mentioned this pull request Jul 25, 2018

Infer nullability of receiver and member from ?. expression #25870

Closed

jcouv mentioned this pull request Aug 5, 2018

Possible Null Reference on t?.ToString() for unconstrained T #28792

Closed

Added support for setting null-conditional access receiver state. #28722

Added support for setting null-conditional access receiver state. #28722

Conversation

333fred commented Jul 20, 2018

cston Jul 20, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

333fred commented Jul 20, 2018

cston Jul 20, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

cston Jul 21, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

jcouv Jul 21, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

333fred Jul 21, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 22, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 21, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 21, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

jcouv Jul 21, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

333fred commented Jul 23, 2018

Choose a reason for hiding this comment

333fred Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

cston Jul 23, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jul 23, 2018 • edited by 333fred 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

jcouv commented Jul 25, 2018

cston Jul 20, 2018 •

edited by 333fred

Loading

cston Jul 20, 2018 •

edited by 333fred

Loading

cston Jul 20, 2018 •

edited by 333fred

Loading

cston Jul 20, 2018 •

edited by 333fred

Loading

cston Jul 20, 2018 •

edited by jcouv

Loading

cston Jul 20, 2018 •

edited by 333fred

Loading

cston Jul 20, 2018 •

edited by jcouv

Loading

cston Jul 20, 2018 •

edited by 333fred

Loading

cston Jul 20, 2018 •

edited by 333fred

Loading

cston Jul 20, 2018 •

edited by 333fred

Loading

cston Jul 20, 2018 •

edited by 333fred

Loading

cston Jul 20, 2018 •

edited by 333fred

Loading

cston Jul 20, 2018 •

edited by 333fred

Loading

cston Jul 20, 2018 •

edited by 333fred

Loading

cston Jul 20, 2018 •

edited by 333fred

Loading

cston Jul 20, 2018 •

edited by 333fred

Loading

cston Jul 21, 2018 •

edited by 333fred

Loading

jcouv Jul 21, 2018 •

edited by 333fred

Loading

333fred Jul 21, 2018 •

edited by jcouv

Loading

jcouv Jul 22, 2018 •

edited

Loading

jcouv Jul 21, 2018 •

edited

Loading

jcouv Jul 21, 2018 •

edited by 333fred

Loading

jcouv Jul 21, 2018 •

edited by 333fred

Loading

333fred Jul 23, 2018 •

edited

Loading

cston Jul 23, 2018 •

edited by 333fred

Loading

cston Jul 23, 2018 •

edited by 333fred

Loading