Skip to content
New issue

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

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

Already on GitHub? # to your account

Cohosting semantic tokens follow up #10103

Closed
davidwengier opened this issue Mar 15, 2024 · 1 comment
Closed

Cohosting semantic tokens follow up #10103

davidwengier opened this issue Mar 15, 2024 · 1 comment
Assignees
Milestone

Comments

@davidwengier
Copy link
Member

davidwengier commented Mar 15, 2024

This is not an exhaustive TODO list for cohosting in general, but rather the items of follow up that were not done in #10097

  • Refresh queue
  • FilePathService
  • OOP initialization
  • UsePreciseCSharpRanges
    • Needs to be implemented in OOP, and can be removed from the CSharpSemanticTokensProvider API, since it was only put there thinking LanguageServerFeatureOptions wouldn't be available, but it is
    • Remove unnecessary parameter #10120
  • Telemetry
  • Logging
  • Tests
    • Are they needed? The service that does the work is the same. To follow a more Roslyn style, we would have the ISemanticTokensService do the work to switch OOP if it can, and do the work in-proc if it can't. In OOP, it then calls back into the same ISemanticTokensService, but this time there is no OOP client, and some different service impls, so it does the work in-proc. Will all of that, there would be no actual testing of OOP, just the service.

The only other things that are "todo" are doing something about IDocumentContextFactory, but that currently throws an exception, so will be fixed whenever the first usage of it is needed.

@davidwengier davidwengier self-assigned this Mar 15, 2024
@davidwengier davidwengier added this to the 17.10 P3 milestone Mar 15, 2024
davidwengier added a commit that referenced this issue Mar 20, 2024
One of the items in #10103

Part of #9519

Rather than move telemetry down into the service, I decided to leave it
in the endpoint, and C# specific service, so we can track separate
telemetry from existing LSP, to make it easier to report on. In theory
this is unnecessary because we already would know the status of A/B
flights in the telemetry data, but that doesn't allow for manually
switching the flag. If people feel strongly about not using different
properties, I'm fine to change. Perhaps all of our telemetry should
automatically log cohosting and fuse status as properties?
davidwengier added a commit that referenced this issue Mar 20, 2024
Turns out the code path is the same for OOP, whether precise ranges is
on or not :)

One of the items in #10103

Part of #9519
davidwengier added a commit that referenced this issue Mar 20, 2024
davidwengier added a commit that referenced this issue Mar 20, 2024
@davidwengier
Copy link
Member Author

Going to close this out. There are two items above without PRs next to them, and they are:

Testing: I'm not going to bother doing anything specifically for this, because everything that actually does any work in the semantic tokens system is already fully covered by existing tests. I have been looking at how Roslyn tackles OOP testing and I have some ideas about how to write future OOP services so that we don't have to rely on "trust me bro" to know the code is shared, and covered by tests. But in this case, trust me bro.

Semantic tokens refresh: I think the bit that I had to leave out when I refactored Roslyn, I'm pretty sure is a complete no-op, just with a 2 second delay, and that the existing refresh queue is working fine and doesn't need us to do anything for it. Having said that, the Roslyn side needs to change to allow us to move to dynamic registration, so I'm incorporating those changes in my Roslyn PR (dotnet/roslyn#72609) that will set us up for that move.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant