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

Entrypoint detection stages #19093

Merged
merged 18 commits into from
May 10, 2017

Conversation

TyOverby
Copy link
Contributor

No description provided.

@TyOverby TyOverby added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 28, 2017
@TyOverby TyOverby force-pushed the async-main-stages branch from fc3946f to 42b3d19 Compare April 28, 2017 20:25
@TyOverby TyOverby force-pushed the async-main-stages branch from d0473dc to 83c2f0d Compare May 2, 2017 18:10
@TyOverby
Copy link
Contributor Author

TyOverby commented May 2, 2017

Ready for initial review @dotnet/roslyn-compiler @VSadov

@TyOverby TyOverby removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 2, 2017
var intOrVoidEntryPoints = ArrayBuilder<MethodSymbol>.GetInstance();
// Validity and diagnostics are also tracked because they must be conditionally handled
// if there are not any "traditional" entrypoints found.
var taskEntryPoints = ArrayBuilder<(bool IsValid, MethodSymbol Candidate, DiagnosticBag SpecificDiagnostics)>.GetInstance();
Copy link
Member

@cston cston May 2, 2017

Choose a reason for hiding this comment

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

intOrVoidEntryPoints and taskEntryPoints are not freed.

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 candidate in entryPointCandidates)
{
if (!HasEntryPointSignature(candidate, warnings))
var perCandidateBag = DiagnosticBag.GetInstance();
Copy link
Member

@cston cston May 2, 2017

Choose a reason for hiding this comment

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

perCandidateBag is not freed. #Resolved

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.

else
{
noMainFoundDiagnostics.Add(ErrorCode.WRN_InvalidMainSig, candidate.Locations.First(), candidate);
noMainFoundDiagnostics.AddRange(perCandidateBag);
Copy link
Member

@cston cston May 2, 2017

Choose a reason for hiding this comment

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

perCandidateBag is not freed, in if or 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.

Done.

@jaredpar
Copy link
Member

jaredpar commented May 2, 2017

CC @dotnet/roslyn-compiler

Don't think the edit of an existing comment to add that alias produces new emails. At least I didn't see one.

viableEntryPoints.Add(candidate);
}

if ((object)mainType == null || viableEntryPoints.Count == 0)
if (viableEntryPoints.IsEmpty())
Copy link
Member

Choose a reason for hiding this comment

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

Does IsEmpty require an IEnumerable? If so, consider using .Count == 0 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.

@TyOverby TyOverby force-pushed the async-main-stages branch from af46feb to dad384c Compare May 2, 2017 20:26
@TyOverby
Copy link
Contributor Author

TyOverby commented May 2, 2017

@cston: This PR implements a discussion that we had earlier about splitting the found entrypoints into different collections. Thanks for taking the time to review it!

viableEntryPoints.Add(candidate);
}

