Refactor VisitLambda to not create a child walker by RikkiGibson · Pull Request #45527 · dotnet/roslyn

RikkiGibson · 2020-06-29T20:10:46Z

I had hoped to outright remove the _useDelegateInvokeParameterTypes field and ctor parameter, as well as the delegateInvokeMethodOpt ctor parameter, but I cannot prove to my satisfaction that they will never make a difference in the one scenario where we create a NullableWalker specifically from a lambda, which is UnboundLambda.GetInferredReturnType. There are no tests which fail when the delegateInvokeMethodOpt is forced to null in this context.

I also found that after this change, we were throwing on ImmutableDictionary creation in the bootstrap build, because dictionary values of type TypeWithAnnotations were being compared for equality. This is solved by manually passing a value equality comparer when creating the dictionary.

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

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

jaredpar · 2020-07-01T17:50:10Z

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

            }

-            LeaveParameters(localFuncSymbol.Parameters, localFunc.Syntax, location);
+            LeaveParameters(lambdaOrFunctionSymbol.Parameters, lambdaOrFunction.Syntax, location);


Why do we condition EnterParameters but not LeaveParameters?

Good question! I found that the IsCompilerGenerated check went by unremarked when it was introduced: https://github.com/dotnet/roslyn/pull/40422/files#diff-e3717bc6a837e218d79277c0e45f213aR1620

One thing I did notice is that LeaveParameters does absolutely nothing in NullableWalker, it only has an effect in DefiniteAssignment.

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

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

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

cston · 2020-07-07T21:28:07Z

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

        private void EnterParameters()
        {
-            if (!(_symbol is MethodSymbol methodSymbol))
+            if (!(CurrentSymbol is MethodSymbol methodSymbol))


CurrentSymbol [](start = 18, length = 13)

There are other references to _symbol that are not updated. Are those other references correct?

I made a pass over existing usages of _symbol and changed the ones that seemed wrong.

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

cston · 2020-07-07T21:45:53Z

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

                _variableSlot.ToImmutableDictionary(),
                ImmutableArray.Create(variableBySlot, start: 0, length: nextVariableSlot),
-                _variableTypes.ToImmutableDictionary(),
+                _variableTypes.ToImmutableDictionary(_variableTypes.Comparer, TypeWithAnnotations.EqualsComparer.ConsiderEverythingComparer),


_variableTypes.Comparer, TypeWithAnnotations.EqualsComparer.ConsiderEverythingComparer [](start = 53, length = 86)

Is this change needed? (The key comparer is the default, and the value comparer doesn't appear to be used.)

Yes, without this change, the bootstrap build crashes because it attempts to call TypeWithAnnotations.Equals on the dictionary values.

Where are we calling Equals on the dictionary values?

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

ToImmutableDictionary() may call Equals on dictionary values internally when constructing the immutable dictionary. This is something we might want to examine in greater detail. It's not obvious to me why sharing the _variableTypes between a lambda and its containing method would make this behavior start to surface in the bootstrap build.

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

cston · 2020-07-07T22:07:00Z

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

+                // (15,17): warning CS8602: Dereference of a possibly null reference.
+                //             _ = x.ToString(); // 1
+                Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(15, 17));
+        }


} [](start = 8, length = 1)

Please add a similar test where the local function is passed as a delegate.

cston · 2020-07-08T23:04:23Z

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

        private void EnforceDoesNotReturn(SyntaxNode? syntaxOpt)
        {
-            if (_symbol is MethodSymbol method &&
+            // DoesNotReturn is only supported in member methods


DoesNotReturn [](start = 15, length = 13)

Can [DoesNotReturn] be applied to a local function?

We don't complain if it is applied, but we do not change analysis when it is present.

I actually expected us to mark regions unreachable in NullableWalker when calling a local function that does not return, and thus give no warnings in those regions--definite assignment already works such that the following scenario gives no errors. Unfortunately this does not seem to be the case. It's an inconsistency we might want to follow up on. Once we start doing this, it should not be necessary to use [DoesNotReturn] or [DoesNotReturnIf] on local functions.

string s; local1(); s.ToString(); void local1() => throw null!;

string? s = null; local1(); s.ToString(); // this warns, but shouldn't void local1() => throw null!;

I will try and ensure that DoesNotReturn does not impact analysis of local functions for the moment, and file a follow-up issue to confirm that this is expected.

cston · 2020-07-08T23:08:10Z

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

+
+        [Theory]
+        [InlineData("")]
+        [InlineData("static ")]


Does [DoesNotReturn] analysis depend on whether the local function is static?

No, I pushed this by accident, I believe this function should be deleted.

Simplified the test down to a [Fact] to simplify, since 'static' differences are not expected to impact the test.

cston · 2020-07-08T23:14:28Z

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

+        [DoesNotReturn]
+        " + modifiers + @"void boom()
+        {
+        }


} [](start = 8, length = 1)

Should report WRN_ShouldNotReturn. Not blocking this PR.

cston · 2020-07-08T23:14:55Z

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

+using System.Diagnostics.CodeAnalysis;
+class D
+{
+    void Main(object? o)


object? o [](start = 14, length = 9)

Not used.

333fred · 2020-07-09T00:57:43Z

            if (_symbol is MethodSymbol method)

Why should this not be CurrentSymbol?

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

333fred · 2020-07-09T00:58:08Z

            if (_symbol is MethodSymbol method)

Why should this not be CurrentSymbol?

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

333fred · 2020-07-09T00:58:25Z

            if (_symbol is MethodSymbol method)

Why should this not be CurrentSymbol?

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

333fred · 2020-07-09T00:59:49Z

                        var method = getTopLevelMethod(_symbol as MethodSymbol);

It seems like _symbol here should always be the top level method? Is there a time when it's not? If there is, the docs on _symbol are probably wrong.

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

RikkiGibson · 2020-07-09T01:05:37Z

                        var method = getTopLevelMethod(_symbol as MethodSymbol);
It seems like _symbol here should always be the top level method? Is there a time when it's not? If there is, the docs on _symbol are probably wrong.

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

_symbol is not the top level method when we are reinferring a lambda return type.

RikkiGibson · 2020-07-09T01:10:10Z

            if (_symbol is MethodSymbol method)
Why should this not be CurrentSymbol?

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

I would prefer to address these cases in a follow-up PR which resolves #45821

333fred · 2020-07-09T01:15:50Z

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

        }

-        public override BoundNode? VisitLocalFunctionStatement(BoundLocalFunctionStatement localFunc)
+        public override BoundNode? VisitLocalFunctionStatement(BoundLocalFunctionStatement node)


Alright, so I think speculative analysis in local functions was already broken in current master: we're never calling snapshotBuilderOpt?.EnterNewWalker() on the local function symbol, so state saving and restore will be broken.

This should reproduce in that variables declared in the local function don't have correct state when restoring for semantic analysis (if previous statements in the local function checked them for null, for example.

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

Opened #45825 to track this. I think that in order to not introduce this bug for lambdas in this PR you're going to have to fix this one as well.

RikkiGibson · 2020-07-09T01:16:57Z

It appears that tests already verify that we respect [DoesNotReturn] on local functions. Therefore I will revert the behavior and delete my contradictory test. I think we should revisit this when we decide 1. whether to respect/check DoesNotReturn on local functions and 2. whether to change nullable analysis to understand that a local function may never return and to make regions unreachable as a result. #45814

333fred · 2020-07-09T01:17:13Z

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

                SetUpdatedSymbol(node, node.Symbol, delegateTypeOpt);
            }

-            Analyze(


By no longer calling this analyze overload, we're not entering a new scope for the lambda, so speculation will be broken. We need to restore the calls to snapshotManager?.EnterNewWalker with the new symbol. And exiting the walker after lambda analysis is done.

333fred · 2020-07-09T01:20:10Z

Done review pass (commit 7). I didn't look at tests.

333fred

Explicitly blocking since there are 2 approvals, want to make sure this doesn't accidentally go in.

333fred

LGTM (commit 8)

…)" This reverts commit ffb3ca7.

RikkiGibson force-pushed the lambda-reuse-walker branch from c0dc4ba to 0d4d671 Compare July 1, 2020 15:25

