Skip to content

Fix overly aggressive html completion #7928

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Fix overly aggressive html completion #7928

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aea81ab
Ignore non-HTML Razor tigger characters in HTML content
alexgav Jan 15, 2025
b7fcfc4
Fixing a typo
alexgav Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/razor/src/completion/completionHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,36 @@ export class CompletionHandler {
projectedPosition: Position,
triggerCharacter: string | undefined
) {
// The following characters are defined as trigger characters in Razor language server,
Copy link
Member

@davidwengier davidwengier Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@alexgav alexgav Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

  1. 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

  2. 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

  3. 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?

Copy link
Contributor Author

@alexgav alexgav Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@davidwengier davidwengier Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@alexgav alexgav Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@davidwengier davidwengier Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@alexgav alexgav Jan 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// but are not normally trigger characters for VS Code HTML language server. We will
// explicitely ignore them to avoid unexpected completion coming up. VS HTML language server
// simply returns nothign when completion is not appropriate. VS Code HTML completion is much
// more aggressive and will always put "someting" (e.g. all words in the document) into the
// completion list whenever asked for one.
const ignorableTriggerCharacters = new Set<string>([
' ',
'(',
'=',
'[',
'{',
'"',
'/',
'\\',
':',
'~',
'*',
',',
'-',
'&',
"'",
'"',
'`',
]);

if (triggerCharacter && ignorableTriggerCharacters.has(triggerCharacter)) {
return CompletionHandler.emptyCompletionList;
}

const completions = await vscode.commands.executeCommand<vscode.CompletionList | vscode.CompletionItem[]>(
'vscode.executeCompletionItemProvider',
virtualDocumentUri,
Expand Down
Loading