Remove CapturedVariablesByLambda #20878

agocke · 2017-07-14T04:34:26Z

Swaps out all uses of CapturedVariablesByLambda for functions on the
Scope tree.

Fixes https://github.com/dotnet/roslyn/projects/26#card-3753318

Swaps out all uses of CapturedVariablesByLambda for functions on the Scope tree.

jaredpar · 2017-07-14T16:24:55Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs

+            /// <summary>
+            /// The root of the scope tree for this method.
+            /// </summary>
+            public Scope ScopeTree { get; private set; }


Delete the private set. It's unnecessary since you only modify the value in the ctor. Makes it clear this is a readonly property.

? It's set in the visitor, not in the constructor. I could set it in the constructor but that's a dangerous dance as I would be handing out this before fully constructed.

jaredpar · 2017-07-14T16:26:26Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs

+            /// <returns>
+            /// A tuple of the found <see cref="Closure"/> and the <see cref="Scope"/> it was found in.
+            /// </returns>
+            public static (Closure, Scope) FindClosureInScope(Scope startingScope, MethodSymbol closureSymbol)


Consider GetClosureInScope vs. Find. The general rule for funtions like this is:

Find: returns null on failure. Essentially a method that expects failure as an outcome.

Get: throws on failure. A method which expects to find the value.

Hmm also consider simply GetClosure. The InScope part to me can read as "find a closure inside this scope" which is contrary to the implementation.

I was trying to find some way to say "this method searches up", as opposed to the other one, which searches down. Any suggestions?

GetVisibleClosure?

jaredpar · 2017-07-14T16:29:22Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs

+            /// <summary>
+            /// Finds a <see cref="Closure"/> with a matching original symbol somewhere in the given scope or nested scopes.
+            /// </summary>
+            public static Closure FindClosureInTree(Scope rootScope, MethodSymbol closureSymbol)


Another place to consider Get vs. Find

jaredpar · 2017-07-14T16:30:34Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs

+            /// <summary>
+            /// Finds a <see cref="Closure"/> with a matching original symbol somewhere in the given scope or nested scopes.
+            /// </summary>
+            public static Closure FindClosureInTree(Scope rootScope, MethodSymbol closureSymbol)


Consider using treeRoot vs. rootScope. The term root scope has a particular meaning in this code base already: the absolute root of the scopes. This method though works on any arbitrary scope.

jaredpar · 2017-07-14T16:31:10Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs

+                {
+                    return result;
+                }
+                throw ExceptionUtilities.Unreachable;


return result ?? throw ...

Don't actually care if you do this, just a reminder to myself that you could 😄

jaredpar · 2017-07-14T16:32:04Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs

+
+                Closure Helper(Scope scope)
+                {
+                    foreach (var closure in scope.Closures)


In the FindClosureInScope you used SingleOrDefault to accomplish the exact same operation. Why use foreach here but SingleOrDefault there? Seems like these should do the same operation.

I'll change the other one to use a foreach loop too. Avoids allocating a capture environment and a delegate 😄

jaredpar · 2017-07-14T23:27:21Z

A number of the tests needed to be re-baselined due to the generated closure names changing. Can you please add an explanation for why that happened? Want to make sure this is expected and we understand why it's changing.

agocke · 2017-07-14T23:41:38Z

@jaredpar Certainly.

The reason the baseline changed is that the order of enumerating closures changed. I briefly looked into matching the behavior, but the problem is that the closure order in the old visitors is determined by the visitation of captured variables, not visitation of the closures themselves. More specifically, an item is only added to the CapturedVariablesByLambda dictionary when ReferenceVariable encounters a captured variable.

In contrast, the visitation of the Scope tree looks at closures in an in-order traversal of the tree, stopping at the first introduced closure, not necessarily the closure which first captured a variable.

One obvious consequence of this changing is that the old ordering for a series of nested lambdas was effectively post-order -- the interior closures would be added to the visitation first, since they would be added from the perspective of the captured variable looking up, instead of the visitor looking down. This particular case is responsible for all the baseline changes that I have seen.

When I looked into replicating this traversal order I found it complicated and pretty counter-intuitive. Since the baseline change was so low in emitted IL, I thought it better to keep a very natural in-order tree traversal and just rebaseline a few tests.

jaredpar · 2017-07-14T23:48:33Z

@agocke

Thanks for the explanation. Could you find a way to get that explanation into one of the commit messages here? That way future code historians can understand why these baselines changed?

Would recomend just ammend the change that updated the baseline to have the commit message.

agocke · 2017-07-15T00:13:14Z

@jaredpar I plan on squashing these changes, so how about I add it to the squashed message?

VSadov · 2017-07-15T00:28:25Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs

@@ -328,22 +328,20 @@ protected override NamedTypeSymbol ContainingType
        /// </summary>
        private void MakeFrames(ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
        {
-            var closures = _analysis.CapturedVariablesByLambda.Keys;
-
-            foreach (var closure in closures)


Is this why the names changed? We are traversing closures in different order? I think this is ok.
I think we may be changing the order more in the future. It may make sense eventually for the frame generator to just traverse Scopes/DeclaredVariables instead.

VSadov · 2017-07-15T00:28:46Z

VSadov

LGTM

jaredpar · 2017-07-16T15:28:46Z

I plan on squashing these changes, so how about I add it to the squashed message?

That's fine. Achieves the goal of "developer sees change, uses git blame, and gets a good explanation".

…c-member-reference * dotnet/features/ioperation: (134 commits) Fix build break in IOperation feature branch Fix unit tests that broke with previous commit Address PR feedback from Chuck Fix unit test Add IOperation support for variations of object creation expressions (type parameter, nopia and dynamic) Remove dead code from GenerateConstructor. Disable NETCore version check Fixed review notes. Add comment Disable NETCore version check Show modifier/method/type block declaration keywords after attribute lists Improve type argument list parsing in error conditions. (dotnet#19467) Don't report modifier errors while parsing. (dotnet#16339) Fix assert and merge Remove extra space Restore failure behavior. Simplify implementation of parsing constraints. (dotnet#16429) Fix csproj Remove CapturedVariablesByLambda (dotnet#20878) Remove testing wait ...

Remove CapturedVariablesByLambda

6064aac

Swaps out all uses of CapturedVariablesByLambda for functions on the Scope tree.

agocke added the Area-Compilers label Jul 14, 2017

agocke requested review from jaredpar, khyperia, gafter, VSadov and a team July 14, 2017 04:34

dnfclas added the cla-already-signed label Jul 14, 2017

jaredpar reviewed Jul 14, 2017

View reviewed changes

agocke added 2 commits July 14, 2017 10:44

Re-baseline IL in EE test

4da70bc

PR comments

21924dd

agocke requested a review from a team July 14, 2017 23:11

VSadov reviewed Jul 15, 2017

View reviewed changes

VSadov approved these changes Jul 15, 2017

View reviewed changes

jaredpar approved these changes Jul 16, 2017

View reviewed changes

agocke merged commit e240a70 into dotnet:master Jul 17, 2017

agocke deleted the remove-capture-by-lambda branch July 17, 2017 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove CapturedVariablesByLambda #20878

Remove CapturedVariablesByLambda #20878

agocke commented Jul 14, 2017

jaredpar Jul 14, 2017

agocke Jul 14, 2017

jaredpar Jul 14, 2017

jaredpar Jul 14, 2017

agocke Jul 14, 2017

jaredpar Jul 14, 2017

jaredpar Jul 14, 2017

jaredpar Jul 14, 2017

jaredpar Jul 14, 2017

agocke Jul 14, 2017

jaredpar Jul 14, 2017

agocke Jul 14, 2017

jaredpar commented Jul 14, 2017

agocke commented Jul 14, 2017 •

edited

Loading

jaredpar commented Jul 14, 2017 •

edited

Loading

agocke commented Jul 15, 2017

VSadov Jul 15, 2017

agocke Jul 15, 2017

VSadov commented Jul 15, 2017

VSadov left a comment

jaredpar commented Jul 16, 2017

Remove CapturedVariablesByLambda #20878

Remove CapturedVariablesByLambda #20878

Conversation

agocke commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Jul 14, 2017

agocke commented Jul 14, 2017 • edited Loading

jaredpar commented Jul 14, 2017 • edited Loading

agocke commented Jul 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov commented Jul 15, 2017

VSadov left a comment

Choose a reason for hiding this comment

jaredpar commented Jul 16, 2017

agocke commented Jul 14, 2017 •

edited

Loading

jaredpar commented Jul 14, 2017 •

edited

Loading