[pull] canary from vercel:canary - #1332
Merged
Merged
Conversation
### What? Adds unit test coverage for `FileSystemPath::hash_file`, the helper introduced in #97507. It pins the contract per filesystem entry type: a regular file is hashed by its content, a symlink by its *own* link content (target and whether it is absolute or relative), and a directory or a path that doesn't exist can't be hashed at all. Tests only — no production code is touched. ### Why? `hash_file` exists because hashing a traced path through `read()` was wrong: `read()` follows a symlink, so a link pointing at a directory made the build fail outright with `reading file "…" Is a directory (os error 21)`, and a link pointing at a file silently produced the hash of the target's bytes. The latter matters because consumers of the trace re-create a symlink entry *as a symlink* (`copyTracedFiles` does `readlink` + `symlink`) rather than copying the resolved file, so the link — not the file behind it — is what actually gets written out and therefore what the hash has to describe. Nothing guarded that. The distinction is invisible at the call site (`hash_file` reads like an ordinary "hash this path" helper) and "just read the file and hash it" is a natural-looking simplification, which is exactly how the original crash was introduced. A missing symlink case is also the kind of thing that only shows up in the wild, on someone else's repository layout, at build time. ### How? The test drives a real `DiskFileSystem` over a temporary directory rather than exercising `LinkContent::hash` directly, because the bug being guarded lives in the entry-type dispatch — which branch gets chosen — not in the hashing itself. A test that hashed a `LinkContent` it constructed would still pass against the buggy implementation. It reuses the harness style of the existing `read_glob` tests (a `turbo_tasks::function(operation, root)` wrapper read strongly consistently), and creates symlinks through a helper that mirrors their platform handling, using a junction point for a directory link on Windows. The interesting design point is how "the link is hashed, not its target" is asserted. Doing it by mutating a file and re-hashing would pull in filesystem watching and invalidation, so it is expressed structurally instead: two directories each contain a file of the same name but with different content, and each has a symlink pointing at it through the same relative target. Those two links must hash *equally* (proving the target's content is irrelevant) while a third link with a different target must hash *differently* (proving the target itself is what counts). Both of those assertions fail against the pre-#97507 behaviour, as does the symlink-to-directory case. The remaining cases cover a dangling link and a link whose target leaves the filesystem root — both still hashable, since they are still links — and assert that every distinct link hashes distinctly, so nothing collapses into one shared "is a symlink" hash. The two assertions covering directories and missing paths document today's hard-error behaviour and carry a comment pointing at the open question already noted in `hash_file` about whether those should return `None` instead, so it is clear which assertions to revisit if that changes. ### Verification - `cargo test -p turbo-tasks-fs` (125 tests) - `cargo clippy -p turbo-tasks-fs --all-targets`, `cargo check -p turbo-tasks-fs --all-targets` - Confirmed the test actually guards the regression: reverting the symlink arm of `hash_file` to `read().hash()` makes it fail with the original `Is a directory (os error 21)` error, and it passes again once restored. <!-- NEXT_JS_LLM --> Co-authored-by: vercel-fleet-prod[bot] <318278635+vercel-fleet-prod[bot]@users.noreply.github.com> Co-authored-by: Tobias Koppers <1365881+sokra@users.noreply.github.com>
…#97763) ### What? Makes `CurrentCellRef::conditional_update_with_shared_reference` compile once instead of once per cell type. **−345 KiB** of code `__text` 66,832,376 → 66,479,352, measured on `libnext_napi_bindings.dylib` with the shipped release profile (thin LTO, `codegen-units = 1`, `--icf=all`). The 1,302 instantiations of this function collapse to **1**. ### Why? The function took its callback as `impl FnOnce`, so it was monomorphized for every `VcValueType` in the dependency graph — but its body is identical for all of them: read the cell, call the callback, hand the result to the *non-generic* `update_own_task_cell`. Nothing in it depends on the cell type.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )