MakeMethodAsync: fix iterator methods by jcouv · Pull Request #31846 · dotnet/roslyn

jcouv · 2018-12-16T22:24:24Z

If you have a method returning IEnumerable<>, then add an await, the fixer helps you fix the declaration to make an async-iterator.
If you have an async-iterator method, the remove all awaits, the fixer helps you fix the declaration to make an iterator.

Fixes #31841

FYI @Neme12 @CyrusNajmabadi

Neme12 · 2018-12-16T22:33:43Z

Note: you might also want to take a look at fixing "Make method sync". In case you have an async iterator method but there are no awaits inside, there should be a code fix offered for the compiler warning to remove async and change the return type from IAsyncEnumerable to IEnumerable.

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

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

Neme12 · 2018-12-16T23:20:08Z

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

-            bool ContainsYield(Location l)
-                => l.FindNode(default).DescendantNodes().Any(n => IsYield(n));
+            bool ContainsYield(SyntaxNode node)
+                => node.DescendantNodes(n => n == node || !n.IsKind(SyntaxKind.LocalFunctionStatement)).Any(n => IsYield(n));


Very tiny nit: consider just using .IsReturnableConstruct instead of IsKind. I understand that will make no difference on all correct code because lambdas cannot have yields but it seems appropriate to stop the search at any kind of nested function, and that this method shouldn't really have to think about which of those can contain yields and which cannot.

Or alternatively, feel free to tell me to shut up at this point 😄

Good catch with the n == node part. I would have missed that. Do you have a test where the iterator method is a local function, to make sure this check is covered?

Yes, I caught that case with a test. I'd missed it initially too ;-)

CyrusNajmabadi · 2018-12-17T01:18:07Z

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

+                var returnType = methodSymbol.ReturnType;
+                if (IsIEnumerable(returnType, knownTypes) && IsIterator(methodSymbol))
+                {
+                    newReturnType = MakeGenericType("IAsyncEnumerable", methodSymbol.ReturnType);


can you not use nameof here?

That type is not referenced (it has not shipped yet).

CyrusNajmabadi · 2018-12-17T01:18:56Z

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

+            TypeSyntax MakeGenericType(string type, ITypeSymbol typeArgumentFrom)
+            {
+                return SyntaxFactory.GenericName(
+                    SyntaxFactory.Identifier(type),


this seems wrong. how will this work if they don't have an import for the namespace where these types live? We should be fully qualifying these.

In that case you will get an error and a suggestion to import the namespace.
I'm ok to fully qualify though.

It's easy to do that too just using SyntaxGenerator.TypeExpression. Or ITypeSymbol.GenerateTypeSyntax. They will even put the right annotations on things so things will simplify properly :)

CyrusNajmabadi · 2018-12-17T01:19:46Z

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

+            {
+                return SyntaxFactory.GenericName(
+                    SyntaxFactory.Identifier(type),
+                    SyntaxFactory.TypeArgumentList(SyntaxFactory.SeparatedList(new[] { typeArgumentFrom.GetTypeArguments()[0].GenerateTypeSyntax() })));


Use .SingletonSeparatedList.

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

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

CyrusNajmabadi · 2018-12-17T04:13:36Z

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

                if (!keepVoid)
                {
-                    newReturnType = knownTypes.TaskType.GenerateTypeSyntax();
+                    newReturnType = knownTypes._taskType.GenerateTypeSyntax();


note, if these are public readonlys, they don't need to be named _...

That seems to be the configured naming convention

sounds liek a mistake. IDE has lots of of public readonly fields that are not underscored. underscores are intended to only be for private members. @sharwell do you know what's up here?

CyrusNajmabadi · 2018-12-17T04:14:02Z

...Features/Core/Portable/MakeMethodSynchronous/AbstractMakeMethodSynchronousCodeFixProvider.cs

            }
        }
+
+        protected struct KnownTypes


why is KnownTypes defined in two places?

jcouv · 2018-12-18T01:16:27Z

@CyrusNajmabadi Please take another look when you have some time. Thanks

CyrusNajmabadi · 2018-12-18T09:50:13Z

.../Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.KnownTypes.cs

+            public readonly INamedTypeSymbol _iEnumeratorOfTType;
+
+            public readonly INamedTypeSymbol _iAsyncEnumerableOfTType;
+            public readonly INamedTypeSymbol _iAsyncEnumeratorOfTType;


technically these last 5 types should have the Opt suffix (matching _valueTaskOfTTypeOpt).

jcouv · 2018-12-18T17:59:57Z

@dotnet/roslyn-ide for review. Thanks

jcouv · 2018-12-19T18:26:48Z

@dotnet/roslyn-ide for review. Thanks

mavasani · 2018-12-19T20:54:35Z

.../Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.KnownTypes.cs

+    {
+        internal readonly struct KnownTypes
+        {
+            public readonly INamedTypeSymbol _taskType;


Nit: Consider using auto-props instead.

in general, IDE is totally fine with readonly-fields for internal simple-data structs. The props don't really buy anything since we have no need to change the impl over time. or, in other words, if we did need to change the impl, we own all the consumers, so we can just change them. so, the readonly field is just the simplest way of saying "this is just raw data, and nothing more".

I agree. I think I prefer simple field for now.
Thanks

jcouv · 2018-12-20T18:10:55Z

@jinujoseph Please approve for preview2. Thanks

MakeMethodAsync: fix iterator methods

4356275

jcouv added Area-IDE Feature - Async Streams Async Streams labels Dec 16, 2018

jcouv added this to the 16.0.P2 milestone Dec 16, 2018

jcouv self-assigned this Dec 16, 2018

jcouv requested a review from a team as a code owner December 16, 2018 22:24

Neme12 reviewed Dec 16, 2018

View reviewed changes

src/EditorFeatures/CSharpTest/Diagnostics/MakeMethodAsynchronous/MakeMethodAsynchronousTests.cs Outdated Show resolved Hide resolved

Detect iterator methods

a1d7ec3

Neme12 reviewed Dec 16, 2018

View reviewed changes

...atures/CSharp/Portable/MakeMethodAsynchronous/CSharpMakeMethodAsynchronousCodeFixProvider.cs Outdated Show resolved Hide resolved

Yields inside local functions don't count

4df805b

Neme12 reviewed Dec 16, 2018

View reviewed changes

CyrusNajmabadi reviewed Dec 17, 2018

View reviewed changes

...atures/CSharp/Portable/MakeMethodAsynchronous/CSharpMakeMethodAsynchronousCodeFixProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 17, 2018

View reviewed changes

...atures/CSharp/Portable/MakeMethodAsynchronous/CSharpMakeMethodAsynchronousCodeFixProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 17, 2018

View reviewed changes

...atures/Core/Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 17, 2018

View reviewed changes

...atures/Core/Portable/MakeMethodAsynchronous/AbstractMakeMethodAsynchronousCodeFixProvider.cs Outdated Show resolved Hide resolved

MakeMethodSync: handle async-iterator methods

3a42749

CyrusNajmabadi reviewed Dec 17, 2018

View reviewed changes

jcouv and others added 2 commits December 16, 2018 20:29

Address feedback from Cyrus

a550cc4

Address feedback from Cyrus

af346ba

CyrusNajmabadi reviewed Dec 18, 2018

View reviewed changes

CyrusNajmabadi approved these changes Dec 18, 2018

View reviewed changes

Add 'Opt' suffix

aec722e

mavasani approved these changes Dec 19, 2018

View reviewed changes

jinujoseph added the Approved to merge label Dec 21, 2018

jcouv merged commit 3bb73cf into dotnet:master Dec 21, 2018

jcouv deleted the make-async branch December 21, 2018 06:19

jcouv mentioned this pull request Dec 26, 2018

[Umbrella] Work items and test plan for async streams #24037

Closed

Conversation

jcouv commented Dec 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Neme12 commented Dec 16, 2018

Uh oh!

Uh oh!

Uh oh!

Neme12 Dec 16, 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!

Neme12 Dec 16, 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!

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!

Uh oh!

Uh oh!

Uh oh!

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!

jcouv commented Dec 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Dec 18, 2018

Uh oh!

jcouv commented Dec 19, 2018

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!

jcouv commented Dec 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jcouv commented Dec 16, 2018 •

edited

Loading

Neme12 Dec 16, 2018 •

edited

Loading

Neme12 Dec 16, 2018 •

edited

Loading