Conversation
Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
@ as an argument@ CLI argument
| if fileName == "" { | ||
| p.errors = append(p.errors, ast.NewCompilerDiagnostic(diagnostics.Cannot_read_file_0, fileName)) | ||
| return | ||
| } |
There was a problem hiding this comment.
Why isn't this in tryReadFile?
There was a problem hiding this comment.
Moved the guard into tryReadFile in the latest commit.
|
@copilot listen to Jake |
Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
| // this function adds a compiler diagnostic if the file cannot be read | ||
| if fileName == "" { | ||
| errors = append(errors, ast.NewCompilerDiagnostic(diagnostics.Cannot_read_file_0, fileName)) | ||
| return "", errors | ||
| } | ||
| text, e := readFile(fileName) |
There was a problem hiding this comment.
ultimately I'm confused why readFile does not handle this, too? I each refactor makes me see that there's a problem further down the stack
There was a problem hiding this comment.
Moved the fix to Common.ReadFile in the VFS layer — it now returns ("", false) for non-absolute paths instead of panicking. Reverted the tryReadFile guard since it's no longer needed.
|
@copilot listen to Jake |
Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
| if tspath.GetEncodedRootLength(path) == 0 { | ||
| return "", false | ||
| } |
There was a problem hiding this comment.
at this point, surely this means RootAndPath should just return nil. I don't know if I like how deep this is going, though. it was sort of nice to know when you passed a bad path in
There was a problem hiding this comment.
Pull request overview
Fixes a crash in the CLI when a bare @ argument is provided by preventing a VFS-level panic and adding a regression test to ensure the parser reports a diagnostic instead.
Changes:
- Updated
vfs.Common.ReadFileto return("", false)for non-absolute paths rather than panicking. - Adjusted
iovfstests to expect graceful failure (no panic) on unrootedReadFilecalls. - Added a command-line parser regression test for
tsgo @to ensure an error is produced without panicking.
Show a summary per file
| File | Description |
|---|---|
| internal/vfs/iovfs/iofs_test.go | Updates the unrooted ReadFile test to assert graceful failure instead of a panic. |
| internal/vfs/internal/internal.go | Changes Common.ReadFile to avoid panicking on non-absolute paths and return ok=false. |
| internal/tsoptions/commandlineparser_test.go | Adds a regression test ensuring bare @ produces diagnostics rather than crashing. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 3
| func (vfs *Common) ReadFile(path string) (contents string, ok bool) { | ||
| if tspath.GetEncodedRootLength(path) == 0 { | ||
| return "", false | ||
| } | ||
| fsys, _, rest := vfs.RootAndPath(path) |
| t.Parallel() | ||
| // Regression test: `tsgo @` (bare @ with no filename) should not panic. | ||
| cmdLine := tsoptions.ParseCommandLine([]string{"@"}, tsoptionstest.NewVFSParseConfigHost(nil, "/", true)) | ||
| assert.Assert(t, len(cmdLine.Errors) > 0, "expected an error for empty response file name") |
| testutil.AssertPanics(t, func() { fs.ReadFile("bar") }, `vfs: path "bar" is not absolute`) | ||
| contents, ok := fs.ReadFile("bar") | ||
| assert.Assert(t, !ok) | ||
| assert.Assert(t, contents == "") |
Bug Fix
What was the bug?
tsgo @panics becauseparseResponseFilereceives an empty string after stripping the@prefix, andvfs.ReadFile("")asserts the path is absolute.How did you fix it?
Fixed
Common.ReadFilein the VFS layer to gracefully return("", false)for non-absolute paths instead of panicking. This is the correct level to handle the issue sinceReadFilealready has a boolean return to signal failure. The fix protects all callers of VFSReadFile, not just response file parsing.Testing
TestParseEmptyResponseFileregression test confirming no panic and that a diagnostic error is produced.ReadFileUnrootedtest iniovfsto expect graceful failure instead of a panic.