Skip to content
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.

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

Adding LSP client completion for Razor handler and removing legacy completion #7205

Merged
merged 12 commits into from
Jun 23, 2024

Conversation

alexgav
Copy link
Contributor

@alexgav alexgav commented Jun 6, 2024

Adding LSP client completion handler and removing legacy completion

Implementation notes:

  1. Most of the code is a combination of what VS client does for razor/completion and razor/completionResolve requests https://github.com/dotnet/razor/blob/main/src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/LanguageClient/Endpoints/RazorCustomMessageTarget_Completion.cs and what old code did in https://github.com/dotnet/vscode-csharp/blob/88bbc90365504031abf62eccde21b402211facce/src/razor/src/completion/razorCompletionItemProvider.ts#L27C14-L27C41 and https://github.com/dotnet/vscode-csharp/blob/88bbc90365504031abf62eccde21b402211facce/src/razor/src/completion/provisionalCompletionOrchestrator.ts#L17C14-L17C47
  2. In a nutshell,
    a. for C# (in non-provisional completion case) we call roslyn completion directly to get completion list.
    That will already be in LSP format and we can just return it.
    b. for HTML, we execute vscode command to get completions. Those will get returned in vscode format, so we need to translate them to LSP format before returning them to Razor server.
    c. for C# (in provisional case) we currently have to use vscode command as well instead of calling Roslyn directly. That's what existing code currently does and calling Roslyn directly via Roslyn command causes null to be returned. There is Provisional Completion in Razor should show full tooltip info #7250 filed to follow up on that.
  3. We do have to "massage" to Roslyn LSP completion results a little before returning them to Razor server, mostly to copy default item data to each completion item. Otherwise completionResolve fails in Roslyn
  4. Most of the provisional completion code in the deleted ProvisionalCompletionOrchestrator was
    a. Detecting that this is in fact a provisional completion case. We don't need that now since server tells us when it is the case by supplying non-null provisional edit.
    b. Detecting when the user is done interacting with completion so that provisional edit (temporarily added '.' in C# projected document) can be removed. We don't need to do that since we just get the data from Roslyn and return to the server. We can remove provisionally added '.' immediately after getting the data from Roslyn.

Main wins after this PR:

  1. Legacy completion code in Razor server can be deleted.

  2. Tag helpers are now added to HTML completion
    image

  3. Correct glyphs/icons are shown for HTML and provisional completion C#
    image
    image

…upport

Pushing as draft initially to get early feedback while I am finishing
- completionResolve
- type clean-up (switch to vscode-languageserver-protocol types when possible)
- provisional completion support
@alexgav alexgav requested a review from a team as a code owner June 6, 2024 20:57
@alexgav alexgav changed the title Adding LSP client completion handler and removing legacy completion Adding LSP client completion for Razor handler and removing legacy completion Jun 6, 2024
@alexgav alexgav marked this pull request as draft June 6, 2024 23:29
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Mostly looks good. Definitely on the right track.

src/razor/src/completion/completionHandler.ts Outdated Show resolved Hide resolved
src/razor/src/completion/completionHandler.ts Outdated Show resolved Hide resolved
@davidwengier
Copy link
Contributor

I don't see the change to opt the server into the new completion system. Would need a command line argument added to where rzls is called. Without that, I'm not sure this endpoint would get hit.

@alexgav
Copy link
Contributor Author

alexgav commented Jun 10, 2024

I don't see the change to opt the server into the new completion system. Would need a command line argument added to where rzls is called. Without that, I'm not sure this endpoint would get hit.

There is matching change in Razor repo here

    if (featureOptions.SingleServerCompletionSupport)

I haven't gotten that PR out yet. It basically removes the "if" check and the "else" clause. I am guessing there is more clean-up of unnecessary "Legacy" classes. I'll get that out shortly.

I confirmed that after that change the new endpoint in VSCode is getting hit and returned data processed. Is there something else needed?

@davidwengier
Copy link
Contributor

I haven't gotten that PR out yet. It basically removes the "if" check and the "else" clause

Thats cool, but it means we need a dual insertion for things to work. The alternative is to pass --SingleServerCompletionSupport false to the language server (here) and then this can be merged and will work with whatever Razor bits happen to be inserted, and once this is merged we're free to independently remove the old functionality because nothing is using it. That is all dependent on their being no other changes to the server needed for this to work.

- Using correct LSP types for Roslyn completion items per CR feedback
@alexgav
Copy link
Contributor Author

alexgav commented Jun 11, 2024

--SingleServerCompletionSupport false

I'm guessing you meant true since false seems to do the opposite of what we want? Confirmed that true makes server-side code changes unnecessary (or not immediately necessary).

…e need default item data copied to each item.
- We currently have to invoke provisional completion in Roslyn via vscode command vs Roslyn command
- Minor cleanup of function names, TODOs
- Fixing item kind translation from vscode to LSP (it's off by one)
…provisionally added '.' is removed after we get completions from Roslyn
@alexgav alexgav marked this pull request as ready for review June 20, 2024 07:52
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Tests would be wonderful...

src/razor/src/completion/completionHandler.ts Outdated Show resolved Hide resolved
src/razor/src/completion/completionHandler.ts Outdated Show resolved Hide resolved
src/razor/src/completion/completionHandler.ts Outdated Show resolved Hide resolved
src/razor/src/completion/completionHandler.ts Outdated Show resolved Hide resolved
src/razor/src/completion/completionHandler.ts Outdated Show resolved Hide resolved
@alexgav
Copy link
Contributor Author

alexgav commented Jun 21, 2024

Tests would be wonderful...

Agreed, I'd prefer to add in a follow up PR though to make sure this gets in - unless you feel strongly otherwise. Starting to work on them.

@davidwengier
Copy link
Contributor

Follow up is fine, not going to block.

@alexgav alexgav merged commit 98fa780 into main Jun 23, 2024
13 checks passed
@JoeRobich JoeRobich deleted the dev/alexgav/RemovingLegacyCompletion branch October 8, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants