Repository navigation
Fix crash in non-relative module completions for URLs with a query or fragment - #65341
Open
Ryan Cavanaugh (RyanCavanaugh) wants to merge 1 commit into
Open
Ryan Cavanaugh (RyanCavanaugh) wants to merge 1 commit into
Ryan Cavanaugh (RyanCavanaugh) wants to merge 1 commit into
Conversation
… fragment Completing a non-relative module specifier like "https://esm.sh/pkg?alias=react:preact/" (with "paths" configured) tried to resolve the URL fragment directory against each ancestor node_modules (and, for wildcard path mappings, against the paths base directory), which panics in tspath because a rooted path cannot contain a URL query or fragment. Such a path can never name a directory on disk, so skip those lookups, matching the 6.0 behavior of producing no file system completions. Part of microsoft#65068 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dec07792-6b96-4d6d-b2c8-c8a49abd0ce6
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are recorded for the URL module-specifier completion crash fix.
No open issues 🎉
What changed in this PR
Fixes a language-server completion crash for URL module specifiers containing queries or fragments when paths mappings are configured, addressing part of #65068.
Changes:
- Skips invalid URL directory lookups in non-relative module completions and wildcard path expansion, preserving rooted-path assertions.
- Adds fourslash regression tests for query and fragment URLs with both alias and wildcard mappings.
| File | Description |
|---|---|
tsc/internal/ls/string_completions.go |
Adds a URL validation helper and guards both affected directory lookups. |
tsc/internal/fourslash/tests/completionsPaths_nonRelativeUrlWithQuery_test.go |
Verifies empty completion results without crashes for both mapping scenarios. |
Review effort: Balanced
Jake Bailey (jakebailey)
approved these changes
Nov 5, 2026
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Part of #65068 (comment 3:
panic handling request textDocument/completion: path must not contain a URL query or fragmentfromgetCompletionEntriesForNonRelativeModules→getCompletionEntriesForDirectoryFragment→RootedDirectoryPath.ResolveDirectory)Analysis
Module specifiers that look like URLs are only routed to the "relative" completion path when
pathsis not set:So in a project with any
pathsmapping (e.g. the very common"@/*": ["./src/*"]) andbundler/node16/nodenextresolution, completing insidegoes through
getCompletionEntriesForNonRelativeModules. The fragment directory (https://esm.sh/foo?alias=react:preact/) is a URL whose query/fragment is part of the "directory". For each ancestor that has anode_modulesfolder,ancestorLookuppasses the fragment togetCompletionEntriesForDirectoryFragment, which callsnodeModules.ResolveDirectory(fragment). Because the fragment is itself rooted, resolution yields the URL unchanged, andtspath.ToRootedPathpanics since aRootedPathmust never contain a URL query or fragment.The same input with a wildcard mapping (
"*": ["./src/*"]) hits the same problem one step earlier, ingetModulesForPathsPattern:CombinePaths(prefixDirectory, fragmentDirectory)returns the rooted URL andpackageDirectory.ResolveDirectory(...)panics.In 6.0 (
src/services/stringCompletions.ts) these paths are just resolved to the URL string;tryDirectoryExists/tryReadDirectorythen find nothing, so no file-system completions are produced from them.Fix
Add a small
isURLWithQueryOrFragmenthelper (a URL path thattspath.TryRootedPathFromAbsoluterejects, i.e. exactly the condition the tspath assertion fires on) and use it to skip directory lookups that can never exist:getCompletionEntriesForNonRelativeModules, skip listingnode_modules/<fragment>when the fragment directory is such a URL. The package.jsonimports/exportslookups still run as before.getModulesForPathsPattern, return no matches when the expanded prefix directory is such a URL.This matches 6.0's observable behavior (no file completions from those directories; ambient-module and other completions are unaffected) and leaves the tspath assertion intact.
Alternatives considered: guarding inside
getCompletionEntriesForDirectoryFragmentwould also fix the reported stack, but that function is shared with relative-module completion, whose crash from #65068 is being fixed separately; keeping the guards in the non-relative callers avoids overlapping changes (if that fix lands, thenode_modulesguard here becomes redundant but harmless). Weakening the tspath assertion was ruled out.Tests (new file
tsc/internal/fourslash/tests/completionsPaths_nonRelativeUrlWithQuery_test.go):TestCompletionsPaths_nonRelativeUrlWithQueryreproduces the reported stack exactly (panicked before the fix).TestCompletionsPaths_nonRelativeUrlWithQueryWildcardPathMappingcovers thegetModulesForPathsPatternvariant. It usesrequire(...)in a.tsfile (not resolved during program construction) because, with a"*"mapping, animportof such a URL currently also panics in module resolution (tryLoadModuleUsingPaths→resolutionCandidateFromDirectoryPath), which is a separate crash outside the scope of this PR.Copilot Checklist
I successfully ran the applicable command at the end of my session, and it completed without error: