Set null state on dereference #30564

cston · 2018-10-17T19:38:49Z

cston · 2018-10-17T19:39:07Z

@dotnet/roslyn-compiler please review.

cston · 2018-10-18T14:08:09Z

@dotnet/roslyn-compiler, please review. This change prevents redundant (and incorrect) warnings on subsequent dereferences of the same variable. The actual code change is small - most of the changes are to the tests.

gafter · 2018-10-18T17:19:35Z

@gafter is added to the review. #Closed

gafter

AlekseyTs · 2018-10-18T17:55:51Z

        CheckPossibleNullReceiver(node.Expression);

It feels incorrect to mark node.Expression as not null at this point, the value won't be "checked" until after all indices are evaluated. #Closed

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:1357 in cf7548d. [](commit_id = cf7548d58a41cf652d841bfea163dcbd21081a6d, deletion_comment = False)

AlekseyTs · 2018-10-18T17:56:52Z

            CheckPossibleNullReceiver(receiverOpt);

It feels incorrect to mark receiverOpt as not null at this point, the value won't be "checked" until after all arguments are evaluated. #Closed

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:1946 in cf7548d. [](commit_id = cf7548d58a41cf652d841bfea163dcbd21081a6d, deletion_comment = False)

AlekseyTs · 2018-10-18T17:57:58Z

        CheckPossibleNullReceiver(receiverOpt);

It feels incorrect to mark receiverOpt as not null at this point, the value won't be "checked" until after all arguments are evaluated. #Closed

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:3679 in cf7548d. [](commit_id = cf7548d58a41cf652d841bfea163dcbd21081a6d, deletion_comment = False)

AlekseyTs · 2018-10-18T18:01:50Z

        CheckPossibleNullReceiver(receiver);

If this node can be a part of dynamic invocation, it feels incorrect to mark receiver as not null at this point, the value won't be "checked" until after all arguments are evaluated.

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:4185 in cf7548d. [](commit_id = cf7548d58a41cf652d841bfea163dcbd21081a6d, deletion_comment = False)

AlekseyTs · 2018-10-18T18:03:10Z

            CheckPossibleNullReceiver(receiverOpt);

It feels incorrect to mark receiverOpt as not null at this point. Is value "checked" before argument is evaluated? #Closed

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:4213 in cf7548d. [](commit_id = cf7548d58a41cf652d841bfea163dcbd21081a6d, deletion_comment = False)

AlekseyTs · 2018-10-18T18:03:57Z

        CheckPossibleNullReceiver(receiver);

It feels incorrect to mark receiver as not null at this point, the value won't be "checked" until after all arguments are evaluated. #Closed

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:4301 in cf7548d. [](commit_id = cf7548d58a41cf652d841bfea163dcbd21081a6d, deletion_comment = False)

AlekseyTs · 2018-10-18T18:06:54Z

                    VisitMemberAccess(propertyAccess.ReceiverOpt, propertyAccess.PropertySymbol, asLvalue: true);

If this is a target of an assignment, is receiver actually going to be checked before the value (right hand side) is evaluated?

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:453 in cf7548d. [](commit_id = cf7548d58a41cf652d841bfea163dcbd21081a6d, deletion_comment = False)

AlekseyTs · 2018-10-18T18:07:49Z

                    VisitMemberAccess(eventAccess.ReceiverOpt, eventAccess.EventSymbol, asLvalue: true);

Similar question here

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:459 in cf7548d. [](commit_id = cf7548d58a41cf652d841bfea163dcbd21081a6d, deletion_comment = False)

AlekseyTs · 2018-10-18T18:09:52Z

        VisitMemberAccess(node.ReceiverOpt, node.PropertySymbol, asLvalue: false);

It would be good to have a clear understanding what constructs go through here

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:3671 in cf7548d. [](commit_id = cf7548d58a41cf652d841bfea163dcbd21081a6d, deletion_comment = False)

AlekseyTs · 2018-10-18T18:10:37Z

        VisitMemberAccess(node.ReceiverOpt, node.FieldSymbol, asLvalue: false);

It would be good to have a clear understanding what constructs go through here

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:3665 in cf7548d. [](commit_id = cf7548d58a41cf652d841bfea163dcbd21081a6d, deletion_comment = False)

AlekseyTs · 2018-10-18T18:11:34Z

                    VisitMemberAccess(fieldAccess.ReceiverOpt, fieldAccess.FieldSymbol, asLvalue: true);

If this is a target of an assignment, is receiver actually going to be checked before the value (right hand side) is evaluated? #Pending

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:447 in cf7548d. [](commit_id = cf7548d58a41cf652d841bfea163dcbd21081a6d, deletion_comment = False)

AlekseyTs · 2018-10-18T18:12:25Z

        VisitMemberAccess(node.ReceiverOpt, node.EventSymbol, asLvalue: false);

It would be good to have a clear understanding what constructs go through here

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:3690 in cf7548d. [](commit_id = cf7548d58a41cf652d841bfea163dcbd21081a6d, deletion_comment = False)

AlekseyTs · 2018-10-18T18:14:38Z

Done with review pass (iteration 2). Tests are not looked at. #Closed

cston · 2018-10-18T21:29:13Z

@dotnet-bot retest this please

cston · 2018-10-18T22:41:35Z

        CheckPossibleNullReceiver(node.Expression);

Good catch. In these cases, we're reporting one warning dereferencing the variable, but not reporting the same warning for other dereferences within the same expression. Logged ##30598 and added test NullableReferenceTypesTests.NotNullAfterDereference_Indexer.

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

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:1357 in cf7548d. [](commit_id = cf7548d58a41cf652d841bfea163dcbd21081a6d, deletion_comment = False)

AlekseyTs · 2018-10-18T22:57:08Z

        // https://github.com/dotnet/roslyn/issues/29855: there should only be two diagnostics

The comment probably should be adjusted #Closed

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

AlekseyTs · 2018-10-18T23:00:16Z

        // https://github.com/dotnet/roslyn/issues/29855: there should only be two diagnostics

Here too #Closed

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

AlekseyTs · 2018-10-18T23:00:34Z

        // https://github.com/dotnet/roslyn/issues/29855: there should only be two diagnostics

And here #Closed

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

AlekseyTs

LGTM (iteration 3), identified issues can be followed up on later.

cston · 2018-10-18T23:43:33Z

                    VisitMemberAccess(fieldAccess.ReceiverOpt, fieldAccess.FieldSymbol, asLvalue: true);

Added tests and added comment to VisitMemberAccess.

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

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:447 in cf7548d. [](commit_id = cf7548d58a41cf652d841bfea163dcbd21081a6d, deletion_comment = False)

cston · 2018-10-18T23:50:16Z

@AlekseyTs, all comments have been addressed. Thanks.

cston · 2018-10-19T00:06:15Z

@jaredpar for approval

jaredpar · 2018-10-19T14:40:24Z

approved to merge

sharwell · 2018-11-10T16:24:18Z

@cston Do we have a test covering this behavior?

void Method(object? obj)
{
  obj.Extension(); // Should not warn
  obj.ToString(); // Should warn
}

static void Extension(this object? obj) { }

cston · 2018-11-11T05:22:57Z

@sharwell, there are a couple of tests with extension methods (see NotNullAfterDereference_03 for instance), but there is not a test of a call to an extension method followed by an instance method. I'll add that, thanks.

cston requested a review from a team as a code owner October 17, 2018 19:38

cston force-pushed the 23270 branch from 06917cd to cf21f3e Compare October 18, 2018 15:17

cston requested a review from a team as a code owner October 18, 2018 15:26

gafter approved these changes Oct 18, 2018

View reviewed changes

AlekseyTs approved these changes Oct 18, 2018

View reviewed changes

Set null state on dereference

f8ba770

cston force-pushed the 23270 branch from 0a1575d to f8ba770 Compare October 19, 2018 00:05

jaredpar added the Approved to merge label Oct 19, 2018

cston merged commit 5bb48db into dotnet:dev16.0.x Oct 19, 2018

cston deleted the 23270 branch October 19, 2018 14:44

jcouv mentioned this pull request Nov 10, 2018

Nullability flow analysis ignores dereferences #23270

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set null state on dereference #30564

Set null state on dereference #30564

cston commented Oct 17, 2018

cston commented Oct 17, 2018

cston commented Oct 18, 2018

gafter commented Oct 18, 2018

gafter left a comment

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018

AlekseyTs commented Oct 18, 2018

AlekseyTs commented Oct 18, 2018

AlekseyTs commented Oct 18, 2018

AlekseyTs commented Oct 18, 2018 •

edited by cston

AlekseyTs commented Oct 18, 2018

AlekseyTs commented Oct 18, 2018 •

edited

cston commented Oct 18, 2018

cston commented Oct 18, 2018

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs left a comment

cston commented Oct 18, 2018

cston commented Oct 18, 2018

cston commented Oct 19, 2018

jaredpar commented Oct 19, 2018

sharwell commented Nov 10, 2018

cston commented Nov 11, 2018

Set null state on dereference #30564

Set null state on dereference #30564

Conversation

cston commented Oct 17, 2018

cston commented Oct 17, 2018

cston commented Oct 18, 2018

gafter commented Oct 18, 2018

gafter left a comment

Choose a reason for hiding this comment

AlekseyTs commented Oct 18, 2018 • edited

AlekseyTs commented Oct 18, 2018 • edited

AlekseyTs commented Oct 18, 2018 • edited

AlekseyTs commented Oct 18, 2018

AlekseyTs commented Oct 18, 2018 • edited

AlekseyTs commented Oct 18, 2018 • edited

AlekseyTs commented Oct 18, 2018

AlekseyTs commented Oct 18, 2018

AlekseyTs commented Oct 18, 2018

AlekseyTs commented Oct 18, 2018

AlekseyTs commented Oct 18, 2018 • edited by cston

AlekseyTs commented Oct 18, 2018

AlekseyTs commented Oct 18, 2018 • edited

cston commented Oct 18, 2018

cston commented Oct 18, 2018

AlekseyTs commented Oct 18, 2018 • edited

AlekseyTs commented Oct 18, 2018 • edited

AlekseyTs commented Oct 18, 2018 • edited

AlekseyTs left a comment

Choose a reason for hiding this comment

cston commented Oct 18, 2018

cston commented Oct 18, 2018

cston commented Oct 19, 2018

jaredpar commented Oct 19, 2018

sharwell commented Nov 10, 2018

cston commented Nov 11, 2018

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018 •

edited by cston

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018 •

edited

AlekseyTs commented Oct 18, 2018 •

edited