if ((object)mainType == null || viableEntryPoints.Count == 0)
if (viableEntryPoints.Count() == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Count is a property. Is this binding to an IEnumerable extension method 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.

Oh yep, you're right.

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.

@TyOverby TyOverby force-pushed the async-main-stages branch from dad384c to ee1a204 Compare May 2, 2017 22:37
@cston
Copy link
Member

cston commented May 2, 2017

    internal bool ReturnsAwaitableToVoidOrInt(MethodSymbol method, DiagnosticBag diagnostics)

private?


Refers to: src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs:1594 in ee1a204. [](commit_id = ee1a204, deletion_comment = False)

@TyOverby
Copy link
Contributor Author

TyOverby commented May 2, 2017

@dotnet-bot test ubuntu_16_debug_prtest please

@TyOverby
Copy link
Contributor Author

TyOverby commented May 2, 2017

@cston: I don't know what code review tool you are using, but I can't "reply" to your comment on internal bool ReturnsAwaitableToVoidOrInt. The reason that it is internal instead of private is that this method is also used by the emitter.

{
noMainFoundDiagnostics.Add(ErrorCode.WRN_InvalidMainSig, candidate.Locations.First(), candidate);
noMainFoundDiagnostics.AddRange(perCandidateBag);
perCandidateBag.Free();
}
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps check IsTaskLike first. That would simplify the IsCandidate check and perCandidateBag could be freed in one place rather than two.

if (IsTaskLike)
{
    taskEntryPoints.Add(...);
}
else
{
    if (IsCandidate)
    {
        intOrVoidEntryPoints.Add(candidate);
    }
    else
    {
        noMainFoundDiagnostics.Add(...);
    }
    perCandidatesBag.Free();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good!

Done

@@ -5074,4 +5074,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_VoidInTuple" xml:space="preserve">
<value>A tuple may not contain a value of type 'void'.</value>
</data>
</root>
<data name="ERR_NonTaskMainCantBeAsync" xml:space="preserve">
<value>Async Main methods must return Task or Task&lt;int&gt;</value>
Copy link
Contributor

@AlekseyTs AlekseyTs May 2, 2017

Choose a reason for hiding this comment

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

Task<int> [](start = 50, length = 15)

Does it have to be Task<int> though? #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.

Not technically, but I think this is fine for an error message.

I'd be willing to change it, but "Async Main methods must return either Task or Task whereby the return type of GetAwaiter().GetResult() is either void or int" seems a bit verbose for an error message.

Copy link
Contributor

@AlekseyTs AlekseyTs May 3, 2017

Choose a reason for hiding this comment

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

I think we can simply say that int or void returning Main cannot be async. #Closed

Copy link
Member

Choose a reason for hiding this comment

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

+1 for "void returning Main cannot be async"


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

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.


In reply to: 114619543 [](ancestors = 114619543,114608904)

[Fact]
[CompilerTrait(CompilerFeature.AsyncMain)]
Copy link
Contributor

@AlekseyTs AlekseyTs May 2, 2017

Choose a reason for hiding this comment

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

[CompilerTrait(CompilerFeature.AsyncMain)] [](start = 8, length = 42)

I think this can simply be applied to the test type instead. #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

var model = comp.GetSemanticModel(tree);
var token = tree.GetRoot().DescendantTokens().Where(t => t.Kind() == SyntaxKind.VoidKeyword).Skip(1).First();
var node = tree.GetRoot().FindNode(token.Span);
var node = tree.GetRoot().DescendantNodes().OfType<PredefinedTypeSyntax>().Where(n => n.Keyword.Kind() == SyntaxKind.VoidKeyword).ToList()[1];
Copy link
Contributor

@AlekseyTs AlekseyTs May 2, 2017

Choose a reason for hiding this comment

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

ToList()[1] [](start = 142, length = 11)

Last(), or ElementAt(1)? #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

warnings.Add(ErrorCode.WRN_InvalidMainSig, candidate.Locations.First(), candidate);
continue;
intOrVoidEntryPoints.Add(candidate);
perCandidateBag.Free();
Copy link
Contributor

@AlekseyTs AlekseyTs May 2, 2017

Choose a reason for hiding this comment

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

perCandidateBag.Free(); [](start = 24, length = 23)

Consider asserting that the bag is empty. #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.

@@ -1479,5 +1479,6 @@ internal enum ErrorCode
ERR_BadDynamicMethodArgDefaultLiteral = 9000,
ERR_DefaultLiteralNotValid = 9001,
WRN_DefaultInSwitch = 9002,
ERR_NonTaskMainCantBeAsync = 9003,
Copy link
Contributor

@AlekseyTs AlekseyTs May 3, 2017

Choose a reason for hiding this comment

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

ERR_NonTaskMainCantBeAsync [](start = 8, length = 26)

Can we simply reuse ERR_MainCantBeAsync slot? #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 wasn't sure about this. Are error numbers relevant to back-compat?

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'm reusing the old number, but with a new name and message.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented May 3, 2017

It looks like this entry should be removed. #Closed


Refers to: src/Compilers/CSharp/Portable/CSharpResources.resx:3599 in ee1a204. [](commit_id = ee1a204, deletion_comment = False)

continue;
}

if (!IsValid)
Copy link
Contributor

@AlekseyTs AlekseyTs May 3, 2017

Choose a reason for hiding this comment

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

if (!IsValid) [](start = 24, length = 13)

It feels like this if should be first in this loop (for consistency with int/void entry points). #Closed

Copy link
Member

Choose a reason for hiding this comment

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

Can these two if blocks be extracted to a helper method used in both the non-task-like and task-like candidates? (It would mean performing both checks for non-task-like candidates in the same loop, perhaps the first foreach, so the candidate is never added to intOrVoidEntryPoints.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlekseyTs: Yep, that makes sense. Done.

@cston: Unless I'm misunderstanding you, I don't think that there would be much to gain from pulling this out into a local function and performing the check earlier would break the diagnostic guarantees

// We can't add those Errors to the general diagnostics bag because it would break previously-working programs.
// The fact that these warnings are not added when csc is invoked with /main is possibly a bug, and is tracked at
// https://github.com/dotnet/roslyn/issues/18964
diagnostics.AddRange(noMainFoundDiagnostics.AsEnumerable().Where(d => d.Severity == DiagnosticSeverity.Warning));
Copy link
Contributor

@AlekseyTs AlekseyTs May 3, 2017

Choose a reason for hiding this comment

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

noMainFoundDiagnostics.AsEnumerable().Where(d => d.Severity == DiagnosticSeverity.Warning) [](start = 41, length = 90)

Linq is banned in compilers. Also, if the point of this code is to preserve old behavior, then we should probably be more precise and add only WRN_MainCantBeGeneric and WRN_InvalidMainSig warnings. #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.

Linq is banned in compilers.

Fixed.

Also, if the point of this code is to preserve old behavior

Nah, the point is the emit errors where there were errors before, and emit warnings where there were warnings before. I don't care about emitting the same ones because it's possible to add more helpful diagnostics now.

}").WithArguments("async main", "7.1").WithLocation(6, 5)
);
}").WithArguments("async main", "7.1").WithLocation(6, 5),
Diagnostic(ErrorCode.ERR_NoEntryPoint).WithLocation(1, 1));
Copy link
Contributor

