Sanitize non-finite range coordinates in Diagnostic.from#318520
Open
yogeshwaran-c wants to merge 1 commit into
Open
Sanitize non-finite range coordinates in Diagnostic.from#318520yogeshwaran-c wants to merge 1 commit into
yogeshwaran-c wants to merge 1 commit into
Conversation
Diagnostics whose range contains non-finite numbers (NaN, Infinity) currently flow through to IMarkerData unchanged. When marshalled across the RPC boundary, NaN is serialized as JSON null, which breaks LSP code action requests (the spec requires uinteger line/character values). Sanitize the converted range in Diagnostic.from: if any of the four coordinates is non-finite, replace the range with a known-good (1,1,1,1) default so downstream IMarkerData consumers and LSP clients receive a valid range. Fixes microsoft#177094
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds sanitization for Diagnostic range coordinates to prevent non-finite values (NaN/Infinity) from producing invalid JSON/LSP requests, and introduces regression tests to validate the converter behavior.
Changes:
- Sanitize converted diagnostic ranges when any coordinate is non-finite.
- Add unit tests covering well-formed ranges and NaN/Infinity edge cases for diagnostics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/workbench/api/common/extHostTypeConverters.ts | Sanitizes diagnostic range coordinates to avoid non-finite values propagating into marker DTOs. |
| src/vs/workbench/api/test/common/extHostTypeConverters.test.ts | Adds tests to ensure Diagnostic.from preserves valid ranges and sanitizes NaN/Infinity inputs. |
Comment on lines
+256
to
+273
| // Sanitize range to ensure all coordinates are finite numbers. Non-finite | ||
| // values (NaN, Infinity) get serialized to JSON null and produce invalid | ||
| // LSP code action requests, see https://github.com/microsoft/vscode/issues/177094. | ||
| const range = Range.from(value.range); | ||
| if (range && ( | ||
| !Number.isFinite(range.startLineNumber) | ||
| || !Number.isFinite(range.startColumn) | ||
| || !Number.isFinite(range.endLineNumber) | ||
| || !Number.isFinite(range.endColumn) | ||
| )) { | ||
| range.startLineNumber = 1; | ||
| range.startColumn = 1; | ||
| range.endLineNumber = 1; | ||
| range.endColumn = 1; | ||
| } | ||
|
|
||
| return { | ||
| ...Range.from(value.range), | ||
| ...range, |
Member
There was a problem hiding this comment.
I agree here. We should however make sure to not create a range with a end -> start direction. So if end is non-finite it should be > start for both line and character.
Comment on lines
+290
to
+300
| test('from sanitizes Infinity positions to a valid range', function () { | ||
| const diag = new Diagnostic( | ||
| new Range(new Position(0, 0), new Position(Infinity, 0)), | ||
| 'broken', | ||
| ); | ||
| const marker = DiagnosticConverter.from(diag); | ||
| assert.ok(Number.isFinite(marker.startLineNumber)); | ||
| assert.ok(Number.isFinite(marker.startColumn)); | ||
| assert.ok(Number.isFinite(marker.endLineNumber)); | ||
| assert.ok(Number.isFinite(marker.endColumn)); | ||
| }); |
dbaeumer
requested changes
May 27, 2026
Comment on lines
+256
to
+273
| // Sanitize range to ensure all coordinates are finite numbers. Non-finite | ||
| // values (NaN, Infinity) get serialized to JSON null and produce invalid | ||
| // LSP code action requests, see https://github.com/microsoft/vscode/issues/177094. | ||
| const range = Range.from(value.range); | ||
| if (range && ( | ||
| !Number.isFinite(range.startLineNumber) | ||
| || !Number.isFinite(range.startColumn) | ||
| || !Number.isFinite(range.endLineNumber) | ||
| || !Number.isFinite(range.endColumn) | ||
| )) { | ||
| range.startLineNumber = 1; | ||
| range.startColumn = 1; | ||
| range.endLineNumber = 1; | ||
| range.endColumn = 1; | ||
| } | ||
|
|
||
| return { | ||
| ...Range.from(value.range), | ||
| ...range, |
Member
There was a problem hiding this comment.
I agree here. We should however make sure to not create a range with a end -> start direction. So if end is non-finite it should be > start for both line and character.
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.
Fixes #177094
Problem
Diagnostics with non-finite range coordinates (
NaN,Infinity) are passed throughDiagnostic.fromintoIMarkerDataunchanged. When marshalled across the RPC boundary to the main thread (and onward to LSP language servers),NaNis serialized as JSONnull, which violates the LSP spec (uinteger required) and causes downstream language servers to fail when handling code action requests.The original report shows the malformed payload:
This is the case @jrieken called out in #177094 (comment): "diagnostics are created for ill-formed ranges (nan, null, -1) and our validation doesn't catch all of them ... anyone is invited to improve range validation."
Fix
After converting
value.rangeviaRange.from(...), check that all four coordinates are finite. If any aren't, replace the range with the known-good degenerate range(1,1)-(1,1). The diagnostic is preserved (extension authors still see their message in the Problems view) and downstream consumers + LSP clients receive a valid range.Scope is intentionally narrow: only the diagnostic conversion path.
Range.fromitself is unchanged, and other call sites (location, document symbol, etc.) are unaffected.Tests
Added a
Diagnosticsuite toextHostTypeConverters.test.tscovering:(1,1,1,1)Infinityis sanitized