From 71ca5dbd725a85ec7cf9c2fcd350bfe52ce00501 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 1 Jul 2025 21:11:42 +0000 Subject: [PATCH 1/4] Initial plan From ae5870b55e3de262c98b026d8cc7cf1b50a05c09 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 1 Jul 2025 21:37:26 +0000 Subject: [PATCH 2/4] Fix JSDocLink panic in Node.Text() method Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com> --- internal/ast/ast.go | 6 +++ internal/ast/jsdoc_link_test.go | 36 +++++++++++++ .../reference/compiler/jsDocLinkHoverPanic.js | 54 +++++++++++++++++++ .../compiler/jsDocLinkHoverPanic.symbols | 38 +++++++++++++ .../compiler/jsDocLinkHoverPanic.types | 42 +++++++++++++++ .../cases/compiler/jsDocLinkHoverPanic.ts | 30 +++++++++++ 6 files changed, 206 insertions(+) create mode 100644 internal/ast/jsdoc_link_test.go create mode 100644 testdata/baselines/reference/compiler/jsDocLinkHoverPanic.js create mode 100644 testdata/baselines/reference/compiler/jsDocLinkHoverPanic.symbols create mode 100644 testdata/baselines/reference/compiler/jsDocLinkHoverPanic.types create mode 100644 testdata/tests/cases/compiler/jsDocLinkHoverPanic.ts diff --git a/internal/ast/ast.go b/internal/ast/ast.go index ab2f247e68..11c0fd53d3 100644 --- a/internal/ast/ast.go +++ b/internal/ast/ast.go @@ -309,6 +309,12 @@ func (n *Node) Text() string { return n.AsRegularExpressionLiteral().Text case KindJSDocText: return strings.Join(n.AsJSDocText().text, "") + case KindJSDocLink: + return strings.Join(n.AsJSDocLink().text, "") + case KindJSDocLinkCode: + return strings.Join(n.AsJSDocLinkCode().text, "") + case KindJSDocLinkPlain: + return strings.Join(n.AsJSDocLinkPlain().text, "") } panic(fmt.Sprintf("Unhandled case in Node.Text: %T", n.data)) } diff --git a/internal/ast/jsdoc_link_test.go b/internal/ast/jsdoc_link_test.go new file mode 100644 index 0000000000..63dc4a4ea3 --- /dev/null +++ b/internal/ast/jsdoc_link_test.go @@ -0,0 +1,36 @@ +// Test to verify JSDoc link processing doesn't panic + +package ast + +import ( + "testing" +) + +func TestJSDocLinkText(t *testing.T) { + factory := NewNodeFactory(NodeFactoryHooks{}) + + // Create JSDoc links with text + text := []string{"some", " text"} + + // Test JSDocLink + jsDocLink := factory.NewJSDocLink(nil, text) + result := jsDocLink.Text() + expected := "some text" + if result != expected { + t.Errorf("JSDocLink.Text() = %q, want %q", result, expected) + } + + // Test JSDocLinkCode + jsDocLinkCode := factory.NewJSDocLinkCode(nil, text) + result = jsDocLinkCode.Text() + if result != expected { + t.Errorf("JSDocLinkCode.Text() = %q, want %q", result, expected) + } + + // Test JSDocLinkPlain + jsDocLinkPlain := factory.NewJSDocLinkPlain(nil, text) + result = jsDocLinkPlain.Text() + if result != expected { + t.Errorf("JSDocLinkPlain.Text() = %q, want %q", result, expected) + } +} \ No newline at end of file diff --git a/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.js b/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.js new file mode 100644 index 0000000000..5728220f41 --- /dev/null +++ b/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.js @@ -0,0 +1,54 @@ +//// [tests/cases/compiler/jsDocLinkHoverPanic.ts] //// + +//// [jsDocLinkHoverPanic.ts] +/** + * A function with JSDoc links that should not cause hover to panic. + * See {@link someFunction} for more details. + * Also check {@linkcode anotherFunction}. + * And {@linkplain plainLinkFunction}. + */ +function someFunction(): string { + return "test"; +} + +/** + * Another function referenced in links. + */ +function anotherFunction(): number { + return 42; +} + +/** + * Plain link function. + */ +function plainLinkFunction(): boolean { + return true; +} + +// This should trigger hover functionality which may cause the panic +const result = someFunction(); + +//// [jsDocLinkHoverPanic.js] +/** + * A function with JSDoc links that should not cause hover to panic. + * See {@link someFunction} for more details. + * Also check {@linkcode anotherFunction}. + * And {@linkplain plainLinkFunction}. + */ +function someFunction() { + return "test"; +} +/** + * Another function referenced in links. + */ +function anotherFunction() { + return 42; +} +/** + * Plain link function. + */ +function plainLinkFunction() { + return true; +} +// This should trigger hover functionality which may cause the panic +const result = someFunction(); diff --git a/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.symbols b/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.symbols new file mode 100644 index 0000000000..e6f925a5c1 --- /dev/null +++ b/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.symbols @@ -0,0 +1,38 @@ +//// [tests/cases/compiler/jsDocLinkHoverPanic.ts] //// + +=== jsDocLinkHoverPanic.ts === +/** + * A function with JSDoc links that should not cause hover to panic. + * See {@link someFunction} for more details. + * Also check {@linkcode anotherFunction}. + * And {@linkplain plainLinkFunction}. + */ +function someFunction(): string { +>someFunction : Symbol(someFunction, Decl(jsDocLinkHoverPanic.ts, 0, 0)) + + return "test"; +} + +/** + * Another function referenced in links. + */ +function anotherFunction(): number { +>anotherFunction : Symbol(anotherFunction, Decl(jsDocLinkHoverPanic.ts, 8, 1)) + + return 42; +} + +/** + * Plain link function. + */ +function plainLinkFunction(): boolean { +>plainLinkFunction : Symbol(plainLinkFunction, Decl(jsDocLinkHoverPanic.ts, 15, 1)) + + return true; +} + +// This should trigger hover functionality which may cause the panic +const result = someFunction(); +>result : Symbol(result, Decl(jsDocLinkHoverPanic.ts, 25, 5)) +>someFunction : Symbol(someFunction, Decl(jsDocLinkHoverPanic.ts, 0, 0)) + diff --git a/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.types b/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.types new file mode 100644 index 0000000000..43c0748a9f --- /dev/null +++ b/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.types @@ -0,0 +1,42 @@ +//// [tests/cases/compiler/jsDocLinkHoverPanic.ts] //// + +=== jsDocLinkHoverPanic.ts === +/** + * A function with JSDoc links that should not cause hover to panic. + * See {@link someFunction} for more details. + * Also check {@linkcode anotherFunction}. + * And {@linkplain plainLinkFunction}. + */ +function someFunction(): string { +>someFunction : () => string + + return "test"; +>"test" : "test" +} + +/** + * Another function referenced in links. + */ +function anotherFunction(): number { +>anotherFunction : () => number + + return 42; +>42 : 42 +} + +/** + * Plain link function. + */ +function plainLinkFunction(): boolean { +>plainLinkFunction : () => boolean + + return true; +>true : true +} + +// This should trigger hover functionality which may cause the panic +const result = someFunction(); +>result : string +>someFunction() : string +>someFunction : () => string + diff --git a/testdata/tests/cases/compiler/jsDocLinkHoverPanic.ts b/testdata/tests/cases/compiler/jsDocLinkHoverPanic.ts new file mode 100644 index 0000000000..ab9295d82d --- /dev/null +++ b/testdata/tests/cases/compiler/jsDocLinkHoverPanic.ts @@ -0,0 +1,30 @@ +// @target: esnext +// @module: preserve +// @strict: true + +/** + * A function with JSDoc links that should not cause hover to panic. + * See {@link someFunction} for more details. + * Also check {@linkcode anotherFunction}. + * And {@linkplain plainLinkFunction}. + */ +function someFunction(): string { + return "test"; +} + +/** + * Another function referenced in links. + */ +function anotherFunction(): number { + return 42; +} + +/** + * Plain link function. + */ +function plainLinkFunction(): boolean { + return true; +} + +// This should trigger hover functionality which may cause the panic +const result = someFunction(); \ No newline at end of file From 0e79c6cb4832c0949ed9e1fb7e00e56f30423194 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 1 Jul 2025 21:57:13 +0000 Subject: [PATCH 3/4] Revert previous approach and implement proper hover test for JSDoc links Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com> --- internal/ast/jsdoc_link_test.go | 36 ------- internal/ls/hover.go | 22 +++++ internal/ls/hover_test.go | 97 +++++++++++++++++++ .../reference/compiler/jsDocLinkHoverPanic.js | 54 ----------- .../compiler/jsDocLinkHoverPanic.symbols | 38 -------- .../compiler/jsDocLinkHoverPanic.types | 42 -------- .../cases/compiler/jsDocLinkHoverPanic.ts | 30 ------ 7 files changed, 119 insertions(+), 200 deletions(-) delete mode 100644 internal/ast/jsdoc_link_test.go create mode 100644 internal/ls/hover_test.go delete mode 100644 testdata/baselines/reference/compiler/jsDocLinkHoverPanic.js delete mode 100644 testdata/baselines/reference/compiler/jsDocLinkHoverPanic.symbols delete mode 100644 testdata/baselines/reference/compiler/jsDocLinkHoverPanic.types delete mode 100644 testdata/tests/cases/compiler/jsDocLinkHoverPanic.ts diff --git a/internal/ast/jsdoc_link_test.go b/internal/ast/jsdoc_link_test.go deleted file mode 100644 index 63dc4a4ea3..0000000000 --- a/internal/ast/jsdoc_link_test.go +++ /dev/null @@ -1,36 +0,0 @@ -// Test to verify JSDoc link processing doesn't panic - -package ast - -import ( - "testing" -) - -func TestJSDocLinkText(t *testing.T) { - factory := NewNodeFactory(NodeFactoryHooks{}) - - // Create JSDoc links with text - text := []string{"some", " text"} - - // Test JSDocLink - jsDocLink := factory.NewJSDocLink(nil, text) - result := jsDocLink.Text() - expected := "some text" - if result != expected { - t.Errorf("JSDocLink.Text() = %q, want %q", result, expected) - } - - // Test JSDocLinkCode - jsDocLinkCode := factory.NewJSDocLinkCode(nil, text) - result = jsDocLinkCode.Text() - if result != expected { - t.Errorf("JSDocLinkCode.Text() = %q, want %q", result, expected) - } - - // Test JSDocLinkPlain - jsDocLinkPlain := factory.NewJSDocLinkPlain(nil, text) - result = jsDocLinkPlain.Text() - if result != expected { - t.Errorf("JSDocLinkPlain.Text() = %q, want %q", result, expected) - } -} \ No newline at end of file diff --git a/internal/ls/hover.go b/internal/ls/hover.go index f0fb670c3b..632d52262d 100644 --- a/internal/ls/hover.go +++ b/internal/ls/hover.go @@ -407,6 +407,28 @@ func writeComments(b *strings.Builder, comments []*ast.Node) { } } b.WriteString(text) + case ast.KindJSDocLinkCode: + name := comment.Name() + text := comment.AsJSDocLinkCode().Text() + if name != nil { + if text == "" { + writeEntityName(b, name) + } else { + writeEntityNameParts(b, name) + } + } + b.WriteString(text) + case ast.KindJSDocLinkPlain: + name := comment.Name() + text := comment.AsJSDocLinkPlain().Text() + if name != nil { + if text == "" { + writeEntityName(b, name) + } else { + writeEntityNameParts(b, name) + } + } + b.WriteString(text) } } } diff --git a/internal/ls/hover_test.go b/internal/ls/hover_test.go new file mode 100644 index 0000000000..d4057f9756 --- /dev/null +++ b/internal/ls/hover_test.go @@ -0,0 +1,97 @@ +package ls_test + +import ( + "context" + "testing" + + "github.com/microsoft/typescript-go/internal/bundled" + "github.com/microsoft/typescript-go/internal/core" + "github.com/microsoft/typescript-go/internal/fourslash" + "github.com/microsoft/typescript-go/internal/ls" + "github.com/microsoft/typescript-go/internal/lsp/lsproto" + "github.com/microsoft/typescript-go/internal/testutil/projecttestutil" + "gotest.tools/v3/assert" +) + +func TestHover(t *testing.T) { + t.Parallel() + if !bundled.Embedded { + // Without embedding, we'd need to read all of the lib files out from disk into the MapFS. + // Just skip this for now. + t.Skip("bundled files are not embedded") + } + + testCases := []struct { + title string + input string + expected map[string]*lsproto.Hover + }{ + { + title: "JSDocLinksPanic", + input: ` +// @filename: index.ts +/** + * A function with JSDoc links that previously caused panic + * {@link console.log} and {@linkcode Array.from} and {@linkplain Object.keys} + */ +function myFunction() { + return "test"; +} + +/*marker*/myFunction();`, + expected: map[string]*lsproto.Hover{ + "marker": { + Contents: lsproto.MarkupContentOrMarkedStringOrMarkedStrings{ + MarkupContent: &lsproto.MarkupContent{ + Kind: lsproto.MarkupKindMarkdown, + Value: "```tsx\nfunction myFunction(): string\n```\nA function with JSDoc links that previously caused panic\n`console.log` and `Array.from` and `Object.keys`\n", + }, + }, + }, + }, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.title, func(t *testing.T) { + t.Parallel() + runHoverTest(t, testCase.input, testCase.expected) + }) + } +} + +func runHoverTest(t *testing.T, input string, expected map[string]*lsproto.Hover) { + testData := fourslash.ParseTestData(t, input, "/mainFile.ts") + file := testData.Files[0].FileName() + markerPositions := testData.MarkerPositions + ctx := projecttestutil.WithRequestID(t.Context()) + languageService, done := createLanguageServiceForHover(ctx, file, map[string]any{ + file: testData.Files[0].Content, + }) + defer done() + + for markerName, expectedResult := range expected { + marker, ok := markerPositions[markerName] + if !ok { + t.Fatalf("No marker found for '%s'", markerName) + } + result, err := languageService.ProvideHover( + ctx, + ls.FileNameToDocumentURI(file), + marker.LSPosition) + assert.NilError(t, err) + if expectedResult == nil { + assert.Assert(t, result == nil) + } else { + assert.Assert(t, result != nil) + assert.DeepEqual(t, *result, *expectedResult) + } + } +} + +func createLanguageServiceForHover(ctx context.Context, fileName string, files map[string]any) (*ls.LanguageService, func()) { + projectService, _ := projecttestutil.Setup(files, nil) + projectService.OpenFile(fileName, files[fileName].(string), core.GetScriptKindFromFileName(fileName), "") + project := projectService.Projects()[0] + return project.GetLanguageServiceForRequest(ctx) +} diff --git a/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.js b/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.js deleted file mode 100644 index 5728220f41..0000000000 --- a/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.js +++ /dev/null @@ -1,54 +0,0 @@ -//// [tests/cases/compiler/jsDocLinkHoverPanic.ts] //// - -//// [jsDocLinkHoverPanic.ts] -/** - * A function with JSDoc links that should not cause hover to panic. - * See {@link someFunction} for more details. - * Also check {@linkcode anotherFunction}. - * And {@linkplain plainLinkFunction}. - */ -function someFunction(): string { - return "test"; -} - -/** - * Another function referenced in links. - */ -function anotherFunction(): number { - return 42; -} - -/** - * Plain link function. - */ -function plainLinkFunction(): boolean { - return true; -} - -// This should trigger hover functionality which may cause the panic -const result = someFunction(); - -//// [jsDocLinkHoverPanic.js] -/** - * A function with JSDoc links that should not cause hover to panic. - * See {@link someFunction} for more details. - * Also check {@linkcode anotherFunction}. - * And {@linkplain plainLinkFunction}. - */ -function someFunction() { - return "test"; -} -/** - * Another function referenced in links. - */ -function anotherFunction() { - return 42; -} -/** - * Plain link function. - */ -function plainLinkFunction() { - return true; -} -// This should trigger hover functionality which may cause the panic -const result = someFunction(); diff --git a/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.symbols b/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.symbols deleted file mode 100644 index e6f925a5c1..0000000000 --- a/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.symbols +++ /dev/null @@ -1,38 +0,0 @@ -//// [tests/cases/compiler/jsDocLinkHoverPanic.ts] //// - -=== jsDocLinkHoverPanic.ts === -/** - * A function with JSDoc links that should not cause hover to panic. - * See {@link someFunction} for more details. - * Also check {@linkcode anotherFunction}. - * And {@linkplain plainLinkFunction}. - */ -function someFunction(): string { ->someFunction : Symbol(someFunction, Decl(jsDocLinkHoverPanic.ts, 0, 0)) - - return "test"; -} - -/** - * Another function referenced in links. - */ -function anotherFunction(): number { ->anotherFunction : Symbol(anotherFunction, Decl(jsDocLinkHoverPanic.ts, 8, 1)) - - return 42; -} - -/** - * Plain link function. - */ -function plainLinkFunction(): boolean { ->plainLinkFunction : Symbol(plainLinkFunction, Decl(jsDocLinkHoverPanic.ts, 15, 1)) - - return true; -} - -// This should trigger hover functionality which may cause the panic -const result = someFunction(); ->result : Symbol(result, Decl(jsDocLinkHoverPanic.ts, 25, 5)) ->someFunction : Symbol(someFunction, Decl(jsDocLinkHoverPanic.ts, 0, 0)) - diff --git a/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.types b/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.types deleted file mode 100644 index 43c0748a9f..0000000000 --- a/testdata/baselines/reference/compiler/jsDocLinkHoverPanic.types +++ /dev/null @@ -1,42 +0,0 @@ -//// [tests/cases/compiler/jsDocLinkHoverPanic.ts] //// - -=== jsDocLinkHoverPanic.ts === -/** - * A function with JSDoc links that should not cause hover to panic. - * See {@link someFunction} for more details. - * Also check {@linkcode anotherFunction}. - * And {@linkplain plainLinkFunction}. - */ -function someFunction(): string { ->someFunction : () => string - - return "test"; ->"test" : "test" -} - -/** - * Another function referenced in links. - */ -function anotherFunction(): number { ->anotherFunction : () => number - - return 42; ->42 : 42 -} - -/** - * Plain link function. - */ -function plainLinkFunction(): boolean { ->plainLinkFunction : () => boolean - - return true; ->true : true -} - -// This should trigger hover functionality which may cause the panic -const result = someFunction(); ->result : string ->someFunction() : string ->someFunction : () => string - diff --git a/testdata/tests/cases/compiler/jsDocLinkHoverPanic.ts b/testdata/tests/cases/compiler/jsDocLinkHoverPanic.ts deleted file mode 100644 index ab9295d82d..0000000000 --- a/testdata/tests/cases/compiler/jsDocLinkHoverPanic.ts +++ /dev/null @@ -1,30 +0,0 @@ -// @target: esnext -// @module: preserve -// @strict: true - -/** - * A function with JSDoc links that should not cause hover to panic. - * See {@link someFunction} for more details. - * Also check {@linkcode anotherFunction}. - * And {@linkplain plainLinkFunction}. - */ -function someFunction(): string { - return "test"; -} - -/** - * Another function referenced in links. - */ -function anotherFunction(): number { - return 42; -} - -/** - * Plain link function. - */ -function plainLinkFunction(): boolean { - return true; -} - -// This should trigger hover functionality which may cause the panic -const result = someFunction(); \ No newline at end of file From 6fb8ecadf11cc6b3d89d02d78204e2ac9c635bb0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 1 Jul 2025 22:11:10 +0000 Subject: [PATCH 4/4] Add TODO comments indicating temporary placeholder implementation for JSDoc links Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com> --- internal/ls/hover.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/ls/hover.go b/internal/ls/hover.go index 632d52262d..5720b920eb 100644 --- a/internal/ls/hover.go +++ b/internal/ls/hover.go @@ -408,6 +408,7 @@ func writeComments(b *strings.Builder, comments []*ast.Node) { } b.WriteString(text) case ast.KindJSDocLinkCode: + // !!! TODO: This is a temporary placeholder implementation that needs to be updated later name := comment.Name() text := comment.AsJSDocLinkCode().Text() if name != nil { @@ -419,6 +420,7 @@ func writeComments(b *strings.Builder, comments []*ast.Node) { } b.WriteString(text) case ast.KindJSDocLinkPlain: + // !!! TODO: This is a temporary placeholder implementation that needs to be updated later name := comment.Name() text := comment.AsJSDocLinkPlain().Text() if name != nil {