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 8 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
32 changes: 29 additions & 3 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,8 +199,27 @@ private static MethodSymbol GetEntryPoint(CSharpCompilation compilation, PEModul
diagnostics.AddRange(entryPointAndDiagnostics.Diagnostics);

var entryPoint = entryPointAndDiagnostics.MethodSymbol;
var synthesizedEntryPoint = entryPoint as SynthesizedEntryPointSymbol;
if (((object)synthesizedEntryPoint != null) &&

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;
}

SynthesizedEntryPointSymbol synthesizedEntryPoint = null;
bool addedDefinition = false;
if (entryPoint is SynthesizedEntryPointSymbol s)
Copy link
Member

Choose a reason for hiding this comment

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

is SynthesizedEntryPointSymbol s followed by synthesizedEntryPoint = s; seems more convoluted (to me at least) than:

var synthesizedEntryPoint = entryPoint as SynthesizedEntryPointSymbol;
if ((object)synthesizedEntryPoint == null)
{
    // other cases
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doen.

{
synthesizedEntryPoint = s;
Copy link
Member

Choose a reason for hiding this comment

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

What case hits this branch of the if?

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 other two other subclasses of SynthesizedEntryPointSymbol are ScriptEntryPoint and SubmissionEntryPoint both of which (I think) are for scripting support.

}
else if (entryPoint.HasAsyncMainReturnType(compilation) && compilation.LanguageVersion >= LanguageVersion.CSharp7_1)
{
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 ((synthesizedEntryPoint != null) &&
Copy link
Member

Choose a reason for hiding this comment

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

Need to use the ((object)synthesizedEntryPoint) != null style of comparison 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.

Done

(moduleBeingBuilt != null) &&
!hasDeclarationErrors &&
!diagnostics.HasAnyErrors())
Expand All @@ -221,7 +243,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,169 @@ 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 MethodSymbol _userMain;
Copy link
Member

Choose a reason for hiding this comment

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

readonly

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

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 CSharpCompilation _compilation;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this stored as a field? It seems to only be used in the constructor of 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.

You're right, this reference must have survived some refactors.


private ReturnKind _returnKind;
private 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.

both of these can be readonly

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


private ImmutableArray<ParameterSymbol> _parameters;
Copy link
Member

Choose a reason for hiding this comment

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

readonly

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.


// 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)
{
if (returnType.IsGenericTaskType(compilation)) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: more code formatting issues all through this file. Please review the entire PR and correct these.


return compilation.GetSpecialType(SpecialType.System_Int32);
}
else
{
return 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.

Consider using conditional operator here and in method below:

return returnType.IsGenericTaskType(compilation) ? ... : ...;

Copy link
Contributor Author

@TyOverby TyOverby Apr 6, 2017

Choose a reason for hiding this comment

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

What's the style formatting for breaking this across multiple lines?

I'm partial to

return returnType.IsGenericTaskType(compilation) 
    ? compilation.GetSpecialType(SpecialType.System_Int32)
    : compilation.GetSpecialType(SpecialType.System_Void);

}

private static ReturnKind GetReturnKind(CSharpCompilation compilation, TypeSymbol returnType) {
if (returnType.IsGenericTaskType(compilation)) {
return ReturnKind.IntReturning;
}
else
{
return ReturnKind.VoidReturning;
}
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps: _parameters = SynthesizedParameterSymbol.DeriveParameters(userMain, 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.

Oh wow, thanks!

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.

The if/else is no longer 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.

Done. Also this debug assert was redundant.

}

internal AsyncForwardEntryPoint(CSharpCompilation compilation, DiagnosticBag diagnosticBag, NamedTypeSymbol containingType, MethodSymbol userMain):
base(containingType, TranslateReturnType(compilation, userMain.ReturnType)) {
Debug.Assert(userMain != null);
Copy link
Member

Choose a reason for hiding this comment

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

Debug.Assert seems unnecessary since userMain was dereferenced in base call, and also below. (NullReferenceException seems sufficient.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense

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.


_userMain = userMain;
_diagnosticBag = diagnosticBag;
_compilation = compilation;
_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));
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 refKinds = Parameters.Length == 0 ? ImmutableArray<RefKind>.Empty : ImmutableArray.Create(RefKind.None);
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 strange to assume there is exactly zero or one parameter for refKinds but not for arguments below. Consider generalizing this. Perhaps: var refKinds = Parameters.SelectAsArray(p => RefKind.None);

Copy link
Member

Choose a reason for hiding this comment

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

Better yet, argumentRefKindsOpt is an optional parameter so we can pass in default(ImmutableArray<RefKind>) instead.

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 arguments = new List<BoundExpression>();
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 optimized by using new List<BoundExpression>(capacity: Parameters.Length)

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

foreach (var parameterSymbol in Parameters)
{
arguments.Add(new BoundParameter(syntax, parameterSymbol, parameterSymbol.Type));
}
Copy link
Member

Choose a reason for hiding this comment

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

var arguments = Parameters.SelectAsArray((p, s) => new BoundParameter(s, p, p.Type), 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.

I actually had this code originally but removed it because I thought that LINQ was more-or-less banned in the compiler


// 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.ToImmutableArray(),
argumentNamesOpt: ImmutableArray<string>.Empty,
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we aren't replicated the names on the user authored Main method? True it's legal to generate source without parameter names but some tools still don't handle it gracefully.

Copy link
Contributor Author

@TyOverby TyOverby Apr 6, 2017

Choose a reason for hiding this comment

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

Ugh fine :P

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 think that you meant for this comment to be on another line though. Our main method parameter names is defined in the call to SynthesizedParameterSymbol.Create inside the constructor.

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.

Ah you're correct. The use of ImmutableArray<string>.Empty is correct in this location.

argumentRefKindsOpt: refKinds,
isDelegateCall: false,
expanded: false,
invokedAsExtensionMethod: false,
argsToParamsOpt: ImmutableArray<int>.Empty,
Copy link
Member

Choose a reason for hiding this comment

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

I believe argumentNamesOpt and argsToParamsOpt should be default(ImmutableArray<T>) rather than Empty if not 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.

Done.

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 (
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 +508,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