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 25 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
9 changes: 6 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2059,28 +2059,31 @@ private AssemblySymbol GetForwardedToAssembly(string fullName, int arity, Diagno
return null;
}

internal static void CheckFeatureAvailability(SyntaxNode syntax, MessageID feature, DiagnosticBag diagnostics, Location locationOpt = null)
internal static bool CheckFeatureAvailability(SyntaxNode syntax, MessageID feature, DiagnosticBag diagnostics, Location locationOpt = null)
Copy link
Member

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.

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 yep, not used anymore.

Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

{
var options = (CSharpParseOptions)syntax.SyntaxTree.Options;
if (options.IsFeatureEnabled(feature))
{
return;
return true;
}

var location = locationOpt ?? syntax.GetLocation();
string requiredFeature = feature.RequiredFeature();
if (requiredFeature != null)
{
diagnostics.Add(ErrorCode.ERR_FeatureIsExperimental, location, feature.Localize(), requiredFeature);
return;
return false;
}

LanguageVersion availableVersion = options.LanguageVersion;
LanguageVersion requiredVersion = feature.RequiredVersion();
if (requiredVersion > availableVersion)
{
diagnostics.Add(availableVersion.GetErrorCode(), location, feature.Localize(), new CSharpRequiredLanguageVersion(requiredVersion));
return false;
}

return true;
}
}
}
31 changes: 23 additions & 8 deletions src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1454,7 +1454,9 @@ private MethodSymbol FindEntryPoint(CancellationToken cancellationToken, out Imm
}

DiagnosticBag warnings = DiagnosticBag.GetInstance();
DiagnosticBag possibleAsyncMainDiagnostics = DiagnosticBag.GetInstance();
var viableEntryPoints = ArrayBuilder<MethodSymbol>.GetInstance();
bool asyncOk = true;
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.

Consider replacing Ok with something more meaningful #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've actually removed all of this

foreach (var candidate in entryPointCandidates)
{
if (!candidate.HasEntryPointSignature(this))
Expand All @@ -1474,7 +1476,7 @@ private MethodSymbol FindEntryPoint(CancellationToken cancellationToken, out Imm
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);
asyncOk &= CheckFeatureAvailability(candidate.DeclaringSyntaxReferences.Single().GetSyntax(), MessageID.IDS_FeatureAsyncMain, possibleAsyncMainDiagnostics);
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.

asyncOk &= CheckFeatureAvailability(candidate.DeclaringSyntaxReferences.Single().GetSyntax(), MessageID.IDS_FeatureAsyncMain, possibleAsyncMainDiagnostics) [](start = 24, length = 155)

It feels like if the feature is not available we should add a warning into warnings and skip the candidate. #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.

As above, removed.

}

viableEntryPoints.Add(candidate);
Expand All @@ -1488,7 +1490,11 @@ private MethodSymbol FindEntryPoint(CancellationToken cancellationToken, out Imm
warnings.Free();

MethodSymbol entryPoint = null;
if (viableEntryPoints.Count == 0)

int nonAsyncCount = viableEntryPoints.Count(c => !c.IsAsync);
int asyncCount = viableEntryPoints.Count - nonAsyncCount;

if (nonAsyncCount == 0 && asyncCount == 0)
{
if ((object)mainType == null)
{
Expand All @@ -1499,22 +1505,31 @@ private MethodSymbol FindEntryPoint(CancellationToken cancellationToken, out Imm
diagnostics.Add(ErrorCode.ERR_NoMainInClass, mainType.Locations.First(), mainType);
}
}
else if (viableEntryPoints.Count > 1)
else if (nonAsyncCount == 1)
{
entryPoint = viableEntryPoints.Single(c => !c.IsAsync);
}
else if (nonAsyncCount == 0 && asyncCount == 1)
{
diagnostics.AddRange(possibleAsyncMainDiagnostics);
entryPoint = viableEntryPoints.Single();
}
else if (nonAsyncCount == 0 && !asyncOk)
{
diagnostics.AddRange(possibleAsyncMainDiagnostics);
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 testing this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I went and did some more testing, and apparently all of this complex logic isn't actually very useful. In shipping csc, there is no way to have an async method that meets the entrypoint requirements without compiler errors.

So all of this stuff that was supposed to be for backcompat is pretty useless...

}
else
{
viableEntryPoints.Sort(LexicalOrderSymbolComparer.Instance);
var info = new CSDiagnosticInfo(
ErrorCode.ERR_MultipleEntryPoints,
args: Array.Empty<object>(),
symbols: viableEntryPoints.OfType<Symbol>().AsImmutable(),
additionalLocations: viableEntryPoints.Select(m => m.Locations.First()).OfType<Location>().AsImmutable());

diagnostics.Add(new CSDiagnostic(info, viableEntryPoints.First().Locations.First()));
}
Copy link
Member

Choose a reason for hiding this comment

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

possibleAsyncMainDiagnostics is not freed in the last else. Consider replacing all the diagnostics.AddRangeAndFree(possibleAsyncMainDiagnostics) with diagnostics.AddRange(possibleAsyncMainDiagnostics) and free possibleAsyncMainDiagnostics explicitly at the end of the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Done.

else
{
entryPoint = viableEntryPoints[0];
}

possibleAsyncMainDiagnostics.Free();
viableEntryPoints.Free();
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.

viableEntryPoints.Free(); [](start = 16, length = 25)

Please keep the empty line. Since this is the only change in the file, consider reverting it to the original state.#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.

Ok

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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

// entryPoint can be a SynthesizedEntryPointSymbol if a script is being compiled.
SynthesizedEntryPointSymbol synthesizedEntryPoint = entryPoint as SynthesizedEntryPointSymbol;
if ((object)synthesizedEntryPoint == null && 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 SynthesizedEntryPointSymbol.AsyncForwardEntryPoint(compilation, diagnostics, entryPoint.ContainingType, entryPoint);
entryPoint = synthesizedEntryPoint;
if ((object)moduleBeingBuilt != null)
{
moduleBeingBuilt.AddSynthesizedDefinition(entryPoint.ContainingType, synthesizedEntryPoint);
}
}

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

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

@AlekseyTs AlekseyTs Apr 13, 2017

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return entryPoint;
}

Expand Down
49 changes: 24 additions & 25 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 ((object)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.
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 @@ -603,37 +626,13 @@ internal bool IsEntryPointCandidate
/// </summary>
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 @@ -301,12 +302,17 @@ private static void ReportUseSiteDiagnostics(Symbol symbol, DiagnosticBag diagno
}
}

private static MethodSymbol GetRequiredMethod(TypeSymbol type, string methodName, DiagnosticBag diagnostics)
private static MethodSymbol GetRequiredMethod(TypeSymbol type, string methodName, DiagnosticBag diagnostics, Location location = null)
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.

private static MethodSymbol GetRequiredMethod(TypeSymbol type, string methodName, DiagnosticBag diagnostics, Location location = null) [](start = 8, length = 134)

This helper doesn't report use-site diagnostics for the member it returns. The errors should be reported. #Closed

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.

Please add tests for that as well. #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.

How do you do that?

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.

How do you do that?

You can take a look at the Binder.GetWellKnownTypeMember helper as an example. #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.

What is a use-site diagnostic? How do you inject a "use-site-diagnostic" into a document that has no real "uses" of the synthesized main?


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

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 25, 2017

Choose a reason for hiding this comment

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

private static MethodSymbol GetRequiredMethod(TypeSymbol type, string methodName, DiagnosticBag diagnostics, Location location = null) [](start = 8, length = 134)

Are we still using this function for the purpose of this PR? If not, consider reverting changes in it. #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 ((object)location == null)
{
location = NoLocation.Singleton;
}

var method = type.GetMembers(methodName).SingleOrDefault() as MethodSymbol;
if ((object)method == null)
{
diagnostics.Add(ErrorCode.ERR_MissingPredefinedMember, NoLocation.Singleton, type, methodName);
diagnostics.Add(ErrorCode.ERR_MissingPredefinedMember, location, type, methodName);
}
return method;
}
Expand All @@ -329,6 +335,123 @@ 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 asynchronous 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.

// The user-defined asynchronous main method. [](start = 12, length = 45)

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.

private readonly MethodSymbol _userMain;

private readonly MethodSymbol _getAwaiterMethod;
private readonly MethodSymbol _getResultMethod;

