-
Notifications
You must be signed in to change notification settings - Fork 659
Fix invalid URI for untitled references/definitions #1344
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.
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
Open
Copilot
wants to merge
8
commits into
main
Choose a base branch
from
copilot/fix-1343
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
dfb8f98
Initial plan
Copilot a7f73df
Add test case to reproduce untitled file URI conversion issue
Copilot f18ed82
Fix untitled file URI handling by treating ^/ paths as rooted
Copilot 43a1c3e
Add integration test verifying complete untitled file URI flow
Copilot 0fd6461
Move untitled path check to before URL checks for performance
Copilot 7d3c7e7
Remove redundant untitled path check in NewSourceFile
Copilot d2d36de
Merge branch 'main' into copilot/fix-1343
DanielRosenwasser 1201772
Fix syntax error in NewSourceFile condition
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,156 @@ | ||
package ls_test | ||
|
||
import ( | ||
"strings" | ||
"testing" | ||
|
||
"github.com/microsoft/typescript-go/internal/bundled" | ||
"github.com/microsoft/typescript-go/internal/ls" | ||
"github.com/microsoft/typescript-go/internal/lsp/lsproto" | ||
"github.com/microsoft/typescript-go/internal/testutil/projecttestutil" | ||
"github.com/microsoft/typescript-go/internal/tspath" | ||
"gotest.tools/v3/assert" | ||
) | ||
|
||
func TestUntitledReferences(t *testing.T) { | ||
if !bundled.Embedded { | ||
t.Skip("bundled files are not embedded") | ||
} | ||
|
||
// First test the URI conversion functions to understand the issue | ||
untitledURI := lsproto.DocumentUri("untitled:Untitled-2") | ||
convertedFileName := ls.DocumentURIToFileName(untitledURI) | ||
t.Logf("URI '%s' converts to filename '%s'", untitledURI, convertedFileName) | ||
|
||
backToURI := ls.FileNameToDocumentURI(convertedFileName) | ||
t.Logf("Filename '%s' converts back to URI '%s'", convertedFileName, backToURI) | ||
|
||
if string(backToURI) != string(untitledURI) { | ||
t.Errorf("Round-trip conversion failed: '%s' -> '%s' -> '%s'", untitledURI, convertedFileName, backToURI) | ||
} | ||
|
||
// Create a test case that simulates how untitled files should work | ||
testContent := `let x = 42; | ||
|
||
x | ||
|
||
x++;` | ||
|
||
// Use the converted filename that DocumentURIToFileName would produce | ||
untitledFileName := convertedFileName // "^/untitled/ts-nul-authority/Untitled-2" | ||
t.Logf("Would use untitled filename: %s", untitledFileName) | ||
|
||
// Set up the file system with an untitled file - | ||
// But use a regular file first to see the current behavior | ||
files := map[string]any{ | ||
"/Untitled-2.ts": testContent, | ||
} | ||
|
||
ctx := projecttestutil.WithRequestID(t.Context()) | ||
service, done := createLanguageService(ctx, "/Untitled-2.ts", files) | ||
defer done() | ||
|
||
// Test the filename that the source file reports | ||
program := service.GetProgram() | ||
sourceFile := program.GetSourceFile("/Untitled-2.ts") | ||
t.Logf("SourceFile.FileName() returns: '%s'", sourceFile.FileName()) | ||
|
||
// Calculate position of 'x' on line 3 (zero-indexed line 2, character 0) | ||
position := 13 // After "let x = 42;\n\n" | ||
|
||
// Call ProvideReferences using the test method | ||
refs := service.TestProvideReferences("/Untitled-2.ts", position) | ||
|
||
// Log the results | ||
t.Logf("Input file name: %s", "/Untitled-2.ts") | ||
t.Logf("Number of references found: %d", len(refs)) | ||
for i, ref := range refs { | ||
t.Logf("Reference %d: URI=%s, Range=%+v", i+1, ref.Uri, ref.Range) | ||
} | ||
|
||
// We expect to find 3 references | ||
assert.Assert(t, len(refs) == 3, "Expected 3 references, got %d", len(refs)) | ||
|
||
// Also test definition using ProvideDefinition | ||
uri := ls.FileNameToDocumentURI("/Untitled-2.ts") | ||
lspPosition := lsproto.Position{Line: 2, Character: 0} | ||
definition, err := service.ProvideDefinition(t.Context(), uri, lspPosition) | ||
assert.NilError(t, err) | ||
if definition != nil && definition.Locations != nil { | ||
t.Logf("Definition found: %d locations", len(*definition.Locations)) | ||
for i, loc := range *definition.Locations { | ||
t.Logf("Definition %d: URI=%s, Range=%+v", i+1, loc.Uri, loc.Range) | ||
} | ||
} | ||
} | ||
|
||
func TestUntitledFileNameDebugging(t *testing.T) { | ||
if !bundled.Embedded { | ||
t.Skip("bundled files are not embedded") | ||
} | ||
|
||
// Test the URI conversion flow | ||
untitledURI := lsproto.DocumentUri("untitled:Untitled-2") | ||
convertedFileName := ls.DocumentURIToFileName(untitledURI) | ||
t.Logf("1. URI '%s' converts to filename '%s'", untitledURI, convertedFileName) | ||
|
||
// Test the path handling | ||
currentDir := "/home/daniel/TypeScript" | ||
path := tspath.ToPath(convertedFileName, currentDir, true) | ||
t.Logf("2. ToPath('%s', '%s') returns: '%s'", convertedFileName, currentDir, string(path)) | ||
|
||
// Verify the path is NOT resolved against current directory | ||
if strings.HasPrefix(string(path), currentDir) { | ||
t.Errorf("Path was incorrectly resolved against current directory: %s", string(path)) | ||
} | ||
|
||
// Test converting back to URI | ||
backToURI := ls.FileNameToDocumentURI(string(path)) | ||
t.Logf("3. Path '%s' converts back to URI '%s'", string(path), backToURI) | ||
|
||
if string(backToURI) != string(untitledURI) { | ||
t.Errorf("Round-trip conversion failed: '%s' -> '%s' -> '%s'", untitledURI, string(path), backToURI) | ||
} | ||
|
||
t.Logf("✅ Fix working: untitled paths are not resolved against current directory") | ||
} | ||
|
||
func TestUntitledFileIntegration(t *testing.T) { | ||
if !bundled.Embedded { | ||
t.Skip("bundled files are not embedded") | ||
} | ||
|
||
// This test simulates the exact scenario from the issue: | ||
// 1. VS Code sends untitled:Untitled-2 URI | ||
// 2. References/definitions should return untitled:Untitled-2 URIs, not file:// URIs | ||
|
||
// Simulate exactly what happens in the LSP flow | ||
originalURI := lsproto.DocumentUri("untitled:Untitled-2") | ||
|
||
// Step 1: URI gets converted to filename when file is opened | ||
fileName := ls.DocumentURIToFileName(originalURI) | ||
t.Logf("1. Opening file: URI '%s' -> fileName '%s'", originalURI, fileName) | ||
|
||
// Step 2: fileName gets processed through ToPath in project service | ||
currentDir := "/home/daniel/TypeScript" // Current directory from the original issue | ||
path := tspath.ToPath(fileName, currentDir, true) | ||
t.Logf("2. Project service processes: fileName '%s' -> path '%s'", fileName, string(path)) | ||
|
||
// Step 3: Verify path is NOT corrupted by current directory resolution | ||
if strings.HasPrefix(string(path), currentDir) { | ||
t.Fatalf("❌ BUG: Path was incorrectly resolved against current directory: %s", string(path)) | ||
} | ||
|
||
// Step 4: When references are found, the path gets converted back to URI | ||
resultURI := ls.FileNameToDocumentURI(string(path)) | ||
t.Logf("3. References return: path '%s' -> URI '%s'", string(path), resultURI) | ||
|
||
// Step 5: Verify the round-trip conversion works | ||
if string(resultURI) != string(originalURI) { | ||
t.Fatalf("❌ Round-trip failed: %s != %s", originalURI, resultURI) | ||
} | ||
|
||
t.Logf("✅ SUCCESS: Untitled file URIs are preserved correctly") | ||
t.Logf(" Original URI: %s", originalURI) | ||
t.Logf(" Final URI: %s", resultURI) | ||
} |
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
package tspath_test | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/microsoft/typescript-go/internal/tspath" | ||
"gotest.tools/v3/assert" | ||
) | ||
|
||
func TestUntitledPathHandling(t *testing.T) { | ||
// Test that untitled paths are treated as rooted | ||
untitledPath := "^/untitled/ts-nul-authority/Untitled-2" | ||
|
||
// GetEncodedRootLength should return 2 for "^/" | ||
rootLength := tspath.GetEncodedRootLength(untitledPath) | ||
assert.Equal(t, rootLength, 2, "GetEncodedRootLength should return 2 for untitled paths") | ||
|
||
// IsRootedDiskPath should return true | ||
isRooted := tspath.IsRootedDiskPath(untitledPath) | ||
assert.Assert(t, isRooted, "IsRootedDiskPath should return true for untitled paths") | ||
|
||
// ToPath should not resolve untitled paths against current directory | ||
currentDir := "/home/user/project" | ||
path := tspath.ToPath(untitledPath, currentDir, true) | ||
// The path should be the original untitled path | ||
assert.Equal(t, string(path), "^/untitled/ts-nul-authority/Untitled-2", "ToPath should not resolve untitled paths against current directory") | ||
|
||
// Test GetNormalizedAbsolutePath doesn't resolve untitled paths | ||
normalized := tspath.GetNormalizedAbsolutePath(untitledPath, currentDir) | ||
assert.Equal(t, normalized, "^/untitled/ts-nul-authority/Untitled-2", "GetNormalizedAbsolutePath should not resolve untitled paths") | ||
} | ||
|
||
func TestUntitledPathEdgeCases(t *testing.T) { | ||
// Test edge cases | ||
testCases := []struct { | ||
path string | ||
expected int | ||
isRooted bool | ||
}{ | ||
{"^/", 2, true}, // Minimal untitled path | ||
{"^/untitled/ts-nul-authority/test", 2, true}, // Normal untitled path | ||
{"^", 0, false}, // Just ^ is not rooted | ||
{"^x", 0, false}, // ^x is not untitled | ||
{"^^/", 0, false}, // ^^/ is not untitled | ||
{"x^/", 0, false}, // x^/ is not untitled (doesn't start with ^) | ||
{"^/untitled/ts-nul-authority/path/with/deeper/structure", 2, true}, // Deeper path | ||
} | ||
|
||
for _, tc := range testCases { | ||
t.Run(tc.path, func(t *testing.T) { | ||
rootLength := tspath.GetEncodedRootLength(tc.path) | ||
assert.Equal(t, rootLength, tc.expected, "GetEncodedRootLength for path %s", tc.path) | ||
|
||
isRooted := tspath.IsRootedDiskPath(tc.path) | ||
assert.Equal(t, isRooted, tc.isRooted, "IsRootedDiskPath for path %s", tc.path) | ||
}) | ||
} | ||
} |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is uncommon, so move it right before the URL checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the untitled path check to before the URL checks as requested. This improves performance by checking common cases (POSIX/DOS paths) first before the uncommon untitled paths. Fixed in 0fd6461.