Reduce SumConverter exceptions by peeking at array element types#83708
Merged
ToddGrun merged 11 commits intoMay 18, 2026
Merged
Conversation
SumConverter previously used exception-based type probing for all SumType arms, attempting deserialization and catching failures. For array variants like SumType<string[], VSInternalCommitCharacter[]> (used by VSInternalCompletionItem.VsCommitCharacters and VSInternalCompletionList.CommitCharacters), this caused InvalidOperationException on every mismatched attempt. This change adds array element peeking: before attempting deserialization, it reads the first array element's token type to determine compatibility. A string token matches string[] but not VSInternalCommitCharacter[], and a StartObject token matches VSInternalCommitCharacter[] but not string[], eliminating exception-based fallback for these cases. Refactors IsTokenCompatibleWithType into a core Type overload so array element peeking can reuse the same compatibility logic. Adds SumConverterTypeFilteringTests that verify correct arm selection and assert zero first-chance exceptions during deserialization.
Contributor
Author
|
@kayle -- If this is taken on the roslyn side, I'd be interesting in doing something similar on the VS side. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the LSP SumConverter to reduce exception-driven “probe deserialization” when a SumType contains multiple array arms by peeking the first array element’s token type to pre-filter incompatible arms. It also adds unit tests intended to validate correct arm selection and to assert that deserialization no longer relies on internal exceptions for these cases.
Changes:
- Refactors
IsTokenCompatibleWithTypeto route through aType-based overload and adds array element peeking forJsonTokenType.StartArray. - Introduces
IsSerializedAsJsonPrimitiveTypeto support StartObject/array-element compatibility decisions. - Adds
SumConverterTypeFilteringTestscovering several array disambiguation scenarios and asserting no first-chance exceptions during deserialization.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/LanguageServer/Protocol/Protocol/Converters/SumConverter.cs | Adds array-element peeking and refactors token/type compatibility logic to avoid exception-based fallback. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.UnitTests/Serialization/SumConverterTypeFilteringTests.cs | Adds tests for array-arm selection and attempts to validate “no internal exceptions” during deserialization. |
Array element peeking incorrectly rejected arrays whose element type has a custom JsonConverter (e.g., SumType<string, int>[]). The peeking logic would evaluate the element's token type against IsTokenCompatibleWithType, which doesn't understand converter-driven types that accept multiple token kinds. This caused a "No sum type match" JsonException at runtime. Fix: skip peeking when the element type has a JsonConverterAttribute, falling back to the existing try/catch path. Also adds a [Collection] attribute to SumConverterTypeFilteringTests to prevent xUnit parallel execution from causing false positives in the FirstChanceException assertions, and fixes em dashes in comments.
davidwengier
approved these changes
May 14, 2026
Member
davidwengier
left a comment
davidwengier
left a comment
There was a problem hiding this comment.
Surprised the tests are in a Razor folder, but it's always nice to be invited to something :)
LGTM though
Move the JsonConverterAttribute check from IsArrayElementCompatible into IsTokenCompatibleWithType, where it protects all callers rather than just the array peeking path. Add a regression test for SumType element arrays (e.g., SumType<string, int>[]) that would fail without this guard. Move SumConverterTypeFilteringTests from the Razor test project to ProtocolUnitTests/Converters, colocating them with the product code they test. Fix license header and file encoding to match repo conventions.
Contributor
Author
Yeah, that was a weird place for those tests. Moved to a more appropriate location in commit 3. |
…rialization-exceptions
…ed type helpers
Add [CollectionDefinition("SumConverterTypeFiltering", DisableParallelization = true)] to ensure the FirstChanceException-based tests don't observe exceptions from unrelated tests running in parallel.
Extract IsBooleanType, IsStringLikeType, and IsNumericType helpers from the duplicated allowlists in IsTokenCompatibleWithType and IsSerializedAsJsonPrimitiveType. Each type category is now defined in exactly one place, eliminating the risk of the two methods drifting out of sync.
- Cache GetCustomAttribute<JsonConverterAttribute>() results in a ConcurrentDictionary so reflection runs at most once per Type. This matters on hot paths like completion items where thousands of SumType arms are checked per response. - Move SumTypeCache and HasCustomJsonConverterCache to the non-generic SumConverter class so they are shared across all SumConverter<T> instantiations instead of duplicated per generic type argument. - Scope the FirstChanceException handler in tests to the current thread ID, eliminating false failures from unrelated exceptions on other threads. This removes the need for DisableParallelization.
| /// </summary> | ||
| private static bool IsArrayElementCompatible(ref Utf8JsonReader reader, Type arrayType) | ||
| { | ||
| if (!arrayType.IsArray) |
Contributor
There was a problem hiding this comment.
does this represent a bug? (i'm fine with the code as it is, i'm just trying to figure out if this is needed, or just paranoia, or desirable).
Contributor
Author
There was a problem hiding this comment.
This is just desirable. Some other razor completion work I'm doing takes in completion items from html/C#/Razor and tries to do completion list promotion, but some of the items aren't compatible, and thus currently require completion level specification of the commit characters. This ends up meaning over 100 exceptions thrown during deserialization, and it slows completion down significantly.
Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com>
…peFilteringTests.cs Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com>
…rialization-exceptions
…tom converters Replace the single-pass token filtering loop with a two-pass approach: - Pass 1 tries only token-compatible arms, avoiding costly exception-based type probing. - Pass 2 retries any arms that were skipped by token filtering, as a fallback for types whose converters are registered at the property level or via JsonSerializerOptions rather than as type-level attributes. This makes the token filtering a best-effort optimization rather than a correctness gate. A wrong prediction from IsTokenCompatibleWithType can no longer cause deserialization failure — it just falls back to the old try/catch behavior for that arm. As a result, the HasCustomJsonConverterAttribute cache and reflection check are no longer needed and are removed. Extract TryDeserializeArm helper to deduplicate the try/catch logic shared by both passes. Add a test proving the fallback works for a type with an options-registered converter that the token filter doesn't know about.
…c types Add !type.IsArray to the StartObject case so array element types (e.g., int[] as the element type of int[][]) are correctly rejected when the peeked token is a JSON object. Without this, nested array SumType arms like int[][] would pass the StartObject filter and cause an unnecessary deserialization attempt that falls back to the try/catch path. Add decimal to IsNumericType since System.Text.Json serializes/deserializes it as a JSON number. Add test Deserialize_ObjectArray_SkipsNestedArrayTypeArm that verifies no internal exceptions are thrown when deserializing SumType<int[][], SimpleObject[]> from JSON containing objects.
dibarbet
approved these changes
May 18, 2026
This was referenced May 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SumConverter previously used exception-based type probing for all SumType arms, attempting deserialization and catching failures. For array variants like SumType<string[], VSInternalCommitCharacter[]> (used by VSInternalCompletionItem.VsCommitCharacters and VSInternalCompletionList.CommitCharacters), this caused InvalidOperationException on every mismatched attempt.
This change adds array element peeking: before attempting deserialization, it reads the first array element's token type to determine compatibility. A string token matches string[] but not VSInternalCommitCharacter[], and a StartObject token matches VSInternalCommitCharacter[] but not string[], eliminating exception-based fallback for these cases.
Refactors IsTokenCompatibleWithType into a core Type overload so array element peeking can reuse the same compatibility logic. Adds SumConverterTypeFilteringTests that verify correct arm selection and assert zero first-chance exceptions during deserialization.
Microsoft Reviewers: Open in CodeFlow