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

Remove isFinalized semantic tokens logic #6194

Closed
wants to merge 1 commit into from

Conversation

allisonchou
Copy link
Contributor

Summary of the changes

  • This is unfortunate, but after talking with Cyrus, it turns out Roslyn doesn't have an accurate way to tell if frozen partial semantics have been finalized. Occasionally, this issue results in C# continually returning false for the isFinalized property, which in turn means we never cache tokens.

  • Currently, the only options here are to (1) remove frozen partial semantics or (2) don't use caching. This PR opts for (1), since frozen partial semantics are most valuable at document open, while caching should be valuable throughout the use the document. (1) also means that we don't have to ask the C# server for tokens every time the LSP server sends us a request.

  • I tested and besides for the initial colorization potentially seeming a bit more delayed, there don't seem to be other noticeable drawbacks.

  • There will also be a Roslyn side PR that will have to be dual inserted with this one: [to-do: add link]

  • Fixes:
    [LSP] Semantic tokens isFinalized property continually returning false on document open roslyn#59777

Copy link
Contributor

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

Seems good from our end. I'm curious about the Removal of the Frozen Semantic stuff, is that something that's just going to be turned off for the LSP or is it getting removed entirely?

@allisonchou
Copy link
Contributor Author

Seems good from our end. I'm curious about the Removal of the Frozen Semantic stuff, is that something that's just going to be turned off for the LSP or is it getting removed entirely?

Just turned off for Razor LSP since the isFinalized property only affects Razor in the first place

@allisonchou
Copy link
Contributor Author

Superseded by dotnet/roslyn#60484

@allisonchou allisonchou deleted the allichou/RemoveIsFinalizedLogic branch March 30, 2022 22:14
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