Skip to content

Conversation

@alexgav
Copy link
Contributor

@alexgav alexgav commented Dec 3, 2024

We weren't always using correct serializer options when serializing and deserializing in cohosting. E.g. deserializing to VS LSP types should use VS LSP converters, and deserializing to Roslyn LSP types should use Roslyn LSP converters. Otherwise, things mostly work, but you can end up with base versions of the types instead of VSInternal subtypes.

I found this issue when I realized that VSInternalClientCapabilities wasn't getting deserialized quite right - top level was VSInternal version, but all fields (and their fields) were not properly deserialized to their VSInternal versions.

Also adding a couple of common helper methods to make serialization/deserializion code easier to keep consistent.

### Summary of the changes

-Common helpers for serialization and deserialization
-Make sure correct converters are used for both VL LSP and Roslyn LSP types

… deserializing in cohosting. Deserializing to VS LSP types should use VS LSP converters, and deserializing to Roslyn LSP types should use Roslyn LSP converters. Otherwise things mostly work, but you can end up with base versions of the types instead of VSInternal subtypes.
@alexgav alexgav requested a review from a team as a code owner December 3, 2024 20:37
Copy link
Member

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

Probably didn't strictly need to be two separate sets of converters, and conversion functions, but not worth worrying about. I genuinely hope to remove half of this code in the next few weeks :)

@alexgav alexgav merged commit 6651d16 into main Dec 3, 2024
12 checks passed
@alexgav alexgav deleted the dev/alexgav/FixSerializationInCohosting branch December 3, 2024 22:18
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Dec 3, 2024
@jjonescz jjonescz modified the milestones: Next, 17.13 P3 Mar 11, 2025
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.

5 participants