Prevent crash when computing emit output paths - #4900
Conversation
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
|
Copilot this really needs an integration/compiler test as well. |
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
| // @rootDir: /a/K.ts | ||
| // @outDir: /a/out | ||
| // @useCaseSensitiveFileNames: false |
There was a problem hiding this comment.
Copilot why do these need to differ by casing for this test case?
You need to explain the motivation of this test case, especially in this test file.
There was a problem hiding this comment.
The casing difference was unnecessary, so I removed it in a551473. The renamed test now uses identical source and rootDir paths and explains that the compiler-added trailing separator caused the old [8:7] slice panic. Commit 4465775 also clarifies that the file-valued rootDir is intentional malformed input.
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Prevents emit-path crashes by aligning path relocation with the reference implementation.
Changes:
- Delegate output-path computation to the shared worker.
- Add unit and compiler regression coverage.
- Add generated emit, type, and symbol baselines.
Show a summary per file
| File | Description |
|---|---|
internal/outputpaths/outputpaths.go |
Delegates path computation to the shared worker. |
internal/outputpaths/outputpaths_test.go |
Tests the reported trailing-separator case. |
testdata/tests/cases/compiler/emitOutputPathWithSourceEqualToRootDir.ts |
Adds compiler regression coverage. |
testdata/baselines/reference/compiler/emitOutputPathWithSourceEqualToRootDir.js |
Records emitted JavaScript. |
testdata/baselines/reference/compiler/emitOutputPathWithSourceEqualToRootDir.symbols |
Records symbol output. |
testdata/baselines/reference/compiler/emitOutputPathWithSourceEqualToRootDir.types |
Records inferred types. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| sourceFilePath = sourceFilePath[len(commonSourceDirectory):] | ||
| } | ||
| return tspath.CombinePaths(newDirPath, sourceFilePath) | ||
| return GetSourceFilePathInNewDirWorker(fileName, newDirPath, currentDirectory, commonSourceDirectory, useCaseSensitiveFileNames) |
There was a problem hiding this comment.
Copilot add a test with that example, ensure that it fails, and then fix up the worker appropriately.
Also investigate whether Strada/TypeScript 6.0 has the same issue and elaborate.
There was a problem hiding this comment.
Added the exact example as a unit test (TestGetSourceFilePathInNewDirCanonicalizationShrinksCommonDirectory), confirmed it panics with [13:9]/[11:9]-style slice bounds errors on the old code, and added an end-to-end compiler regression test (emitOutputPathWithCaseFoldingRootDir.ts) with @useCaseSensitiveFileNames: false that panics on the old worker and passes after the fix.
Fixed the worker to trim the common-directory prefix by rune count (via the canonicalized commonDir, which the prefix check already verified) instead of raw byte length, clamping to the end of the string — GetCanonicalFileName's case-folding preserves rune count even when it changes UTF-8 byte width (e.g. the Kelvin sign \u212A → k), so this can't panic.
Strada/TypeScript 6.0 doesn't hit this: getSourceFilePathInNewDirWorker uses sourceFilePath.substring(commonSourceDirectory.length), and JS's String.prototype.substring silently clamps out-of-range indices to the string's end rather than throwing, so the equivalent case there just produces an empty/short suffix instead of crashing.
| isSourceFileInCommonSourceDirectory := strings.HasPrefix(canonFile, commonDir) | ||
| if isSourceFileInCommonSourceDirectory { | ||
| sourceFilePath = sourceFilePath[len(commonSourceDirectory):] | ||
| // The reference implementation strips the prefix by slicing on |
There was a problem hiding this comment.
Delete this massive overcomplicated comment and replace with just
// Use `trimRunePrefix` instead of raw slice to support length mismatches caused by case-folding, eg, of the kelvin sign '\u212A' (where two runes case fold into one) in `commonDir`
Also:
Copilot Is there anywhere else in our codebase we're doing a basic slice on case-folded paths using non-case-folded indexes that has a similar error? This feels like a footgun that we didn't anticipate in a straight port that has to have cropped up in more places than just here, which makes me think we can't be just slicing strings based on the lengths of other strings-with-differing-case-fold-state willy-nilly.
There was a problem hiding this comment.
Replaced with tspath.TrimFilePathPrefix (see internal/tspath/path.go) — a shared helper that both outputpaths.go and other call sites now use, instead of inlining trimRunePrefix with a bespoke comment.
Yes — found two more occurrences of the exact same footgun (canonical-prefix check via GetCanonicalFileName, then slicing the raw path by len(rawPrefix)):
internal/ls/string_completions.go'stryRemoveDirectoryPrefix(used for typings-directory completions)internal/ls/file_rename.go'screatePathUpdater(used by rename-file refactoring, viatspath.StartsWithDirectory+path[len(oldPath):])
Both are ported verbatim from Strada's tryRemovePrefix/equivalent JS code, which relies on substring's clamping — so yes, this pattern shows up wherever we ported string-slicing logic that assumed JS's forgiving substring. Fixed both to use tspath.TrimFilePathPrefix in b12e1e2.
|
|
||
| // trimRunePrefix returns the suffix of s after skipping up to runeCount runes, | ||
| // clamping to the end of s if it has fewer runes than runeCount. | ||
| func trimRunePrefix(s string, runeCount int) string { |
There was a problem hiding this comment.
Copilot Is there really no golang builtin method for slicing a string by rune count like this? Or just, like, a case-insensitivity-respecting strings.CutPrefix that already handles this? Minimally, reusing existing helpers and operating at a slightly higher level of abstraction, I feel like it could be something like (in tspath):
func TrimFilePathPrefix(path string, prefix string, useCaseSensitiveFileNames bool) string {
// fast path, case-sensitive comparison
caseMatchedRes, caseMatchedFound := strings.CutPrefix(path, prefix)
if caseMatchedFound || useCaseSensitiveFileNames { return caseMatchedRes }
canonPrefix := GetCanonicalFileName(prefix, false)
res, found := strings.CutPrefix(GetCanonicalFileName(path, false), canonPrefix)
if found {
// `path` is non-canonical (potentially uppercased), and we found a case-differing match.
// Returning `res` would implicitly canonicalize the result, but we also can't just slice based on the length of `prefix` because of case-folding byte count differences (e.g. the Kelvin sign '\u212A' folds to the single-byte 'k')
prefixRuneLen := utf8.RuneCountInString(canonPrefix)
runeCount := 0
for idx, _ := range path { // leverage rune iteration to identify rune-based slice point
runeCount++
if prefixRuneLen == runeCount { return path[idx:] } // This is silly, why is there no stdlib method for slicing a string based on rune indices
}
return ""
}
return path
}There some reason that doesn't work?
There was a problem hiding this comment.
That design works and is basically what I implemented in 2d688cf — tspath.TrimFilePathPrefix(path, prefix, useCaseSensitiveFileNames) (string, bool), with the case-sensitive fast path via strings.CutPrefix and a rune-indexed trim for the case-insensitive path (I named the helper trimRuneCount instead of inlining the loop).
No stdlib rune-count slicing helper — strings/utf8 don't expose one; the manual DecodeRuneInString loop (or []rune(s)[n:], which is worse since it copies/allocates the whole string) is the idiomatic way to do this in Go.
Co-authored-by: weswigham <2932786+weswigham@users.noreply.github.com>
…e one code path Co-authored-by: weswigham <2932786+weswigham@users.noreply.github.com>
Co-authored-by: weswigham <2932786+weswigham@users.noreply.github.com>
|
Copilot can you find and add regression tests for the new sites you updated to use |
Co-authored-by: weswigham <2932786+weswigham@users.noreply.github.com>
Added in c0521eb: |
|
TypeScript Bot (@typescript-bot) perf test this |
|
Daniel Rosenwasser (@DanielRosenwasser) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
lspComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Jake Bailey (jakebailey)
left a comment
Jake Bailey (jakebailey)
left a comment
There was a problem hiding this comment.
I feel like this is fairly cursed and I wonder if we have this same problem elsewhere, but, I guess it's fine?
|
Wesley Wigham (@weswigham) Do you have any last comments on this? |
Probably! Copilot found two extras just in this PR - I don't see how it's not just a systemic problem with string slices not being the same (w.r.t. bounds checking) as the js substrings we ported from. Hopefully these are the easier-to-trigger ones, though. |
…go#4900) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com> Co-authored-by: weswigham <2932786+weswigham@users.noreply.github.com>
GetSourceFilePathInNewDircould panic when normalized path containment succeeded but the raw common-directory string was longer than the source path, causing an out-of-bounds slice.Output path handling
GetSourceFilePathInNewDirWorker, matching the TypeScript reference implementation’s canonical prefix behavior.Regression coverage
GetSourceFilePathInNewDir#4899