Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async main codegen #18472

Merged
merged 60 commits into from
Apr 28, 2017
Merged
Show file tree
Hide file tree
Changes from 52 commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
e27fb34
attempt to get main entrypoints working
TyOverby Apr 3, 2017
b4cee90
async main codegen
TyOverby Apr 5, 2017
61e4705
remove garbage file
TyOverby Apr 5, 2017
39a8600
fix vb tests
TyOverby Apr 5, 2017
0cb90e3
add some comments
TyOverby Apr 5, 2017
4af405a
moduleBeingBuilt can sometimes be null apparently
TyOverby Apr 5, 2017
5a3fbcf
fix style nits
TyOverby Apr 6, 2017
b16987f
more whitespace fixes
TyOverby Apr 6, 2017
8f0e223
address feedback
TyOverby Apr 7, 2017
ed6271d
automated style fixer
TyOverby Apr 7, 2017
578e811
move most work out into the constructor
TyOverby Apr 7, 2017
92d3bfc
protect against null in error condition
TyOverby Apr 8, 2017
af675d3
add comment
TyOverby Apr 8, 2017
1f71b66
address cston review
TyOverby Apr 10, 2017
671c53b
autoformat
TyOverby Apr 10, 2017
e99bf98
use cast
TyOverby Apr 10, 2017
1114921
check module being built
TyOverby Apr 10, 2017
21a892c
add another object cast
TyOverby Apr 10, 2017
c692009
address review
TyOverby Apr 11, 2017
9144795
check for old cases where async methods called Main also exist
TyOverby Apr 11, 2017
cd2488e
more suggestions
TyOverby Apr 11, 2017
d0675b1
autoformat
TyOverby Apr 12, 2017
04b5380
mostly cleanup
TyOverby Apr 12, 2017
112f4fe
use shorter Count and Single
TyOverby Apr 12, 2017
f532f23
remove superfluous diagnostics add
TyOverby Apr 12, 2017
617c791
check exit code / revert to old entrypoint finding code
TyOverby Apr 13, 2017
5f177ba
remove language version check in entrypoint compiler
TyOverby Apr 13, 2017
d1e3e72
fix debug.assert
TyOverby Apr 13, 2017
39cf05d
switch to assert equal
TyOverby Apr 13, 2017
41cddf9
codereview
TyOverby Apr 14, 2017
90edc8f
add more tests; change conditions for test execution
TyOverby Apr 17, 2017
0407c20
fix up emit methods
TyOverby Apr 18, 2017
8acedcc
narrow down error location
TyOverby Apr 18, 2017
9cbf02c
ammend GetAwaitableExpressionInfo to return a bound call to GetAwaite…
TyOverby Apr 19, 2017
fe158be
more comments
TyOverby Apr 19, 2017
89a15d8
find entrypoint methods uses the async binder
TyOverby Apr 20, 2017
1465d2c
autoformat
TyOverby Apr 20, 2017
2fc8c62
use diagnostics from async binder
TyOverby Apr 20, 2017
7d01d2e
relocate tests
TyOverby Apr 21, 2017
e9e2875
fix suggestions for review 8346
TyOverby Apr 21, 2017
81b01e2
add back no-async tests with better location
TyOverby Apr 21, 2017
4cd16ea
only allow named types
TyOverby Apr 21, 2017
8a8afe2
Merge branch 'features/async-main' into async-main-codegen
TyOverby Apr 21, 2017
1c2c643
remove empty file; remove null checks from helper
TyOverby Apr 24, 2017
c8b7b07
forgot to add csproj file
TyOverby Apr 24, 2017
d975ba2
emit into warning diagnostics instead of regular diagnostics
TyOverby Apr 24, 2017
bad2923
autoformat
TyOverby Apr 24, 2017
85de26c
remove unused file from merge
TyOverby Apr 24, 2017
f121bb4
use diagnostic locations
TyOverby Apr 25, 2017
0a84b1d
move some methods around, split some tests up
TyOverby Apr 26, 2017
e4e355e
fix non-returning async tests, remove bool return from check availabi…
TyOverby Apr 26, 2017
b290268
add params test
TyOverby Apr 26, 2017
eb68cea
style and expected output fix
TyOverby Apr 26, 2017
247e05d
switch back to null because default emits warnings
TyOverby Apr 26, 2017
8e60326
add partial method tests
TyOverby Apr 27, 2017
881dea0
fix locations for unix
TyOverby Apr 27, 2017
1d2b4db
better partial syntax tests
TyOverby Apr 27, 2017
3e8237a
use expected length of 0
TyOverby Apr 27, 2017
2047833
use default length of 0 for expected output
TyOverby Apr 27, 2017
7187917
check implementation symbol
TyOverby Apr 27, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 23 additions & 12 deletions src/Compilers/CSharp/Portable/Binder/Binder_Await.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ private BoundExpression BindAwait(BoundExpression expression, SyntaxNode node, D
bool hasErrors =
ReportBadAwaitWithoutAsync(node, diagnostics) |
ReportBadAwaitContext(node, diagnostics) |
!GetAwaitableExpressionInfo(expression, out getAwaiter, out isCompleted, out getResult, node, diagnostics);
!GetAwaitableExpressionInfo(expression, out getAwaiter, out isCompleted, out getResult, out _, node, diagnostics);

// Spec 7.7.7.2:
// The expression await t is classified the same way as the expression (t).GetAwaiter().GetResult(). Thus,
Expand Down Expand Up @@ -209,17 +209,19 @@ private bool ReportBadAwaitContext(SyntaxNode node, DiagnosticBag diagnostics)
/// Finds and validates the required members of an awaitable expression, as described in spec 7.7.7.1.
/// </summary>
/// <returns>True if the expression is awaitable; false otherwise.</returns>
private bool GetAwaitableExpressionInfo(
internal bool GetAwaitableExpressionInfo(
BoundExpression expression,
out MethodSymbol getAwaiter,
out PropertySymbol isCompleted,
out MethodSymbol getResult,
out BoundExpression getAwaiterGetResultCall,
SyntaxNode node,
DiagnosticBag diagnostics)
{
getAwaiter = null;
isCompleted = null;
getResult = null;
getAwaiterGetResultCall = null;

if (!ValidateAwaitedExpression(expression, node, diagnostics))
{
Expand All @@ -231,15 +233,16 @@ private bool GetAwaitableExpressionInfo(
return true;
}

if (!GetGetAwaiterMethod(expression, node, diagnostics, out getAwaiter))
BoundExpression getAwaiterCall = null;
if (!GetGetAwaiterMethod(expression, node, diagnostics, out getAwaiter, out getAwaiterCall))
{
return false;
}

TypeSymbol awaiterType = getAwaiter.ReturnType;
return GetIsCompletedProperty(awaiterType, node, expression.Type, diagnostics, out isCompleted)
&& AwaiterImplementsINotifyCompletion(awaiterType, node, diagnostics)
&& GetGetResultMethod(awaiterType, node, expression.Type, diagnostics, out getResult);
&& GetGetResultMethod(getAwaiterCall, node, expression.Type, diagnostics, out getResult, out getAwaiterGetResultCall);
}

/// <summary>
Expand Down Expand Up @@ -273,26 +276,29 @@ private static bool ValidateAwaitedExpression(BoundExpression expression, Syntax
/// NOTE: this is an error in the spec. An extension method of the form
/// Awaiter&lt;T&gt; GetAwaiter&lt;T&gt;(this Task&lt;T&gt;) may be used.
/// </remarks>
private bool GetGetAwaiterMethod(BoundExpression expression, SyntaxNode node, DiagnosticBag diagnostics, out MethodSymbol getAwaiterMethod)
private bool GetGetAwaiterMethod(BoundExpression expression, SyntaxNode node, DiagnosticBag diagnostics, out MethodSymbol getAwaiterMethod, out BoundExpression getAwaiterCall)
{
if (expression.Type.SpecialType == SpecialType.System_Void)
{
Error(diagnostics, ErrorCode.ERR_BadAwaitArgVoidCall, node);
getAwaiterMethod = null;
getAwaiterCall = null;
return false;
}

var getAwaiterCall = MakeInvocationExpression(node, expression, WellKnownMemberNames.GetAwaiter, ImmutableArray<BoundExpression>.Empty, diagnostics);
getAwaiterCall = MakeInvocationExpression(node, expression, WellKnownMemberNames.GetAwaiter, ImmutableArray<BoundExpression>.Empty, diagnostics);
if (getAwaiterCall.HasAnyErrors) // && !expression.HasAnyErrors?
{
getAwaiterMethod = null;
getAwaiterCall = null;
return false;
}

if (getAwaiterCall.Kind != BoundKind.Call)
{
Error(diagnostics, ErrorCode.ERR_BadAwaitArg, node, expression.Type);
getAwaiterMethod = null;
getAwaiterCall = null;
return false;
}

Expand All @@ -303,6 +309,7 @@ private bool GetGetAwaiterMethod(BoundExpression expression, SyntaxNode node, Di
{
Error(diagnostics, ErrorCode.ERR_BadAwaitArg, node, expression.Type);
getAwaiterMethod = null;
getAwaiterCall = null;
return false;
}

Expand Down Expand Up @@ -383,35 +390,39 @@ private bool AwaiterImplementsINotifyCompletion(TypeSymbol awaiterType, SyntaxNo
/// Spec 7.7.7.1:
/// An Awaiter A has an accessible instance method GetResult with no parameters and no type parameters.
/// </remarks>
private bool GetGetResultMethod(TypeSymbol awaiterType, SyntaxNode node, TypeSymbol awaitedExpressionType, DiagnosticBag diagnostics, out MethodSymbol getResultMethod)
private bool GetGetResultMethod(BoundExpression awaiterExpression, SyntaxNode node, TypeSymbol awaitedExpressionType, DiagnosticBag diagnostics, out MethodSymbol getResultMethod, out BoundExpression getAwaiterGetResultCall)
{
var receiver = new BoundLiteral(node, ConstantValue.Null, awaiterType);
var getResultCall = MakeInvocationExpression(node, receiver, WellKnownMemberNames.GetResult, ImmutableArray<BoundExpression>.Empty, diagnostics);
if (getResultCall.HasAnyErrors)
var awaiterType = awaiterExpression.Type;
getAwaiterGetResultCall = MakeInvocationExpression(node, awaiterExpression, WellKnownMemberNames.GetResult, ImmutableArray<BoundExpression>.Empty, diagnostics);
if (getAwaiterGetResultCall.HasAnyErrors)
{
getResultMethod = null;
getAwaiterGetResultCall = null;
return false;
}

if (getResultCall.Kind != BoundKind.Call)
if (getAwaiterGetResultCall.Kind != BoundKind.Call)
{
Error(diagnostics, ErrorCode.ERR_NoSuchMember, node, awaiterType, WellKnownMemberNames.GetResult);
getResultMethod = null;
getAwaiterGetResultCall = null;
return false;
}

getResultMethod = ((BoundCall)getResultCall).Method;
getResultMethod = ((BoundCall)getAwaiterGetResultCall).Method;
if (getResultMethod.IsExtensionMethod)
{
Error(diagnostics, ErrorCode.ERR_NoSuchMember, node, awaiterType, WellKnownMemberNames.GetResult);
getResultMethod = null;
getAwaiterGetResultCall = null;
return false;
}

if (HasOptionalOrVariableParameters(getResultMethod) || getResultMethod.IsConditional)
{
Error(diagnostics, ErrorCode.ERR_BadAwaiterPattern, node, awaiterType, awaitedExpressionType);
getResultMethod = null;
getAwaiterGetResultCall = null;
return false;
}

Expand Down
106 changes: 104 additions & 2 deletions src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1457,7 +1457,7 @@ private MethodSymbol FindEntryPoint(CancellationToken cancellationToken, out Imm
var viableEntryPoints = ArrayBuilder<MethodSymbol>.GetInstance();
foreach (var candidate in entryPointCandidates)
{
if (!candidate.HasEntryPointSignature(this))
if (!HasEntryPointSignature(candidate, warnings))
{
// a single error for partial methods
warnings.Add(ErrorCode.WRN_InvalidMainSig, candidate.Locations.First(), candidate);
Expand All @@ -1471,10 +1471,13 @@ private MethodSymbol FindEntryPoint(CancellationToken cancellationToken, out Imm
continue;
}

// PROTOTYPE(async-main): CheckFeatureAvailability should be called on non-async methods
// that return Task or Task<T>
if (candidate.IsAsync)
{
// PROTOTYPE(async-main): Get the diagnostic to point to a smaller syntax piece.
CheckFeatureAvailability(candidate.DeclaringSyntaxReferences.Single().GetSyntax(), MessageID.IDS_FeatureAsyncMain, diagnostics);
// PROTOTYPE(async-main): Switch diagnostics around if the type is not Task or Task<T>
CheckFeatureAvailability(candidate.GetNonNullSyntaxNode(), MessageID.IDS_FeatureAsyncMain, diagnostics);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CheckFeatureAvailability(candidate.GetNonNullSyntaxNode(), MessageID.IDS_FeatureAsyncMain, diagnostics); [](start = 24, length = 104)

I do not understand the motivation behind the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make sense to attach the error to the return type, it is much nicer to have it on the entire method body.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is much nicer to have it on the entire method body.

Why do you think it is much nice to squiggle entire method body for the feature error? I there something wrong with the code in the method? How is this going to interact with other errors in the body? Will the customer be able to see that location?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather squiggle the whole signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squiggling just the return type when the only real problem is the "async" isn't intuitive at all.

}

viableEntryPoints.Add(candidate);
Expand Down Expand Up @@ -1525,6 +1528,105 @@ private MethodSymbol FindEntryPoint(CancellationToken cancellationToken, out Imm
}
}

internal bool ReturnsAwaitableToVoidOrInt(MethodSymbol method, DiagnosticBag diagnostics)
{
// Common case optimization
if (method.ReturnType.SpecialType == SpecialType.System_Void || method.ReturnType.SpecialType == SpecialType.System_Int32)
{
return false;
}

if (!(method.ReturnType is NamedTypeSymbol namedType))
{
return false;
}

// Early bail so we only even check things that are System.Threading.Tasks.Task(<T>)
if (!(namedType.ConstructedFrom == GetWellKnownType(WellKnownType.System_Threading_Tasks_Task) ||
namedType.ConstructedFrom == GetWellKnownType(WellKnownType.System_Threading_Tasks_Task_T)))
{
return false;
}


var syntax = method.ExtractReturnTypeSyntax();
var dumbInstance = new BoundLiteral(syntax, ConstantValue.Null, method.ReturnType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BoundDefaultExpression(syntax, method.ReturnType)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, swore I fixed this earlier.

Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var dumbInstance = new BoundLiteral(syntax, ConstantValue.Null, method.ReturnType); [](start = 12, length = 83)

Were you going to change this to BoundDefaultExpression?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was, but the synthesized expression was emitting warnings about using "dot" after default, so I reverted it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

// PROTOTYPE(async-main): We might need to adjust the containing member of the binder to be the Main method
var binder = GetBinder(syntax);
BoundExpression result;
var success = binder.GetAwaitableExpressionInfo(dumbInstance, out _, out _, out _, out result, syntax, diagnostics);

return success &&
(result.Type.SpecialType == SpecialType.System_Void || result.Type.SpecialType == SpecialType.System_Int32);
}

/// <summary>
/// Checks if the method has an entry point compatible signature, i.e.
/// - the return type is either void, int, <see cref="System.Threading.Tasks.Task" />,
/// or <see cref="System.Threading.Tasks.Task{T}" /> where T is an int.
/// - has either no parameter or a single parameter of type string[]
/// </summary>
private bool HasEntryPointSignature(MethodSymbol method, DiagnosticBag bag)
{
if (method.IsVararg)
{
return false;
}

TypeSymbol returnType = method.ReturnType;
bool returnsTaskOrTaskOfInt = false;
if (returnType.SpecialType != SpecialType.System_Int32 && returnType.SpecialType != SpecialType.System_Void)
{
// Never look for ReturnsAwaitableToVoidOrInt on int32 or void
returnsTaskOrTaskOfInt = ReturnsAwaitableToVoidOrInt(method, bag);
if (!returnsTaskOrTaskOfInt)
{
return false;
}
}

// Prior to 7.1, async methods were considered to "have the entrypoint signature".
// In order to keep back-compat, we need to let these through if compiling using a previous language version.
if (!returnsTaskOrTaskOfInt && method.IsAsync && this.LanguageVersion >= LanguageVersion.CSharp7_1)
{
return false;
}

if (method.RefKind != RefKind.None)
{
return false;
}

if (method.RefKind != RefKind.None)
{
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


if (method.Parameters.Length == 0)
{
return true;
}

if (method.Parameters.Length > 1)
{
return false;
}

if (!method.ParameterRefKinds.IsDefault)
{
return false;
}

var firstType = method.Parameters[0].Type;
if (firstType.TypeKind != TypeKind.Array)
{
return false;
}

var array = (ArrayTypeSymbol)firstType;
return array.IsSZArray && array.ElementType.SpecialType == SpecialType.System_String;
}

internal override bool IsUnreferencedAssemblyIdentityDiagnosticCode(int code)
=> code == (int)ErrorCode.ERR_NoTypeDef;

Expand Down
24 changes: 21 additions & 3 deletions src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,8 @@ public static void CompileMethodBodies(
}
}

// Returns the MethodSymbol for the assembly entrypoint. If the user has a Task returning main,
// this function returns the synthesized Main MethodSymbol.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to specify the "MoveNext" as an entry point for debug purposes?
I think it was required for some debugging scenarios like F11 to work if there is any code that runs before the actual Main, but I am not sure if that applies here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have to specify two entry points - actual and for debugging purposes, it would be a different change, I guess.


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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll manually test this and see if I run into anything.

private static MethodSymbol GetEntryPoint(CSharpCompilation compilation, PEModuleBuilder moduleBeingBuilt, bool hasDeclarationErrors, DiagnosticBag diagnostics, CancellationToken cancellationToken)
{
var entryPointAndDiagnostics = compilation.GetEntryPointAndDiagnostics(cancellationToken);
Expand All @@ -194,9 +196,26 @@ private static MethodSymbol GetEntryPoint(CSharpCompilation compilation, PEModul

Debug.Assert(!entryPointAndDiagnostics.Diagnostics.IsDefault);
diagnostics.AddRange(entryPointAndDiagnostics.Diagnostics);

var entryPoint = entryPointAndDiagnostics.MethodSymbol;
var synthesizedEntryPoint = entryPoint as SynthesizedEntryPointSymbol;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a comment to the top of the function noting that this returns the real entry point vs. the user defined one. The name of the function is ambiguous with this feature now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if ((object)entryPoint == null)
{
Debug.Assert(entryPointAndDiagnostics.Diagnostics.HasAnyErrors() || !compilation.Options.Errors.IsDefaultOrEmpty);
return null;
}

// entryPoint can be a SynthesizedEntryPointSymbol if a script is being compiled.
SynthesizedEntryPointSymbol synthesizedEntryPoint = entryPoint as SynthesizedEntryPointSymbol;
if ((object)synthesizedEntryPoint == null && compilation.ReturnsAwaitableToVoidOrInt(entryPoint, diagnostics))
{
synthesizedEntryPoint = new SynthesizedEntryPointSymbol.AsyncForwardEntryPoint(compilation, diagnostics, entryPoint.ContainingType, entryPoint);
entryPoint = synthesizedEntryPoint;
if ((object)moduleBeingBuilt != null)
{
moduleBeingBuilt.AddSynthesizedDefinition(entryPoint.ContainingType, synthesizedEntryPoint);
}
}

if (((object)synthesizedEntryPoint != null) &&
(moduleBeingBuilt != null) &&
!hasDeclarationErrors &&
Expand All @@ -221,7 +240,6 @@ private static MethodSymbol GetEntryPoint(CSharpCompilation compilation, PEModul
moduleBeingBuilt.SetMethodBody(synthesizedEntryPoint, emittedBody);
}

Debug.Assert((object)entryPoint != null || entryPointAndDiagnostics.Diagnostics.HasAnyErrors() || !compilation.Options.Errors.IsDefaultOrEmpty);
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug.Assert((object)entryPoint != null || entryPointAndDiagnostics.Diagnostics.HasAnyErrors() || !compilation.Options.Errors.IsDefaultOrEmpty); [](start = 12, length = 144)

It is not clear why this assert is removed.#Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assert wasn't deleted, it was moved upwards in the file to line 203

return entryPoint;
}

Expand Down
Loading