MakeMethodAsync: don't rename entry point methods by jcouv · Pull Request #26316 · dotnet/roslyn

jcouv · 2018-04-21T23:41:02Z

Customer scenario

Invoked MakeMethodAsync fixer on Main method. The method should be left as Main rather than renamed to MainAync.

Bugs this fixes

Fixes #26312

Workarounds, if any

Edit the name back.

Risk

Performance impact

Low. The fix is limited to the MakeMethodAsync fixer.

Is this a regression from a previous update?

No.

How was the bug found?

Reported by customer.

Neme12 · 2018-04-22T00:51:55Z

src/EditorFeatures/CSharpTest/Diagnostics/MakeMethodAsynchronous/MakeMethodAsynchronousTests.cs

+class Program
+{
+    public static async void Main()
+    {


please add another test for the version of this feature that changes the return type to Task

by removing the "index: 1"

thanks

also, i would personally consider disabling the "stay void" version entirely here because the resulting code doesn't make sense

Good idea. Thanks

also, i would personally consider disabling the "stay void" version entirely here because the resulting code doesn't make sense

Sorry, can you explain that? Why does "stay void" need to be disabled . We added "stay void" because we got a bunch of user feedback from people saying they wanted "fire and forget" async methods. and they didn't like being forced to make the return type 'Task'. 'async void Main' seems like a prime candidate for something people would want ot keep 'void'.

IN other words, i don't see a reason for 'async Main' to behave differently from the rules we have for all other methods. Without a strong good reason, i would not special case it here.

@CyrusNajmabadi (resolved in a comment below) async void Main is a compiler error: error CS4009: A void or int returning entry point cannot be async

Neme12 · 2018-04-22T10:14:43Z

...atures/Core/Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.cs

                    new MyCodeAction(GetMakeAsyncTaskFunctionResource(), c => FixNodeAsync(
-                        context.Document, diagnostic, keepVoid: false, cancellationToken: c)),
+                        context.Document, diagnostic, keepVoid: false, isEntryPoint, cancellationToken: c)),
                    context.Diagnostics);


nit: it looks to me like this code is exactly the same as in the other branch. could this be simplified to offer this fix unconditionally and then have a check if (isOrdinaryOrLocalFunction && symbol.ReturnsVoid && !isEntryPoint) to additionally offer the "keep void" fix?

CyrusNajmabadi · 2018-04-22T18:13:26Z

...atures/Core/Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.cs


            var symbol = semanticModel.GetDeclaredSymbol(node, cancellationToken) as IMethodSymbol;
+            var entryPoint = compilation.GetEntryPoint(cancellationToken);
+            bool isEntryPoint = entryPoint?.Equals(symbol) == true;


why not remove hte "as IMethodSybol" and then just do: var isEntryPoint = symbol.Equals(entryPoint)?

Also, what's the perf of "GetEntryPoint"? wouldn't it have to scan the entire compilation? Should we instead compute this during the fix itself?

"var isEntryPoint" plz :)

Ok, i looked at GetEntryPoint. CUrrently it's very expensive. It realizes every namespace, namedtype and member symbol in the compilation.

Note: it could be made much faster through use of hte Decl table. The decl table already stores the names of children in the SingleTypeDeclaration node. So it would be possible to only examine types that actually included a member called 'Main' in it.

With such a change i would be ok with teh expensive GetEntryPoint being called here. Otherwise, i don't think ti's a good idea to do something so expensive.

Note: we already wrote a function to find symbols efficiently by name in the compilation by using:

GetSymbolsWithName. Unfortunately, because it takes a predicate and not a string, it needs to realize a type to get all the member symbols. That code could be augmented to just take a string, in which case it could efficiently check if it even needed to check the members of a type or not.

CyrusNajmabadi · 2018-04-22T18:15:22Z

...atures/Core/Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.cs

+            // If it's a void returning method (and not an entry point), also offer to keep the void return type
            bool isOrdinaryOrLocalFunction = symbol.IsOrdinaryMethodOrLocalFunction();
-            if (isOrdinaryOrLocalFunction && symbol.ReturnsVoid)
+            if (isOrdinaryOrLocalFunction && symbol.ReturnsVoid && !isEntryPoint)


why would we gate this on isEntryPoint. Can you have "async Task Main"? the only rule is that it is still named Main and not MainAsync right? it can be 'async void' or 'async Task'

@CyrusNajmabadi You can't have async void Main, it is a compiler error (at first I thought it would just get treated as if the method isn't awaited and the program would exit prematurely, which would have been even worse because it could trip people up - that's probably why it's explicitly disallowed)

