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 10 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
27 changes: 25 additions & 2 deletions src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Emit;
using Roslyn.Utilities;
using static Microsoft.CodeAnalysis.CSharp.Symbols.SynthesizedEntryPointSymbol;
Copy link
Member

Choose a reason for hiding this comment

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

Is this using static needed?

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 mean, yeah, it does a member that is being used statically. However, I could just fully-qualify that use.


namespace Microsoft.CodeAnalysis.CSharp
{
Expand Down Expand Up @@ -184,6 +185,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 @@ -196,7 +199,23 @@ private static MethodSymbol GetEntryPoint(CSharpCompilation compilation, PEModul
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 (entryPoint == null)
{
Debug.Assert(entryPointAndDiagnostics.Diagnostics.HasAnyErrors() || !compilation.Options.Errors.IsDefaultOrEmpty);
return null;
}

bool addedDefinition = false;
SynthesizedEntryPointSymbol synthesizedEntryPoint = entryPoint as SynthesizedEntryPointSymbol;

if ((object)synthesizedEntryPoint == null && entryPoint.HasAsyncMainReturnType(compilation) && compilation.LanguageVersion >= LanguageVersion.CSharp7_1)
Copy link
Member

Choose a reason for hiding this comment

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

Are we reporting an error for Task-returning Main when compiling with earlier versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but only if there are no non-async mains available.

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 really need to check compilation.LanguageVersion here? We've already checked the version in FindEntryPoint and reported any diagnostics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, good point.

Done.

{
synthesizedEntryPoint = new AsyncForwardEntryPoint(compilation, diagnostics, entryPoint.ContainingType, entryPoint);
Copy link
Member

Choose a reason for hiding this comment

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

It seems odd that we are attaching the diagnostics to only the async sythesized method symbol. What is the rationale for this?

In the case the diagnostics parameter had existing diagnostics in it, possible for a bag, it would be attaching those errors to the generated main as well which seems incorrect. As say using entryPointAndDiagnostics.Diagnostics. Even that though feels like we are double tracking diagnostics here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now the only place that the diagnosticsBag is used is during the GetRequiredMethod lookup for GetAwaiter and GetResult.

entryPoint = synthesizedEntryPoint;
addedDefinition = true;
}

if (((object)synthesizedEntryPoint != null) &&
(moduleBeingBuilt != null) &&
!hasDeclarationErrors &&
Expand All @@ -221,7 +240,11 @@ 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

if (addedDefinition && moduleBeingBuilt != null)
{
moduleBeingBuilt.AddSynthesizedDefinition(entryPoint.ContainingType, synthesizedEntryPoint);
}
Copy link
Member

Choose a reason for hiding this comment

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

Could addedDefinition be removed and this if moved to where addedDefinition is set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason I pulled this out specifically, but that might have been because I was fixing other bugs in the implementation and wanted to isolate this.

Changing it back to what you recommend does work.

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


return entryPoint;
}

Expand Down
47 changes: 24 additions & 23 deletions src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,29 @@ internal bool IsEntryPointCandidate
get { return IsStatic && Name == WellKnownMemberNames.EntryPointMethodName; }
}

internal bool HasAsyncMainReturnType(CSharpCompilation compilation)
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.

internal bool HasAsyncMainReturnType(CSharpCompilation compilation) [](start = 8, length = 67)

This looks like a very specialized helper, I wouldn't put it into MethodSymbol. Consider putting it next to the consumer and making it private. Also, the name is somewhat confusing because the method doesn't care whether the method is async or not. It only checks the return type for Task. Consider renaming the method to reflect that. #Closed

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.

I see that the method is used in more than one place, it is fine to keep it here, I guess.#Closed #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.

I'll move it, but I think the name is fine as-is. "HasAsyncMainReturnType" tells me that the return type needs to be Task or Task<int>,

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.

"HasAsyncMainReturnType" tells me that the return type needs to be Task or Task<int>

Then the name should covey just that, for example ReturnsTaskOrTaskOfInt, describes exactly what to expect from the function. #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.

Changed name.

I left it in MethodSymbol because MethodSymbol also has HasEntryPointSignature, which is also a pretty specialized helper. Maybe both of these should be moved somewhere else?

{
var namedType = ReturnType as NamedTypeSymbol;
if (namedType == null)
Copy link
Member

Choose a reason for hiding this comment

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

(object)namedType == null

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

{
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.
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.

// Change this to namedType.IsNonGenericTaskType if you want to support "task-like" objects. [](start = 16, length = 94)

Should this be a PROTOTYPE comment?#Closed #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.

Right now we are making a conscious decision to ban task-likes. I left the comment in case we wanted to loosen the rules later on.

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.
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.

// Change this to namedType.IsGenericTaskType if you want to support "task-like" objects. [](start = 16, length = 91)

Should this be a PROTOTYPE comment?#Closed #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.

Right now we are making a conscious decision to ban task-likes. I left the comment in case we wanted to loosen the rules later on.

return namedType.TypeArguments[0].SpecialType == SpecialType.System_Int32;
}
else
{
return false;
}
}

/// <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" />,
Expand All @@ -604,36 +627,14 @@ internal bool IsEntryPointCandidate
internal bool HasEntryPointSignature(CSharpCompilation compilation)
{

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)
Copy link
Member

Choose a reason for hiding this comment

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

Empty lines above.

{
return false;
}

TypeSymbol returnType = ReturnType;
bool isAsyncMainReturnType = IsAsyncMainReturnType(returnType);
bool isAsyncMainReturnType = HasAsyncMainReturnType(compilation);
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.

isAsyncMainReturnType [](start = 17, length = 21)

The name of the local is confusing, it implies that the method is async, yet it may disagree with IsAsync value.#Closed #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.

Done.

if (returnType.SpecialType != SpecialType.System_Int32 && returnType.SpecialType != SpecialType.System_Void && !isAsyncMainReturnType)
{
return false;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
Expand Down Expand Up @@ -329,6 +330,157 @@ private static BoundCall CreateParameterlessCall(CSharpSyntaxNode syntax, BoundE
{ WasCompilerGenerated = true };
}

// A synthesized entrypoint that forwards all calls to an async Main Method
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.

// A synthesized entrypoint that forwards all calls to an async Main Method [](start = 8, length = 75)

Please use standard doc comments.#Closed #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.

Done.

internal sealed class AsyncForwardEntryPoint : SynthesizedEntryPointSymbol
{
// The user-defined asyncrhonous main method.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: asynchronous

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

private readonly MethodSymbol _userMain;
private DiagnosticBag _diagnosticBag;
Copy link
Member

Choose a reason for hiding this comment

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

This can be readonly as well.

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.

Copy link
Member

Choose a reason for hiding this comment

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

Have we figured out my earlier questionts about this bag in general? It seems like it's opening the door for duplicate / unnecessary errors here. Feel like they should be resolved at the point of creating this symbol, not later.

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've moved this bag to the constructor and provide the userMain location as the location for the diagnostic to go.


private readonly ReturnKind _returnKind;
private readonly ParamKind _paramKind;
Copy link
Member

Choose a reason for hiding this comment

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

Can be a local or removed if ParamKind is removed.


private readonly ImmutableArray<ParameterSymbol> _parameters;

// Which "kind" of Main is this: `void Main` or `int Main`?
// This is determined by looking at the return type of the user-provided Main
private enum ReturnKind
{
VoidReturning,
IntReturning,
}
// Does this main method need to forward arguments to the user-provided Main?
private enum ParamKind
{
NoArgs,
StringArrayArgs,
}

// Task -> Void
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.

// Task -> Void [](start = 12, length = 15)

Please use standard doc comments.#Closed #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.

Done.

// Task<_> -> Int32
private static TypeSymbol TranslateReturnType(CSharpCompilation compilation, TypeSymbol returnType)
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 17, 2017

Choose a reason for hiding this comment

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

private static TypeSymbol TranslateReturnType(CSharpCompilation compilation, TypeSymbol returnType) [](start = 12, length = 99)

According to the C# language specification type of the task's result is determined based on the return type of the GetResult method, rather than based on the shape of the task type itself. Implementation of this function doesn't follow this. #Closed

{
return returnType.IsGenericTaskType(compilation)
? compilation.GetSpecialType(SpecialType.System_Int32)
: compilation.GetSpecialType(SpecialType.System_Void);
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for GetSpecialType to return null here or are we sufficiently protected by this point? If it can return null then we need to account for that as the return here is used in places where null isn't acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should be protected at this point, as this code is only executed if an async-main entrypoint was found, which can't happen if GetSpecialType is broken.

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.

compilation.GetSpecialType(SpecialType.System_Void) [](start = 22, length = 51)

It feels like this can fail without producing any errors, we should be using a helper from the Binder instead that makes sure the right diagnostics is reported. #Closed

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.

Please add tests for cases when int or void are missing (two separate cases). #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.

I believe that this code can't fail due to the checks in IsEntryPointSignature.

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 17, 2017

Choose a reason for hiding this comment

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

I believe that this code can't fail due to the checks in IsEntryPointSignature.

I see nothing in IsEntryPointSignature that would prevent us from from getting here when the void type is missing. #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.

Done.


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

}

// Task -> ReturnKind.VoidReturning
// Task<_> -> ReturnKind.IntReturning
private static ReturnKind GetReturnKind(CSharpCompilation compilation, TypeSymbol returnType)
{
return returnType.IsGenericTaskType(compilation) ? ReturnKind.IntReturning : ReturnKind.VoidReturning;
}

internal AsyncForwardEntryPoint(CSharpCompilation compilation, DiagnosticBag diagnosticBag, NamedTypeSymbol containingType, MethodSymbol userMain) :
base(containingType, TranslateReturnType(compilation, userMain.ReturnType))
{
// There should be no way for a userMain to be passed in unless it already passed the
// parameter checks for determining entrypoint validity.
Debug.Assert(userMain.ParameterCount == 0 || userMain.ParameterCount == 1);

_userMain = userMain;
_diagnosticBag = diagnosticBag;
_paramKind = userMain.ParameterCount == 0 ? ParamKind.NoArgs : ParamKind.StringArrayArgs;
_returnKind = GetReturnKind(compilation, userMain.ReturnType);
Copy link
Member

Choose a reason for hiding this comment

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

Consider moving GetReturnKind() call to CreateBody and remove _returnKind field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GetReturnKind requires a Compilation object, so I'd rather keep it here.


switch (_paramKind)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps switch (userMain.ParameterCount) instead. Then ParamKind can be removed.

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

{
case ParamKind.StringArrayArgs:
var stringType = compilation.GetSpecialType(SpecialType.System_String);
var stringArrayType = ArrayTypeSymbol.CreateCSharpArray(compilation.Assembly, stringType);
_parameters = ImmutableArray.Create(
SynthesizedParameterSymbol.Create(this, stringArrayType, 0, RefKind.None, userMain.Parameters[0].Name));
break;
default:
_parameters = ImmutableArray<ParameterSymbol>.Empty;
break;
Copy link
Member

Choose a reason for hiding this comment

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

Consider separate cases for case 0: and default: and throw UnexpectedValue() in the latter. That would replace the Debug.Assert(...ParameterCount) above.

}
}

public override string Name => MainName;

public override ImmutableArray<ParameterSymbol> Parameters => _parameters;

internal override BoundBlock CreateBody()
{
var syntax = this.GetSyntax();
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 14, 2017

Choose a reason for hiding this comment

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

var syntax = this.GetSyntax(); [](start = 16, length = 30)

Does this return the root of CSharpSyntaxTree.Dummy? It feels like we might want to use more specific node. #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.

It does. Just like the rest of the synthesized entry points. What other tree would we use?

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 17, 2017

Choose a reason for hiding this comment

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

It does. Just like the rest of the synthesized entry points. What other tree would we use?

We should use syntax corresponding to the method we are wrapping. #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.

There's no equivalent syntax from what we've made to what we're producing. How would red squiggles all over their async main that don't correspond to the actual error be helpful?

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 17, 2017

Choose a reason for hiding this comment

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

There's no equivalent syntax from what we've made to what we're producing

Actually there is - it is the declaration of the user's Main that we are wrapping. Pretty much every time we synthesize code, there is a user syntax that is related to that and we are using that syntax rather than providing no location at all. You are using bad code as a template, given the amount of issues with it, it is a prototype quality code at best - some experiments around scripting.

How would red squiggles all over their async main that don't correspond to the actual error be helpful?

First of all they won't be "all over their async main", I would expect as to squiggle just its name, this what we usually do when we report errors for a declaration. And this will be helpful as it would point the declaration that triggered the errors, i.e. the source of the problem. #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.

Which specific node? What is the point of having a specific "dummy" node?


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


var arguments = Parameters.SelectAsArray(p => new BoundParameter(syntax, p, p.Type) as BoundExpression);
Copy link
Member

Choose a reason for hiding this comment

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

Use (BoundExpression) cast rather than as since the cast cannot fail.

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


// Main(args) or Main()
BoundCall userMainInvocation = new BoundCall(
syntax: this.GetSyntax(),
Copy link
Member

Choose a reason for hiding this comment

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

syntax: syntax

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.

receiverOpt: null,
method: _userMain,
arguments: arguments,
argumentNamesOpt: default(ImmutableArray<string>),
argumentRefKindsOpt: default(ImmutableArray<RefKind>),
isDelegateCall: false,
expanded: false,
Copy link
Member

Choose a reason for hiding this comment

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

Consider testing synthesized call to static Task Main(params string[] args) { ... }.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you expand on what you mean by this?

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've added a test for params

invokedAsExtensionMethod: false,
argsToParamsOpt: default(ImmutableArray<int>),
resultKind: LookupResultKind.Viable,
type: _userMain.ReturnType)
{ WasCompilerGenerated = true };

// PROTOTYPE(async-main): Errors should likely be reported here instead of bailing.
Copy link
Member

Choose a reason for hiding this comment

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

Agree. Likely we can just move this up into the creation of the type. This would also avoid the need to store the bag on the type.

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.

var getAwaiterMethod = GetRequiredMethod(_userMain.ReturnType, WellKnownMemberNames.GetAwaiter, _diagnosticBag);
Debug.Assert(getAwaiterMethod != null);

var getResultMethod = GetRequiredMethod(getAwaiterMethod.ReturnType, WellKnownMemberNames.GetResult, _diagnosticBag);
Debug.Assert(getResultMethod != null);

// GetAwaiter().GetResult()
BoundCall getAwaiterGetResult =
CreateParameterlessCall(
syntax: syntax,
method: getResultMethod,
receiver: CreateParameterlessCall(
syntax: syntax,
method: getAwaiterMethod,
receiver: userMainInvocation
)
);

BoundStatement statement = null;
switch (_returnKind)
Copy link
Member

Choose a reason for hiding this comment

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

Is _returnKind necessary or could this be replaced with if (ReturnsVoid) { ... } else { ... }?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's clean enough I guess.

{
case ReturnKind.IntReturning:
// return (GetAwaiter().GetResult())
statement = new BoundReturnStatement(
syntax: syntax,
refKind: RefKind.None,
expressionOpt: getAwaiterGetResult
);
break;
case ReturnKind.VoidReturning:
// { GetAwaiter().GetResult(); return; }
statement = new BoundBlock(
Copy link
Member

Choose a reason for hiding this comment

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

Can we just return this BoundBlock rather than creating an additional BoundBlock below?

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.

syntax: syntax,
locals: ImmutableArray<LocalSymbol>.Empty,
statements: ImmutableArray.Create<BoundStatement>(
new BoundExpressionStatement(
syntax: syntax,
expression: getAwaiterGetResult
),
new BoundReturnStatement(
syntax: syntax,
refKind: RefKind.None,
expressionOpt: null
)
)
);
break;
};

return new BoundBlock(
syntax: syntax,
locals: ImmutableArray<LocalSymbol>.Empty,
statements: ImmutableArray.Create<BoundStatement>(statement));
}
}

private sealed class ScriptEntryPoint : SynthesizedEntryPointSymbol
{
private readonly MethodSymbol _getAwaiterMethod;
Expand All @@ -344,15 +496,9 @@ internal ScriptEntryPoint(NamedTypeSymbol containingType, TypeSymbol returnType,
_getResultMethod = getResultMethod;
}

public override string Name
{
get { return MainName; }
}
public override string Name => MainName;

public override ImmutableArray<ParameterSymbol> Parameters
{
get { return ImmutableArray<ParameterSymbol>.Empty; }
}
public override ImmutableArray<ParameterSymbol> Parameters => ImmutableArray<ParameterSymbol>.Empty;

// private static void <Main>()
// {
Expand Down
Loading