Add async modifier on await completion #48352

Youssef1313 · 2020-10-06T01:15:52Z

~~![await](https://user-images.githubusercontent.com/31348972/110206522-e2c1c800-7e86-11eb-8827-7bda1c4edcdf.gif)~~ (Outdated)

TODO:

Make sure this doesn't cause problems with top-level statements.

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs

Youssef1313 · 2020-10-19T20:10:52Z

Pinging @genlu for review.

genlu · 2020-11-05T00:54:17Z

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs

+            var method = syntaxContext.TargetToken.GetAncestor(node => node.IsAsyncSupportingFunctionSyntax());
+            if (method is not null && !method.GetModifiers().Any(SyntaxKind.AsyncKeyword))
+            {
+                context.AddItem(CommonCompletionItem.Create("await", string.Empty, CompletionItemRules.Default, inlineDescription: "Make container async"));


This might end up conflicting with the item provided by keyword recommender, and one would get deduplicated: since they have seem to have identical full display text (prefix + text + suffix), this will be decided by the order of providers being called.

See:
http://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/Completion/CompletionServiceWithProviders.cs,484

Hiding the keyword might be the behavior we want, if so, we should figure out a way to make it explicit.
Otherwise, we need to show them both and give one higher priority . Before we get to the detail on how to implement this, let's figure out which approach we'd like to take.

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

I don't think we have reached a conclusion on this yet. Current behavior relies on the implementation detail of completion service which is fragile. One way to resolve the interaction issue between two providers that provide "await" item is to fold the keyword provider into this one.

@CyrusNajmabadi thoughts?

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs

genlu · 2020-11-05T01:12:26Z

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs

+
+namespace Microsoft.CodeAnalysis.CSharp.Completion.Providers
+{
+    [ExportCompletionProvider(nameof(AwaitCompletionProvider), LanguageNames.CSharp)]


I think this would be valuable in VB too. Any plan to add VB support in this PR?

@genlu I'll work on supporting VB.

👍 would def want VB here.

Are you planning to add VB support in this PR?

question still remains. but i'm ok with no vb support initially if you'd prefer to punt on that for now.

@CyrusNajmabadi I'll likely do that in a separate PR :(

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs

genlu · 2020-11-05T01:17:58Z

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs

+        {
+        }
+
+        internal override ImmutableHashSet<char> TriggerCharacters => CompletionUtilities.CommonTriggerCharactersWithArgumentList;


@dibarbet @allisonchou is this the proper TriggerCharacters to use?

Ping @dibarbet @allisonchou

genlu · 2020-11-05T01:20:21Z

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs

+
+            // MethodDeclarationSyntax, LocalFunctionStatementSyntax, AnonymousMethodExpressionSyntax, ParenthesizedLambdaExpressionSyntax, SimpleLambdaExpressionSyntax.
+
+            var newDeclaration = AddAsyncModifier(declaration, SyntaxGenerator.GetGenerator(document));


Does this work in debugging scenarios? e.g. EnC, watch window, etc

@genlu I'm not sure.

genlu · 2020-11-05T01:23:38Z

FYI @JoeRobich @333fred This is a new completion provider that make changes outside of current span, I guess it might need some additional work in OmniSharp to enable it in VS Code?

genlu · 2020-11-05T01:24:29Z

Could you please added some tests as well? Thanks!

genlu

Done with pass. Thanks for the contribution :)
In general, I like the approach of creating a separate provider. One of the major question I have is about dealing with item of await keyword and item from this provider. @CyrusNajmabadi have you discussed this with @Youssef1313 already?

genlu · 2021-01-05T23:46:52Z

Hi @Youssef1313, any update on this PR? Please let me know if there's anything I can do to help moving this forward. Thanks! :)

Youssef1313 · 2021-01-06T11:56:10Z

Hi @Youssef1313, any update on this PR? Please let me know if there's anything I can do to help moving this forward. Thanks! :)

@genlu Thanks for pinging. I might take some time to be able to work on this again. I'm okay if you (or any other external contributor) would like to take this over and continue the work.

Youssef1313 · 2021-06-28T11:49:54Z

Ping @CyrusNajmabadi

CyrusNajmabadi · 2021-06-28T16:37:31Z

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs

+
+            return;
+
+            static CompletionItem GetCompletionItem(bool shouldMakeContainerAsync)


this helper method seems pointless. inline?

CyrusNajmabadi · 2021-06-28T16:39:41Z

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs

+            var documentWithAsyncModifier = document.WithSyntaxRoot(root.ReplaceNode(declaration, AddAsyncModifier(declaration)));
+            using var _ = ArrayBuilder<TextChange>.GetInstance(out var builder);
+
+            builder.AddRange(await documentWithAsyncModifier.GetTextChangesAsync(document, cancellationToken).ConfigureAwait(false));


this part is actually a bit terrifying. tehre is no invariant that GetTextChanges returns good edits. Instead of going this route, i would instead either:

just literally hardcode in the logic for determining the span where async should go.

use AddAsyncModifier, then directly check the new declaration for the async token and figure out the span from taht

Either of those will give a nice narrow text edit that we can depend on.

This is a blocker for this PR currently.

Note: This was following what's currently done in another completion provider:

roslyn/src/Features/Core/Portable/Completion/Providers/ImportCompletionProvider/AbstractImportCompletionProvider.cs

Line 156 in 5624337

var importChanges = await formattedDocumentWithImport.GetTextChangesAsync(document, cancellationToken).ConfigureAwait(false);

I'll try with either of the approaches you mentioned.

tehre is no invariant that GetTextChanges returns good edits

I'm curious what "good edits" mean?

I'm curious what "good edits" mean?

So if you added async , you'd ideally get an edit like 'async ' was added at position 15. But there's no guarantee of that. it might literally say: i added the entire contents of the file at position 0.

CyrusNajmabadi · 2021-06-28T16:40:58Z

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs

+            return CompletionChange.Create(CodeAnalysis.Completion.Utilities.Collapse(newText, builder.ToImmutableArray()));
+        }
+
+        private static SyntaxNode AddAsyncModifier(SyntaxNode declaration)


since we're manually adding the async modifier anyways, it's fairly trivial for us to instead just determine the text-edit for where the async modifier should go directly. please switch to that. thanks!

CyrusNajmabadi

definitely some necesxsary changes to make :)

Youssef1313 · 2021-06-30T19:29:10Z

@CyrusNajmabadi This is ready for another look.

Youssef1313 · 2021-07-16T19:35:08Z

Ping @CyrusNajmabadi

CyrusNajmabadi · 2021-07-16T22:03:42Z

Thanks! really looking forward to this :)

All feedback responded to.

Youssef1313 force-pushed the await-completion branch 4 times, most recently from 65354d1 to d60bef3 Compare October 6, 2020 01:22

Youssef1313 commented Oct 7, 2020

View reviewed changes

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs Outdated Show resolved Hide resolved

Youssef1313 commented Oct 7, 2020

View reviewed changes

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs Outdated Show resolved Hide resolved

Youssef1313 marked this pull request as ready for review October 7, 2020 13:25

Youssef1313 requested a review from a team as a code owner October 7, 2020 13:25

jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Oct 8, 2020

jinujoseph assigned genlu Oct 8, 2020

genlu reviewed Nov 5, 2020

View reviewed changes

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs Outdated Show resolved Hide resolved

genlu reviewed Nov 5, 2020

View reviewed changes

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs Outdated Show resolved Hide resolved

genlu reviewed Nov 5, 2020

View reviewed changes

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs Outdated Show resolved Hide resolved

genlu reviewed Nov 5, 2020

View reviewed changes

src/Features/CSharp/Portable/Completion/CompletionProviders/AwaitCompletionProvider.cs Outdated Show resolved Hide resolved

genlu reviewed Nov 5, 2020

View reviewed changes

genlu previously requested changes Nov 5, 2020

View reviewed changes

Base automatically changed from master to main March 3, 2021 23:52

Add async modifier on await completion

f1df660

Youssef1313 force-pushed the await-completion branch from 8423322 to f1df660 Compare March 6, 2021 12:19

CyrusNajmabadi reviewed Jun 28, 2021

View reviewed changes

CyrusNajmabadi requested changes Jun 28, 2021

View reviewed changes

Youssef1313 added 3 commits June 30, 2021 21:02

Address feedback

2e2f041

Merge remote-tracking branch 'upstream/main' into await-completion

d259bc9

Handle explicit lambda return type

a376f7c

Youssef1313 closed this Jul 5, 2021

Youssef1313 reopened this Jul 5, 2021

Fix tests

8f85b57

Youssef1313 requested review from CyrusNajmabadi and genlu July 9, 2021 10:29

CyrusNajmabadi approved these changes Jul 16, 2021

View reviewed changes

CyrusNajmabadi merged commit ad31382 into dotnet:main Jul 16, 2021

ghost added this to the Next milestone Jul 16, 2021

Youssef1313 deleted the await-completion branch July 17, 2021 05:14

Youssef1313 mentioned this pull request Jul 18, 2021

Add AwaitCompletionProvider that makes container Async in VB #54908

Merged

allisonchou modified the milestones: Next, 17.0.P3 Jul 27, 2021

MaStr11 mentioned this pull request Oct 14, 2021

Completion: Suggest "await" after dot of awaitable expression #55817

Merged

sharwell mentioned this pull request Nov 23, 2023

Generate async methods #70941

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add async modifier on await completion #48352

Add async modifier on await completion #48352

Youssef1313 commented Oct 6, 2020 •

edited

Loading

Youssef1313 commented Oct 19, 2020

genlu Nov 5, 2020

genlu Nov 5, 2020

genlu Mar 19, 2021 •

edited

Loading

genlu Nov 5, 2020

Youssef1313 Mar 6, 2021

CyrusNajmabadi Mar 25, 2021

genlu Mar 31, 2021

CyrusNajmabadi Jun 28, 2021

Youssef1313 Jun 30, 2021

genlu Nov 5, 2020

Youssef1313 Jun 30, 2021

genlu Nov 5, 2020

Youssef1313 Mar 6, 2021

genlu commented Nov 5, 2020

genlu commented Nov 5, 2020

genlu left a comment •

edited

Loading

genlu commented Jan 5, 2021

Youssef1313 commented Jan 6, 2021

Youssef1313 commented Jun 28, 2021

CyrusNajmabadi Jun 28, 2021

CyrusNajmabadi Jun 28, 2021

Youssef1313 Jun 30, 2021

CyrusNajmabadi Jun 30, 2021

CyrusNajmabadi Jun 28, 2021

CyrusNajmabadi left a comment

Youssef1313 commented Jun 30, 2021

Youssef1313 commented Jul 16, 2021

CyrusNajmabadi commented Jul 16, 2021


		// MethodDeclarationSyntax, LocalFunctionStatementSyntax, AnonymousMethodExpressionSyntax, ParenthesizedLambdaExpressionSyntax, SimpleLambdaExpressionSyntax.

		var newDeclaration = AddAsyncModifier(declaration, SyntaxGenerator.GetGenerator(document));


		return;

		static CompletionItem GetCompletionItem(bool shouldMakeContainerAsync)

Add async modifier on await completion #48352

Add async modifier on await completion #48352

Conversation

Youssef1313 commented Oct 6, 2020 • edited Loading

Youssef1313 commented Oct 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genlu Mar 19, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genlu commented Nov 5, 2020

genlu commented Nov 5, 2020

genlu left a comment • edited Loading

Choose a reason for hiding this comment

genlu commented Jan 5, 2021

Youssef1313 commented Jan 6, 2021

Youssef1313 commented Jun 28, 2021

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

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Youssef1313 commented Jun 30, 2021

Youssef1313 commented Jul 16, 2021

CyrusNajmabadi commented Jul 16, 2021

Youssef1313 commented Oct 6, 2020 •

edited

Loading

genlu Mar 19, 2021 •

edited

Loading

genlu left a comment •

edited

Loading