@AlekseyTs AlekseyTs May 3, 2017

Choose a reason for hiding this comment

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

Diagnostic(ErrorCode.ERR_NoEntryPoint).WithLocation(1, 1) [](start = 16, length = 57)

Please add corresponding message as a comment. #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

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 0.0f;
}
}";
var compilation = CreateCompilationWithMscorlib45(source, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp7)).VerifyDiagnostics(
Copy link
Contributor

@AlekseyTs AlekseyTs May 4, 2017

Choose a reason for hiding this comment

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

, parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp7) [](start = 101, length = 80)

If we don't expect the behavior to be version specific, we probably shouldn't use specific version for this test. Perhaps we should test it against default version and the latest version, latest will give us coverage for 7.1. #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

return 0.0f;
}
}";
var compilation = CreateCompilationWithMscorlib45(source, options: TestOptions.ReleaseExe.WithMainTypeName("A"), parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp7)).VerifyDiagnostics();
Copy link
Contributor

@AlekseyTs AlekseyTs May 4, 2017

Choose a reason for hiding this comment

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

, parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp7) [](start = 123, length = 80)

If we don't expect the behavior to be version specific, we probably shouldn't use specific version for this test. Perhaps we should test it against default version and the latest version, latest will give us coverage for 7.1. #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

}
}";
var compilation = CreateCompilationWithMscorlib45(source, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp7_1)).VerifyDiagnostics(
// (6,23): warning CS0028: 'A.Main(string[])' has the wrong signature to be an entry point
Copy link
Contributor

@AlekseyTs AlekseyTs May 4, 2017

Choose a reason for hiding this comment

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

// (6,23): warning CS0028: 'A.Main(string[])' has the wrong signature to be an entry point [](start = 16, length = 90)

Why don't we report the same error as in the previous test? #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.

Discussed in person. Fixed.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented May 4, 2017

Done with review pass. #Closed

@TyOverby TyOverby force-pushed the async-main-stages branch from 0b2b378 to ba23bae Compare May 5, 2017 00:23
Diagnostic(ErrorCode.WRN_SameFullNameThisAggAgg, "Task<int>").WithArguments("", "System.Threading.Tasks.Task<T>", "mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089", "System.Threading.Tasks.Task<TResult>"),
// (11,22): warning CS0028: 'Program.Main()' has the wrong signature to be an entry point
// static Task<int> Main() {
Diagnostic(ErrorCode.WRN_InvalidMainSig, "Main").WithArguments("Program.Main()"));
Copy link
Contributor

@AlekseyTs AlekseyTs May 5, 2017

Choose a reason for hiding this comment

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

Diagnostic(ErrorCode.WRN_InvalidMainSig, "Main").WithArguments("Program.Main()")); [](start = 16, length = 82)

It looks like we lost locations in the base-line again. Please restore and check other tests too. #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.

@@ -657,14 +746,14 @@ class A
}";
var compilation = CreateCompilationWithMscorlib45(source, options: TestOptions.DebugExe, parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp7_1));
compilation.VerifyDiagnostics(
// (6,22): error CS1983: The return type of an async method must be void, Task or Task<T>
// (6,22): error CS1983: The return type of an async method must be void, Task or Task<T>
// async static int Main()
Copy link
Contributor

@AlekseyTs AlekseyTs May 5, 2017

Choose a reason for hiding this comment

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

It looks like formatting is off. #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.

Huh, fixed.

return 1;
}
}";
var compilation = CreateCompilationWithMscorlib45(source, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp7)).VerifyDiagnostics(
Copy link
Contributor

@AlekseyTs AlekseyTs May 5, 2017

Choose a reason for hiding this comment

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

parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp7) [](start = 103, length = 78)

Do you expect the behavior to change in the next version? #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.

Fixed.

}
}";
var compilation = CreateCompilationWithMscorlib45(source, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp7_1)).VerifyDiagnostics(
// (6,23): error CS4009: A void or int returning entry point cannot be async
Copy link
Contributor

@AlekseyTs AlekseyTs May 5, 2017

Choose a reason for hiding this comment

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

It looks like formatting is off. #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.

Fixed.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented May 5, 2017

Done with review pass. #Closed

@TyOverby
Copy link
Contributor Author

TyOverby commented May 8, 2017

@AlekseyTs Ping for another review

// (11,12): error CS1986: 'await' requires that the type Task<int> have a suitable GetAwaiter method
// static Task<int> Main() {
Diagnostic(ErrorCode.ERR_BadAwaitArg, "Task<int>").WithArguments("System.Threading.Tasks.Task<int>").WithLocation(11, 12),
Diagnostic(ErrorCode.WRN_SameFullNameThisAggAgg, "Task<int>").WithArguments("", "System.Threading.Tasks.Task<T>", "mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089", "System.Threading.Tasks.Task<TResult>"),
Copy link
Contributor

@AlekseyTs AlekseyTs May 8, 2017

Choose a reason for hiding this comment

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

Diagnostic(ErrorCode.WRN_SameFullNameThisAggAgg, "Task").WithArguments("", "System.Threading.Tasks.Task", "mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089", "System.Threading.Tasks.Task"), [](start = 16, length = 233)

It looks like we lost locations in the base-line again. Please restore and check other tests too. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented May 8, 2017

Done with review pass. #Closed

@TyOverby
Copy link
Contributor Author

TyOverby commented May 8, 2017

@AlekseyTs done

@jaredpar
Copy link
Member

jaredpar commented May 9, 2017

retest windows_debug_unit64_prtest please

@TyOverby
Copy link
Contributor Author

TyOverby commented May 9, 2017

ping @AlekseyTs

Diagnostic(ErrorCode.WRN_SameFullNameThisAggAgg, "Task<int>").WithArguments("", "System.Threading.Tasks.Task<T>", "mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089", "System.Threading.Tasks.Task<TResult>"),
// (11,22): warning CS0028: 'Program.Main()' has the wrong signature to be an entry point
// static Task<int> Main() {
Diagnostic(ErrorCode.WRN_InvalidMainSig, "Main").WithArguments("Program.Main()"));
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2017

Choose a reason for hiding this comment

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

Diagnostic(ErrorCode.WRN_InvalidMainSig, "Main").WithArguments("Program.Main()")) [](start = 16, length = 81)

Location is not verified. #Closed

@@ -3434,6 +3434,8 @@ class Test
// (5,30): warning CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
// async public static Task Main()
Diagnostic(ErrorCode.WRN_AsyncLacksAwaits, "Main"),
// error CS5001: Program does not contain a static 'Main' method suitable for an entry point
Diagnostic(ErrorCode.ERR_NoEntryPoint),
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2017

Choose a reason for hiding this comment

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

Diagnostic(ErrorCode.ERR_NoEntryPoint), [](start = 16, length = 39)

Location is not verified. And above as well. #Closed

@AlekseyTs
Copy link
Contributor

Done with review pass.

@TyOverby
Copy link
Contributor Author

TyOverby commented May 9, 2017

@AlekseyTs I temporarily made it so that missing WithLocation was a test failure, then I fixed all the failing tests. Assuming that my failure-inducing code was correct, I've cleaned up the remainder of the missing WithLocation errors.

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM

@TyOverby TyOverby merged commit cc4ea46 into dotnet:features/async-main May 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants