Update null state for add/remove on field-like events by RikkiGibson · Pull Request #46477 · dotnet/roslyn

RikkiGibson · 2020-07-31T18:26:58Z

This work supports the new nullable constructor analysis, since we need to be able to tell that an event no longer contains 'null' after a non-null delegate is added to it.

🦔

RikkiGibson · 2020-07-31T22:28:41Z

Please take a look @dotnet/roslyn-compiler

RikkiGibson · 2020-08-06T18:25:06Z

Please take a look @dotnet/roslyn-compiler

RikkiGibson · 2020-08-07T00:05:16Z

Please review @dotnet/roslyn-compiler

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

cston · 2020-08-07T03:55:04Z

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

+                {
+                    if (ResultType.IsNotNull)
+                    {
+                        LearnFromNonNullTest(MakeMemberSlot(receiverOpt, @event), ref State);


MakeMemberSlot(receiverOpt, @event) [](start = 45, length = 35)

Does LearnFromNonNullTest() handle an invalid slot? For instance, F().E += () => { };

It seems like no, not really, and we don't have any debug asserts preventing e.g. this.State[-1] = .... When we write it through to the BitVector, it ends up getting bit-shifted into something arbitrary and undesirable.

Logged #46621 to fix BitVector.this[int].

have made the change to only try and update state if we have a valid member slot.

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

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

RikkiGibson · 2020-08-07T18:57:38Z

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

+                }
+                else
+                {
+                    this.State[memberSlot] = NullableFlowState.MaybeNull;


I decided to change these to more direct state modifications because event assignment is more limited in what it can operate on. For example the LHS of an event assignment must be an lvalue, so it can't be a conditional-access expression like x?.y += SomeDelegate.

If this feels wrong from a coding patterns POV we can change this to use the LearnFromNullTest/LearnFromNonNullTest or some other more appropriate helpers that I may have overlooked in NullableWalker.

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

333fred · 2020-08-07T19:02:56Z

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

+            {
+                if (node.IsAddition)
+                {
+                    this.State[memberSlot] = this.State[memberSlot].Meet(ResultType.State);


I believe we should using SetResult to set these, rather than updating the state directly like this. Please add some tests that observe the change in field-like event's nullability from the public API.

Discussed offline, and the difficulty is that it's not clear what bound node would be passed to SetResult. Will add the test.

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

cston · 2020-08-07T20:28:35Z

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

            VisitRvalue(node.Argument);
            // https://github.com/dotnet/roslyn/issues/31018: Check for delegate mismatch.
+            if (node.Argument.ConstantValue?.IsNull != true
+                && MakeMemberSlot(receiverOpt, @event) is not -1 and var memberSlot)


is not -1 [](start = 55, length = 9)

Consider using is > 0 for consistency with other checks.

cston · 2020-08-07T21:30:46Z

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

+                else
+                {
+                    this.State[memberSlot] = NullableFlowState.MaybeNull;
+                }


Perhaps use conditional: this.State[memberSlot] = node.IsAddition ? ... : ...;

333fred · 2020-08-07T21:55:49Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/NullablePublicAPITests.cs

+
+            var lhs = root.DescendantNodes().OfType<AssignmentExpressionSyntax>().Single().Left;
+            typeInfo = model.GetTypeInfo(lhs);
+            Assert.Equal(PublicNullableAnnotation.None, typeInfo.Type.NullableAnnotation);


This seems like a bug, can you file to follow up?

333fred

LGTM (commit 5)

RikkiGibson · 2020-08-07T23:17:46Z

I have the required sign offs and the CI failure is due to a known flaky test, so I will just merge the PR.

…features/function-pointers * upstream/master: (465 commits) Stop specifying Rich Nav's version of dotnet Fix nullable reference warning Fix up some calls to SymbolKey.Resolve now that it's annotated Remove try/catch of ArgumentException Null annotate SymbolKey Annotate Location.IsInSource as ensuring SourceTree is non-null Fix nullable annotation of Compilation.CreateErrorTypeSymbol Update null state for add/remove on field-like events (#46477) Throw IndexOutOfRangeException from BitVector.this[int] if index < 0 (#46627) Delete some dead support for changing legacy options Delete our use of Microsoft.VisualStudio.CodingConventions Fix crash in the preview of a code action that modified an .editorconfig Move null check above where the variable is used (#46558) Add integration test to configure diagnostic severity via editorconfig Disable a few Mac tests Consider nullability in conversion to constraint type (#46405) Added interpolated strings and tests Introduce request context (#46557) Simplify NativeIntegerAttribute encoding (#46522) Update extension getenumerator status. (#46607) ...

Update null state for add/remove on field-like events

e04d400

RikkiGibson added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jul 31, 2020

RikkiGibson marked this pull request as ready for review July 31, 2020 18:37

RikkiGibson requested a review from a team as a code owner July 31, 2020 18:37

cston reviewed Aug 7, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs Show resolved Hide resolved

cston reviewed Aug 7, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs Outdated Show resolved Hide resolved

Address feedback

6ffa2c6

cston reviewed Aug 7, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs Show resolved Hide resolved

Consolidate tests

2b1e3c4

cston reviewed Aug 7, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs Show resolved Hide resolved

cston reviewed Aug 7, 2020

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated Show resolved Hide resolved

cston mentioned this pull request Aug 7, 2020

Microsoft.CodeAnalysis.BitVector.this[int] should throw IndexOutOfRangeException if index < 0 #46621

Closed

Address feedback

3b2f014

RikkiGibson commented Aug 7, 2020

View reviewed changes

333fred reviewed Aug 7, 2020

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated Show resolved Hide resolved

333fred reviewed Aug 7, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs Show resolved Hide resolved

cston reviewed Aug 7, 2020

View reviewed changes

cston approved these changes Aug 7, 2020

View reviewed changes

Address feedback

a458165

333fred reviewed Aug 7, 2020

View reviewed changes

333fred approved these changes Aug 7, 2020

View reviewed changes

RikkiGibson mentioned this pull request Aug 7, 2020

GetTypeInfo on left of event assignment has no nullable annotation #46647

Open

RikkiGibson merged commit a433234 into dotnet:master Aug 7, 2020

ghost added this to the Next milestone Aug 7, 2020

RikkiGibson deleted the nullable-event-assignment branch August 7, 2020 23:18

RikkiGibson modified the milestones: Next, 16.8.P2 Aug 11, 2020

Conversation

RikkiGibson commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson commented Jul 31, 2020

Uh oh!

RikkiGibson commented Aug 6, 2020

Uh oh!

RikkiGibson commented Aug 7, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Aug 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RikkiGibson commented Jul 31, 2020 •

edited

Loading