Fix overly aggressive html completion

[alexgav]

After we switched to the new Razor completion endpoint, we made a larger set of trigger characters available in VS Code Razor pages. They are needed in VS to maintain VS HTML completion trigger behavior, but they cause unnecessary completion lists with unfiltered entries (e.g. all words in the document) pop-up whenever those trigger characters are typed anywhere in the document.

In this PR I'm adding a simple fix to suppress those characters in VS Code HTML context (while leaving them enabled for C#). This should eliminate overly aggressive completion list behavior on typing.

There will be a separate fix for the case when wrong completion entries could get inserted (e.g. @... could get inserted when / is typed).

Fixes:
#7678
#7871

[davidwengier]

This statement makes me think this change is not in the right repo.

Aside from the wasted effort of all the LSP messages that are sent to the point where we ignore these characters, this is only ignoring them for Html completions, but presumably Razor completions will still show. Is that desirable? The comment makes it sound like we don't want any completion with these trigger characters in VS Code, so we should not be reporting them as trigger characters in our capabilities.

It also seems like if these were ever desirable trigger characters (eg, for C#) then we'd have to have some logic here to only filter them out if we're in a Html scenario, and I don't see that, which makes it seem even more like we should just not report these as trigger characters.

[alexgav]

Thanks David, to address the concerns:

1. The filter in question is actually executed for HTML only, not for C#. The method name probably could be improved, as int - getHtmlCompletionList similar to getCSharpCompletionList, but here's the calling code with the comment

vscode-csharp/src/razor/src/completion/completionHandler.ts

Line 140 in e185471

return this.provideVscodeCompletions(

2. There are no concerns about Razor completions - the code providing Razor completion items is exactly the same for VSCode vs VS, and will do the right thing not provide completion items when there are no applicable completions

3. I may have not worded the comment the best way - it's not so much that completion shouldn't be triggered is the issue, it's the fact that VSCode provides poor matches (e.g. all words in the document) when it's triggered in locations with no good completion context

4. As far as LSP chattiness goes - it's exactly the same as in VS for VS HTML code. E.g. if you type a Space in plain text part of an HTML document in VS, that space will actually get processed as the trigger characters, sent to our Razor completion handler, which will detect that we are in HTML, and will send it to VS HTML Language Server, which will actually look at HTML parse tree and determine that there are no good completions in "plain text" and return nothing (same for other excluded characters). In VSCode, when there is no good completion context, random non-matches (i.e. seemingly all words in the document) are returned.

So VSCode LSP messages aren't any "chattier" than VS ones.

In general this still feels like a reasonable fix to me. It deals with VSCode-specific behavior, thus it seems to be better to handle it in VSCode rather than putting the burden on the server. What if we had 10 different clients - would the server have to understand specifics of all 10?

[alexgav]

Having said the above (and I still stand by all of the points above), I did give you suggestion quite a bit of thought, and I think we could handle this slightly better and generically for all clients (if there are to be more clients in the future, e.g. web-based interface, etc.).

It seems we would need to pass a set of HTML trigger characters to the server via something like our languageserverfeatureoptions. So VS would have it in the VSLanguageServerFeatureOptions instead of having it hardcoded (as it is today), while VSCode would pass in via command-line parameter to rzls and have it in the ConfigurableLanguageServerFeatureOptions. Then RazorCompletionEndpoint would take in LanguageServerFeatureOptions to figure out which set of HTML trigger characters to use. that would be generic enough for different clients yet would avoid even attempting to send a message back to the client to get HTML completions if the character is not in the trigger character set for the current client.

[davidwengier]

I've thought about this a bit, and I have one question which might inform my answer a bit more: Are there any trigger characters that we are reporting in our capabilities, that we would never want to be a trigger for completion of any kind (ie, Razor, C# or Html) in VS Code?

If the answer is yes, then we should not report those trigger characters. The capabilities system exists for this reason in LSP, and we should faithfully report the trigger characters we care about, and only the ones we care about.

Having said that, there are always going to be some trigger characters that we have to report, but that in certain circumstances we want to ignore. The question becomes where that code goes. Personally I'm a fan of as much code as possible in the Razor server, and more importantly perhaps in the Razor repo. Having code spread around the place just makes testing harder, makes this more fragile, etc. Yes, sometimes we need to write client specific code, and sometimes that code makes sense to put in the client itself. Personally I don't think a character list rises to that level, but I don't feel too strongly about it.

What I do feel strongly about is how we can make sure we don't regress this in future. This issue has come about after the 3rd iteration of a completion system in Razor. We're already halfway through our 4th iteration, and I'm sure there will be 5 or 6. So I think as long as we do a good job to protect the future developers of iteration 5 or 6, be they human or AI, from accidentally re-introducing this bug, then I'm not really fussed where the code is. If the code, and test, are in the Razor repo, we'll find the regression sooner, but the test is probably not as good. If the code and test are here, then we'll find the bug on insertion, which is a little more painful, but still prevents it from affecting users.

[alexgav]

These characters would not be triggers for HTML or C# (i.e. wouldn't be triggers for anything in VS Code)

'*', ',', '&' , "'", '`'

Did you like my suggestion about passing in a set of trigger characters as the server parameter or do you have another suggestion?

[davidwengier]

I wouldn't bother with the complexity of passing in individual characters on the command line, but just pass a bool. There are limits to command lines, and whilst I think we should aim to be usable by other clients, I'm fine with not bending over backwards until someone actually asks

[alexgav]

There was discussion on the other PR (dotnet/razor#11398) about whether even passing an argument makes sense since it's not user-configurable, or simply hardcoding the value in the rzls options is sufficient. Especially if we not passing specific characters, seems like we can just hardcode the choice into the ConfigurationLanguageServerFeatureOptions. What do you think?