private readonly ImmutableArray<ParameterSymbol> _parameters;

// 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)

}

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;
_parameters = SynthesizedParameterSymbol.DeriveParameters(userMain, this);

var userMainLocation = userMain.DeclaringSyntaxReferences.SingleOrDefault()?.GetLocation();

Copy link
Member

Choose a reason for hiding this comment

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

Double blank line.

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

_getAwaiterMethod = GetRequiredMethod(_userMain.ReturnType, WellKnownMemberNames.GetAwaiter, diagnosticBag, userMainLocation);
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.

_getAwaiterMethod = GetRequiredMethod(_userMain.ReturnType, WellKnownMemberNames.GetAwaiter, diagnosticBag, userMainLocation); [](start = 16, length = 126)

It looks like GetRequiredMethod doesn't follow the rules from C# language specification. In particular it wouldn't look for an inherited or an extension method. It doesn't verify restrictions outlined in section 7.7.8.1 "Awaitable expressions" of the specification. I would recommend to reuse relevant code from the binder, see GetAwaitableExpressionInfo function, for example. #Closed

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.

Also, it looks like GetRequiredMethod doesn't check accessibility of the methods. #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.

see GetAwaitableExpressionInfo function, for example

How do I get a Binder so that I can reuse this method?

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 18, 2017

Choose a reason for hiding this comment

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

How do I get a Binder so that I can reuse this method?

Let's discuss this offline. #Closed

if ((object)_getAwaiterMethod != null)
{
_getResultMethod = GetRequiredMethod(_getAwaiterMethod.ReturnType, WellKnownMemberNames.GetResult, diagnosticBag, userMainLocation);
}
}

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 => (BoundExpression)new BoundParameter(syntax, p, p.Type));
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 arguments = Parameters.SelectAsArray(p => (BoundExpression)new BoundParameter(syntax, p, p.Type)); [](start = 16, length = 102)

Consider using SyntheticBoundNodeFactory to synthesize the tree. #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 rest of this file uses manual tree creation. If this is an anti-pattern, the whole file could be fixed in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the benefit of doing it this way?


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


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

// GetAwaiter().GetResult()
BoundCall getAwaiterGetResult =
CreateParameterlessCall(
syntax: syntax,
method: _getResultMethod,
Copy link
Member

Choose a reason for hiding this comment

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

Can CreateBody be called if the constructor for AsyncForwardEntryPoint added an error to the diagnostic bag? If so this would end up passing a null value for _getResultMethod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CreateBody is called from MethodCompiler.GetEntryPoint only if there are no error diagnostics.

receiver: CreateParameterlessCall(
syntax: syntax,
method: _getAwaiterMethod,
receiver: userMainInvocation
)
);

if (ReturnsVoid)
{
return new BoundBlock(
syntax: syntax,
locals: ImmutableArray<LocalSymbol>.Empty,
statements: ImmutableArray.Create<BoundStatement>(
new BoundExpressionStatement(
syntax: syntax,
expression: getAwaiterGetResult
)
{ WasCompilerGenerated = true },
new BoundReturnStatement(
syntax: syntax,
refKind: RefKind.None,
expressionOpt: null
)
{ WasCompilerGenerated = true }
)
)
{ WasCompilerGenerated = true };

}
else
{
return new BoundBlock(
syntax: syntax,
locals: ImmutableArray<LocalSymbol>.Empty,
statements: ImmutableArray.Create<BoundStatement>(
new BoundReturnStatement(
syntax: syntax,
refKind: RefKind.None,
expressionOpt: getAwaiterGetResult
)
)
)
{ WasCompilerGenerated = true };
}
Copy link
Member

Choose a reason for hiding this comment

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

Should these BoundBlock instances have WasCompilerGenerated = true? If so, consider creating statements inside the if and using a single new BoundBlock. If not (because the method is synthesized?), then WasCompilerGenerated = true can probably be removed from BoundCall earlier.

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 would expect all of them to have WasCompilerGenerated = true, so I'll change that.

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 sealed class ScriptEntryPoint : SynthesizedEntryPointSymbol
{
private readonly MethodSymbol _getAwaiterMethod;
Expand All @@ -344,15 +467,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