Fix result and slot of conditional receiver by jcouv · Pull Request #33997 · dotnet/roslyn

jcouv · 2019-03-09T20:35:02Z

As the Dump() for a conditional access (excerpted below) shows, the call or member access uses a placeholder ("conditional receiver") in place of the actual receiver.

There are two parts of this PR:

we ensure that visiting a conditional receiver produces the correct result type,
we ensure that asking for a slot for a conditional receiver produces the correct slot.

To do that, we store some information (the result and the slot) about the real receiver when we visit a conditional access.

Fixes Roslyn fails to apply inferred type arguments' nullability after ?. access #33289 (fails to apply inferred type arguments' nullability after ?. access)
Fixes Dereference doesn't update null state if there's an unrelated conditional qualifier #31909 (Dereference doesn't update null state if there's an unrelated conditional qualifier)
Fixes Variables known to be non-null sometimes artifically restricted to one variable #33347 (Variables known to be non-null sometimes artifically restricted to one variable)
Fixes Nullable flow analysis works incorrectly with nested conditional accesses #31905 (Nullable flow analysis works incorrectly with nested conditional accesses)
Fixes Incorrect nullability analysis in conditional access #29956 (PROTOTYPE markers in VisitConditionalAccess and VisitConditionalReceiver)

conditionalAccess
    ├─receiver
    │ └─local
    │   ├─localSymbol: Container<System.String> x
    │   ├─type: Container<System.String>
    │   └─isSuppressed: False
    ├─accessExpression
    │ └─call
    │   ├─receiverOpt
    │   │ └─fieldAccess
    │   │   ├─receiverOpt
    │   │   │ └─conditionalReceiver
    │   │   │   ├─id: 0
    │   │   │   ├─type: Container<System.String>
    │   │   │   └─isSuppressed: False
    │   │   ├─fieldSymbol: System.String Container<System.String>.Field
    │   │   └─isSuppressed: False
    │   ├─method: System.String System.String.ToString()
    │   ├─type: System.String
    │   └─isSuppressed: False
    ├─type: System.String
    └─isSuppressed: False

jaredpar · 2019-03-10T23:39:09Z

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

-                // so we reset before leaving the function.
-                _lastConditionalAccessSlot = -1;
-
+                _lastConditionalAccessSlot = previousConditionalAccessSlot;


Did you consider a finally block here? Whenever we have resets like this I worry about the risk of a later change inserting a return in the middle. I may be over paranoid here. #Pending

I don't think we need a try/finally here because any exception will terminate the analysis, so the state we leave it in won't matter.
We have very limited exception handling in flow analysis (only one finally to free the pooled object when hitting stackguard). #Pending

The concern is with returning early rather than exceptions. I think I'd prefer to have this reset in a finally block.

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

Sorry, I'd misunderstood. Will fix #Pending

jaredpar · 2019-03-10T23:43:35Z

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


            var receiver = node.Receiver;
            var receiverType = VisitRvalueWithState(receiver);
+            _currentConditionalReceiverVisitResult = _visitResult;


Consider a Debug.Assert(_currentConditionalReceiverVisitResult == default predicate to this method. #Resolved

I think that wouldn't be the case in x?.y?.f when we get the y. I'll double-check. #Resolved

jaredpar

jcouv · 2019-03-11T17:11:28Z

@dotnet/roslyn-compiler for a second review. Thanks #Resolved

cston · 2019-03-11T17:43:39Z

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

+            var rvalueType = _currentConditionalReceiverVisitResult.RValueType.Type;
+            if (rvalueType?.IsNullableType() == true)
+            {
+                rvalueType = rvalueType.GetNullableUnderlyingTypeWithAnnotations().TypeSymbol;


GetNullableUnderlyingTypeWithAnnotations().TypeSymbol [](start = 40, length = 53)

GetNullableUnderlyingType() #Pending

cston · 2019-03-11T17:49:36Z

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

+        var x = Create(s1);
+        var y = Create(s2);
+        x?.Field /*1*/
+            .Extension(y?.Field.ToString());


Does this test that the conditional access receiver field is reset? Is there overlap between the two receivers?

I don't think any scenario can currently hit the "reset". I've included the "reset" to be safe for future changes.
The reason we can't hit it now is that to have a nested conditional, you must have some invocation or indexer with arguments, but then we're no longer dealing with fields (ie. slots).

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

cston · 2019-03-11T17:52:19Z

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

+    public C? f;
+    int? Test1(C? c) => c?.f.M(c.f.ToString()); // nested use of `c.f` is safe
+    int? Test2(C? c) => c.f.M(c.f.ToString());
+    int M(string s) => s.Length;


int M(string s) => s.Length; [](start = 4, length = 28)

Can this be removed?

I've simplified M a bit, but otherwise left it. I need it to test access on c.f nested in expression with c?.f.

cston · 2019-03-11T17:53:52Z

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

+    void Test2(C? c) => c.Nested?.M(c.Nested.ToString());
+    void Test3(C c) => c?.Nested?.M(c.Nested.ToString());
+
+    void M(string s) => throw null!;


void M(string s) => throw null!; [](start = 4, length = 32)

Can this be removed?

I didn't understand. M is used above.

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

gafter · 2019-04-12T00:44:05Z

We can see from these three lines that the underlying issue has not been fixed.

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

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

jcouv added this to the 16.1.P1 milestone Mar 9, 2019

jcouv self-assigned this Mar 9, 2019

Fix result of conditional receiver

6e2ba71

jcouv force-pushed the cond-nullability branch from 1b382a9 to 948e974 Compare March 10, 2019 01:40

jcouv changed the title ~~Fix result of conditional receiver~~ Fix result and slot of conditional receiver Mar 10, 2019

jcouv marked this pull request as ready for review March 10, 2019 03:44

jcouv requested a review from a team as a code owner March 10, 2019 03:44

jcouv force-pushed the cond-nullability branch from 948e974 to 69f7810 Compare March 10, 2019 03:50

Track slots for conditional receivers

2e410b8

jcouv force-pushed the cond-nullability branch from 69f7810 to 2e410b8 Compare March 10, 2019 03:58

jaredpar reviewed Mar 10, 2019

View reviewed changes

jaredpar approved these changes Mar 10, 2019

View reviewed changes

cston reviewed Mar 11, 2019

View reviewed changes

Address feedback

0df246e

cston approved these changes Mar 11, 2019

View reviewed changes

jcouv closed this Mar 11, 2019

jcouv reopened this Mar 11, 2019

jcouv closed this Mar 11, 2019

jcouv reopened this Mar 11, 2019

jcouv merged commit 74502ba into dotnet:master Mar 12, 2019

jcouv deleted the cond-nullability branch March 12, 2019 03:07

gafter mentioned this pull request Apr 12, 2019

Incorrect nullability analysis in conditional access #29956

Closed

Conversation

jcouv commented Mar 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar Mar 10, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Mar 10, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Mar 11, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

cston Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

cston Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

gafter commented Apr 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcouv commented Mar 9, 2019 •

edited

Loading

jaredpar Mar 10, 2019 •

edited by jcouv

Loading

jcouv Mar 11, 2019 •

edited

Loading

jcouv Mar 11, 2019 •

edited

Loading

jaredpar Mar 10, 2019 •

edited by jcouv

Loading

jcouv Mar 11, 2019 •

edited

Loading

jcouv commented Mar 11, 2019 •

edited

Loading

cston Mar 11, 2019 •

edited by jcouv

Loading