Fix compareNodes silently treating unindexed source files as file index 0 - #64217
Fix compareNodes silently treating unindexed source files as file index 0#64217NAVEENKUMARKR777 wants to merge 1 commit into
Conversation
…ex 0 compareNodes looks up each node's source file in fileIndexMap, which only contains the program's own files. A plain map lookup on a file that isn't a key (a synthetic file from a custom host, or a content-mapper-produced supplemental file) returns Go's zero value for int, which is indistinguishable from a real file actually occupying index 0. That made an unindexed file compare as equal to whichever file holds index 0, and made any two different unindexed files compare as equal to each other, silently producing an arbitrary/incorrect order instead of a real one. Extract the file-index comparison into compareFileIndices, which tracks missing entries explicitly: indexed files are ordered by their real index, a file missing from the map sorts after every indexed file, and two missing files tiebreak by file name instead of collapsing to "equal". Full compiler baseline suite, checker, project, and fourslash tests all pass unchanged, since every file in a normal compilation is indexed; the new test exercises the previously-mishandled unindexed case directly and fails against the old lookup without the fix.
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
There was a problem hiding this comment.
🟡 Changes recommended
The regression tests do not verify the required ordering direction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes deterministic ordering for unindexed source files in the Go checker.
Changes:
- Adds explicit indexed/unindexed comparison with filename fallback.
- Adds regression tests for ordering behavior.
File summaries
| File | Description |
|---|---|
tsc/internal/checker/utilities.go |
Implements safe file-index comparison. |
tsc/internal/checker/comparenodes_test.go |
Tests unindexed-file comparisons. |
Review details
Suppressed comments (1)
tsc/internal/checker/comparenodes_test.go:48
- These assertions only prove that the result is nonzero and antisymmetric, so descending filename order would pass even though
compareFileIndicesis specified to usestrings.Compareordering. Assert the expected signs forunindexed-a.tsversusunindexed-b.tsto cover the filename tiebreak behavior.
assert.Assert(t, c1 != 0, "two different unindexed files must not compare equal")
assert.Equal(t, c1, -c2, "comparison must be antisymmetric")
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
|
Good catch — the old assertions only checked non-zero + antisymmetric, so a sign-flipped implementation would've slipped through. Pushed 360c29f asserting the exact documented direction (indexed sorts before unindexed; file-name order for the unindexed/unindexed tiebreak) instead, and confirmed it now fails against a deliberately sign-flipped variant of compareFileIndices, not just the original bug. |
Fixes #64218 (the Go-checker-specific slice of #64203).
compareNodeslooks up each node's source file infileIndexMap, which only contains the program's own files (internal/checker/checker.go,createFileIndexMap). A plain map lookup on a file that isn't a key there — a synthetic file from a custom host, or a content-mapper-produced supplemental file that was parsed outside the program — returns Go's zero value forint, which is indistinguishable from a real file actually occupying index 0:That silently makes an unindexed file compare as equal to whichever file holds index 0, and makes any two different unindexed files compare as equal to each other, rather than producing a real, deterministic order.
compareNodes/CompareTypesaren't just cosmetic here — they're used during real type inference (inference.go) as a tiebreaker, so a wrong comparison here can affect more than display order.As noted in #64203, this doesn't hang the Go checker the way it hangs the JS one (Go's zero-valued
intcan't produceNaNthe way JS'sundefined - undefineddoes), so this PR only addresses the Go-side defect — the silent-incorrect-ordering one, not the JS-side hang, which is out of scope here.Fix
Extracted the file-index comparison into
compareFileIndices, which tracks missing entries explicitly via the two-value map form:Testing
TestCompareFileIndicesUnindexedFile, which builds afileIndexMapwith one real file and two independently-parsed, never-indexed files, and asserts the specific documented direction of each previously-broken comparison (indexed sorts before unindexed; the unindexed/unindexed tiebreak follows file-name order) rather than just "not equal." Verified it fails against both the original lookup and a sign-flipped variant, and passes with the fix.go test ./...intsc/(compiler baselines, checker, project, fourslash, ls) passes unchanged — every file in a normal compilation is indexed, so this only changes behavior for the previously-mishandled unindexed case. The one pre-existing failure (internal/astnav, missingnode_modules/typescriptfor a Node-subprocess comparison) reproduces identically onmainwith no changes and is unrelated.