Skip to content

[LSP] Remove the majority of usages of client name.#60357

Merged
dibarbet merged 5 commits intodotnet:mainfrom
dibarbet:remove_client_name
Mar 29, 2022
Merged

[LSP] Remove the majority of usages of client name.#60357
dibarbet merged 5 commits intodotnet:mainfrom
dibarbet:remove_client_name

Conversation

@dibarbet
Copy link
Copy Markdown
Member

@dibarbet dibarbet commented Mar 24, 2022

There was previously a bug where both the liveshare c# server and razor c# server were asked for results in a liveshare session. It has since been resolved so I no longer see the need to carry around the majority of the client name complexity that we had.

Instead of filtering documents by client name, we just respond with what we have that matches the URI. It is up to the client to appropriately delegate to the correct server.

This at least reduces the complexity in my brain when I try and reason about how we map URIs -> documents.

The usages of client name that still remain are essentially only for diagnostic purposes. We use it to
a) prevent razor file diagnostics from going to the error list (razor always asks us for them)
b) always calculate razor file diagnostics regardless of if the file is 'open' or not (when asked by razor)
c) prevent razor file diagnostics from being returned in LSP workspace pull diagnostics from the C# server (again, razor will directly ask us for diagnostics if and when they need them).

Can't merge until https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1505437 is fixed so I can verify liveshare

@ghost ghost added the Area-IDE label Mar 24, 2022
@dibarbet dibarbet added the LSP issues related to the roslyn language server protocol implementation label Mar 24, 2022
…return results for what it was asked for while the platform is responsible for asking for the correct buffers based on client name.
@dibarbet dibarbet marked this pull request as ready for review March 24, 2022 20:06
@dibarbet dibarbet requested a review from a team as a code owner March 24, 2022 20:06
@NTaylorMullen
Copy link
Copy Markdown

Will this mean the AlwaysActive language server will get requests to Razor C# buffers if we don't specifically target the Razor C# LSP server?

using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Classification;
using Microsoft.CodeAnalysis.ExternalAccess.Razor.Api;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

needed?

@dibarbet
Copy link
Copy Markdown
Member Author

dibarbet commented Mar 24, 2022

Will this mean the AlwaysActive language server will get requests to Razor C# buffers if we don't specifically target the Razor C# LSP server?

@NTaylorMullen this doesn't change the behavior of what the client asks for - we still export our ILC with the client name and expect the buffer to have it. But on the server side now we assume that the client knows what it wants and answer the request without filtering by client name.

The client is already asking the razor server for razor C# and the normal C# to the C# server (we would be seeing a ton of errors in logs and telemetry if that were not the case). There did used to be bugs in liveshare where the client did request all servers for razor C#, but they have since been fixed.

@dibarbet
Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 4 pipeline(s).

@dibarbet
Copy link
Copy Markdown
Member Author

https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1505437 is merged, waiting on new build to test

@dibarbet
Copy link
Copy Markdown
Member Author

https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1505437 is merged, waiting on new build to test

verified, working as before

@dibarbet dibarbet requested a review from a team March 28, 2022 18:53
@dibarbet dibarbet merged commit 0ead73d into dotnet:main Mar 29, 2022
@ghost ghost added this to the Next milestone Mar 29, 2022
@dibarbet dibarbet deleted the remove_client_name branch March 29, 2022 00:21
@dibarbet dibarbet modified the milestones: Next, 17.3.P1 Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE LSP issues related to the roslyn language server protocol implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants