Remove remaining prototype comments #19392

TyOverby · 2017-05-10T17:26:41Z

No description provided.

TyOverby · 2017-05-10T17:27:36Z

cston · 2017-05-10T17:39:44Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

+                    ref lazyVariableSlotAllocator,
+                    lambdaDebugInfoBuilder,
+                    closureDebugInfoBuilder,
+                    out stateMachineTypeOpt);


Could we call LocalRewriter.Rewrite() instead?

@cston are you talking about this method?

http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp/Lowering/LocalRewriter/LocalRewriter.cs,64

Yes, the same method that LowerBodyOrInitializer calls. It looks like LowerBodyOrInitializer handles extra cases (await, local functions, iterators) that are not needed here.

My understanding is that LowerBodyOrInitializer is the main entrypoint to lowering a method body, so even though all of its functionality isn't used.

cston · 2017-05-10T17:40:24Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

+                Debug.Assert((object)stateMachineTypeOpt == null);
+                Debug.Assert(dynamicAnalysisSpans.IsEmpty);
+                Debug.Assert(lambdaDebugInfoBuilder.IsEmpty());
+                Debug.Assert(closureDebugInfoBuilder.IsEmpty());


ArrayBuilder instances and diagsForCurrentMethod are not freed

cston · 2017-05-10T17:42:49Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

+                BoundStatement body = synthesizedEntryPoint.CreateBody();
+
+                var dynamicAnalysisSpans = ImmutableArray<SourceSpan>.Empty;
+                var diagsForCurrentMethod = DiagnosticBag.GetInstance();


Any chance of errors in lowering? If so, please test that case. Otherwise assert diagsForCurrentMethod is empty.

Perhaps just pass diagnostics to LowerBodyOrInitializer instead, to ensure we don't drop diagnostics.

Sounds good!

cston · 2017-05-10T17:48:07Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedEntryPointSymbol.cs

@@ -19,7 +19,6 @@ internal abstract class SynthesizedEntryPointSymbol : MethodSymbol
        internal const string FactoryName = "<Factory>";

        private readonly NamedTypeSymbol _containingType;
-        // PROTOTYPE(async-main): remove this and move it out into inheriting classes?
        private TypeSymbol _returnType;


Why not move _returnType to the derived classes, and make ReturnType abstract?

Yeah, this was one of the solutions that I was thinking about. I just implemented it and like how it looks.

cston · 2017-05-10T17:54:27Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMainTests.cs

+            sourceCompilation.VerifyEmitDiagnostics(
+                // (30,12): warning CS0436: The type 'Task' in '' conflicts with the imported type 'Task' in 'mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089'. Using the type defined in ''.
+                //     static Task Main() {
+                Diagnostic(ErrorCode.WRN_SameFullNameThisAggAgg, "Task").WithArguments("", "System.Threading.Tasks.Task", "mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089", "System.Threading.Tasks.Task").WithLocation(30, 12),


Does this error prevent ReturnsAwaitableToVoidOrInt from treating the return type as the well-known type Task? In short, does the extension method test the expected code path?

Yes, the compiler considers this Task to be the "real" task.

cston · 2017-05-10T19:38:02Z

LGTM

TyOverby · 2017-05-10T20:11:07Z

Ping: @AlekseyTs, @VSadov

AlekseyTs · 2017-05-10T20:47:50Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMainTests.cs

+    }
+}";
+            var sourceCompilation = CreateStandardCompilation(source, options: TestOptions.DebugExe, parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp7_1));
+            sourceCompilation.VerifyEmitDiagnostics(


sourceCompilation.VerifyEmitDiagnostics( [](start = 12, length = 40)

sourceCompilation.VerifyEmitDiagnostics( [](start = 12, length = 40)

Success cases should be executed and runtime behavior should be verified. #Closed

AlekseyTs · 2017-05-10T20:49:28Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMainTests.cs

-        System.Console.WriteLine(""Task<int>"");
-        return 0;
-    }").WithArguments("async main", "7.1").WithLocation(6, 5),
+                Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion7, "Task<int>").WithArguments("async main", "7.1").WithLocation(6, 18),


Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion7, "Task").WithArguments("async main", "7.1").WithLocation(6, 18), [](start = 16, length = 124)

It looks like all tests for this error use async method, we should have a test for anon-async scenario as well. #Closed

I've doubled the number of tests testing ERR_FeatureNotAvailable and each new one doesn't use async.

AlekseyTs · 2017-05-10T20:50:06Z

Done with review pass. #Closed

VSadov · 2017-05-10T22:56:09Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMainTests.cs

