-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Async main codegen #18472
Changes from 49 commits
e27fb34
b4cee90
61e4705
39a8600
0cb90e3
4af405a
5a3fbcf
b16987f
8f0e223
ed6271d
578e811
92d3bfc
af675d3
1f71b66
671c53b
e99bf98
1114921
21a892c
c692009
9144795
cd2488e
d0675b1
04b5380
112f4fe
f532f23
617c791
5f177ba
d1e3e72
39cf05d
41cddf9
90edc8f
0407c20
8acedcc
9cbf02c
fe158be
89a15d8
1465d2c
2fc8c62
7d01d2e
e9e2875
81b01e2
4cd16ea
8a8afe2
1c2c643
c8b7b07
d975ba2
bad2923
85de26c
f121bb4
0a84b1d
e4e355e
b290268
eb68cea
247e05d
8e60326
881dea0
1d2b4db
3e8237a
2047833
7187917
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 (!candidate.HasEntryPointSignature(this, warnings)) | ||
{ | ||
// a single error for partial methods | ||
warnings.Add(ErrorCode.WRN_InvalidMainSig, candidate.Locations.First(), candidate); | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I do not understand the motivation behind the change? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather squiggle the whole signature. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -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; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 && entryPoint.ReturnsAwaitableToVoidOrInt(compilation, diagnostics)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can we simplify this condition? For example, could we simply check that return type is not int or void? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've changed ReturnsAwaitableToVoidOrInt to be faster in the case that the return type is int or void. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think we should call it at all, even when it returns true it does to much work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's being called at most once per emit. I think this is fine. |
||
{ | ||
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 && | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It is not clear why this assert is removed.#Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
using System.Diagnostics; | ||
using System.Linq; | ||
using Microsoft.CodeAnalysis.CSharp.Symbols; | ||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
using Microsoft.CodeAnalysis.Symbols; | ||
using Roslyn.Utilities; | ||
|
||
|
@@ -595,53 +596,67 @@ internal bool IsEntryPointCandidate | |
get { return IsStatic && Name == WellKnownMemberNames.EntryPointMethodName; } | ||
} | ||
|
||
|
||
internal bool ReturnsAwaitableToVoidOrInt(CSharpCompilation compilation, DiagnosticBag diagnostics) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider moving There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done (except that ReturnsAwaitableToVoidOrInt needs to be internal) |
||
{ | ||
// Common case optimization | ||
if (ReturnType.SpecialType == SpecialType.System_Void || ReturnType.SpecialType == SpecialType.System_Int32) | ||
{ | ||
return false; | ||
} | ||
|
||
if (!(ReturnType is NamedTypeSymbol namedType)) | ||
{ | ||
return false; | ||
} | ||
|
||
// Early bail so we only even check things that are System.Threading.Tasks.Task(<T>) | ||
if (!(namedType.ConstructedFrom == compilation.GetWellKnownType(WellKnownType.System_Threading_Tasks_Task) || | ||
namedType.ConstructedFrom == compilation.GetWellKnownType(WellKnownType.System_Threading_Tasks_Task_T))) | ||
{ | ||
return false; | ||
} | ||
|
||
|
||
var syntax = this.ExtractReturnTypeSyntax(); | ||
var dumbInstance = new BoundLiteral(syntax, ConstantValue.Null, ReturnType); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
// PROTOTYPE(async-main): We might need to adjust the containing member of the binder to be the Main method | ||
var binder = compilation.GetBinder(syntax); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please point me to the definition of this method? #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Please add a prototype comment that we might need to adjust the containing member of the binder to be the Main method. #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doen. |
||
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> | ||
internal bool HasEntryPointSignature(CSharpCompilation compilation) | ||
internal bool HasEntryPointSignature(CSharpCompilation compilation, DiagnosticBag bag) | ||
{ | ||
|
||
bool IsAsyncMainReturnType(TypeSymbol type) | ||
{ | ||
var namedType = type as NamedTypeSymbol; | ||
if (namedType == null) | ||
{ | ||
return false; | ||
} | ||
else if (namedType.ConstructedFrom == compilation.GetWellKnownType(WellKnownType.System_Threading_Tasks_Task)) | ||
{ | ||
// Change this to `namedType.IsNonGenericTaskType` if you want to support "task-like" objects. | ||
return true; | ||
} | ||
else if (namedType.ConstructedFrom == compilation.GetWellKnownType(WellKnownType.System_Threading_Tasks_Task_T)) | ||
{ | ||
// Change this to `namedType.IsGenericTaskType` if you want to support "task-like" objects. | ||
return namedType.TypeArguments[0].SpecialType == SpecialType.System_Int32; | ||
} | ||
else | ||
{ | ||
return false; | ||
} | ||
} | ||
|
||
if (IsVararg) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Empty lines above. |
||
{ | ||
return false; | ||
} | ||
|
||
TypeSymbol returnType = ReturnType; | ||
bool isAsyncMainReturnType = IsAsyncMainReturnType(returnType); | ||
if (returnType.SpecialType != SpecialType.System_Int32 && returnType.SpecialType != SpecialType.System_Void && !isAsyncMainReturnType) | ||
bool returnsTaskOrTaskOfInt = false; | ||
if (returnType.SpecialType != SpecialType.System_Int32 && returnType.SpecialType != SpecialType.System_Void) | ||
{ | ||
return false; | ||
// Never look for ReturnsAwaitableToVoidOrInt on int32 or void | ||
returnsTaskOrTaskOfInt = ReturnsAwaitableToVoidOrInt(compilation, 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 (!isAsyncMainReturnType && IsAsync && compilation.LanguageVersion >= LanguageVersion.CSharp7_1) | ||
if (!returnsTaskOrTaskOfInt && IsAsync && compilation.LanguageVersion >= LanguageVersion.CSharp7_1) | ||
{ | ||
return false; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
using System.Collections.Generic; | ||
using System.Collections.Immutable; | ||
using System.Diagnostics; | ||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
|
||
namespace Microsoft.CodeAnalysis.CSharp.Symbols | ||
{ | ||
|
@@ -300,5 +301,19 @@ public static bool IsGenericTaskReturningAsync(this MethodSymbol method, CSharpC | |
return method.IsAsync | ||
&& method.ReturnType.IsGenericTaskType(compilation); | ||
} | ||
|
||
internal static CSharpSyntaxNode ExtractReturnTypeSyntax(this MethodSymbol method) | ||
{ | ||
method = method.PartialDefinitionPart ?? method; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Please make sure there are tests for partial methods:
Result produced by this method should be observable in the tests, probably through location of an error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please make sure there are tests for partial methods:
Result produced by this method should be observable in the tests, probably through location of an error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Partial methods must be void returning, so they won't hit any of these new code paths. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we discussed offline, let's add direct tests for this function then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AlekseyTs: I've added tests for this API on non-main methods. |
||
foreach (var reference in method.DeclaringSyntaxReferences) | ||
{ | ||
if (reference.GetSyntax() is MethodDeclarationSyntax methodDeclaration) | ||
{ | ||
return methodDeclaration.ReturnType; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is this going to properly handle partial methods? How exactly? #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since partial methods needs to be void, this code path is never hit. If we ever decide to add this in the future, I've modified ExtractReturnTypeSyntax to first find the implementation syntax. |
||
} | ||
} | ||
|
||
return (CSharpSyntaxNode)CSharpSyntaxTree.Dummy.GetRoot(); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like the return value is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yep, not used anymore.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed