- Notifications
You must be signed in to change notification settings - Fork664
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
The issue was that untitled filenames (^/untitled/ts-nul-authority/...) were being treated as relative paths and resolved against the current working directory, corrupting their format.- Modified GetEncodedRootLength to recognize ^/ as a rooted path prefix- Added comprehensive tests for untitled path handling- Verified fix resolves the URI conversion round-trip issueCo-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
This test simulates the exact issue scenario:- VS Code sends untitled:Untitled-2 URI- Verifies the path is not corrupted by current directory resolution- Confirms round-trip URI conversion works correctly- Proves references/definitions will return correct untitled URIsCo-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
uri for untitled references/definitionsCopilot finished work on behalf ofDanielRosenwasserJuly 2, 2025 00:18
internal/tspath/path.go Outdated
Comment on lines 167 to 171
| // Untitled paths (e.g., "^/untitled/ts-nul-authority/Untitled-1") | ||
| if ch0 == '^' && ln > 1 && path[1] == '/' { | ||
| return 2 // Untitled: "^/" | ||
| } | ||
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 in0fd6461.
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Copilot finished work on behalf ofDanielRosenwasserJuly 2, 2025 00:50
There's a check for |
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Removed the redundant check as requested. Since |
Copilot finished work on behalf ofDanielRosenwasserJuly 2, 2025 02:01
internal/ast/ast.go Outdated
| @@ -10072,7 +10072,7 @@ type SourceFile struct { | |||
| } | |||
| func (f *NodeFactory) NewSourceFile(opts SourceFileParseOptions, text string, statements *NodeList, endOfFileToken *TokenNode) *Node { | |||
| if (tspath.GetEncodedRootLength(opts.FileName) == 0&& !strings.HasPrefix(opts.FileName, "^/"))|| opts.FileName != tspath.NormalizePath(opts.FileName) { | |||
| if (tspath.GetEncodedRootLength(opts.FileName) == 0 || opts.FileName != tspath.NormalizePath(opts.FileName) { | |||
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.
Suggested change
| if(tspath.GetEncodedRootLength(opts.FileName)==0||opts.FileName!=tspath.NormalizePath(opts.FileName) { | |
| iftspath.GetEncodedRootLength(opts.FileName)==0||opts.FileName!=tspath.NormalizePath(opts.FileName) { |
and then run tests
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.
Fixed the syntax error in the condition as suggested. Removed the extra parenthesis and fixed the missing closing parenthesis. Also fixed a related test type issue. Fixed in1201772.
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
This is not work to do, just noting my thoughts (do not take these as instructions). I applied this to Strada, and it failed: Yet, kinda in a good way: diff --git a/./tests/baselines/reference/tsserver/documentRegistry/works-when-reusing-orphan-script-info-with-different-scriptKind.js b/./tests/baselines/local/tsserver/documentRegistry/works-when-reusing-orphan-script-info-with-different-scriptKind.jsindex 215648946d..1e7a396fd5 100644--- a/./tests/baselines/reference/tsserver/documentRegistry/works-when-reusing-orphan-script-info-with-different-scriptKind.js+++ b/./tests/baselines/local/tsserver/documentRegistry/works-when-reusing-orphan-script-info-with-different-scriptKind.js@@ -35,12 +35,6 @@ Info seq [hh:mm:ss:mss] getConfigFileNameForFile:: File: ^/inmemory/model/6 Pro Info seq [hh:mm:ss:mss] Creating InferredProject: /dev/null/inferredProject1*, currentDirectory: /users/user/projects/san Info seq [hh:mm:ss:mss] Starting updateGraphWorker: Project: /dev/null/inferredProject1* Info seq [hh:mm:ss:mss] FileWatcher:: Added:: WatchInfo: /home/src/tslibs/TS/Lib/lib.d.ts 500 undefined WatchType: Closed Script info-Info seq [hh:mm:ss:mss] DirectoryWatcher:: Added:: WatchInfo: /users/user/projects/san/^ 1 undefined Project: /dev/null/inferredProject1* WatchType: Failed Lookup Locations-Info seq [hh:mm:ss:mss] Elapsed:: *ms DirectoryWatcher:: Added:: WatchInfo: /users/user/projects/san/^ 1 undefined Project: /dev/null/inferredProject1* WatchType: Failed Lookup Locations-Info seq [hh:mm:ss:mss] DirectoryWatcher:: Added:: WatchInfo: /users/user/projects/san/node_modules 1 undefined Project: /dev/null/inferredProject1* WatchType: Failed Lookup Locations-Info seq [hh:mm:ss:mss] Elapsed:: *ms DirectoryWatcher:: Added:: WatchInfo: /users/user/projects/san/node_modules 1 undefined Project: /dev/null/inferredProject1* WatchType: Failed Lookup Locations-Info seq [hh:mm:ss:mss] DirectoryWatcher:: Added:: WatchInfo: /users/user/projects/node_modules 1 undefined Project: /dev/null/inferredProject1* WatchType: Failed Lookup Locations-Info seq [hh:mm:ss:mss] Elapsed:: *ms DirectoryWatcher:: Added:: WatchInfo: /users/user/projects/node_modules 1 undefined Project: /dev/null/inferredProject1* WatchType: Failed Lookup Locations Info seq [hh:mm:ss:mss] DirectoryWatcher:: Added:: WatchInfo: /users/user/projects/san/node_modules/@types 1 undefined Project: /dev/null/inferredProject1* WatchType: Type roots Info seq [hh:mm:ss:mss] Elapsed:: *ms DirectoryWatcher:: Added:: WatchInfo: /users/user/projects/san/node_modules/@types 1 undefined Project: /dev/null/inferredProject1* WatchType: Type roots Info seq [hh:mm:ss:mss] DirectoryWatcher:: Added:: WatchInfo: /users/user/projects/node_modules/@types 1 undefined Project: /dev/null/inferredProject1* WatchType: Type roots@@ -79,14 +73,8 @@ Info seq [hh:mm:ss:mss] response: After request PolledWatches::-/users/user/projects/node_modules: *new*- {"pollingInterval":500} /users/user/projects/node_modules/@types: *new* {"pollingInterval":500}-/users/user/projects/san/^: *new*- {"pollingInterval":500}-/users/user/projects/san/node_modules: *new*- {"pollingInterval":500} /users/user/projects/san/node_modules/@types: *new* {"pollingInterval":500} I think this fix is reasonably correct. |
jakebailey approved these changesJul 7, 2025
andrewbranch approved these changesJul 8, 2025
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.
I came to the same conclusion in DMs with Daniel.
RyanCavanaugh requested changesJul 14, 2025
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.
Runnpx hereby lint and fix the errors
Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
Fixed all lint errors by adding |
Copilot finished work on behalf ofRyanCavanaughJuly 14, 2025 22:58
DanielRosenwasser approved these changesJul 15, 2025
jakebailey approved these changesJul 15, 2025
andrewbranch approved these changesJul 15, 2025
I think the blocker is@RyanCavanaugh’s review |
RyanCavanaugh approved these changesJul 15, 2025
Merged via the queue intomain with commitJul 15, 2025
eb83c1c 22 checks passed
Uh oh!
There was an error while loading.Please reload this page.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
None yet
5 participants
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.
Uh oh!
There was an error while loading.Please reload this page.
This PR addresses the issue where find references and go-to-definition operations on untitled files return incorrect URIs that cannot be linked back to the original location.
Root Cause Analysis
The issue was in the path handling logic in
internal/tspath/path.go. When VS Code sends requests with untitled URIs likeuntitled:Untitled-2, the following sequence occurs:DocumentURIToFileNamecorrectly convertsuntitled:Untitled-2to^/untitled/ts-nul-authority/Untitled-2GetEncodedRootLengthdid not recognize^/as a rooted path, returning 0ToPathtreated the untitled filename as relative and resolved it against current directory^/untitled/ts-nul-authority/Untitled-2into/home/user/project/^/untitled/ts-nul-authority/Untitled-2FileNameToDocumentURIwas called, it couldn't recognize the corrupted path as untitled and returned a regular file URISolution
Modified
GetEncodedRootLengthto recognize paths starting with^/as rooted paths (similar to how/andc:are handled). This prevents untitled filenames from being resolved against the current directory.Implementation
^/prefix inGetEncodedRootLengthfunction^/)Before Fix
After Fix
The integration test confirms the complete flow works correctly, simulating the exact scenario from the original issue where references and definitions on untitled files now return proper
untitled:URIs instead of brokenfile://URIs.Fixes#1343.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn moreCopilot coding agent tips in the docs.