Enable Nullability Information in IOperation.Type by 333fred · Pull Request #42403 · dotnet/roslyn

333fred · 2020-03-14T00:01:36Z

This enables nullability information from rewritten BoundNodes
in IOperation, exposed via the NullableAnnotation property on
IOperation.Type. The bulk of the change is creating a simple helper
on BoundExpression to return the correctly annotated ITypeSymbol,
and using that in the factory instead of .Type.GetPublicSymbol. This
exposed another issue with a few of our tests though: in DEBUG mode,
when nullable is gobally disabled, we perform a full nullable analysis
and rewrite and then discard the results, to ensure that we don't
crash and to run our general verification code. However, there are
a few nodes that get cached by binders (such as LockOrUsingBinder).
These nodes do not get rewritten as their constituent elements did
not change, so when we set lazy properties like TopLevelNullability
info, the cache from the throwaway binding gets updated as well.
To remedy this and still get coverage, we pass along a flag to abort
before the actual rewrite in DEBUG scenarios. This fixes #40352

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

333fred · 2020-03-30T22:17:22Z

@dotnet/roslyn-compiler @AlekseyTs This is now ready for review. My testing strategy here was to update everywhere that we are currently testing the nullable api to get the IOperation tree for that location, and assert that it matches the info from GetTypeInfo.

/cc @CyrusNajmabadi @dpoeschl

333fred · 2020-03-31T16:37:13Z

@dotnet/roslyn-compiler @AlekseyTs for a review please.

jaredpar · 2020-03-31T16:51:12Z

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

            out SnapshotManager? snapshotManager,
-            ref ImmutableDictionary<Symbol, Symbol>? remappedSymbols)
+            ref ImmutableDictionary<Symbol, Symbol>? remappedSymbols,
+            bool avoidRewrite)


The prefix avoid does not line up with the documentation that says "force". Feel like one of those needs to change. #Resolved

Splitting this up.

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

jaredpar · 2020-03-31T16:52:51Z

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

+#if DEBUG
+            Debug.Assert(avoidRewrite != compilation.NullableSemanticAnalysisEnabled);
+#else
+            if (avoidRewrite) throw ExceptionUtilities.Unreachable;


Overall this dance feels a bit awkward. We're using runtime checks to enforce behavior that is actually codified at compile time. I think this would be a lot cleaner if we split it up into two methods: Analyze and AnalyzeAndRewrite. #Resolved

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

AlekseyTs · 2020-04-03T00:22:20Z

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

+
+            if (avoidRewrite)
+            {
+                return node;


return node [](start = 16, length = 11)

return node [](start = 16, length = 11)

Would it make sense to push this down to the Rewrite to disable actual node modifications, but do all the other work? #Closed

Responded in the other comment, but I don't believe that running the rewriter actually gets us any real verification and it would significantly complicate the generated code.

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

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

AlekseyTs · 2020-04-03T00:36:57Z

src/Compilers/Test/Utilities/CSharp/CompilationTestUtils.cs

+                // in the IOperation tree, and we should retrieve the IOperation node underlying the suppression.
+                if (expression.IsKind(SyntaxKind.SuppressNullableWarningExpression))
+                {
+                    expression = ((PostfixUnaryExpressionSyntax)expression).Operand;


expression = ((PostfixUnaryExpressionSyntax)expression).Operand; [](start = 20, length = 64)

expression = ((PostfixUnaryExpressionSyntax)expression).Operand; [](start = 20, length = 64)

Should we do this in a loop and, perhaps, dig through parenthesis too? #Closed

I believe this was originally in getOperation, I am proceeding with that assumption.

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

AlekseyTs

LGTM (iteartion 6)

333fred · 2020-06-10T23:47:53Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs


+            static bool shouldAvoidRewrite(CSharpCompilation comp) =>
+#if DEBUG
+                !comp.NullableSemanticAnalysisEnabled;


Putting this comment back in line after rebasing:

Doesn't this disable a good chunk of testing we were trying to do in DEBUG? #Resolved

The only portion that is disabled is the actual rewrite step. I does technically add more coverage, but in practice I don't believe that's a huge gain. The rewriter is almost entirely generated and does very little validation.

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

333fred · 2020-06-11T23:24:43Z

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

-            Assert.Equal("System.String?", model.GetTypeInfo(declaration.Type).ConvertedType.ToTestDisplayString());
-            Assert.Equal(CodeAnalysis.NullableAnnotation.Annotated, model.GetTypeInfo(declaration.Type).ConvertedNullability.Annotation);
+            Assert.Equal("System.String?", model.GetTypeInfoAndVerifyIOperation(declaration.Type).Type.ToTestDisplayString());
+            Assert.Equal(CodeAnalysis.NullableAnnotation.Annotated, model.GetTypeInfoAndVerifyIOperation(declaration.Type).Nullability.Annotation);


GetTypeInfoAndVerifyIOperation [](start = 74, length = 30)

Putting this comment back in line after rebase:

Perhaps the second time we can skip IOperation check? There are several more similar cases like this. #Resolved

333fred · 2020-06-11T23:52:03Z

@AlekseyTs @jaredpar this is ready for another review.

333fred · 2020-06-12T00:59:52Z

When I have sign-offs on this change I'm going to change the base branch to be 16.8 as this doesn't meant the 16.7 bar. Keeping this in master for now though so review comments don't jump.

333fred · 2020-06-12T19:33:40Z

@AlekseyTs @jaredpar for another review please.

333fred · 2020-06-15T16:07:05Z

@AlekseyTs @jaredpar for another review please.

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

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

AlekseyTs · 2020-06-16T22:22:16Z

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

                return null;
            }
+
+            public override BoundNode? VisitSwitchExpressionArm(BoundSwitchExpressionArm node)


VisitSwitchExpressionArm [](start = 39, length = 24)

What is the actual change in behavior for a switch expression here? What does base implementation do differently and why is that a problem? #Resolved

The base unconditionally visits the when clause. This is an issue for the reason in the comment.

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

333fred · 2020-06-18T00:18:01Z

@dotnet/roslyn-compiler for a second review.

AlekseyTs · 2020-06-18T16:05:07Z

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

+                if (node.Left is BoundObjectInitializerMember { MemberSymbol: null })
+                {
+                    VerifyExpression(node);
+                    return Visit(node.Left);


return Visit(node.Left); [](start = 20, length = 24)

This looks suspicious. Why are we replacing an assignment with its left side? If return value is not important, let's return null. #Pending

333fred · 2020-06-19T05:30:11Z

@dotnet/roslyn-compiler for a second review.

333fred · 2020-06-19T19:37:48Z

@dotnet/roslyn-compiler for a second review please.

333fred · 2020-06-22T22:46:59Z

@dotnet/roslyn-compiler for another review.

jaredpar · 2020-06-23T04:07:11Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

+        /// Performs just the analysis step of getting nullability information for a semantic model. This is only used during DEBUG builds where nullable
+        /// is turned off on a compilation level, giving some extra debug verification of the nullable flow analysis. 
+        /// </summary>
+        protected abstract void AnalyzeBoundNodeNullability(BoundNode boundRoot, Binder binder, DiagnosticBag diagnostics, bool takeSnapshots);


The declaration of this method has the parameter name takeSnapshots but all of the overrides have the parameter craeteSnapshots. Feel like we should be consistent here. #Pending

jaredpar · 2020-06-23T04:10:54Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

+#if DEBUG
+                if (!Compilation.NullableSemanticAnalysisEnabled)
+                {
+                    AnalyzeBoundNodeNullability(boundRoot, binder, diagnosticBag, takeSnapshots: true);


I understand that diagnosticBag here will be a freshly created bag whenever we hit this code. This still feels very indirect though. Have to think a bit to verify this code isn't potentially adding new diagnostics in DEBUG mode. Is there a reason to not just use new DiagnosticBag() here to make this clearer? #ByDesign

I wanted to avoid having the debug verification step create multiple throwaway diagnostic bags.

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

This enables nullability information from rewritten BoundNodes in IOperation, exposed via the NullableAnnotation property on IOperation.Type. The bulk of the change is creating a simple helper on BoundExpression to return the correctly annotated ITypeSymbol, and using that in the factory instead of .Type.GetPublicSymbol. This exposed another issue with a few of our tests though: in DEBUG mode, when nullable is gobally disabled, we perform a full nullable analysis and rewrite and then discard the results, to ensure that we don't crash and to run our general verification code. However, there are a few nodes that get cached by binders (such as LockOrUsingBinder). These nodes do not get rewritten as their constituent elements did not change, so when we set lazy properties like TopLevelNullability info, the cache from the throwaway binding gets updated as well. To remedy this and still get coverage, we pass along a flag to abort before the actual rewrite in DEBUG scenarios.

…s are an rvalue context). Add testing to everywhere we're currently testing GetTypeInfo with nullable enabled. Enable digging through nullable suppression operators like we do for GetTypeInfo.

* Split debug verification out into AnalyzeWithoutRewrite to simplify code. * Add additional GetPublicTypeSymbol calls in OperationFactory with changes in master. * Add a few NullableWalker.DebugVerifier cases for bugs.

…ssions when validating IOperation nullability.

ghost

Auto-approval

gafter

😷

333fred added Area-Compilers Feature - IOperation IOperation labels Mar 14, 2020

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 18, 2020

333fred commented Mar 27, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs Outdated Show resolved Hide resolved

333fred force-pushed the iop-nullable branch from 8b419da to 64c4850 Compare March 27, 2020 23:12

333fred marked this pull request as ready for review March 30, 2020 22:16

333fred requested a review from a team as a code owner March 30, 2020 22:16

333fred requested a review from AlekseyTs March 30, 2020 22:16

333fred removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 30, 2020

jaredpar reviewed Mar 31, 2020

View reviewed changes

AlekseyTs reviewed Apr 3, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Apr 3, 2020

View reviewed changes

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

AlekseyTs reviewed Apr 3, 2020

View reviewed changes

AlekseyTs approved these changes Apr 3, 2020

View reviewed changes

333fred marked this pull request as draft June 10, 2020 23:41

333fred force-pushed the iop-nullable branch from bcf70db to b40489e Compare June 10, 2020 23:42

333fred commented Jun 10, 2020

View reviewed changes

333fred commented Jun 11, 2020

View reviewed changes

333fred marked this pull request as ready for review June 11, 2020 23:51

333fred requested a review from AlekseyTs June 12, 2020 23:09

AlekseyTs reviewed Jun 16, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Jun 16, 2020

View reviewed changes

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

AlekseyTs reviewed Jun 16, 2020

View reviewed changes

AlekseyTs reviewed Jun 18, 2020

View reviewed changes

jaredpar approved these changes Jun 23, 2020

View reviewed changes

333fred added 12 commits June 23, 2020 09:59

Fix other test.

072d975

Mark #40352 as fixed, adjust baselines.

b02a31a

Adjust the nullability being used to be the flow state (as IOperation…

0d348eb

…s are an rvalue context). Add testing to everywhere we're currently testing GetTypeInfo with nullable enabled. Enable digging through nullable suppression operators like we do for GetTypeInfo.

Add annotation after rebase.

1a0667f

Move nullable suppression compenstation to test utils.

81cb43d

Update after rebase:

c3530bb

* Split debug verification out into AnalyzeWithoutRewrite to simplify code. * Add additional GetPublicTypeSymbol calls in OperationFactory with changes in master. * Add a few NullableWalker.DebugVerifier cases for bugs.

Don't do recursive calls.

615793f

Remove unnecessary duplicate GetTypeInfoAndVerifyOperation calls.

32de4a0

Recursively dig through nullable suppressions and parenthesized expre…

8082610

…ssions when validating IOperation nullability.

Remove extra space.

c40a6be

Cleanup from review.

1a98c67

333fred force-pushed the iop-nullable branch from 7b94520 to 1a98c67 Compare June 23, 2020 17:00

333fred requested review from a team as code owners June 23, 2020 17:00

333fred changed the base branch from master to release/dev16.8-preview1 June 23, 2020 17:00

333fred added the auto-merge label Jun 23, 2020

ghost approved these changes Jun 23, 2020

View reviewed changes

Add case for switch case operations.

da2187b

gafter approved these changes Jun 23, 2020

View reviewed changes

333fred mentioned this pull request Jun 24, 2020

Ensure we have a bound node for the unsafe block so that region analysis works correctly. #45334

Merged

ghost merged commit 47e6ea9 into release/dev16.8-preview1 Jun 24, 2020

ghost deleted the iop-nullable branch June 24, 2020 00:09

This pull request was closed.

Conversation

333fred commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

333fred commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred commented Mar 31, 2020

Uh oh!

jaredpar Mar 31, 2020 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

jaredpar Mar 31, 2020 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlekseyTs Apr 3, 2020 • 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!

333fred Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlekseyTs Apr 3, 2020 • 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!

333fred Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

333fred Jun 10, 2020 • 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!

333fred Jun 10, 2020 • 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!

333fred Jun 11, 2020 • 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!

333fred commented Jun 11, 2020

Uh oh!

333fred commented Jun 12, 2020

Uh oh!

333fred commented Jun 12, 2020

Uh oh!

333fred commented Jun 15, 2020

Uh oh!

Uh oh!

Uh oh!

AlekseyTs Jun 16, 2020 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jun 17, 2020

Choose a reason for hiding this comment

Uh oh!

333fred commented Jun 18, 2020

Uh oh!

AlekseyTs Jun 18, 2020 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred commented Jun 19, 2020

Uh oh!

333fred commented Jun 19, 2020

Uh oh!

333fred commented Jun 22, 2020

333fred commented Mar 14, 2020 •

edited

Loading

333fred commented Mar 30, 2020 •

edited

Loading

jaredpar Mar 31, 2020 •

edited by 333fred

Loading

jaredpar Mar 31, 2020 •

edited by 333fred

Loading

AlekseyTs Apr 3, 2020 •

edited

Loading

AlekseyTs Apr 3, 2020 •

edited

Loading

333fred Jun 10, 2020 •

edited

Loading

333fred Jun 10, 2020 •

edited

Loading

333fred Jun 11, 2020 •

edited

Loading

AlekseyTs Jun 16, 2020 •

edited by 333fred

Loading

AlekseyTs Jun 18, 2020 •

edited by 333fred

Loading

jaredpar Jun 23, 2020 •

edited by 333fred

Loading

jaredpar Jun 23, 2020 •

edited by 333fred

Loading

gafter left a comment •

edited

Loading