From f065cf6bd24223ff886d452d36cbf4bea5e512a5 Mon Sep 17 00:00:00 2001 From: Seth Raphael Date: Sun, 21 Dec 2025 13:53:52 -0800 Subject: [PATCH 1/2] Enhance getMappedLocation to validate end position in source maps, ensuring it remains within bounds and avoids inverted ranges. --- internal/ls/source_map.go | 12 +- internal/ls/source_map_test.go | 219 +++++++++++++++++++++++++++++++++ 2 files changed, 230 insertions(+), 1 deletion(-) create mode 100644 internal/ls/source_map_test.go diff --git a/internal/ls/source_map.go b/internal/ls/source_map.go index dee315cf45..b3643dfc5b 100644 --- a/internal/ls/source_map.go +++ b/internal/ls/source_map.go @@ -27,7 +27,17 @@ func (l *LanguageService) getMappedLocation(fileName string, fileRange core.Text } } debug.Assert(endPos.FileName == startPos.FileName, "start and end should be in same file") - newRange := core.NewTextRange(startPos.Pos, endPos.Pos) + + // Validate that the end position is valid (same file and after start position). + // This handles non-monotonic source map mappings where the next mapping entry + // might have an original position that's earlier in the source file. + // If end is before start, clamp to start position to avoid inverted ranges. + endPosValue := endPos.Pos + if endPos.FileName != startPos.FileName || endPos.Pos < startPos.Pos { + endPosValue = startPos.Pos + } + + newRange := core.NewTextRange(startPos.Pos, endPosValue) lspRange := l.createLspRangeFromRange(newRange, l.getScript(startPos.FileName)) return lsproto.Location{ Uri: lsconv.FileNameToDocumentURI(startPos.FileName), diff --git a/internal/ls/source_map_test.go b/internal/ls/source_map_test.go new file mode 100644 index 0000000000..9d1603c6d3 --- /dev/null +++ b/internal/ls/source_map_test.go @@ -0,0 +1,219 @@ +package ls + +import ( + "testing" + + "github.com/microsoft/typescript-go/internal/format" + "github.com/microsoft/typescript-go/internal/ls/lsconv" + "github.com/microsoft/typescript-go/internal/ls/lsutil" + "github.com/microsoft/typescript-go/internal/lsp/lsproto" + "github.com/microsoft/typescript-go/internal/sourcemap" + "gotest.tools/v3/assert" +) + +// mockHost is a minimal implementation of Host for testing +type mockHost struct { + files map[string]string +} + +func (m *mockHost) UseCaseSensitiveFileNames() bool { + return false +} + +func (m *mockHost) ReadFile(path string) (contents string, ok bool) { + contents, ok = m.files[path] + return +} + +func (m *mockHost) Converters() *lsconv.Converters { + return lsconv.NewConverters(lsproto.PositionEncodingKindUTF16, func(fileName string) *lsconv.LSPLineMap { + return nil + }) +} + +func (m *mockHost) UserPreferences() *lsutil.UserPreferences { + return lsutil.NewDefaultUserPreferences() +} + +func (m *mockHost) FormatOptions() *format.FormatCodeSettings { + return nil +} + +func (m *mockHost) GetECMALineInfo(fileName string) *sourcemap.ECMALineInfo { + return nil +} + +// mockDocumentPositionMapper simulates a source map with non-monotonic mappings +type mockDocumentPositionMapper struct { + // Maps from .d.ts position to source position + // In this test, we simulate non-monotonic mappings where + // position 100 in .d.ts maps to position 800 in source + // position 200 in .d.ts maps to position 200 in source (goes backwards!) + mappings map[int]int + sourceFile string +} + +func (m *mockDocumentPositionMapper) GetSourcePosition(pos *sourcemap.DocumentPosition) *sourcemap.DocumentPosition { + if mappedPos, ok := m.mappings[pos.Pos]; ok { + return &sourcemap.DocumentPosition{ + FileName: m.sourceFile, + Pos: mappedPos, + } + } + return nil +} + +func (m *mockDocumentPositionMapper) GetGeneratedPosition(pos *sourcemap.DocumentPosition) *sourcemap.DocumentPosition { + // Reverse lookup - not needed for this test + return nil +} + +// TestGetMappedLocation_NonMonotonicMappings tests that getMappedLocation +// correctly handles non-monotonic source map mappings by clamping inverted ranges. +// This test verifies the fix logic that prevents inverted ranges when source maps +// have non-monotonic mappings (where declaration order differs from source order). +func TestGetMappedLocation_NonMonotonicMappings(t *testing.T) { + t.Parallel() + + t.Run("inverted range should be clamped", func(t *testing.T) { + // This test verifies the fix logic: + // If endPos.Pos < startPos.Pos, endPosValue should be clamped to startPos.Pos + + startPos := &sourcemap.DocumentPosition{ + FileName: "/source/index.ts", + Pos: 800, // getAllTools position + } + + endPos := &sourcemap.DocumentPosition{ + FileName: "/source/index.ts", + Pos: 200, // getAvailableTools position (before start!) + } + + // Simulate the fix logic + endPosValue := endPos.Pos + if endPos.FileName != startPos.FileName || endPos.Pos < startPos.Pos { + endPosValue = startPos.Pos + } + + // Verify the range is clamped + assert.Equal(t, endPosValue, startPos.Pos, "endPos should be clamped to startPos when inverted") + assert.Equal(t, endPosValue, 800, "clamped value should be 800") + + // Verify the range is valid (not inverted) + assert.Assert(t, endPosValue >= startPos.Pos, "endPosValue should be >= startPos.Pos") + }) + + t.Run("valid range should not be clamped", func(t *testing.T) { + startPos := &sourcemap.DocumentPosition{ + FileName: "/source/index.ts", + Pos: 200, + } + + endPos := &sourcemap.DocumentPosition{ + FileName: "/source/index.ts", + Pos: 800, // After start - valid + } + + // Simulate the fix logic + endPosValue := endPos.Pos + if endPos.FileName != startPos.FileName || endPos.Pos < startPos.Pos { + endPosValue = startPos.Pos + } + + // Verify the range is not clamped + assert.Equal(t, endPosValue, endPos.Pos, "endPos should not be clamped when valid") + assert.Equal(t, endPosValue, 800, "value should remain 800") + }) + + t.Run("different files should be clamped", func(t *testing.T) { + startPos := &sourcemap.DocumentPosition{ + FileName: "/source/index.ts", + Pos: 200, + } + + endPos := &sourcemap.DocumentPosition{ + FileName: "/source/other.ts", // Different file + Pos: 800, + } + + // Simulate the fix logic + endPosValue := endPos.Pos + if endPos.FileName != startPos.FileName || endPos.Pos < startPos.Pos { + endPosValue = startPos.Pos + } + + // Verify the range is clamped due to different files + assert.Equal(t, endPosValue, startPos.Pos, "endPos should be clamped when files differ") + }) +} + +// TestGetMappedLocation_ZeroLengthRange tests that zero-length ranges +// (clamped ranges) are handled correctly. +func TestGetMappedLocation_ZeroLengthRange(t *testing.T) { + t.Parallel() + + t.Run("zero-length range is valid", func(t *testing.T) { + startPos := &sourcemap.DocumentPosition{ + FileName: "/source/index.ts", + Pos: 800, + } + + // Clamped end position (same as start) + endPosValue := startPos.Pos + + // Create a zero-length range + rangeLen := endPosValue - startPos.Pos + assert.Equal(t, rangeLen, 0, "clamped range should be zero-length") + assert.Assert(t, rangeLen >= 0, "range length should never be negative") + }) +} + +// TestValidateAndClampSpanLogic tests the core validation logic used in the fix. +// This directly tests the logic that prevents inverted spans. +func TestValidateAndClampSpanLogic(t *testing.T) { + t.Parallel() + + // This is the exact validation logic from getMappedLocation + validateAndClamp := func(startPos, endPos *sourcemap.DocumentPosition) int { + endPosValue := endPos.Pos + if endPos.FileName != startPos.FileName || endPos.Pos < startPos.Pos { + endPosValue = startPos.Pos + } + return endPosValue + } + + t.Run("inverted span is clamped", func(t *testing.T) { + start := &sourcemap.DocumentPosition{FileName: "/file.ts", Pos: 800} + end := &sourcemap.DocumentPosition{FileName: "/file.ts", Pos: 200} // Before start! + + clamped := validateAndClamp(start, end) + assert.Equal(t, clamped, 800, "inverted span should be clamped to start") + assert.Assert(t, clamped >= start.Pos, "clamped value must be >= start") + }) + + t.Run("valid span is not clamped", func(t *testing.T) { + start := &sourcemap.DocumentPosition{FileName: "/file.ts", Pos: 200} + end := &sourcemap.DocumentPosition{FileName: "/file.ts", Pos: 800} // After start + + clamped := validateAndClamp(start, end) + assert.Equal(t, clamped, 800, "valid span should not be clamped") + }) + + t.Run("different files are clamped", func(t *testing.T) { + start := &sourcemap.DocumentPosition{FileName: "/file1.ts", Pos: 200} + end := &sourcemap.DocumentPosition{FileName: "/file2.ts", Pos: 800} // Different file + + clamped := validateAndClamp(start, end) + assert.Equal(t, clamped, 200, "different files should be clamped to start") + }) + + t.Run("zero-length span is valid", func(t *testing.T) { + start := &sourcemap.DocumentPosition{FileName: "/file.ts", Pos: 800} + end := &sourcemap.DocumentPosition{FileName: "/file.ts", Pos: 800} // Same as start + + clamped := validateAndClamp(start, end) + assert.Equal(t, clamped, 800, "zero-length span should be valid") + assert.Equal(t, clamped-start.Pos, 0, "range length should be zero") + }) +} + From 239aa21141bea121bd27bb0f128f3be169173ac2 Mon Sep 17 00:00:00 2001 From: Seth Raphael Date: Wed, 7 Jan 2026 13:07:08 -0800 Subject: [PATCH 2/2] Add test for non-monotonic source map mappings in go-to-definition functionality This commit introduces a new test, TestDeclarationMapsNonMonotonicMappings, to verify that the go-to-definition feature correctly handles source maps with non-monotonic mappings. The test ensures that declarations appearing in a different order in the .d.ts file compared to the source file are accurately resolved, addressing potential issues with complex type inference and re-exports. --- .../tests/statedeclarationmaps_test.go | 44 ++++ internal/ls/source_map_test.go | 219 ------------------ ...ionMapsNonMonotonicMappings.baseline.jsonc | 21 ++ 3 files changed, 65 insertions(+), 219 deletions(-) delete mode 100644 internal/ls/source_map_test.go create mode 100644 testdata/baselines/reference/fourslash/goToDefinition/declarationMapsNonMonotonicMappings.baseline.jsonc diff --git a/internal/fourslash/tests/statedeclarationmaps_test.go b/internal/fourslash/tests/statedeclarationmaps_test.go index bad3019e82..0ee2f7774e 100644 --- a/internal/fourslash/tests/statedeclarationmaps_test.go +++ b/internal/fourslash/tests/statedeclarationmaps_test.go @@ -385,3 +385,47 @@ fn5(); }) } } + +// TestDeclarationMapsNonMonotonicMappings verifies that getMappedLocation clamps +// inverted ranges caused by non-monotonic source map mappings. +// +// The baseline comparison catches regressions: without the fix, the output shows +// inverted markers (e.g., "|]|>" appearing before "<|"), while with the fix, +// the ranges are clamped to valid zero-length ranges (e.g., "<||>"). +func TestDeclarationMapsNonMonotonicMappings(t *testing.T) { + t.Parallel() + defer testutil.RecoverAndFail(t, "Panic on fourslash test") + // The source map creates a non-monotonic mapping: + // - .d.ts line 0 col 24 ('b' identifier) -> source line 1, col 16 + // - .d.ts line 0 col 25 (right after 'b') -> source line 0, col 0 (EARLIER!) + // + // When looking up 'b' identifier [24, 25), start maps to ~byte 39, + // but end maps to byte 0, creating an inverted range. + // The fix in getMappedLocation clamps this to prevent negative ranges. + const content = ` +// @Filename: /src/index.ts +export function a() {} +export function b() {} +// @Filename: /src/indexdef.d.ts.map +{ + "version": 3, + "file": "indexdef.d.ts", + "sourceRoot": "", + "sources": ["index.ts"], + "names": [], + "mappings": "AACA,wBAAgB,CADhB;AAAA,wBAAgB" +} +// @Filename: /src/indexdef.d.ts +export declare function b(): void; +export declare function a(): void; +//# sourceMappingURL=indexdef.d.ts.map +// @Filename: /src/user.ts +import { a, b } from "./indexdef"; +/*1*/a(); +/*2*/b(); +// @Filename: /src/tsconfig.json +{}` + f, done := fourslash.NewFourslash(t, nil /*capabilities*/, content) + defer done() + f.VerifyBaselineGoToDefinition(t, true, "1", "2") +} diff --git a/internal/ls/source_map_test.go b/internal/ls/source_map_test.go deleted file mode 100644 index 9d1603c6d3..0000000000 --- a/internal/ls/source_map_test.go +++ /dev/null @@ -1,219 +0,0 @@ -package ls - -import ( - "testing" - - "github.com/microsoft/typescript-go/internal/format" - "github.com/microsoft/typescript-go/internal/ls/lsconv" - "github.com/microsoft/typescript-go/internal/ls/lsutil" - "github.com/microsoft/typescript-go/internal/lsp/lsproto" - "github.com/microsoft/typescript-go/internal/sourcemap" - "gotest.tools/v3/assert" -) - -// mockHost is a minimal implementation of Host for testing -type mockHost struct { - files map[string]string -} - -func (m *mockHost) UseCaseSensitiveFileNames() bool { - return false -} - -func (m *mockHost) ReadFile(path string) (contents string, ok bool) { - contents, ok = m.files[path] - return -} - -func (m *mockHost) Converters() *lsconv.Converters { - return lsconv.NewConverters(lsproto.PositionEncodingKindUTF16, func(fileName string) *lsconv.LSPLineMap { - return nil - }) -} - -func (m *mockHost) UserPreferences() *lsutil.UserPreferences { - return lsutil.NewDefaultUserPreferences() -} - -func (m *mockHost) FormatOptions() *format.FormatCodeSettings { - return nil -} - -func (m *mockHost) GetECMALineInfo(fileName string) *sourcemap.ECMALineInfo { - return nil -} - -// mockDocumentPositionMapper simulates a source map with non-monotonic mappings -type mockDocumentPositionMapper struct { - // Maps from .d.ts position to source position - // In this test, we simulate non-monotonic mappings where - // position 100 in .d.ts maps to position 800 in source - // position 200 in .d.ts maps to position 200 in source (goes backwards!) - mappings map[int]int - sourceFile string -} - -func (m *mockDocumentPositionMapper) GetSourcePosition(pos *sourcemap.DocumentPosition) *sourcemap.DocumentPosition { - if mappedPos, ok := m.mappings[pos.Pos]; ok { - return &sourcemap.DocumentPosition{ - FileName: m.sourceFile, - Pos: mappedPos, - } - } - return nil -} - -func (m *mockDocumentPositionMapper) GetGeneratedPosition(pos *sourcemap.DocumentPosition) *sourcemap.DocumentPosition { - // Reverse lookup - not needed for this test - return nil -} - -// TestGetMappedLocation_NonMonotonicMappings tests that getMappedLocation -// correctly handles non-monotonic source map mappings by clamping inverted ranges. -// This test verifies the fix logic that prevents inverted ranges when source maps -// have non-monotonic mappings (where declaration order differs from source order). -func TestGetMappedLocation_NonMonotonicMappings(t *testing.T) { - t.Parallel() - - t.Run("inverted range should be clamped", func(t *testing.T) { - // This test verifies the fix logic: - // If endPos.Pos < startPos.Pos, endPosValue should be clamped to startPos.Pos - - startPos := &sourcemap.DocumentPosition{ - FileName: "/source/index.ts", - Pos: 800, // getAllTools position - } - - endPos := &sourcemap.DocumentPosition{ - FileName: "/source/index.ts", - Pos: 200, // getAvailableTools position (before start!) - } - - // Simulate the fix logic - endPosValue := endPos.Pos - if endPos.FileName != startPos.FileName || endPos.Pos < startPos.Pos { - endPosValue = startPos.Pos - } - - // Verify the range is clamped - assert.Equal(t, endPosValue, startPos.Pos, "endPos should be clamped to startPos when inverted") - assert.Equal(t, endPosValue, 800, "clamped value should be 800") - - // Verify the range is valid (not inverted) - assert.Assert(t, endPosValue >= startPos.Pos, "endPosValue should be >= startPos.Pos") - }) - - t.Run("valid range should not be clamped", func(t *testing.T) { - startPos := &sourcemap.DocumentPosition{ - FileName: "/source/index.ts", - Pos: 200, - } - - endPos := &sourcemap.DocumentPosition{ - FileName: "/source/index.ts", - Pos: 800, // After start - valid - } - - // Simulate the fix logic - endPosValue := endPos.Pos - if endPos.FileName != startPos.FileName || endPos.Pos < startPos.Pos { - endPosValue = startPos.Pos - } - - // Verify the range is not clamped - assert.Equal(t, endPosValue, endPos.Pos, "endPos should not be clamped when valid") - assert.Equal(t, endPosValue, 800, "value should remain 800") - }) - - t.Run("different files should be clamped", func(t *testing.T) { - startPos := &sourcemap.DocumentPosition{ - FileName: "/source/index.ts", - Pos: 200, - } - - endPos := &sourcemap.DocumentPosition{ - FileName: "/source/other.ts", // Different file - Pos: 800, - } - - // Simulate the fix logic - endPosValue := endPos.Pos - if endPos.FileName != startPos.FileName || endPos.Pos < startPos.Pos { - endPosValue = startPos.Pos - } - - // Verify the range is clamped due to different files - assert.Equal(t, endPosValue, startPos.Pos, "endPos should be clamped when files differ") - }) -} - -// TestGetMappedLocation_ZeroLengthRange tests that zero-length ranges -// (clamped ranges) are handled correctly. -func TestGetMappedLocation_ZeroLengthRange(t *testing.T) { - t.Parallel() - - t.Run("zero-length range is valid", func(t *testing.T) { - startPos := &sourcemap.DocumentPosition{ - FileName: "/source/index.ts", - Pos: 800, - } - - // Clamped end position (same as start) - endPosValue := startPos.Pos - - // Create a zero-length range - rangeLen := endPosValue - startPos.Pos - assert.Equal(t, rangeLen, 0, "clamped range should be zero-length") - assert.Assert(t, rangeLen >= 0, "range length should never be negative") - }) -} - -// TestValidateAndClampSpanLogic tests the core validation logic used in the fix. -// This directly tests the logic that prevents inverted spans. -func TestValidateAndClampSpanLogic(t *testing.T) { - t.Parallel() - - // This is the exact validation logic from getMappedLocation - validateAndClamp := func(startPos, endPos *sourcemap.DocumentPosition) int { - endPosValue := endPos.Pos - if endPos.FileName != startPos.FileName || endPos.Pos < startPos.Pos { - endPosValue = startPos.Pos - } - return endPosValue - } - - t.Run("inverted span is clamped", func(t *testing.T) { - start := &sourcemap.DocumentPosition{FileName: "/file.ts", Pos: 800} - end := &sourcemap.DocumentPosition{FileName: "/file.ts", Pos: 200} // Before start! - - clamped := validateAndClamp(start, end) - assert.Equal(t, clamped, 800, "inverted span should be clamped to start") - assert.Assert(t, clamped >= start.Pos, "clamped value must be >= start") - }) - - t.Run("valid span is not clamped", func(t *testing.T) { - start := &sourcemap.DocumentPosition{FileName: "/file.ts", Pos: 200} - end := &sourcemap.DocumentPosition{FileName: "/file.ts", Pos: 800} // After start - - clamped := validateAndClamp(start, end) - assert.Equal(t, clamped, 800, "valid span should not be clamped") - }) - - t.Run("different files are clamped", func(t *testing.T) { - start := &sourcemap.DocumentPosition{FileName: "/file1.ts", Pos: 200} - end := &sourcemap.DocumentPosition{FileName: "/file2.ts", Pos: 800} // Different file - - clamped := validateAndClamp(start, end) - assert.Equal(t, clamped, 200, "different files should be clamped to start") - }) - - t.Run("zero-length span is valid", func(t *testing.T) { - start := &sourcemap.DocumentPosition{FileName: "/file.ts", Pos: 800} - end := &sourcemap.DocumentPosition{FileName: "/file.ts", Pos: 800} // Same as start - - clamped := validateAndClamp(start, end) - assert.Equal(t, clamped, 800, "zero-length span should be valid") - assert.Equal(t, clamped-start.Pos, 0, "range length should be zero") - }) -} - diff --git a/testdata/baselines/reference/fourslash/goToDefinition/declarationMapsNonMonotonicMappings.baseline.jsonc b/testdata/baselines/reference/fourslash/goToDefinition/declarationMapsNonMonotonicMappings.baseline.jsonc new file mode 100644 index 0000000000..4a7de84422 --- /dev/null +++ b/testdata/baselines/reference/fourslash/goToDefinition/declarationMapsNonMonotonicMappings.baseline.jsonc @@ -0,0 +1,21 @@ +// === goToDefinition === +// === /src/index.ts === +// <|export function [|a|]() {} +// export func|>tion b() {} + +// === /src/user.ts === +// import { a, b } from "./indexdef"; +// /*GOTO DEF*/[|a|](); +// b(); + + + +// === goToDefinition === +// === /src/index.ts === +// export function a() {} +// <||>export function [|{ contextId: 0 |}|]b() {} + +// === /src/user.ts === +// import { a, b } from "./indexdef"; +// a(); +// /*GOTO DEF*/[|b|](); \ No newline at end of file