RikkiGibson added the Area-Compilers label Jul 1, 2020

RikkiGibson added 2 commits July 1, 2020 09:30

Refactor VisitLambda to not create a child walker

bf5c479

Fix bootstrap

9f032bd

RikkiGibson force-pushed the lambda-reuse-walker branch from 0d4d671 to 9f032bd Compare July 1, 2020 16:30

RikkiGibson marked this pull request as ready for review July 1, 2020 17:19

RikkiGibson requested a review from a team as a code owner July 1, 2020 17:19

jaredpar reviewed Jul 1, 2020

View reviewed changes

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

jaredpar reviewed Jul 1, 2020

View reviewed changes

Add some tests

aa4b7c2

RikkiGibson commented Jul 6, 2020

View reviewed changes

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

Address feedback

e581e40

RikkiGibson mentioned this pull request Jul 6, 2020

Propagate 'localFuncVarUsages' to lambda NullableWalker #45405

Closed

RikkiGibson requested a review from a team July 7, 2020 19:55

cston reviewed Jul 7, 2020

View reviewed changes

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

cston reviewed Jul 7, 2020

View reviewed changes

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

cston reviewed Jul 7, 2020

View reviewed changes

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

cston reviewed Jul 7, 2020

View reviewed changes

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

cston reviewed Jul 7, 2020

View reviewed changes

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

cston reviewed Jul 7, 2020

View reviewed changes

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

cston reviewed Jul 7, 2020

View reviewed changes

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

cston reviewed Jul 7, 2020

View reviewed changes

RikkiGibson mentioned this pull request Jul 7, 2020

Remove source comments indicating that nullable state flows out of local functions #45770

Closed

Address feedback

423399f

cston reviewed Jul 8, 2020

View reviewed changes

cston approved these changes Jul 8, 2020

View reviewed changes

Do not support DoesNotReturn on local functions

c5b1eec

RikkiGibson requested a review from a team July 9, 2020 00:18

RikkiGibson mentioned this pull request Jul 9, 2020

Consider eliminating usages of AbstractFlowPass._symbol #45821

Open

jaredpar approved these changes Jul 9, 2020

View reviewed changes

Revert behavior change and remove redundant test

c6a1135

333fred reviewed Jul 9, 2020

View reviewed changes

333fred requested changes Jul 9, 2020

View reviewed changes

Address feedback

d518f75

333fred approved these changes Jul 9, 2020

View reviewed changes

RikkiGibson merged commit ffb3ca7 into dotnet:master Jul 10, 2020

ghost added this to the Next milestone Jul 10, 2020

RikkiGibson deleted the lambda-reuse-walker branch July 10, 2020 01:01

JoeRobich modified the milestones: Next, 16.8.P1 Jul 20, 2020

cston added a commit to cston/roslyn that referenced this pull request Dec 5, 2020

Revert "Refactor VisitLambda to not create a child walker (dotnet#45527…

90b77e3

…)" This reverts commit ffb3ca7.

Youssef1313 mentioned this pull request Apr 13, 2021

DoesNotReturn isn't enforced on local functions #52598

Closed

Conversation

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

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jul 8, 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!

Choose a reason for hiding this comment

Uh oh!

cston Jul 8, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jul 8, 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!

Choose a reason for hiding this comment

Uh oh!

333fred commented Jul 9, 2020

Uh oh!

333fred commented Jul 9, 2020

Uh oh!

333fred commented Jul 9, 2020

Uh oh!

333fred commented Jul 9, 2020

Uh oh!

RikkiGibson commented Jul 9, 2020

Uh oh!

RikkiGibson commented Jul 9, 2020

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!

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

Uh oh!

Uh oh!

333fred Jul 9, 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!

RikkiGibson commented Jun 29, 2020 •

edited

Loading

RikkiGibson Jul 8, 2020 •

edited

Loading

cston Jul 8, 2020 •

edited

Loading

cston Jul 8, 2020 •

edited

Loading

RikkiGibson commented Jul 9, 2020 •

edited

Loading

333fred Jul 9, 2020 •

edited

Loading