+            var compilation = CreateCompilationWithMscorlib45(source, options: TestOptions.DebugExe, parseOptions: TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp7));
+            compilation.VerifyDiagnostics(
+                // (6,18): 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)


No async in the actual test code?

Oh yeah, the diagnostics (including comments) were copied from another test.

VSadov · 2017-05-10T22:57:38Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMainTests.cs

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


No async in the test code. I think this test will fail.

This test is correct; it's complaining about Task being returned.

VSadov · 2017-05-10T22:59:23Z

VSadov

LGTM

TyOverby · 2017-05-10T23:29:52Z

@AlekseyTs I've added obsolete tests, and reworked invocations to GetAwaitableExpressionInfo.

AlekseyTs · 2017-05-10T23:35:24Z

            var success = binder.GetAwaitableExpressionInfo(userMainInvocation, out _, out _, out _, out _getAwaiterGetResultCall, _userMainReturnTypeSyntax, diagnosticBag);

It looks like the parameter is no longer used. Consider removing. This is about diagnosticBag parameter of the containing method. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedEntryPointSymbol.cs:365 in a54e0ce. [](commit_id = a54e0ce, deletion_comment = True)

AlekseyTs · 2017-05-10T23:41:53Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMainTests.cs

+                // (31,20): warning CS0436: The type 'Task' in '' conflicts with the imported type 'Task' in 'mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089'. Using the type defined in ''.
+                //         return new Task();
+                Diagnostic(ErrorCode.WRN_SameFullNameThisAggAgg, "Task").WithArguments("", "System.Threading.Tasks.Task", "mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089", "System.Threading.Tasks.Task").WithLocation(31, 20));
+            CompileAndVerify(sourceCompilation);


CompileAndVerify(sourceCompilation); [](start = 12, length = 36)

It is hard to tell what the program is doing, especially that GetAwaiter returns null. Let's print something from GetAwaiter and GetResult and assert the output to verify that they are called. #Closed

AlekseyTs · 2017-05-10T23:45:36Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMainTests.cs

+    }
+}
+
+static class Program {


static class Program [](start = 0, length = 21)

Consider also testing scenario when the Program is obsolete and the Main is obsolete (two separate scenarios). I would expect the obsolete diagnostics to disappear for the first scenario.

I'll try to do this tomorrow morning, but if more important things come up, I don't think that

[Obsolete] public static async Task<T> Main(){}

is a 7.1 blocker.

Agree. #Closed

AlekseyTs · 2017-05-10T23:46:04Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncMainTests.cs

+            var verifier = sourceCompilation.VerifyEmitDiagnostics(
+                // (30,12): warning CS0436: The type 'Task' in '' conflicts with the imported type 'Task' in 'mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089'. Using the type defined in ''.
+                //     static Task Main() {
+                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 = 230, length = 1)

Locations are dropped. #Closed

AlekseyTs · 2017-05-10T23:49:29Z

Done with review pass. #Closed

TyOverby · 2017-05-10T23:56:56Z

           var success = binder.GetAwaitableExpressionInfo(userMainInvocation, out _, out _, out _, out _getAwaiterGetResultCall, _userMainReturnTypeSyntax, diagnosticBag);
It looks like the parameter is no longer used. Consider removing.
Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedEntryPointSymbol.cs:365 in a54e0ce. [](commit_id = a54e0ce, deletion_comment = True)

Which parameter?

EDIT: oh, DiagnosticBag?
EDIT: Done.

AlekseyTs

LGTM, additional tests with Obsolete can be added later

dnfclas added the cla-already-signed label May 10, 2017

TyOverby force-pushed the async-main-cleanup-2 branch from 469c98f to f11dd17 Compare May 10, 2017 17:27

cston reviewed May 10, 2017

View reviewed changes

AlekseyTs reviewed May 10, 2017

View reviewed changes

TyOverby added 4 commits May 10, 2017 15:25

remove remaining prototype comments, add extension method tests

a6a0f43

fix old test, address review feedback

229e33e

ReturnType implemented on each subclass

a54e0ce

added more tests on non-async task returning mains

a38527d

TyOverby force-pushed the async-main-cleanup-2 branch from 39de672 to a38527d Compare May 10, 2017 22:26

VSadov reviewed May 10, 2017

View reviewed changes

VSadov approved these changes May 10, 2017

View reviewed changes

TyOverby added 2 commits May 10, 2017 16:23

add obsolete tests

7aaf3f8

autoformat

be56ee9

AlekseyTs reviewed May 10, 2017

View reviewed changes

add better tests for successful task overrides

8fe23ca

AlekseyTs approved these changes May 11, 2017

View reviewed changes

TyOverby merged commit b0249ee into dotnet:features/async-main May 11, 2017

Remove remaining prototype comments #19392

Remove remaining prototype comments #19392

Conversation

TyOverby commented May 10, 2017

TyOverby commented May 10, 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

cston May 10, 2017 • edited Loading

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

Choose a reason for hiding this comment

cston commented May 10, 2017

TyOverby commented May 10, 2017 • edited Loading

AlekseyTs May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 10, 2017 • edited Loading

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

VSadov commented May 10, 2017

VSadov left a comment

Choose a reason for hiding this comment

TyOverby commented May 10, 2017

AlekseyTs commented May 10, 2017 • edited Loading

AlekseyTs May 10, 2017 • edited Loading

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

AlekseyTs May 11, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 10, 2017 • edited Loading

TyOverby commented May 10, 2017 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

cston May 10, 2017 •

edited

Loading

TyOverby commented May 10, 2017 •

edited

Loading

AlekseyTs May 10, 2017 •

edited

Loading

AlekseyTs May 10, 2017 •

edited

Loading

AlekseyTs commented May 10, 2017 •

edited

Loading

AlekseyTs commented May 10, 2017 •

edited

Loading

AlekseyTs May 10, 2017 •

edited

Loading

AlekseyTs May 11, 2017 •

edited

Loading

AlekseyTs May 10, 2017 •

edited

Loading

AlekseyTs commented May 10, 2017 •

edited

Loading

TyOverby commented May 10, 2017 •

edited

Loading