Track state separately in NullableWalker for nested and containing methods #50417

cston · 2021-01-13T01:46:27Z

NullableWalker now tracks the LocalState separately for separate lambdas or local functions within the same containing method to avoid duplicated state across nested methods. Similarly, the dictionaries that map variables to slots and types are now tracked separately.

The state for a nested method is now a linked list of state for that method and each of the containing methods.

If the method contains M nested functions with a total of N variables and fields across the method and all nested functions, the existing implementation holds O(M*N) state and this implementation holds O(N + M*N0) state where N0 is the number of variables and fields in the outer method. (The duplication of N0 could be addressed by storing one copy of the LocalState for the outer method rather than including a copy with each nested function.)

Fixes #49745

…functions (dotnet#49765)" This reverts commit dedcd58.

333fred · 2021-01-15T23:08:57Z

@cston looks like there's still a prototype marker in the code.

333fred · 2021-01-15T23:14:56Z

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

+ /// The first slot, slot 0, is reserved for indicating reachability, so the first tracked variable will
+ /// be given slot 1. When referring to VariableIdentifier.ContainingSlot, slot 0 indicates
+ /// that the variable in VariableIdentifier.Symbol is a root, i.e. not nested within another
+ /// tracked variable. Slots &lt; 0 are illegal.


< [](start = 36, length = 4)

Consider just saying "less than" in english so I don't have to remember that < is how you write < in xml :) #Closed

333fred · 2021-01-15T23:16:54Z

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

+ _variableSlot.Add(identifier, slot);
+ if (slot >= variableBySlot.Length)
+ {
+ Array.Resize(ref this.variableBySlot, slot * 2);


I know this stuff is just copied, but do we want to consider just making variableBySlot an ArrayBuilder, which handles this already? #Closed

333fred · 2021-01-16T00:16:13Z

Got the the nullable walker. I'll get back to this on Tuesday. #Closed

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

333fred · 2021-01-19T22:15:11Z

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

+ _variableSlot.Add(identifier, slot);
+ while (slot >= variableBySlot.Count)
+ {
+ variableBySlot.Add(default);


Consider using AddMany. #Closed

slot should be at most equal to, not greater than, variableBySlot.Count so the while loop is executed at most once. The alternative seems a little more complicated:

if (slot >= variableBySlot.Count) { variableBySlot.AddMany(default, variableBySlot.Count - slot + 1); }

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

333fred · 2021-01-19T22:27:24Z

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

- _hasComplexity = true;
- return node;
- }
+ _hasInitialState = variables is { };


{ } [](start = 44, length = 3)

Consider using not null for clarity. #ByDesign

I prefer this syntax since it's shorter.

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

@jaredpar what have you done.

In reply to: 560598649 [](ancestors = 560598649,560538257)

AlekseyTs · 2021-01-22T16:10:55Z

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

+ ImmutableDictionary.CreateRange(_variableTypes));
+ }
+
+ internal Variables CreateNestedFunction(MethodSymbol method)


CreateNestedFunction [](start = 31, length = 20)

The name "CreateNestedFunction" feels confusing. The method doesn't create a function. Consider using the name that reflects the purpose of the method. Given the terminology used in doc comments, "CreateNestedMethodScope" comes to mind. #Closed

AlekseyTs · 2021-01-22T16:11:40Z

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

+ internal Variables CreateNestedFunction(MethodSymbol method)
+ {
+ Debug.Assert(GetVariablesForSymbol(method) is null or { Symbol: null });
+ return new Variables(id: GetNextId(), this, method);


this [](start = 54, length = 4)

Can we assert that this is the right enclosing scope? #Closed

AlekseyTs · 2021-01-22T16:15:37Z

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

+ return GetVariablesRoot();
+ }
+
+ private Variables GetVariablesRoot()


"GetRootScope"? #Closed

AlekseyTs · 2021-01-22T16:22:09Z

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

 }
 else
 {
- state.TryAdd(key, new Data(walker._variableSlot.Count, requiresAnalysis));
+ state.TryAdd(key, new Data(_variables.GetTotalVariableCount(), requiredAnalysis));


_variables.GetTotalVariableCount() [](start = 47, length = 34)

~~No need to subtract 1 any more?~~ I see that we didn't subtract initially. #Closed

AlekseyTs · 2021-01-22T16:24:00Z

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

+ /// A bit array containing the nullability of variables associated with a method scope. If the method is a
+ /// nested function (a lambda or a local function), there is a reference to the corresponding instance for
+ /// the containing method scope. The instances in the chain are associated with a corresponding
+ /// Variables chain, and the Id field in this type matches the Id in the Variables chain.


Variables [](start = 12, length = 9)

Consider using cref to point to types and members throughout this statement. #Closed

AlekseyTs · 2021-01-22T16:37:38Z

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

+ return new LocalState(variables.Id, container, CreateBitVector(capacity, reachable));
+ }
+
+ public LocalState CreateNestedFunction(Variables variables)


CreateNestedFunction [](start = 30, length = 20)

"CreateNestedFunctionState"? #Closed

AlekseyTs · 2021-01-22T16:38:48Z

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

+ internal partial class NullableWalker
+ {
+ /// <summary>
+ /// An immutable copy of Variables.


Variables [](start = 33, length = 9)

cref? #Closed

AlekseyTs · 2021-01-22T16:40:49Z

Done with review pass (commit 13) #Closed

cston · 2021-01-22T17:36:58Z

@AlekseyTs, @333fred, all feedback has been addressed, thanks.

333fred · 2021-01-22T18:48:22Z

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

+ internal Variables CreateNestedMethodScope(MethodSymbol method)
+ {
+ Debug.Assert(GetVariablesForMethodScope(method) is null);
+ Debug.Assert(!(method.ContainingSymbol is MethodSymbol containingMethod) ||


!(method.ContainingSymbol is MethodSymbol containingMethod) [](start = 29, length = 59)

Consider simplifying with methodSymbol.ContainingSymbol is not MethodSymbol containingMethod #ByDesign

See #49262.

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

333fred

LGTM (commit 15). Just a minor formatting comment.

AlekseyTs · 2021-01-22T21:32:45Z

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

+ {
+ Debug.Assert(GetVariablesForMethodScope(method) is null);
+ Debug.Assert(!(method.ContainingSymbol is MethodSymbol containingMethod) ||
+ ((object?)GetVariablesForMethodScope(containingMethod) == this) ||


((object?)GetVariablesForMethodScope(containingMethod) == this) [](start = 20, length = 63)

Symbol == (object)containingMethod?

AlekseyTs · 2021-01-22T21:33:11Z

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

+ Debug.Assert(GetVariablesForMethodScope(method) is null);
+ Debug.Assert(!(method.ContainingSymbol is MethodSymbol containingMethod) ||
+ ((object?)GetVariablesForMethodScope(containingMethod) == this) ||
+ Container is null);


Container is null [](start = 20, length = 17)

Symbol is null?

AlekseyTs · 2021-01-22T21:39:45Z

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

+ var variables = this;
+ while (true)
+ {
+ if (variables.Symbol is null)


if (variables.Symbol is null) [](start = 20, length = 29)

The check feels redundant, it looks like the behavior will be the same without this if. #Closed

AlekseyTs · 2021-01-22T21:41:11Z

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

+ /// A bit array containing the nullability of variables associated with a method scope. If the method is a
+ /// nested function (a lambda or a local function), there is a reference to the corresponding instance for
+ /// the containing method scope. The instances in the chain are associated with a corresponding
+ /// <see cref="Variables"/> chain, and the Id field in this type matches the Id in the <see cref="Variables"/> chain.


Id [](start = 51, length = 2)

cref? #Closed

AlekseyTs · 2021-01-22T21:41:27Z

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

+ /// A bit array containing the nullability of variables associated with a method scope. If the method is a
+ /// nested function (a lambda or a local function), there is a reference to the corresponding instance for
+ /// the containing method scope. The instances in the chain are associated with a corresponding
+ /// <see cref="Variables"/> chain, and the Id field in this type matches the Id in the <see cref="Variables"/> chain.


Id [](start = 85, length = 2)

cref? #Closed

AlekseyTs · 2021-01-22T21:47:44Z

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

+ protected override LocalFunctionState CreateLocalFunctionState(LocalFunctionSymbol symbol)
+ {
+ var variables = (symbol.ContainingSymbol is MethodSymbol containingMethod ? _variables.GetVariablesForMethodScope(containingMethod) : null) ??
+ _variables.GetRootScope();


(symbol.ContainingSymbol is MethodSymbol containingMethod ? _variables.GetVariablesForMethodScope(containingMethod) : _variables.GetRootScope())?

AlekseyTs

LGTM (commit 15)

Dotnet-GitSync-Bot added the Area-Compilers label Jan 13, 2021

cston force-pushed the NestedFunctionState branch 4 times, most recently from ddafaae to 65f28fb Compare January 15, 2021 06:51

cston added 3 commits January 15, 2021 00:15

Revert "Avoid sharing NullableWalker state between method and nested …

cf5caa3

…functions (dotnet#49765)" This reverts commit dedcd58.

Separate state for nested functions in NullableWalker

9a350d3

Add snapshot types

648b8e4

cston force-pushed the NestedFunctionState branch from 65f28fb to 648b8e4 Compare January 15, 2021 08:23

cston marked this pull request as ready for review January 15, 2021 08:51

cston requested a review from a team as a code owner January 15, 2021 08:51

cston added the New Language Feature - Nullable Reference Types Nullable Reference Types label Jan 15, 2021

RikkiGibson self-assigned this Jan 15, 2021

333fred reviewed Jan 15, 2021

View reviewed changes

cston added 3 commits January 15, 2021 17:26

PR feedback

a7b4e52

Remove unused code

0cfc45e

Reduce allocations

53e3149

RikkiGibson reviewed Jan 16, 2021

View reviewed changes

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

Avoid using array after free

c65c6e1

cston force-pushed the NestedFunctionState branch from 92c8d13 to da04d3d Compare January 16, 2021 07:08

Misc.

686fcb8

cston force-pushed the NestedFunctionState branch from da04d3d to 686fcb8 Compare January 17, 2021 04:58

Handle PROTOTYPE comments

5baa27d

cston requested review from AlekseyTs and a team January 19, 2021 01:48

333fred reviewed Jan 19, 2021

View reviewed changes

AlekseyTs reviewed Jan 22, 2021

View reviewed changes

PR feedback

6eb222d

cston requested a review from a team January 22, 2021 18:05

Fix tests

e5c45e9

333fred reviewed Jan 22, 2021

View reviewed changes

333fred approved these changes Jan 22, 2021

View reviewed changes

AlekseyTs reviewed Jan 22, 2021

View reviewed changes

AlekseyTs approved these changes Jan 22, 2021

View reviewed changes

PR feedback

8a174ea

cston merged commit 6a28cdc into dotnet:master Jan 23, 2021

ghost added this to the Next milestone Jan 23, 2021

cston deleted the NestedFunctionState branch January 23, 2021 01:06

runfoapp bot mentioned this pull request Jan 25, 2021

Roslyn Test CustomizableTagsForUnnecessaryCode #48671

Closed

Cosifne modified the milestones: Next, 16.9 Jan 27, 2021

JoeRobich modified the milestones: 16.9, 16.9.P4 Jan 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track state separately in NullableWalker for nested and containing methods #50417

Track state separately in NullableWalker for nested and containing methods #50417

cston commented Jan 13, 2021 •

edited

Loading

333fred commented Jan 15, 2021

333fred Jan 15, 2021 •

edited

Loading

333fred Jan 15, 2021 •

edited

Loading

333fred commented Jan 16, 2021 •

edited

Loading

333fred Jan 19, 2021 •

edited

Loading

cston Jan 20, 2021 •

edited

Loading

333fred Jan 19, 2021 •

edited by cston

Loading

cston Jan 20, 2021

333fred Jan 20, 2021

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs commented Jan 22, 2021 •

edited

Loading

cston commented Jan 22, 2021

333fred Jan 22, 2021 •

edited by cston

Loading

cston Jan 22, 2021

333fred left a comment

AlekseyTs Jan 22, 2021

AlekseyTs Jan 22, 2021

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021

AlekseyTs left a comment

Track state separately in NullableWalker for nested and containing methods #50417

Track state separately in NullableWalker for nested and containing methods #50417

Conversation

cston commented Jan 13, 2021 • edited Loading

333fred commented Jan 15, 2021

333fred Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

333fred Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

333fred commented Jan 16, 2021 • edited Loading

333fred Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

cston Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

333fred Jan 19, 2021 • edited by cston Loading

Choose a reason for hiding this comment

cston Jan 20, 2021

Choose a reason for hiding this comment

333fred Jan 20, 2021

Choose a reason for hiding this comment

AlekseyTs Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jan 22, 2021 • edited Loading

cston commented Jan 22, 2021

333fred Jan 22, 2021 • edited by cston Loading

Choose a reason for hiding this comment

cston Jan 22, 2021

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

AlekseyTs Jan 22, 2021

Choose a reason for hiding this comment

AlekseyTs Jan 22, 2021

Choose a reason for hiding this comment

AlekseyTs Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 22, 2021

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

cston commented Jan 13, 2021 •

edited

Loading

333fred Jan 15, 2021 •

edited

Loading

333fred Jan 15, 2021 •

edited

Loading

333fred commented Jan 16, 2021 •

edited

Loading

333fred Jan 19, 2021 •

edited

Loading

cston Jan 20, 2021 •

edited

Loading

333fred Jan 19, 2021 •

edited by cston

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs commented Jan 22, 2021 •

edited

Loading

333fred Jan 22, 2021 •

edited by cston

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading

AlekseyTs Jan 22, 2021 •

edited

Loading