Argument completion tweaks#51655
Argument completion tweaks#51655jasonmalinowski merged 8 commits intodotnet:features/argument-completionfrom
Conversation
sharwell
left a comment
There was a problem hiding this comment.
Mainly waiting for updated integration tests
src/VisualStudio/CSharp/Impl/Snippets/SnippetExpansionClient.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
❔ Was this changed as part of the wrong commit?
There was a problem hiding this comment.
➡️ Yes, there were a few small cases of this
There was a problem hiding this comment.
I think I did screw up the commit rebasing at one point.
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
💡 It should be possible to add an integration test for this by typing the following without waiting
ToString, Tab, Tab, 0
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, that's what I meant. Code is still broken though.
There was a problem hiding this comment.
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.
353818e to
3cf764e
Compare
src/VisualStudio/IntegrationTest/IntegrationTests/CSharp/CSharpArgumentProvider.cs
Outdated
Show resolved
Hide resolved
85e95d3 to
656ec3a
Compare
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.
656ec3a to
e931619
Compare
This does several refactorings for argument completion:
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: