enable main methods to be asynchronous #18157

TyOverby · 2017-03-24T21:00:51Z

This is the first chunk in enabling Async-Main. It merely concerns itself with deciding which methods have valid entrypoint signatures.

CyrusNajmabadi · 2017-03-24T21:06:50Z

src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs

@@ -597,18 +597,47 @@ internal bool IsEntryPointCandidate

        /// <summary>
        /// Checks if the method has an entry point compatible signature, i.e.
-        /// - the return type is either void or int
+        /// - the return type is either void, int, Task, or Task&lt;int&gt;


consider <see cref #Resolved

Done #Resolved

CyrusNajmabadi · 2017-03-24T21:07:49Z

src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs

+                }
+                else
+                {
+                    return false;


is there a concern that ValueTask is not matched here?

For now we are just supporting Task, however, the comments in this method describe how to (very easily) support any "task-like" object including ValueTask.

TyOverby · 2017-03-24T21:08:23Z

@dotnet/roslyn-compiler This is now out for review.

jcouv · 2017-03-24T23:16:50Z

Looks like there's a warning preventing build from succeeding:

15:54:32 Symbols\MethodSymbol.cs(601,27): warning CS1584: XML comment has syntactically incorrect cref attribute 'System.Threading.Tasks.Task{int}' [D:\j\workspace\windows_debug---f56cc009\src\Compilers\CSharp\Portable\CSharpCodeAnalysis.csproj]
15:54:33 
Symbols\MethodSymbol.cs(601,55): warning CS1658: Type parameter declaration must be an identifier not a type. See also error CS0081. [D:\j\workspace\windows_debug---f56cc009\src\Compilers\CSharp\Portable\CSharpCodeAnalysis.csproj]

jaredpar · 2017-03-25T15:52:51Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -1104,7 +1104,7 @@ internal enum ErrorCode
        ERR_VarargsAsync = 4006,
        ERR_ByRefTypeAndAwait = 4007,
        ERR_BadAwaitArgVoidCall = 4008,
-        ERR_MainCantBeAsync = 4009,
+        // ERR_MainCantBeAsync = 4009,


Should we delete the corresponding resource string?

jaredpar · 2017-03-25T15:55:37Z

src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs

+                return false;
+            }
+
+            if (!IsAsyncMainReturnType(returnType) && IsAsync)


We should add a test case for this too.

Already had an async void test, I'll add an async int test too

jaredpar · 2017-03-25T15:56:20Z

src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs

+                {
+                    return false;
+                }
+                // Change this to `namedType.IsNonGenericTaskType` if you want to support "task-like" objects.


Nit: in general we don't break up if / else if structures with comments like this. Instead put them into the body, or add a meta comment to the local function.

jaredpar · 2017-03-25T15:57:31Z

src/Compilers/CSharp/Test/Semantic/Semantics/BindingAsyncTests.cs

+
+class A
+{
+    async static Task<float> Main()


Should add variants that check for async Task / Task<T> and have a Main(string[])

TyOverby · 2017-03-27T16:38:44Z

@jcouv Thanks! Should have it fixed by now.

jcouv · 2017-03-27T17:40:46Z

src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs

@@ -597,18 +597,48 @@ internal bool IsEntryPointCandidate

        /// <summary>
        /// Checks if the method has an entry point compatible signature, i.e.
-        /// - the return type is either void or int
+        /// - the return type is either void, int, <see cref="System.Threading.Tasks.Task" />, 
+        /// or <see cref="System.Threading.Tasks.Task{T}" /> Where T is an int.


Where [](start = 61, length = 5)

Nit: capitalization on "where"

Done #Resolved

jcouv · 2017-03-27T17:47:28Z

src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs

        {
-            if (this.IsVararg)
+
+            bool IsAsyncMainReturnType(TypeSymbol type)


Should the new logic be conditional on language version?
If so, please add a test with language version 7.

Done. Added a version of each of these tests that uses langversion 7

jcouv · 2017-03-27T17:49:20Z

src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs

+                return false;
+            }
+
+            if (!IsAsyncMainReturnType(returnType) && IsAsync)


IsAsyncMainReturnType(returnType) [](start = 17, length = 33)

Consider pulling IsAsyncMainReturnType(returnType) into a local. #Closed

Done #Closed

jcouv · 2017-03-27T17:51:41Z

src/Compilers/CSharp/Test/Semantic/Semantics/BindingAsyncTests.cs

+                //     async static Task<float> Main()
+                Diagnostic(ErrorCode.WRN_InvalidMainSig, "Main").WithArguments("A.Main()").WithLocation(6, 30),
+                // error CS5001: Program does not contain a static 'Main' method suitable for an entry point
+                Diagnostic(ErrorCode.ERR_NoEntryPoint).WithLocation(1, 1) );
        }


Consider adding a test without async (Main method returns a Task by hand).

Never mind, I see that you use string replacement to achieve that.

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

jcouv · 2017-03-27T17:56:19Z

            // (4,23): warning CS0028: 'A.Main(bool)' has the wrong signature to be an entry point

Makes me wonder if this diagnostic should provide more explicit directions about what is allowed (might be lengthy though).

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/BindingAsyncTests.cs:1429 in 3228961. [](commit_id = 3228961, deletion_comment = False)

jcouv · 2017-03-29T17:39:45Z

src/Compilers/CSharp/Test/Semantic/Semantics/BindingAsyncTests.cs

+    }
+}";
+            var compilation = CreateCompilationWithMscorlib45(source, options: TestOptions.DebugExe, parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp7));
+            compilation.VerifyDiagnostics(Diagnostic(ErrorCode.ERR_MainCantBeAsync, "Main").WithArguments("A.Main(string[])").WithLocation(6, 23));


