Add the RazorCSharp keywords to completion#12522
Conversation
Will add more details after verifying this is the desired approach
davidwengier
left a comment
davidwengier
left a comment
There was a problem hiding this comment.
LGTM, though I think its either worth manually testing without cohosting to ensure there aren't duplicate items, or just remove the class from the non-cohosting DI container just in case.
| services.AddSingleton<CompletionItemResolver, DelegatedCompletionItemResolver>(); | ||
| services.AddSingleton<ITagHelperCompletionService, TagHelperCompletionService>(); | ||
| services.AddSingleton<IRazorCompletionFactsService, LspRazorCompletionFactsService>(); | ||
| services.AddSingleton<IRazorCompletionItemProvider, CSharpRazorKeywordCompletionItemProvider>(); |
There was a problem hiding this comment.
If my hypothesis is correct, and this issue is caused by semantic tokens, then I think putting this in the non-cohosting editor would actually result in doubling up of completion items for these, as Roslyn will add them as well, so I think perhaps this line is not desirable.
There was a problem hiding this comment.
Can I have you try out cohosting off and see if you see the keywords in completion after typing just '@'? (I'm not seeing it without my changes)
There was a problem hiding this comment.
Well now I'm really confused. Using Version: 18.3.0 Insiders [11220.113.main]
Without cohosting:
With cohosting:
There was a problem hiding this comment.
If I remember I'll pull this branch down tomorrow and see what I get with these changes
There was a problem hiding this comment.
Huh, behavior seems to vary across more axes than I tested. I'll do a more comprehensive run through of with/without my changes, at various locations, forced completion versus typed trigger, cohosting on/off and report back probably early tomorrow.
...icrosoft.CodeAnalysis.Razor.Workspaces/Completion/CSharpRazorKeywordCompletionDescription.cs
Outdated
Show resolved
Hide resolved
...icrosoft.CodeAnalysis.Razor.Workspaces/Completion/CSharpRazorKeywordCompletionDescription.cs
Outdated
Show resolved
Hide resolved
...crosoft.CodeAnalysis.Razor.Workspaces/Completion/CSharpRazorKeywordCompletionItemProvider.cs
Outdated
Show resolved
Hide resolved
...isualStudioCode.RazorExtension.Test/Endpoints/Shared/CohostDocumentCompletionEndpointTest.cs
Outdated
Show resolved
Hide resolved
|
@davidwengier -- Verified no duplicate items when I tested with cohosting off. Might be nice to get verification from you since this doesn't fit your mental model. In reply to: 3497021163 |
|
Checked out the branch, and yeah I don't see duplicates, but if I type fast, I can still repro the original problem, so I think my guess about semantic tokens is probably wrong. It took a couple of attempts typing Basically the same as Peter says in #12483 (comment). Having said that, this PR doesn't seem to actively cause problems, so I think it's still fine to merge, just shouldn't be marked as fixing those issues. |
|
I've tried a couple dozen times and for the life of me I cannot repro not having "if" in the completion with this change, even with a debugger attached to change the timing. Did you type the full "@if" quickly, or just the "if" part? In reply to: 3568493906 |
|
Ooh, if I bring up completion inside a tag's contents, I'm reproducing. Will debug later this morning. In reply to: 3571458047 |
…as that has some filtering going on that we don't want for razor csharp keyword completion
|
Commit 3 should fix the issue where keywords weren't being suggested in a non-toplevel context. But I still can't reproduce "if" not showing up in the completion list. In reply to: 3571481513 |
...crosoft.CodeAnalysis.Razor.Workspaces/Completion/CSharpRazorKeywordCompletionItemProvider.cs
Outdated
Show resolved
Hide resolved
...crosoft.CodeAnalysis.Razor.Workspaces/Completion/CSharpRazorKeywordCompletionItemProvider.cs
Outdated
Show resolved
Hide resolved
In the picture I put in yesterday, I just typed |
| } | ||
|
|
||
| [Fact] | ||
| public void ShouldProvideCompletions_ReturnsTrueForSimpleImplicitExpressions_WhenInvoked() |
There was a problem hiding this comment.
Not suggesting this is worth changing in this PR, but I'm curious others' opinions.
I personally prefer the "integration test" style of tests, where we have a Razor document, and run the whole completion system, and verify that an item is or isn't present, over this direct unit testing. Am I alone in this?
There was a problem hiding this comment.
I actually prefer the integration test style too, but I just followed what DirectiveCompletionItemProvider did for it's testing, as a lot of the code in this PR was copied over from it.
3 participants
Add the csharp razor keywords to show up in completion after typing an '@'
Should fix #6927 and part of #12483
From David's analysis, which I didn't stray too far away from:
When Roslyn introduced semantic snippets, it broke Razor because it wouldn't offer the if snippet outside of a code block. This is because until you actually write code after an @ in Razor, the compiler doesn't know if it should go in the class, or the render method. Roslyn is "smart" enough not to show if as a completion option if you're at the class level, and of course as soon as you do type @if the Razor compiler is smart enough to put that in the render method, and everyone is happy. Except the user who was just trying to type :)
So as a consequence, we turned off semantic snippets for Razor files, as a temporary workaround. 3 years later is still temporary, right? Anyway, that "turn off for Razor files" is probably broken for cohosting, so we're back in this position. So the plan, in #6927, is for us to add back our own snippets for @ if (and maybe @ for, @ foreach etc.) to this list and then I think we might be good.