Argument completion tweaks #51655

jasonmalinowski · 2021-03-04T07:01:41Z

This does several refactorings for argument completion:

Rather than using snippet function providers, just embed the computed values into the snippet XML directly. This avoids having to have an argument provider "reach back" into the snippet state in the first place.
Because of that prior simplification, remove the "preserve symbols" state management which was required to preserve state during the dismissal of a prior session and the start of a next one.

This doesn't fix one general issue about the experience right now, which is model changes that come from typing rather than up/down arrow cause a new snippet to apply, potentially with problematic effects. I did make two small tweaks though to generally mitigate the impact:

Switch to using SymbolEquivalenceComparer when checking if the method we're applying is the same as the one already applied. Otherwise if we apply a snippet, signature help will update the model to select the same overload. If the symbols were from source (rather than metadata) this would result in an infinite loop of applications.
If you start typing in an empty signature, copy what you've typed into the first parameter of the new method. Otherwise you start typing, a new overload is chosen, and then your typing is immediately overwritten.

sharwell

Mainly waiting for updated integration tests

src/VisualStudio/CSharp/Impl/Snippets/SnippetExpansionClient.cs

sharwell · 2021-03-04T16:46:09Z

src/VisualStudio/Core/Def/Implementation/Snippets/AbstractSnippetExpansionClient.cs

+            // doing so will disrupt signature help.
+            if (_state.IsFullMethodCallSnippet)


❔ Was this changed as part of the wrong commit?

➡️ Yes, there were a few small cases of this

I think I did screw up the commit rebasing at one point.

Oh, for this one, it's intentionally part of the null annotation commit: the comment stated that we had to check _methods because _expansionSession isn't initialized, but the explicit null check being added means the comment is now stale. The null check was added because the null annotation revealed that if it was null, we'd end up throwing at line 177.

src/VisualStudio/Core/Def/Implementation/Snippets/AbstractSnippetExpansionClient.cs

sharwell · 2021-03-04T16:58:02Z

src/VisualStudio/Core/Def/Implementation/Snippets/AbstractSnippetExpansionClient.cs

@@ -802,6 +802,17 @@ public void MoveToSpecificMethod(IMethodSymbol method, CancellationToken cancell

            if (_state._method is not null)
            {
+                // If we didn't have any previous parameters, then there is only the placeholder in the snippet.
+                // We'll use that as text of the first argument; failing to do so means starting to type in the placeholder
+                // will be overwritten once the signature help changes it's overload due to your typing.


💡 It should be possible to add an integration test for this by typing the following without waiting

ToString, Tab, Tab, 0

I actually have to fix this, because this needs to be conditioned on either the method being not null or also there not being a method at all which is if you hit tab before signature help even caught up.

or also there not being a method at all which is if you hit tab before signature help even caught up

Signature help is only triggered by the second Tab key, so for the example here only 0 could be typed between this feature getting invoked and signature help appearing.

Yeah, that's what I meant. Code is still broken though.

So I fixed up the code a bit better, but until we fix or address typing causing model changes, it's hard to really get a sense of what's necessary here. I'd say we fix that, and then sort out a good test for this.

src/VisualStudio/IntegrationTest/IntegrationTests/CSharp/CSharpArgumentProvider.cs

Since we're only using this in snippets we generate ourselves, we can just compute the value directly and insert that into the Snippet XML.

This takes advantage of the prior commit to simplify some state management. When we were using snippet functions to populate each parameter, it was necessary to ensure some state "survives" when we are moving from one overload to the next. This is because there's some reentrancy: the process of moving to a new overload would dismiss the prior snippet, but it had to still be avialable before the call to insert the new snippet returned. Now that the snippet is fully pre-generated, that's not a requirement anymore; instead we can just always clear state when one snippet is dismissed, and set it afterwards again once the snippet is re-inserted. This removes the necessity of having the "_preserveSymbols" flag, as well as removing the requirement to call specific Clear() at various points when we were dismissing sessions. The other simplification is the list of methods that was stored once a session began was only used to grab one thing: the name of the method group. So just replace that with a string.

If the first overload displayed by signature help is a method with no parameters, then we have a placeholder snippet field. If you type in that snippet field, we would switch the active overload to something else which would cause what you typed to disappear.

As long as an edit happens in the middle, the two methods can come from different Compilations. If the methods are in source, they don't compare the same.

Since we create snippets out of XML, ensure we don't mangle parameters if they round-trip in some way.

dotnet-issue-labeler bot added the Area-IDE label Mar 4, 2021

sharwell reviewed Mar 4, 2021

View reviewed changes

jasonmalinowski self-assigned this Mar 4, 2021

CyrusNajmabadi approved these changes Mar 5, 2021

View reviewed changes

jasonmalinowski force-pushed the argument-completion-tweaks branch 4 times, most recently from 353818e to 3cf764e Compare March 11, 2021 01:04

jasonmalinowski marked this pull request as ready for review March 11, 2021 01:40

jasonmalinowski requested a review from a team as a code owner March 11, 2021 01:40

jasonmalinowski marked this pull request as draft March 11, 2021 01:40

jasonmalinowski marked this pull request as ready for review March 11, 2021 02:06

sharwell approved these changes Mar 11, 2021

View reviewed changes

src/VisualStudio/IntegrationTest/IntegrationTests/CSharp/CSharpArgumentProvider.cs Outdated Show resolved Hide resolved

jasonmalinowski force-pushed the argument-completion-tweaks branch 2 times, most recently from 85e95d3 to 656ec3a Compare March 12, 2021 00:27

dotnet deleted a comment from azure-pipelines bot Mar 15, 2021

This was referenced Mar 15, 2021

Agents canceling during dotnet builds jaredpar/runfo#47

Closed

Azure Agent timeout #48030

Closed

jasonmalinowski added 8 commits March 16, 2021 11:46

Prefix fields with _ and make them private

a5f9eff

Null annotate the SnippetExpansionClient

7367def

Remove the ArgumentValue snippet function

6243f78

Since we're only using this in snippets we generate ourselves, we can just compute the value directly and insert that into the Snippet XML.

Switch to SymbolEquivalenceComparer when comparing symbols

65d7e29

As long as an edit happens in the middle, the two methods can come from different Compilations. If the methods are in source, they don't compare the same.

Add tests to cover that parameter content isn't misinterpreted in XML

c7c1de0

Since we create snippets out of XML, ensure we don't mangle parameters if they round-trip in some way.

Add a workaround for an editor bug

e931619

jasonmalinowski force-pushed the argument-completion-tweaks branch from 656ec3a to e931619 Compare March 16, 2021 18:46

jasonmalinowski merged commit c953000 into dotnet:features/argument-completion Mar 16, 2021

jasonmalinowski deleted the argument-completion-tweaks branch March 16, 2021 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Argument completion tweaks #51655

Argument completion tweaks #51655

jasonmalinowski commented Mar 4, 2021 •

edited

Loading

sharwell left a comment

sharwell Mar 4, 2021

sharwell Mar 4, 2021

jasonmalinowski Mar 4, 2021

jasonmalinowski Mar 5, 2021

sharwell Mar 4, 2021

jasonmalinowski Mar 4, 2021

sharwell Mar 4, 2021

jasonmalinowski Mar 5, 2021

jasonmalinowski Mar 11, 2021

		// doing so will disrupt signature help.
		if (_state.IsFullMethodCallSnippet)

Argument completion tweaks #51655

Argument completion tweaks #51655

Conversation

jasonmalinowski commented Mar 4, 2021 • edited Loading

sharwell left a 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

jasonmalinowski commented Mar 4, 2021 •

edited

Loading