With async Task Main on the other hand, most people will get "async main isn't supported in 7.0, please upgrade to 7.1..." so at least we're making people upgrade and use new language features, which sounds good to me.

If they do not want to, they will have to come up with a solution such as .GetAwaiter().GetResult() themselves. We don't currently generate that in any case, and it's probably not worth addign that, especially when there is a better way.

We could of course have a separate feature that would replace await with .GetAwaiter().GetResult() instead of making the method async. I'm not sure if we should encourage people to do that though.

That's fair. That said, i would still not want thsi PR as is with the expensive check during analysis. I can see about making a faster version of GetEntryPoint that would make it ok to check during analysis.

@CyrusNajmabadi Yes, the check is probably more costly than most, but it's in a fixer, not an analyzer, so I don't think it would have much impact.
Alternatively, as simple enough heuristic is to check if the method is called Main (and maybe check that it is public and static too). I'm happy to do that (that was my initial approach).

@jcouv Main doesn't have to be public

I would prefer if we created an actual helper that would check if the method is named "Main", is static, the return type is either void, int, Task or Task<int> and it takes either no parameters or one string[] so that we could reuse this helper in other places. But I guess that can be done later, so it's up to you.

but it's in a fixer, not an analyzer, so I don't think it would have much impact.

Fixers have a large impact :) both for computing if we should have a lightbulb and for expandin ghte lightbulb. It's the actual fix that can be a bit slower (as it will have an actual dialog telling the user it's doing the update).

Yes. I would also be ok with an approach where we first checked if this looked like 'Main' and then did the expensive check then.

That said, i have a PR to make GetEntryPoint fast over here: #26325

CyrusNajmabadi · 2018-04-22T18:16:10Z

...atures/Core/Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.cs

            {
                return await RenameThenAddAsyncTokenAsync(
-                    keepVoid, document, node, methodSymbolOpt, cancellationToken).ConfigureAwait(false);
+                    keepVoid, isEntryPoint, document, node, methodSymbolOpt, cancellationToken).ConfigureAwait(false);


instead of passing this in here, wouldn't you just want to not even do the rename if this is the entrypoint? i.e. hoist !isEntryPoint into the if-check.

If you do that, you'll just fall below to where the async token is added, which is waht you want, right?

Neme12 · 2018-04-23T15:28:10Z

...atures/Core/Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.cs

            var symbol = semanticModel.GetDeclaredSymbol(node, cancellationToken) as IMethodSymbol;
-            var entryPoint = compilation.GetEntryPoint(cancellationToken);
-            bool isEntryPoint = entryPoint?.Equals(symbol) == true;
+            bool isEntryPoint = symbol != null && symbol.IsStatic && string.Equals(symbol.Name, "Main", StringComparison.Ordinal);


StringComparison.Ordinal does this work for VB which should be case insensitive?

jasonmalinowski · 2018-04-23T17:56:00Z

...atures/Core/Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.cs

            }

            var symbol = semanticModel.GetDeclaredSymbol(node, cancellationToken) as IMethodSymbol;
+            bool isEntryPoint = symbol != null && symbol.IsStatic && string.Equals(symbol.Name, "Main", StringComparison.Ordinal);


Considering the large conversation below about performance, should this at least do a quick check to see if CompilationOptions.MainTypeName is set, and if so, bring that into consideration here?

I think you are proposing:

if MainTypeName is set, then do expensive check

otherwise, use heuristic (cheap check on method to see if it looks like Main)

I'm ok to rely purely on heuristic (only doing (2) above), since using an entry point other than Main seems uncommon.

CyrusNajmabadi · 2018-04-24T03:31:00Z

...atures/CSharp/Portable/MakeMethodAsynchronous/CSharpMakeMethodAsynchronousCodeFixProvider.cs

            => node.IsAsyncSupportingFunctionSyntax();

+        protected override bool IsLikelyEntryPointName(string methodName)
+            => string.Equals(methodName, "Main", StringComparison.Ordinal);


general IDE pattern is to get teh ISyntaxFactsService off of the Document you're starting with, then do syntaxFacts.StringComparer.Equals(...). This does the right thing for VB and C# and needs no specific delegation like you have here.

CyrusNajmabadi · 2018-04-24T03:31:13Z

...atures/Core/Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.cs

            }

            var symbol = semanticModel.GetDeclaredSymbol(node, cancellationToken) as IMethodSymbol;