If you use a C# 7 compiler but set language version = 6, using a C# 7 feature like tuples will give you a better error ("Please use C# 7").
Can we do the same here? Look at CheckFeatureAvailability method.

jaredpar · 2017-03-30T00:30:55Z

src/Compilers/CSharp/Test/Semantic/Semantics/BindingAsyncTests.cs

+        }
+
+        [Fact]
+        public void MainCanBeAsync()


Note: please add a test verifying we don't allow variants where main has a ref return.

static ref Task Main() { ... }

Context is that we incorrectly allow this for int today: #17923

I'll cherry-pick your PR into my branch and add a test.

jcouv · 2017-03-30T20:47:18Z

src/Compilers/CSharp/Test/Semantic/Semantics/BindingAsyncTests.cs

+            compilation.VerifyDiagnostics(
+                // (6,5): error CS8107: Feature 'async main' is not available in C# 7. Please use language version 7.1 or greater.
+                //     async static Task Main(string[] args)
+                Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion7, @"async static Task Main(string[] args)


Would it be possible for the diagnostic to point to a narrow piece of syntax? For example "Main" or maybe "async"?

I've added a comment and will get back to this shortly

The Main method cannot have a ref return. closes dotnet#17923

jcouv

LGTM

Nit: I think the marker convention would be PROTOTYPE(async-main) for this branch, not just PROTOTYPE.

TyOverby · 2017-03-31T16:57:38Z

I'm merging now even though the tests aren't done.

The tests were all passing and I only made a change to a comment.

enable main methods to be asynchronous

88542e5

dnfclas added the cla-already-signed label Mar 24, 2017

TyOverby mentioned this pull request Mar 24, 2017

recognize async main methods #18038

Closed

CyrusNajmabadi reviewed Mar 24, 2017

View reviewed changes

add cref to documentation

909560f

jaredpar reviewed Mar 25, 2017

View reviewed changes

TyOverby added 2 commits March 27, 2017 09:19

fix doc comment syntax

b90d121

remove message from resx

2868b64

TyOverby added 2 commits March 27, 2017 09:39

move comments

d7e9e7f

add async int test

2678fbe

jaredpar approved these changes Mar 27, 2017

View reviewed changes

add more test

3228961

jcouv reviewed Mar 27, 2017

View reviewed changes

TyOverby added 2 commits March 28, 2017 09:32

re-add errors for pre-71 compilers

3d4363a

add tests for previous versions of csharp

c3f2482

jcouv reviewed Mar 29, 2017

View reviewed changes

Pilchie added the Area-Compilers label Mar 29, 2017

jaredpar reviewed Mar 30, 2017

View reviewed changes

add feature detection

d88c6e4

jcouv reviewed Mar 30, 2017

View reviewed changes

jaredpar and others added 2 commits March 30, 2017 15:37

Disallow ref returning Main method

c113421

The Main method cannot have a ref return. closes dotnet#17923

add ref tests, add prototype comment

c6c641c

jcouv approved these changes Mar 30, 2017

View reviewed changes

change prototype comment

c96708b

TyOverby merged commit b3243e8 into dotnet:features/async-main Mar 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable main methods to be asynchronous #18157

enable main methods to be asynchronous #18157

TyOverby commented Mar 24, 2017 •

edited

Loading

CyrusNajmabadi Mar 24, 2017 •

edited by jcouv

Loading

TyOverby Mar 24, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Mar 24, 2017

TyOverby Mar 24, 2017

TyOverby commented Mar 24, 2017

jcouv commented Mar 24, 2017

jaredpar Mar 25, 2017

TyOverby Mar 27, 2017

jaredpar Mar 25, 2017

TyOverby Mar 27, 2017

jaredpar Mar 25, 2017

TyOverby Mar 27, 2017

jaredpar Mar 25, 2017

TyOverby Mar 27, 2017

TyOverby commented Mar 27, 2017

jcouv Mar 27, 2017

TyOverby Mar 27, 2017 •

edited by jcouv

Loading

jcouv Mar 27, 2017

TyOverby Mar 29, 2017

jcouv Mar 27, 2017 •

edited

Loading

TyOverby Mar 27, 2017 •

edited by jcouv

Loading

jcouv Mar 27, 2017

jcouv Mar 27, 2017

jcouv commented Mar 27, 2017

jcouv Mar 29, 2017

TyOverby Mar 30, 2017

jaredpar Mar 30, 2017

TyOverby Mar 30, 2017

jcouv Mar 30, 2017

TyOverby Mar 30, 2017

jcouv left a comment

TyOverby commented Mar 31, 2017

enable main methods to be asynchronous #18157

enable main methods to be asynchronous #18157

Conversation

TyOverby commented Mar 24, 2017 • edited Loading

CyrusNajmabadi Mar 24, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

TyOverby Mar 24, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TyOverby commented Mar 24, 2017

jcouv commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TyOverby commented Mar 27, 2017

Choose a reason for hiding this comment

TyOverby Mar 27, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

TyOverby Mar 27, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

TyOverby commented Mar 31, 2017

TyOverby commented Mar 24, 2017 •

edited

Loading

CyrusNajmabadi Mar 24, 2017 •

edited by jcouv

Loading

TyOverby Mar 24, 2017 •

edited by jcouv

Loading

TyOverby Mar 27, 2017 •

edited by jcouv

Loading

jcouv Mar 27, 2017 •

edited

Loading

TyOverby Mar 27, 2017 •

edited by jcouv

Loading