test runner: store Scanner.fs as a raw pointer instead of casting away shared refs - #31850
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Addressed in 6034828: fixed the Windows-only E0502 in the Scanner directory-descent loop (capture the |
There was a problem hiding this comment.
Thanks for addressing the comment nit and the Windows borrow conflict. No further issues from me, but the core change here is a careful rework of unsafe aliasing/provenance invariants around the FileSystem singleton — worth a human sign-off on the soundness argument.
Extended reasoning...
Overview
The substantive change is confined to src/runtime/cli/test/Scanner.rs (plus two accessor call sites in test_command.rs and new behavior-guard tests in bun-test.test.ts): Scanner.fs changes from &'a FileSystem to *mut FileSystem, removing two #[allow(invalid_reference_casting)] &T → &mut T casts that gave writes read-only provenance. Reads are split into a whole-struct fs() accessor (off the callback path) and field-precise place projections top_level_dir()/filename_store()/abs_buf_projected() (on the callback path, where the resolver holds &mut to the singleton's .fs field). The remaining ~200 files are pure autofix.ci/rustfmt noise — trailing-newline removal, derive reflow, import reordering — with no semantic content.
Since my prior inline comment, commit 6034828 fixed both the Windows-only E0502 (by hoisting let fs = self.fs() above the abs_buf_z borrow) and the inaccurate next → is_test_file call-chain citation in the matches_path_ignore_pattern SAFETY comment.
Security risks
None. This is an internal soundness refactor of the test-file-discovery scanner; no parsing of untrusted input, no auth/crypto/permissions surface, no behavior change intended.
Level of scrutiny
High. The change is a strict improvement (it removes lint-silenced UB), but its correctness rests on a subtle, hand-argued aliasing model: which code paths run inside read_directory_with_iterator's &mut RealFS borrow, that field-precise raw-place projections of disjoint FileSystem fields don't conflict with that borrow, that abs_buf_projected is semantically equivalent to FileSystem::abs_buf (both call join_abs_string_buf::<platform::Loose>), and that the residual reentrant &raw mut (*self.fs).fs passed to Entry::kind while the outer &mut (*fs_ptr).fs is live is acceptable per the resolver's documented entries_mutex contract. The SAFETY comments are thorough and I found no flaws, but this is exactly the class of unsafe-Rust reasoning that benefits from a second human reader.
Other factors
The bug-hunting pass found nothing this round. New integration tests exercise deep-nested discovery, dot-dir/node_modules pruning, subdirectory positionals, and the single-file NotDir branch — good behavior guards over the rewritten paths. The robobun CI comment references the pre-fix commit (0872fe0); the Windows build break it reports was fixed in 6034828, and the bunx.test.ts failures are unrelated to the scanner. The autofix.ci formatting churn across 200 files is mechanical and inert.
…y shared refs The bun test file-discovery Scanner held its FileSystem as a shared reference but derived `&mut RealFS` from it in two places (`read_dir_with_name` and `next`) via `#[allow(invalid_reference_casting)]` `&T` -> `&mut T` casts, giving the writes read-only provenance — undefined behavior under Rust's aliasing rules. The fix changes `Scanner.fs` to `*mut FileSystem` (the same shape `Transpiler.fs` already uses), storing `transpiler.fs` directly in `Scanner::init`. The two mutation sites now derive `&mut (*self.fs).fs` / `&raw mut (*self.fs).fs` with genuine write provenance, and the lint allow is gone. Reads outside the directory-iterator callback go through a new `fs()` accessor returning `&'static FileSystem` (sound: the singleton lives for the whole process); reads on the callback path (`next` and callees, which run while the resolver holds `&mut` to the singleton's `fs` field) use field-precise place projections (`top_level_dir`/`filename_store`/`abs_buf_projected`) so no whole-struct `&FileSystem` spans the mutably-borrowed field. The remaining reentrant `Entry::kind` `*mut` reborrow is the resolver's documented, entries_mutex-serialised contract and is called out in the SAFETY comments. The two `scanner.fs.top_level_dir` reads in test_command.rs were updated to use the accessor. New scanner-discovery tests in test/cli/test/bun-test.test.ts (deep nested scanning, dot-dir/node_modules pruning, subdirectory positional, single-file NotDir path) are behavior guards over the rewritten paths — the refactor has no intended behavior change, so they are not pre/post discriminators; verification is the build/test phase.
Capture the &'static FileSystem before building path2 so the mutable borrow of open_dir_buf doesn't conflict with a second self.fs() call (E0502 on windows targets). Also correct the call-chain note on matches_path_ignore_pattern: next() calls it directly.
6034828 to
fdd60ec
Compare
WalkthroughScanner struct refactors its filesystem field from a borrowed reference to a raw mutable pointer, introduces accessor helpers to mediate filesystem access, and updates all directory traversal, path computation, and filtering logic throughout the Scanner methods to use those helpers. TestCommand is updated to use the new accessor, and a comprehensive test suite validates scanner test discovery against nested directories, special directories, and individual file paths. ChangesScanner Filesystem Refactoring
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Remove all these comments.
|
Trimmed in fbe7b89 — cut the commentary from 75 lines to 27. What remains is the one-line SAFETY justification on each |
|
Removed the remaining commentary (1 file changed, 4 insertions(+), 28 deletions(-)). Only one-line SAFETY notes on the unsafe blocks remain (required by the undocumented_unsafe_blocks lint). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/cli/test/Scanner.rs`:
- Line 31: The public raw pointer field Scanner.fs should be made private so
external code cannot mutate it and break the new safety invariant; change the
declaration from a pub field to a non-public (or pub(crate)/module-private)
field and update callsites to use the accessor methods Scanner::fs(),
Scanner::top_level_dir(), and Scanner::filename_store() instead of directly
accessing Scanner.fs so all external access goes through the safe accessors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23df97d2-9d5e-4a3d-bffb-f735924b8c95
📒 Files selected for processing (3)
src/runtime/cli/test/Scanner.rssrc/runtime/cli/test_command.rstest/cli/test/bun-test.test.ts
| // `*mut FileSystem` (the shape `Transpiler.fs` already uses; init would | ||
| // store `transpiler.fs` directly) or RealFS needs interior mutability. | ||
| pub fs: &'a FileSystem, | ||
| pub fs: *mut FileSystem, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Hide Scanner.fs behind the accessor.
Exposing the raw *mut FileSystem publicly makes the new safety invariant unenforceable. Any external write to Scanner.fs can invalidate the 'static references returned by fs(), top_level_dir(), and filename_store(). Now that callers have moved to scanner.fs(), this field should be private (or at least no wider than the module).
Suggested change
- pub fs: *mut FileSystem,
+ fs: *mut FileSystem,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fs: *mut FileSystem, | |
| fs: *mut FileSystem, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/cli/test/Scanner.rs` at line 31, The public raw pointer field
Scanner.fs should be made private so external code cannot mutate it and break
the new safety invariant; change the declaration from a pub field to a
non-public (or pub(crate)/module-private) field and update callsites to use the
accessor methods Scanner::fs(), Scanner::top_level_dir(), and
Scanner::filename_store() instead of directly accessing Scanner.fs so all
external access goes through the safe accessors.
2 participants
The bun test file-discovery Scanner held its FileSystem as a shared reference but derived
&mut RealFSfrom it in two places (read_dir_with_nameandnext) via#[allow(invalid_reference_casting)]&T->&mut Tcasts, giving the writes read-only provenance — undefined behavior under Rust's aliasing rules. The fix changesScanner.fsto*mut FileSystem(the same shapeTranspiler.fsalready uses), storingtranspiler.fsdirectly inScanner::init. The two mutation sites now derive&mut (*self.fs).fs/&raw mut (*self.fs).fswith genuine write provenance, and the lint allow is gone. Reads outside the directory-iterator callback go through a newfs()accessor returning&'static FileSystem(sound: the singleton lives for the whole process); reads on the callback path (nextand callees, which run while the resolver holds&mutto the singleton'sfsfield) use field-precise place projections (top_level_dir/filename_store/abs_buf_projected) so no whole-struct&FileSystemspans the mutably-borrowed field. The remaining reentrantEntry::kind*mutreborrow is the resolver's documented, entries_mutex-serialised contract and is called out in the SAFETY comments. The twoscanner.fs.top_level_dirreads in test_command.rs were updated to use the accessor. New scanner-discovery tests in test/cli/test/bun-test.test.ts (deep nested scanning, dot-dir/node_modules pruning, subdirectory positional, single-file NotDir path) are behavior guards over the rewritten paths — the refactor has no intended behavior change, so they are not pre/post discriminators; verification is the build/test phase.Verification
Implemented and verified on a unified integration branch: full debug build (linux-x64, ASAN), cargo check across the workspace, and the affected test files run against the debug build (failures cross-checked against main's build to exclude pre-existing issues). Each change was reviewed twice (compile/API correctness and GC/concurrency/semantics lenses) with findings repaired before landing.