Fix fatal panic on non-absolute path in VFS - #4248
Fix fatal panic on non-absolute path in VFS#4248Daniel Rosenwasser (DanielRosenwasser) with Copilot wants to merge 3 commits into
Conversation
Instead of panicking when a non-absolute path reaches VFS operations,
RootLength now returns 0, SplitPath returns empty strings, and
RootAndPath returns nil fsys. This causes FileExists/DirectoryExists
to return false and ReadFile to return ("", false) for non-absolute paths,
matching TypeScript's behavior where sys.fileExists simply returns false.
Fixes #1291
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
|
Copilot you did not add a test |
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
|
Copilot can you investigate adding a real world example as a compiler or fourslash test that fails without any fix? |
I investigated compiler and fourslash coverage paths. Standard compiler tests normalize |
| p = tspath.NormalizePath(p) | ||
| l := RootLength(p) | ||
| if l == 0 { | ||
| return "", "" |
There was a problem hiding this comment.
Do we actually want to return "" for the root/rest in these cases?
There was a problem hiding this comment.
Seems weird to do that, but I think the callers check for empty rootName anyway?
Jake Bailey (jakebailey)
left a comment
Jake Bailey (jakebailey)
left a comment
There was a problem hiding this comment.
It pains me to remove this assert, because it has caught bugs, but, I don't feel strongly
There was a problem hiding this comment.
Pull request overview
Prevents VFS panics when module resolution supplies relative paths.
Changes:
- Makes VFS path splitting tolerate unrooted paths.
- Updates unrooted read behavior tests.
- Adds an empty-containing-file resolver regression test.
Show a summary per file
| File | Description |
|---|---|
internal/vfs/internal/internal.go |
Adds graceful handling for unrooted paths. |
internal/vfs/iovfs/iofs_test.go |
Expects failed reads instead of panics. |
internal/module/resolver_test.go |
Tests resolution with an empty containing file. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
| l := tspath.GetEncodedRootLength(p) | ||
| if l == 0 { | ||
| panic(fmt.Sprintf("vfs: path %q is not absolute", p)) | ||
| return 0 |
| if l == 0 { | ||
| return "", "" | ||
| } |
|
Can't fix this in time for the repo move. |
The VFS layer panics when a non-absolute path reaches
RootLength()via the module resolution pipeline. This can occur whencontainingDirectoryis empty, causingnormalizePathForCJSResolution("", "./foo")to produce a relative path that propagates throughFileExists→Stat→RootAndPath→SplitPath→RootLength.TypeScript's
sys.fileExistssimply returnsfalsefor such paths. The Go port's defensivepanicis too strict.Changes
RootLengthreturns0instead of panicking for non-absolute pathsSplitPathreturns("", "")when root length is zeroRootAndPathreturnsnilfsys when rootName is empty, causing downstreamFileExists/DirectoryExiststo returnfalseandReadFileto return("", false)false/empty return