Skip to content

Commit 83e3611

Browse files
333fred333fred
committed
Expand testing coverage and cover more scenarios.
1 parent 8c4a58b commit 83e3611

16 files changed

+636
-192
lines changed

‎src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/RuntimeAsyncRewriter.cs‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ public static BoundStatement Rewrite(
2828
var hoistedLocals = ArrayBuilder<LocalSymbol>.GetInstance();
2929
var factory = new SyntheticBoundNodeFactory(method, node.Syntax, compilationState, diagnostics);
3030
var rewriter = new RuntimeAsyncRewriter(factory, variablesToHoist, hoistedLocals);
31-
var thisStore = cacheThisIfNeeded(rewriter);
31+
var thisStore = hoistThisIfNeeded(rewriter);
3232
var result = (BoundStatement)rewriter.Visit(node);
3333

3434
if (thisStore is not null)
@@ -49,15 +49,14 @@ public static BoundStatement Rewrite(
4949

5050
return SpillSequenceSpiller.Rewrite(result, method, compilationState, diagnostics);
5151

52-
static BoundAssignmentOperator? cacheThisIfNeeded(RuntimeAsyncRewriter rewriter)
52+
static BoundAssignmentOperator? hoistThisIfNeeded(RuntimeAsyncRewriter rewriter)
5353
{
5454
Debug.Assert(rewriter._factory.CurrentFunction is not null);
5555
var thisParameter = rewriter._factory.CurrentFunction.ThisParameter;
56-
if (rewriter._variablesToHoist.Contains(thisParameter))
56+
if (thisParameter is { Type.IsValueType: true, RefKind: RefKind.Ref })
5757
{
58-
// This is a struct or a type parameter, and accessed across an await. We need to replace it with a hoisted local to preserve behavior,
59-
// as `this` is a ref
60-
Debug.Assert(thisParameter is { Type.IsReferenceType: false, RefKind: RefKind.Ref });
58+
// This is a struct or a type parameter, and accessed across an await. We need to replace it with a hoisted local to preserve behavior from
59+
// compiler-generated state machines; `this` is a ref, but results are not observable outside of the method.
6160
var hoistedThis = rewriter._factory.StoreToTemp(rewriter._factory.This(), out BoundAssignmentOperator store, kind: SynthesizedLocalKind.AwaitByRefSpill);
6261
rewriter._hoistedLocals.Add(hoistedThis.LocalSymbol);
6362
rewriter._proxies.Add(thisParameter, new CapturedToExpressionSymbolReplacement<ParameterSymbol>(hoistedThis, hoistedFields: [], isReusable: true));
@@ -85,6 +84,17 @@ private RuntimeAsyncRewriter(SyntheticBoundNodeFactory factory, OrderedSet<Symbo
8584
_hoistedLocals = hoistedLocals;
8685
}
8786

87+
[return: NotNullIfNotNull(nameof(node))]
88+
public override BoundNode? Visit(BoundNode? node)
89+
{
90+
if (node == null) return node;
91+
var oldSyntax = _factory.Syntax;
92+
_factory.Syntax = node.Syntax;
93+
var result = base.Visit(node);
94+
_factory.Syntax = oldSyntax;
95+
return result;
96+
}
97+
8898
[return: NotNullIfNotNull(nameof(node))]
8999
public BoundExpression? VisitExpression(BoundExpression? node)
90100
{
@@ -206,11 +216,13 @@ public override BoundNode VisitAwaitableValuePlaceholder(BoundAwaitableValuePlac
206216
return base.VisitAssignmentOperator(node);
207217
}
208218

209-
BoundExpression originalRight = node.Right;
210219
BoundExpression visitedRight;
211220

212221
if (_variablesToHoist.Contains(leftLocal.LocalSymbol) && !_proxies.ContainsKey(leftLocal.LocalSymbol))
213222
{
223+
Debug.Assert(leftLocal.LocalSymbol.SynthesizedKind == SynthesizedLocalKind.Spill ||
224+
(leftLocal.LocalSymbol.SynthesizedKind == SynthesizedLocalKind.ForEachArray && leftLocal.LocalSymbol.Type.HasInlineArrayAttribute(out _) && leftLocal.LocalSymbol.Type.TryGetInlineArrayElementField() is object));
225+
Debug.Assert(node.IsRef);
214226
visitedRight = VisitExpression(node.Right);
215227
return _refInitializationHoister.HoistRefInitialization(
216228
leftLocal.LocalSymbol,
@@ -259,7 +271,7 @@ private bool TryReplaceWithProxy(Symbol localOrParameter, SyntaxNode syntax, [No
259271
return false;
260272
}
261273

262-
public sealed override BoundNode VisitLocal(BoundLocal node)
274+
public override BoundNode VisitLocal(BoundLocal node)
263275
{
264276
if (TryReplaceWithProxy(node.LocalSymbol, node.Syntax, out BoundNode? replacement))
265277
{
@@ -293,7 +305,7 @@ public sealed override BoundNode VisitLocal(BoundLocal node)
293305
return replacement;
294306
}
295307

296-
Debug.Assert(!_variablesToHoist.Contains(thisParameter));
308+
Debug.Assert(thisParameter is not { Type.IsValueType: true, RefKind: RefKind.Ref });
297309
return base.VisitThisReference(node);
298310
}
299311
}

‎src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/CapturedSymbol.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,13 +66,13 @@ internal sealed class CapturedToExpressionSymbolReplacement<THoistedSymbolType>
6666
where THoistedSymbolType : Symbol
6767
{
6868
private readonly BoundExpression _replacement;
69-
public readonly ImmutableArray<THoistedSymbolType> HoistedFields;
69+
public readonly ImmutableArray<THoistedSymbolType> HoistedSymbols;
7070

7171
public CapturedToExpressionSymbolReplacement(BoundExpression replacement, ImmutableArray<THoistedSymbolType> hoistedFields, bool isReusable)
7272
: base(isReusable)
7373
{
7474
_replacement = replacement;
75-
this.HoistedFields = hoistedFields;
75+
this.HoistedSymbols = hoistedFields;
7676
}
7777

7878
public override BoundExpression Replacement<TArg>(SyntaxNode node, Func<NamedTypeSymbol, TArg, BoundExpression> makeFrame, TArg arg)

‎src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/MethodToStateMachineRewriter.cs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,7 @@ private BoundStatement PossibleIteratorScope(ImmutableArray<LocalSymbol> locals,
369369
}
370370
else
371371
{
372-
foreach (var field in ((CapturedToExpressionSymbolReplacement<StateMachineFieldSymbol>)proxy).HoistedFields)
372+
foreach (var field in ((CapturedToExpressionSymbolReplacement<StateMachineFieldSymbol>)proxy).HoistedSymbols)
373373
{
374374
AddVariableCleanup(variableCleanup, field);
375375

@@ -652,15 +652,16 @@ public override BoundNode VisitAssignmentOperator(BoundAssignmentOperator node)
652652
leftLocal,
653653
visitedRight,
654654
proxies,
655-
doHoist,
655+
createHoistedSymbol,
656656
createHoistedAccess,
657657
this,
658658
isRuntimeAsync: false);
659659

660-
static StateMachineFieldSymbol doHoist(TypeSymbol type, MethodToStateMachineRewriter @this, LocalSymbol assignedLocal)
660+
static StateMachineFieldSymbol createHoistedSymbol(TypeSymbol type, MethodToStateMachineRewriter @this, LocalSymbol assignedLocal)
661661
{
662662
StateMachineFieldSymbol hoistedSymbol;
663663

664+
// https://github.com/dotnet/roslyn/issues/79793 - consider whether runtime async will need some of these optimizations
664665
if (@this.F.Compilation.Options.OptimizationLevel == OptimizationLevel.Debug)
665666
{
666667
const SynthesizedLocalKind kind = SynthesizedLocalKind.AwaitByRefSpill;

‎src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/RefInitializationHoister.cs‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ internal class RefInitializationHoister<THoistedSymbolType, THoistedAccess>(Synt
1818
private readonly SyntheticBoundNodeFactory _factory = f;
1919
private readonly MethodSymbol _originalMethod = originalMethod;
2020
private readonly TypeMap _typeMap = typeMap;
21+
private bool _reportedError;
2122

2223
internal BoundExpression? HoistRefInitialization<TArg>(
2324
LocalSymbol local,
@@ -81,7 +82,7 @@ private BoundExpression HoistExpression<TArg>(
8182
ArrayBuilder<BoundExpression> sideEffects,
8283
ArrayBuilder<THoistedSymbolType> hoistedSymbols,
8384
ref bool needsSacrificialEvaluation,
84-
Func<TypeSymbol, TArg, LocalSymbol, THoistedSymbolType> hoister,
85+
Func<TypeSymbol, TArg, LocalSymbol, THoistedSymbolType> createHoistedSymbol,
8586
Func<THoistedSymbolType, TArg, THoistedAccess> createHoistedAccess,
8687
TArg arg,
8788
bool isRuntimeAsync,
@@ -99,7 +100,7 @@ private BoundExpression HoistExpression<TArg>(
99100
sideEffects,
100101
hoistedSymbols,
101102
ref needsSacrificialEvaluation,
102-
hoister,
103+
createHoistedSymbol,
103104
createHoistedAccess,
104105
arg,
105106
isRuntimeAsync,
@@ -115,7 +116,7 @@ private BoundExpression HoistExpression<TArg>(
115116
sideEffects,
116117
hoistedSymbols,
117118
ref needsSacrificialEvaluation,
118-
hoister,
119+
createHoistedSymbol,
119120
createHoistedAccess,
120121
arg,
121122
isRuntimeAsync,
@@ -151,7 +152,7 @@ private BoundExpression HoistExpression<TArg>(
151152
sideEffects,
152153
hoistedSymbols,
153154
ref needsSacrificialEvaluation,
154-
hoister,
155+
createHoistedSymbol,
155156
createHoistedAccess,
156157
arg,
157158
isRuntimeAsync,
@@ -182,6 +183,7 @@ private BoundExpression HoistExpression<TArg>(
182183
Debug.Assert(refKind is RefKindExtensions.StrictIn or RefKind.Ref or RefKind.Out);
183184
Debug.Assert(call.Method.RefKind != RefKind.None);
184185
_factory.Diagnostics.Add(ErrorCode.ERR_RefReturningCallAndAwait, _factory.Syntax.Location, call.Method);
186+
_reportedError = true;
185187
}
186188
// method call is not referentially transparent, we can only spill the result value.
187189
refKind = RefKind.None;
@@ -200,6 +202,7 @@ private BoundExpression HoistExpression<TArg>(
200202
Debug.Assert(refKind is RefKindExtensions.StrictIn or RefKind.Ref or RefKind.In);
201203
Debug.Assert(conditional.IsRef);
202204
_factory.Diagnostics.Add(ErrorCode.ERR_RefConditionalAndAwait, _factory.Syntax.Location);
205+
_reportedError = true;
203206
}
204207
// conditional expr is not referentially transparent, we can only spill the result value.
205208
refKind = RefKind.None;
@@ -215,12 +218,14 @@ private BoundExpression HoistExpression<TArg>(
215218
{
216219
if (isRuntimeAsync)
217220
{
221+
// If an error was reported about ref escaping earlier, there could be illegal ref accesses later in the method,
222+
// so we track that to ensure that we don't see unexpected cases here.
218223
// This is an access to a field of a struct, or parameter or local of a type parameter, both of which happen by reference.
219224
// The receiver should be a non-ref local or parameter.
220225
// This is safe to hoist into a proxy as the original local will be accessed directly.
221-
Debug.Assert(isFieldAccessOfStruct || expr.Type!.IsTypeParameter());
222-
Debug.Assert(expr is BoundLocal { LocalSymbol.RefKind: RefKind.None }
223-
or BoundParameter { ParameterSymbol.RefKind: RefKind.None });
226+
Debug.Assert(_reportedError || isFieldAccessOfStruct || expr.Type!.IsTypeParameter());
227+
Debug.Assert(_reportedError || expr is BoundLocal { LocalSymbol.RefKind: RefKind.None }
228+
or BoundParameter { ParameterSymbol.RefKind: RefKind.None });
224229
}
225230
else
226231
{
@@ -229,7 +234,7 @@ private BoundExpression HoistExpression<TArg>(
229234
}
230235

231236
Debug.Assert(expr.Type is not null);
232-
var hoistedSymbol = hoister(expr.Type, arg, assignedLocal);
237+
var hoistedSymbol = createHoistedSymbol(expr.Type, arg, assignedLocal);
233238
hoistedSymbols.Add(hoistedSymbol);
234239

235240
var replacement = createHoistedAccess(hoistedSymbol, arg);

‎src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncEHTests.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
namespace Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen
1717
{
18+
[CompilerTrait(CompilerFeature.Async)]
1819
public class CodeGenAsyncEHTests : EmitMetadataTestBase
1920
{
2021
private static readonly MetadataReference[] s_asyncRefs = new[] { MscorlibRef_v4_0_30316_17626, SystemRef_v4_0_30319_17929, SystemCoreRef_v4_0_30319_17929 };

‎src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ internal enum Instruction
3131
YieldBreak
3232
}
3333

34-
[CompilerTrait(CompilerFeature.AsyncStreams)]
34+
[CompilerTrait(CompilerFeature.AsyncStreams, CompilerFeature.Async)]
3535
public class CodeGenAsyncIteratorTests : EmitMetadataTestBase
3636
{
3737
internal static string ExpectedOutput(string output)

‎src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncLocalsTests.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
namespace Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen
1818
{
19+
[CompilerTrait(CompilerFeature.Async)]
1920
public class CodeGenAsyncLocalsTests : EmitMetadataTestBase
2021
{
2122
private static readonly MetadataReference[] s_asyncRefs = new[] { MscorlibRef_v4_0_30316_17626, SystemRef_v4_0_30319_17929, SystemCoreRef_v4_0_30319_17929 };

‎src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMainTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616

1717
namespace Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen
1818
{
19-
[CompilerTrait(CompilerFeature.AsyncMain)]
19+
[CompilerTrait(CompilerFeature.AsyncMain, CompilerFeature.Async)]
2020
public class CodeGenAsyncMainTests : EmitMetadataTestBase
2121
{
2222
[Fact]

‎src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMethodBuilderOverrideTests.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
namespace Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen
1414
{
15+
[CompilerTrait(CompilerFeature.Async)]
1516
public class CodeGenAsyncMethodBuilderOverrideTests : EmitMetadataTestBase
1617
{
1718
private const string AsyncMethodBuilderAttribute =

0 commit comments

Comments
 (0)