+            bool isEntryPoint = symbol != null && symbol.IsStatic && IsLikelyEntryPointName(symbol.Name);


CyrusNajmabadi

LGTM with the change i recommended.

heejaechang · 2018-04-24T08:39:58Z

src/EditorFeatures/TestUtilities/Workspaces/TestWorkspace_XmlCreation.cs

                element.SetAttributeValue(CheckOverflowAttributeName, true);
            }

+            if (options.OutputKind != OutputKind.DynamicallyLinkedLibrary)


is there reason why you set this only when kind is not dynamically linked library? you could always set it? even though default is set to dynamicallylinked library?

No strong reason. Since the default OutputKind is DynamicallyLinkedLibrary, I figured it's not useful to do more work in the common case.

CyrusNajmabadi · 2018-04-26T03:07:12Z

...atures/Core/Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.cs

            }

            var symbol = semanticModel.GetDeclaredSymbol(node, cancellationToken) as IMethodSymbol;
+            var isEntryPoint = symbol != null && symbol.IsStatic && IsLikelyEntryPointName(symbol.Name, context.Document);


very minor nit: since this is a heuristic, it might be worthwhile to mention taht it's not totally accurate, but we're ok with the unlikely false positive for this feature.

CyrusNajmabadi · 2018-04-26T03:08:01Z

...atures/Core/Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.cs

            var syntaxPath = new SyntaxPath(node);

-            // Rename the method to add the 'Async' suffix, then add the 'async' keyword.
+            // Rename the method to add the 'Async' suffix (except if it's the entry point), then add the 'async' keyword.


comment stale.

CyrusNajmabadi · 2018-04-26T03:08:24Z

Only nits about the comments. don't care if they get fixed.

jcouv · 2018-04-26T03:44:18Z

@jinujoseph Please approve for 15.8. Thanks

jcouv · 2018-04-26T16:20:55Z

test windows_debug_unit64_prtest please

jcouv added the Area-IDE label Apr 21, 2018

jcouv added this to the 15.8 milestone Apr 21, 2018

jcouv requested a review from a team as a code owner April 21, 2018 23:41

MakeMethodAsync: don't rename entry point methods

41fe259

jcouv force-pushed the async-main branch from de69f59 to 41fe259 Compare April 22, 2018 00:43

Neme12 reviewed Apr 22, 2018

View reviewed changes

Don't offer fix keeping void, for entry points

8aaaf66

Neme12 reviewed Apr 22, 2018

View reviewed changes

Factor code

5661623

CyrusNajmabadi reviewed Apr 22, 2018

View reviewed changes

CyrusNajmabadi mentioned this pull request Apr 22, 2018

Improve searching for symbols when a name is known ahead of time. (Including Compilation.GetEntryPoint()) #26325

Merged

Use heuristic instead of proper entry point check

d4c774f

Neme12 reviewed Apr 23, 2018

View reviewed changes

jasonmalinowski reviewed Apr 23, 2018

View reviewed changes

Use case-insensitive comparison for VB

1885068

CyrusNajmabadi reviewed Apr 24, 2018

View reviewed changes

CyrusNajmabadi approved these changes Apr 24, 2018

View reviewed changes

heejaechang reviewed Apr 24, 2018

View reviewed changes

heejaechang approved these changes Apr 24, 2018

View reviewed changes

Address PR feedback from Cyrus

42c9997

CyrusNajmabadi approved these changes Apr 26, 2018

View reviewed changes

CyrusNajmabadi reviewed Apr 26, 2018

View reviewed changes

Fix comments

7b5bf25

jcouv self-assigned this Apr 26, 2018

jinujoseph added the Approved to merge label Apr 26, 2018

jcouv merged commit bab0f7e into dotnet:master Apr 26, 2018

jcouv deleted the async-main branch April 26, 2018 17:25

Conversation

jcouv commented Apr 21, 2018

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

How was the bug found?

Uh oh!

Neme12 Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Apr 22, 2018 •

edited

Loading

Neme12 Apr 22, 2018 •

edited

Loading

Neme12 Apr 22, 2018 •

edited

Loading

CyrusNajmabadi Apr 22, 2018 •

edited

Loading

Neme12 Apr 22, 2018 •

edited

Loading

Neme12 Apr 22, 2018 •

edited

Loading

Neme12 Apr 22, 2018 •

edited

Loading

CyrusNajmabadi Apr 22, 2018 •

edited

Loading

Neme12 Apr 23, 2018 •

edited

Loading