提交 d6e70325 编写于 作者: A Andy Gocke 提交者: GitHub

Allow closures to take both struct- and class-based capture environments (#16908)

Lambda rewriting currently allows frames to be structs in the instance
where

The variables in the frame are not captured by any closures
converted to delegates.

A closure nested inside the struct closure captures a separate
variable that does require a class frame.

This poses a problem when the "outer" closure is in the same frame as
the variable that requires a class closure. The problem is that we treat
scopes and frames as essentially the same thing -- if any scope is
perceived to require a particular environment type, all captured
variables in that scope will be captured on that frame and all closures
will be lowered onto that frame.

This creates a conflict between class-based environment capturing, where
we want to capture the frame pointer as a field, and struct-based frame
capturing, where we want to add arguments as ref parameters. Doing both
results in breaking the delegate contract (no signature rewriting) or
losing required struct arguments to intermediate closures.

To elaborate on the problem here, let's consider the example:

```csharp
using System;
class C
{
    public void M()
    {
        int x = 0;
        {
            int y= 0;
            void Local()
            {
                if (x == 0)
                {
                    Action a = () => y++;
                    a();
                }
            }
            Local();
        }
    }
}
```

The current problem in the compiler is in the code now in GetStructClosures. The previous implementation only built struct closure if the closure was being lowered onto a struct frame.

However, in the example, y is being captured by a lambda, meaning that y must be in a class frame. Since Local lives in the same scope it also lives in the same frame and contains the lambda capturing y, meaning that it must live in a class frame as well. Of course, it is not converted to a delegate, nor does it capture any variables that are captured by closures converted to delegates, so its signature is free to be rewritten to take variables which are not captured by closures converted to delegates as struct environment parameters.

Here is the expected lowering for the previous example:

```csharp
void M()
{
    var env1 = new Env1();
    env1.x = 0;
    var env2 = new Env2();
    env2.y = 0;
    env2.<>_L(ref env1);
}

struct Env1
{
    public int x;
}

class Env2
{
    public int y;
    public void <>_L(ref Env1 env1)
    {
        if (env1.x == 0)
        {
            var env3 = new Env3();
            env3.env2 = this;
            Action a = env3.<>_anon;
            a();
        }
    }
}

class Env3
{
     Env2 env2;
     public void <>_anon() => this.env2.y++;
}
```

The problem comes when calculating the struct frames needed to be passed to Local. In the current implementation, unless the "container" is a struct, no struct frames are added. If we fix that to check for struct frames that are required as parent frames, regardless of the type of container we run into another problem.

In this case, the lambda also includes struct frames in its parent "context", but adding any struct frames to a closure that is converted to a delegate is incorrect.

The previous problem can be solved by a check to see if the closure can take new ref parameters. This is true for all closures except for ones marked async or iterators or ones converted to delegates. This works because of something we already know from the analysis: if we assume that struct environments are only created when the their capturing closure can take ref parameters, we can assume that any struct environments in scope are "safe" to use, even if those frames are in the "parent context" of the closure.

Fixes #16895
上级 075ec32c
......@@ -73,7 +73,16 @@ internal sealed class Analysis : BoundTreeWalkerWithStackGuardWithoutRecursionOn
public readonly HashSet<MethodSymbol> MethodsConvertedToDelegates = new HashSet<MethodSymbol>();
/// <summary>
/// Any scope that a method in <see cref="MethodsConvertedToDelegates"/> closes over. If a scope is in this set, don't use a struct closure.
/// True if the method signature can't be rewritten to contain ref/out parameters.
/// </summary>
public bool CanTakeRefParameters(MethodSymbol closure) => !(closure.IsAsync
|| closure.IsIterator
// We can't rewrite delegate signatures
|| MethodsConvertedToDelegates.Contains(closure));
/// <summary>
/// Any scope that a method that <see cref="CanTakeRefParameters(MethodSymbol)"/> doesn't close over.
/// If a scope is in this set, don't use a struct closure.
/// </summary>
public readonly HashSet<BoundNode> ScopesThatCantBeStructs = new HashSet<BoundNode>();
......@@ -325,7 +334,7 @@ internal void ComputeLambdaScopesAndFrameCaptures()
LambdaScopes.Add(lambda, innermostScope);
// Disable struct closures on methods converted to delegates, as well as on async and iterator methods.
var markAsNoStruct = MethodsConvertedToDelegates.Contains(lambda) || lambda.IsAsync || lambda.IsIterator;
var markAsNoStruct = !CanTakeRefParameters(lambda);
if (markAsNoStruct)
{
ScopesThatCantBeStructs.Add(innermostScope);
......
......@@ -1311,35 +1311,15 @@ private DebugId GetLambdaId(SyntaxNode syntax, ClosureKind closureKind, int clos
if (_analysis.LambdaScopes.TryGetValue(node.Symbol, out lambdaScope))
{
containerAsFrame = _frames[lambdaScope];
var structClosureParamBuilder = ArrayBuilder<TypeSymbol>.GetInstance();
while (containerAsFrame != null && containerAsFrame.IsValueType)
structClosures = _analysis.CanTakeRefParameters(node.Symbol)
? GetStructClosures(containerAsFrame, lambdaScope)
: default(ImmutableArray<TypeSymbol>);
if (containerAsFrame?.IsValueType == true)
{
structClosureParamBuilder.Add(containerAsFrame);
if (this._analysis.NeedsParentFrame.Contains(lambdaScope))
{
var found = false;
while (this._analysis.ScopeParent.TryGetValue(lambdaScope, out lambdaScope))
{
if (_frames.TryGetValue(lambdaScope, out containerAsFrame))
{
found = true;
break;
}
}
if (found)
{
continue;
}
}
// can happen when scope no longer needs parent frame, or we're at the outermost level and the "parent frame" is top level "this".
lambdaScope = null;
// Lower directly onto the containing type
containerAsFrame = null;
}
// Reverse it because we're going from inner to outer, and parameters are in order of outer to inner
structClosureParamBuilder.ReverseContents();
structClosures = structClosureParamBuilder.ToImmutableAndFree();
if (containerAsFrame == null)
{
lambdaScope = null;
closureKind = ClosureKind.Static; // not exactly... but we've rewritten the receiver to be a by-ref parameter
translatedLambdaContainer = _topLevelMethod.ContainingType;
closureOrdinal = LambdaDebugInfo.StaticClosureOrdinal;
......@@ -1440,6 +1420,47 @@ private DebugId GetLambdaId(SyntaxNode syntax, ClosureKind closureKind, int clos
return synthesizedMethod;
}
/// <summary>
/// Builds a list of all the struct-based closure environments that will
/// need to be passed as arguments to the method.
/// </summary>
private ImmutableArray<TypeSymbol> GetStructClosures(LambdaFrame containerAsFrame, BoundNode lambdaScope)
{
var structClosureParamBuilder = ArrayBuilder<TypeSymbol>.GetInstance();
while (containerAsFrame?.IsValueType == true ||
_analysis.NeedsParentFrame.Contains(lambdaScope))
{
if (containerAsFrame?.IsValueType == true)
{
structClosureParamBuilder.Add(containerAsFrame);
}
if (_analysis.NeedsParentFrame.Contains(lambdaScope)
&& FindParentFrame(ref containerAsFrame, ref lambdaScope))
{
continue;
}
// can happen when scope no longer needs parent frame, or we're at the outermost level and the "parent frame" is top level "this".
break;
}
// Reverse it because we're going from inner to outer, and parameters are in order of outer to inner
structClosureParamBuilder.ReverseContents();
return structClosureParamBuilder.ToImmutableAndFree();
bool FindParentFrame(ref LambdaFrame container, ref BoundNode scope)
{
while (_analysis.ScopeParent.TryGetValue(scope, out scope))
{
if (_frames.TryGetValue(scope, out container))
{
return true;
}
}
return false;
}
}
private void AddSynthesizedMethod(MethodSymbol method, BoundStatement body)
{
if (_synthesizedMethods == null)
......
......@@ -212,6 +212,9 @@ private void CaptureVariable(Symbol variable, SyntaxNode syntax)
protected override void EnterParameter(ParameterSymbol parameter)
{
// Async and iterators should never have ref parameters aside from `this`
Debug.Assert(parameter.IsThis || parameter.RefKind == RefKind.None);
// parameters are NOT initially assigned here - if that is a problem, then
// the parameters must be captured.
GetOrCreateSlot(parameter);
......
......@@ -30,6 +30,54 @@ public static IMethodSymbol FindLocalFunction(this CommonTestBase.CompilationVer
[CompilerTrait(CompilerFeature.LocalFunctions)]
public class CodeGenLocalFunctionTests : CSharpTestBase
{
[Fact]
[WorkItem(17890, "https://github.com/dotnet/roslyn/issues/17890")]
public void Repro17890()
{
var comp = CreateCompilationWithMscorlib46(@"
using System;
using System.Collections.Generic;
using System.Linq;
public class Class
{
public class Item
{
public int Id { get; set; }
}
public class ItemsContainer : IDisposable
{
public List<Item> Items { get; set; }
public void Dispose()
{
}
}
public static void CompilerError()
{
using (var itemsContainer = new ItemsContainer())
{
Item item = null;
itemsContainer.Items.Where(x => x.Id == item.Id);
void Local1()
{
itemsContainer.Items = null;
}
void Local2()
{
Local1();
}
}
}
}", references: new[] { LinqAssemblyRef });
CompileAndVerify(comp);
}
[Fact(Skip = "https://github.com/dotnet/roslyn/issues/16783")]
[WorkItem(16783, "https://github.com/dotnet/roslyn/issues/16783")]
public void GenericDefaultParams()
......@@ -104,6 +152,312 @@ void Local(string s = nameof(Local))
CompileAndVerify(comp, expectedOutput: "Local");
}
[Fact]
[WorkItem(16895, "https://github.com/dotnet/roslyn/issues/16895")]
public void CaptureVarNestedLambdaSkipScope()
{
var src = @"
using System;
class C
{
public static void Main()
{
var d = """";
{
int x = 0;
void M()
{
if (d != null)
{
Action a = () => x++;
a();
}
}
M();
Console.WriteLine(x);
}
}
}";
CompileAndVerify(src, expectedOutput: "1");
}
[Fact]
[WorkItem(16895, "https://github.com/dotnet/roslyn/issues/16895")]
public void CaptureVarNestedLambdaSkipScope2()
{
var src = @"
using System;
class C
{
class D : IDisposable { public void Dispose() {} }
public static void Main()
{
using (var d = new D())
{
int x = 0;
void M()
{
if (d != null)
{
Action a = () => x++;
a();
}
}
M();
Console.WriteLine(x);
}
}
}";
CompileAndVerify(src, expectedOutput: "1");
}
[Fact]
[WorkItem(16895, "https://github.com/dotnet/roslyn/issues/16895")]
public void CaptureVarNestedLambdaSkipScope3()
{
var src = @"
using System;
class C
{
public static void Main()
{
var d = """";
{
int x = 0;
void M()
{
if (d != null)
{
void Local() => x++;
Action a = Local;
a();
}
}
M();
Console.WriteLine(x);
}
}
}";
CompileAndVerify(src, expectedOutput: "1");
}
[Fact]
[WorkItem(16895, "https://github.com/dotnet/roslyn/issues/16895")]
public void CaptureVarNestedLambdaSkipScope4()
{
var src = @"
using System;
class C
{
public static void Main()
{
var d = """";
{
int y = 0;
{
int x = 0;
void M()
{
if (d != null)
{
Action a = () => x++;;
a();
}
}
M();
Console.WriteLine(x);
}
y++;
}
}
}";
CompileAndVerify(src, expectedOutput: "1");
}
[Fact]
[WorkItem(16895, "https://github.com/dotnet/roslyn/issues/16895")]
public void CaptureVarNestedLambdaSkipScope5()
{
var src = @"
using System;
class C
{
public static void Main()
{
int x = 0;
{
int y = 0;
void L()
{
int z = 0;
void L2()
{
if (x == 0 && z == 0)
{
Action a = () => y++;
a();
}
}
L2();
}
L();
Console.WriteLine(y);
}
}
}";
CompileAndVerify(src, expectedOutput: "1");
}
[Fact]
[WorkItem(16895, "https://github.com/dotnet/roslyn/issues/16895")]
public void CaptureVarNestedLambdaSkipScope6()
{
var src = @"
using System;
class C
{
public static void Main()
{
int x = 0;
{
int y = 0;
void L()
{
int z = 0;
void L2()
{
if (x == 0 && y == 0)
{
Action a = () => z++;
a();
}
y++;
}
L2();
Console.WriteLine(z);
}
L();
Console.WriteLine(y);
}
((Action)(() => x++))();
Console.WriteLine(x);
}
}";
CompileAndVerify(src, expectedOutput: @"1
1
1");
}
[Fact]
[WorkItem(16895, "https://github.com/dotnet/roslyn/issues/16895")]
public void CaptureVarNestedLambdaSkipScope7()
{
var src = @"
using System;
using System.Threading.Tasks;
class C
{
public static void Main()
{
int x = 0;
{
int y = 0;
void L()
{
if (x == 0)
{
async Task L2()
{
await Task.Delay(1);
y++;
}
L2().Wait();
}
}
L();
Console.WriteLine(y);
}
Console.WriteLine(x);
}
}";
CompileAndVerify(src,
additionalRefs: new[] { MscorlibRef_v46 },
expectedOutput: @"1
0");
}
[Fact]
[WorkItem(16895, "https://github.com/dotnet/roslyn/issues/16895")]
public void CaptureVarNestedLambdaSkipScope8()
{
var src = @"
using System;
using System.Collections.Generic;
class C
{
public static void Main()
{
int x = 0;
{
int y = 0;
void L()
{
if (x == 0)
{
IEnumerable<int> L2()
{
yield return 0;
y++;
}
foreach (var i in L2()) { }
}
}
L();
Console.WriteLine(y);
}
Console.WriteLine(x);
}
}";
CompileAndVerify(src,
additionalRefs: new[] { MscorlibRef_v46 },
expectedOutput: @"1
0");
}
[Fact]
[WorkItem(16895, "https://github.com/dotnet/roslyn/issues/16895")]
public void LocalFunctionCaptureSkipScope()
{
var src = @"
using System;
class C
{
public static void Main(string[] args)
{
{
int uncaptured = 0;
uncaptured++;
{
int x = 0;
bool Local(int y) => x == 0 && args == null && y == 0;
Local(0);
}
}
}
}";
CompileAndVerify(src);
}
[Fact]
[WorkItem(16399, "https://github.com/dotnet/roslyn/issues/16399")]
public void RecursiveGenericLocalFunctionIterator()
......
Markdown is supported
0% .
You are about to add 0 people to the discussion. Proceed with caution.
先完成此消息的编辑!
想要评论请 注册