Skip to content

Clamp received lsproto.Positions #2966

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.

Sign up for GitHub

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

Closed
Andarist wants to merge 1 commit into from
+54 −5
Closed

Clamp received lsproto.Positions #2966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions internal/ls/lsconv/converters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package lsconv

import (
"context"
"fmt"
"net/url"
"slices"
"strings"
Expand Down Expand Up @@ -149,13 +148,16 @@ func (c *Converters) LineAndCharacterToPosition(script Script, lineAndCharacter
line := core.TextPos(lineAndCharacter.Line)
char := core.TextPos(lineAndCharacter.Character)

if line < 0 || int(line) >= len(lineMap.LineStarts) {
panic(fmt.Sprintf("bad line number. Line: %d, lineMap length: %d", line, len(lineMap.LineStarts)))
Copy link
Copy Markdown
Contributor Author

@Andarist Andarist Mar 3, 2026

Choose a reason for hiding this comment

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

A rogue client can always send some invalid position. It's definitely a tradeoff and we could return an error instead but clamping to the bounds seems somewhat benign to me here.

if line < 0 {
line = 0
} else if int(line) >= len(lineMap.LineStarts) {
line = core.TextPos(len(lineMap.LineStarts) - 1)
}

textLen := core.TextPos(len(script.Text()))
start := lineMap.LineStarts[line]
if lineMap.AsciiOnly || c.positionEncoding == lsproto.PositionEncodingKindUTF8 {
return start + char
return min(start+char, textLen)
}

var utf8Char core.TextPos
Expand All @@ -170,7 +172,7 @@ func (c *Converters) LineAndCharacterToPosition(script Script, lineAndCharacter
utf8Char = core.TextPos(i + utf8.RuneLen(r))
}

return start + utf8Char
return min(start+utf8Char, textLen)
}

func (c *Converters) PositionToLineAndCharacter(script Script, position core.TextPos) lsproto.Position {
Expand Down
47 changes: 47 additions & 0 deletions internal/project/session_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/microsoft/typescript-go/internal/ls/lsutil"
"github.com/microsoft/typescript-go/internal/lsp/lsproto"
"github.com/microsoft/typescript-go/internal/project"
"github.com/microsoft/typescript-go/internal/testutil"
"github.com/microsoft/typescript-go/internal/testutil/projecttestutil"
"github.com/microsoft/typescript-go/internal/tspath"
"gotest.tools/v3/assert"
Expand Down Expand Up @@ -952,4 +953,50 @@ func TestSession(t *testing.T) {
assert.DeepEqual(t, *actualConfig2.TS(), *expectedPrefs1)
assert.DeepEqual(t, *actualConfig2.JS(), *expectedPrefs2)
})

t.Run("definition with stale position after file shrink does not panic", func(t *testing.T) {
Copy link
Copy Markdown
Member

@jakebailey jakebailey Mar 3, 2026

Choose a reason for hiding this comment

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

Is this not an entirely illegal situation? The client can't give us positions that our out of range for its own view...

Copy link
Copy Markdown
Contributor Author

@Andarist Andarist Mar 5, 2026

Choose a reason for hiding this comment

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

Fair, I mentioned this is is a tradeoff here: #2966 (comment) . I think that in the long run this should return an explicit error (instead panicing) if the drift like this is not acceptable (which is a fair stance!).

As to the "proper" solution. I suspect this will require "snapshot freezing" like the one in #2905 . Once that PR settles - I'll take another look at this specific crash here again.

t.Parallel()
defer testutil.RecoverAndFail(t, "Panic on definition with stale position")
files := map[string]any{
"/home/projects/TS/p1/tsconfig.json": `{"compilerOptions": {"noLib": true}}`,
"/home/projects/TS/p1/src/index.ts": "0\n1\n",
}
session, _ := projecttestutil.Setup(files)
session.DidOpenFile(context.Background(), "file:///home/projects/TS/p1/src/index.ts", 1, "0\n1\n", lsproto.LanguageKindTypeScript)

session.DidChangeFile(context.Background(), "file:///home/projects/TS/p1/src/index.ts", 2, []lsproto.TextDocumentContentChangePartialOrWholeDocument{
{
Partial: &lsproto.TextDocumentContentChangePartial{
Range: lsproto.Range{
Start: lsproto.Position{Line: 0, Character: 0},
End: lsproto.Position{Line: 2, Character: 0},
},
Text: "0\n",
},
},
})

ls, err := session.GetLanguageService(context.Background(), "file:///home/projects/TS/p1/src/index.ts")
assert.NilError(t, err)

_, err = ls.ProvideDefinition(context.Background(), "file:///home/projects/TS/p1/src/index.ts", lsproto.Position{Line: 2, Character: 0})
assert.NilError(t, err)
})

t.Run("definition with out-of-bounds position does not panic", func(t *testing.T) {
t.Parallel()
defer testutil.RecoverAndFail(t, "Panic on definition with out-of-bounds position")
files := map[string]any{
"/home/projects/TS/p1/tsconfig.json": `{"compilerOptions": {"noLib": true}}`,
"/home/projects/TS/p1/src/index.ts": "let x = 1;",
}
session, _ := projecttestutil.Setup(files)
session.DidOpenFile(context.Background(), "file:///home/projects/TS/p1/src/index.ts", 1, "let x = 1;", lsproto.LanguageKindTypeScript)

ls, err := session.GetLanguageService(context.Background(), "file:///home/projects/TS/p1/src/index.ts")
assert.NilError(t, err)

_, err = ls.ProvideDefinition(context.Background(), "file:///home/projects/TS/p1/src/index.ts", lsproto.Position{Line: 999, Character: 0})
assert.NilError(t, err)
})
}