LambdaRewriter should only assign proxies from locals in original frame in EE #19349

cston · 2017-05-08T19:45:45Z

Customer scenario

Evaluate expression containing nested lambdas where the inner lambda closes over a local in outer lambda.

Bugs this fixes:

#18273

Workarounds, if any

Avoid evaluating such expressions

Risk

Low

Performance impact

Low

Is this a regression from a previous update?

Probably not a regression. Unit test included.

cston · 2017-05-08T21:43:11Z

@dotnet/roslyn-compiler, @tmat, @jinujoseph, @ivanbasov please review.

tmat · 2017-05-08T21:46:08Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

+                                variableSlotAllocatorOpt,
+                                diagnosticsThisMethod,
+                                _debugDocumentProvider,
+                                    method.GenerateDebugInfo ? importChain : null,


[](start = 32, length = 4)

nit: Extra indent

tmat · 2017-05-08T21:46:19Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

-                // confuse the method body generator by making a fresh bag and then loading
-                // any diagnostics emitted into it back into the main diagnostic bag.
-                var diagnosticsThisMethod = DiagnosticBag.GetInstance();
+                    // We make sure that an asynchronous mutation to the diagnostic bag does not 


// We mak [](start = 20, length = 9)

Is this block just indented?

Yes, the only change is to indenting.

tmat · 2017-05-08T21:47:11Z

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

@@ -101,7 +101,7 @@ public MappedLocalFunction(SynthesizedLambdaMethod symbol, ClosureKind closureKi
        // True if the rewritten tree should include assignments of the


True [](start = 11, length = 4)

The comment needs update.

Updated comments.

tmat · 2017-05-08T21:49:25Z

    /// <param name="assignLocals">The rewritten tree should include assignments of the original locals to the lifted proxies</param>

Update comment

Refers to: src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs:210 in 7497a20. [](commit_id = 7497a20ac2756ea95b89cb506e7d60af9cd688fd, deletion_comment = False)

tmat · 2017-05-08T21:52:44Z

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

@@ -101,7 +101,7 @@ public MappedLocalFunction(SynthesizedLambdaMethod symbol, ClosureKind closureKi
        // True if the rewritten tree should include assignments of the
        // original locals to the lifted proxies. This is only useful for the
        // expression evaluator where the original locals are left as is.
-        private readonly bool _assignLocals;
+        private readonly HashSet<LocalSymbol> _assignLocals;


_assignLocals [](start = 46, length = 13)

If this is null then all locals are assigned, otherwise only those that are in this set?

If null, no locals are assigned. The use was incorrect. Fixed.

tmat · 2017-05-08T21:55:09Z

src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/EEMethodSymbol.cs

+                        {
+                            localsBuilder.Add(local);
+                            localsSet.Add(local);
+                        }


Can be simplified:

if (localsSet.Add(local)) { localsBuilder.Add(local) }

cston · 2017-05-10T15:14:57Z

@dotnet/roslyn-compiler, @tmat, please review.

AlekseyTs · 2017-05-10T19:42:56Z

src/ExpressionEvaluator/VisualBasic/Test/ExpressionCompiler/LocalsTests.vb

@@ -830,6 +830,50 @@ End Class
        End Sub

        <Fact>
+        Public Sub CapturedLocalInNestedLocalFunction()


LocalFunction [](start = 40, length = 13)

Lambda? VB doesn't support local functions.

AlekseyTs

LGTM

cston · 2017-05-11T14:38:53Z

@dotnet/roslyn-compiler for a second review, thanks.

…me in EE

cston · 2017-05-17T15:01:22Z

@dotnet/roslyn-compiler for a second review for a small change to the compiler.

@MattGertz for approval since this is an EE fix.

cston · 2017-05-17T22:50:06Z

@dotnet-bot retest this please

cston · 2017-05-18T02:56:02Z

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

cston · 2017-05-18T14:24:38Z

@dotnet-bot test windows_release_vs-integration_prtest please

jcouv · 2017-05-18T17:14:15Z

📝 Notes from discussion with Chuck:
In EE scenarios, you can create a closure that didn't exist in original source. For instance, evaluating F(() => local).
In such case, the fields of the closure need to be populated with the value of their respective variables (locals, parameters, etc).
The local rewriter has logic to do this one-time copy.

But in the case of nested lambdas, it is possible to introduce a closure with a field which should not be initialized this way, because it does not exist in the original method.
For instance, if you evaluate F(z => G(() => local + z)). Then local field of the closure must be initialized, but z should not.

The EE already keeps track of all the variables in scope (including locals from source that were compiled/lifted into a closure and don't exist as locals in the IL), so we can use this information to decide which variables need this initialization and which do not. That's the fix in this PR :-)

jcouv

LGTM. Thanks for the background explanation.

dnfclas added the cla-already-signed label May 8, 2017

cston force-pushed the 18273 branch from fbe91be to 7497a20 Compare May 8, 2017 21:40

tmat reviewed May 8, 2017

View reviewed changes

cston force-pushed the 18273 branch from ef0133b to 70956e0 Compare May 9, 2017 05:40

AlekseyTs reviewed May 10, 2017

View reviewed changes

AlekseyTs approved these changes May 10, 2017

View reviewed changes

cston force-pushed the 18273 branch from 70956e0 to daff6fa Compare May 11, 2017 14:37

LambdaRewriter should only assign proxies from locals in original fra…

1935170

…me in EE

cston force-pushed the 18273 branch from daff6fa to 1935170 Compare May 17, 2017 14:59

MattGertz added the Approved to merge label May 17, 2017

gafter added the Area-Compilers label May 17, 2017

jcouv approved these changes May 18, 2017

View reviewed changes

cston merged commit 3f402e9 into dotnet:master May 18, 2017

cston deleted the 18273 branch May 18, 2017 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LambdaRewriter should only assign proxies from locals in original frame in EE #19349

LambdaRewriter should only assign proxies from locals in original frame in EE #19349

cston commented May 8, 2017

cston commented May 8, 2017

tmat May 8, 2017

cston May 8, 2017

tmat May 8, 2017

cston May 8, 2017

tmat May 8, 2017

cston May 9, 2017

tmat commented May 8, 2017

tmat May 8, 2017

cston May 8, 2017

tmat May 8, 2017

cston May 9, 2017

cston commented May 10, 2017

AlekseyTs May 10, 2017

cston May 11, 2017

AlekseyTs left a comment

cston commented May 11, 2017

cston commented May 17, 2017

cston commented May 17, 2017

cston commented May 18, 2017

cston commented May 18, 2017

jcouv commented May 18, 2017

jcouv left a comment

		@@ -101,7 +101,7 @@ public MappedLocalFunction(SynthesizedLambdaMethod symbol, ClosureKind closureKi
		// True if the rewritten tree should include assignments of the

LambdaRewriter should only assign proxies from locals in original frame in EE #19349

LambdaRewriter should only assign proxies from locals in original frame in EE #19349

Conversation

cston commented May 8, 2017

cston commented May 8, 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

tmat commented May 8, 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

cston commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

cston commented May 11, 2017

cston commented May 17, 2017

cston commented May 17, 2017

cston commented May 18, 2017

cston commented May 18, 2017

jcouv commented May 18, 2017

jcouv left a comment

Choose a reason for hiding this comment