Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (70)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a mechanical, compiler-driven newtype refactor, but at 65 files it spans the whole install subsystem including the binary lockfile serialization path, so a human sanity check on the on-disk-layout guarantees is worthwhile.
Checked: the newtype definition (repr(transparent) + Pod, Debug/Display print the bare number, Default = 0); Tree::EXTERNAL_SIZE/to_external/to_tree keep the 20-byte layout; padding_checker.rs pins both ids at 4/4; dependency_ids() covers the same begin()..end() range as the loops it replaces; the removed generic ExternalSlice::contains is fully covered by the typed replacements; the legacy Buffers::load reinterpretation of package ids from dependency-id slots is now explicit and value-identical.
Extended reasoning...
Overview
This PR converts PackageID and DependencyID from type X = u32 aliases into #[repr(transparent)] newtype structs over u32, then follows the compiler through every use site across 65 files: the whole of src/install/ (lockfile serialization, hoisting, isolated install, dedupe, prune, audit, migration from yarn/pnpm/npm), plus src/resolver/, src/jsc/AsyncModule.rs, and seven CLI commands. The core definition lives in src/install_types/resolver_hooks.rs; a bytemuck dep is added there so the ids stay Pod for the raw-buffer serialization and index_sort paths. Everything else is id as usize → id.index(), i as PackageID → PackageID::from_index(i), literal 0 → PackageID::ROOT, plus a handful of type-annotation corrections the newtypes exposed (documented in the description).
Security risks
None identified. This is a type-system refactor with no new I/O, parsing, or trust boundaries. The one area with security-adjacent implications — the on-disk bun.lockb format — is preserved byte-for-byte by repr(transparent) + bytemuck::Pod, and padding_checker.rs now pins both types at size 4 / align 4 to catch any future drift at compile time.
Level of scrutiny
High, because of scope. The change is mechanical and the compiler enforces every conversion, but it touches the binary lockfile round-trip (Tree::to_external/to_tree, Buffers::load's legacy path, bun.lockb.rs), the hoist algorithm, and every install/update/migrate entry point. The PR author ran ~1,900 tests across 24 install suites with only pre-existing network-dependent failures, which is reassuring, but the surface area alone puts this outside what an automated review should approve on its own.
Other factors
- No prior human reviews on the PR; only bot comments so far.
- The description is unusually thorough — it enumerates every mislabeled declaration the newtypes exposed and explains why each is value-identical, and calls out the one deliberate remaining pun (
Diff::generate'sid_mapping). - The bug-hunting system found no issues. I spot-checked the higher-risk conversions:
Tree::EXTERNAL_SIZEstill evaluates to 20 (size_of::<DependencyID>()==size_of::<PackageID>()== 4);ROOT_DEP_IDis stillu32::MAX - 1; thebytemuck::cast_slice_mutindedupe.rsis sound givenPod;Debug/Displaydelegate to the inneru32so the debug JSON stringifier andbun pm lsoutput are unchanged. - Given the scale and that it touches the lockfile wire format, deferring to a human for a final look rather than approving.
|
Status: closed unmerged; too large as one change. The alternatives were measured (source diff, non-LTO and LTO code size) in the comment further down; the recommendation is the small relabel-only change, prepared on the
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a large, compiler-driven refactor across 65 files in the package manager — including the bun.lockb serialization path, two new unsafe impl bytemuck::{Pod,Zeroable} blocks, and a new crate dependency — a human look would still be worthwhile.
What was reviewed:
- On-disk layout:
#[repr(transparent)]+Pod+ the newpadding_checkerpins keepbun.lockbbyte-identical;Tree::to_external/to_treeandBuffers::loadlegacy path round-trip unchanged. ExternalSlice::contains(u32)removal: verified all remaining callers go through the typedDependencySlice/ResolutionSlice::contains(DependencyID).Default= 0,Debug/Display= bare number,ArrayIdentityContextimpl — all preserve prioru32behavior for scratch buffers, log output, and identity-hashed maps.mordant-baseline.tomlalso drops two unrelated entries (always_unwrapped_option,narrowed_two_ways) from the "regenerate the baseline" commit; those files aren't touched here, so they were presumably fixed on main.
Extended reasoning...
Overview
This PR converts PackageID and DependencyID from type X = u32 aliases into #[repr(transparent)] newtypes over u32, then propagates the compiler-forced fallout across ~60 files in src/install/, src/install_types/, src/resolver/, src/jsc/, and src/runtime/cli/. The core change is ~100 lines in resolver_hooks.rs (newtype definitions with new/from_index/get/index/INVALID/ROOT, TryFrom<usize>, bytemuck::Pod/Zeroable, ArrayHashContext, Debug/Display) plus dependency_ids()/typed contains() on the two slice aliases. Everything else is mechanical: id as usize → id.index(), i as PackageID → PackageID::from_index(i), literal 0 → PackageID::ROOT, and correcting a handful of declarations whose type annotation lied (FailFn, PendingResolution::resolve_id, Builder::resolution_lists, etc.). The PR also adds a bytemuck = "1" dependency to bun_install_types and removes three mordant-baseline entries.
Security risks
None identified. This is a pure type-strengthening refactor with no new I/O, parsing, network, auth, or user-input handling. The two unsafe impl bytemuck::{Zeroable,Pod} blocks are sound (#[repr(transparent)] over u32, which is itself Pod), and padding_checker.rs statically pins both types at size = 4, align = 4. The bytemuck::cast_slice_mut calls in dedupe.rs reinterpret &mut [PackageID] as &mut [u32] for index_sort, which is exactly what Pod licenses.
Level of scrutiny
High. The package manager and its lockfile serialization are among the most load-bearing subsystems in Bun — a subtle mismatch in the bun.lockb binary layout or the Tree::to_external encoding would silently corrupt users' lockfiles. That said, the refactor is compiler-enforced end to end: every as usize / as PackageID site had to be touched to compile, so there is no way for a stale conversion to slip through. #[repr(transparent)] guarantees the on-disk bytes are identical, Default is still 0 so zero-initialized scratch buffers behave the same, and Debug/Display print the bare integer so log/snapshot output is unchanged. The author ran ~1,900 tests across the install/lockfile/migration suites with only pre-existing network-dependent failures.
Other factors
- The change is large (65 files) and touches critical code paths, which per the approval guidelines means it should get human eyes even when the mechanics look sound.
- The
mordant-baseline.tomlregeneration removes two entries unrelated to this PR (always_unwrapped_option:src/install/PackageInstall.rsandnarrowed_two_ways:src/runtime/node/node_crypto_binding.rs). Neither file appears in the diff, so those lints presumably stopped firing onmainindependently; worth a quick sanity check that the regeneration was run against the right base. - Removal of the generic
ExternalSlice::contains(u32)was verified: all remaining.contains(...)on external slices go throughDependencySlice/ResolutionSlice, which now have the typed replacement. - No CODEOWNERS conflicts or outstanding reviewer comments; the only timeline entries are bot noise (CodeRabbit rate-limited, robobun build link).
|
@robobun fix conflicts |
|
Merged current main in (96279ba). Two conflicts: |
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. Given the size (65 files) and that it touches the lockfile serialization path (Tree::to_external/to_tree, Buffers::load, bytemuck::Pod), a human sign-off would still be worthwhile.
What was reviewed:
#[repr(transparent)]+Podkeep the on-diskbun.lockblayout byte-identical;padding_checker.rsnow pins both ids at 4 bytes andTree::EXTERNAL_SIZEis unchanged at 20.ROOT_DEP_IDstill evaluates tou32::MAX - 1;Defaultfor both ids is still 0, sovec[PackageID::default(); n]and derived-Defaultstructs keep prior values.- The removed generic
ExternalSlice::contains(u32)— every remaining caller (OverrideMap,audit_fix,lockfile::is_root_dependency,PackageManagerEnqueue) is onDependencySlice/ResolutionSlice, which have the new typedcontains. - The
while counter_i < changes { … counter_i += 1 }→for counter_i in 0..changesrewrite ininstall_with_manager.rspreserves iteration count and ordering.
Extended reasoning...
Overview
This PR converts PackageID and DependencyID from type X = u32 aliases into #[repr(transparent)] newtype structs over u32, then propagates the type change through 65 files in the package manager, resolver, and CLI. The vast majority of the diff is mechanical: id as usize → id.index(), i as PackageID → PackageID::from_index(i), literal 0 → PackageID::ROOT, and for i in slice.begin()..slice.end() → for id in slice.dependency_ids(). A handful of declarations whose stated type disagreed with their actual value (e.g. FailFn's parameter, PendingResolution::resolve_id, Builder::resolution_lists) are retyped to match; the numeric values are unchanged. bytemuck is added as a dependency of bun_install_types so the ids remain Pod for the raw-buffer serialization and index_sort paths.
Security risks
None identified. This is a type-level refactor with no new I/O, parsing, or trust boundaries. The unsafe impl Pod/Zeroable blocks are justified by #[repr(transparent)] over u32 (itself Pod).
Level of scrutiny
High. Although each individual edit is mechanical and the compiler enforces the crossings, the change spans the lockfile's on-disk format: Tree::to_external/to_tree now go through .get()/DependencyID::new() around the same u32::to_ne_bytes round-trip, Buffers::load's legacy path explicitly reinterprets stored DependencyID bytes as PackageID, and bytemuck::cast_slice_mut is used to sort id buffers. A mistake in any of these would silently corrupt user lockfiles rather than fail to compile. I checked that repr(transparent) preserves size/alignment (pinned in padding_checker.rs), that ROOT_DEP_ID's value is unchanged, that Default still yields 0, and that Debug/Display forward to the inner u32 so bun pm ls and the debug-JSON output are unaffected.
Other factors
The PR is well-tested (~2,150 install tests passed locally against a debug build; CI passed 177/179 with the two remaining jobs being darwin agents that never picked up work). A maintainer (alii) has already engaged asking for a conflict resolution, which was done via merge commit 96279bab. No bugs were surfaced by the multi-agent review. However, per the approval guidelines a 65-file change to the package manager's core buffers and serialization is exactly the class of change that warrants a human reviewer's sign-off rather than an automated approval, so I'm deferring.
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
The retype itself checks out: every non-mechanical hunk keeps the same value, no try_from/as drift, on-disk layout pinned, hooks crossing updated. One change requested below on the Default derive, and a rebase note: main has already dropped the two stale baseline entries, so the "regenerate the baseline" commit becomes empty on rebase and can go.
Not for this PR, just noting: x[id.index()] at ~600 subscript sites is still untyped, so the newtype guards params and fields but not the buffer indexing itself. An id-keyed slice type with Index<PackageID> would close that; worth a follow-up, not worth growing this diff.
|
Correction to my review: do the typed indexing in this PR after all, no follow-up. Add an id-keyed slice/vec type (Index / Index, repr(transparent) over the slice so the bun.lockb bytemuck casts and index_sort keep working) and use it for the parallel dependency buffers and the package columns, so |
IdSlice/IdVec (bun_collections) are slices and vectors subscripted with an id newtype; they deref to the plain slice for everything else, and a type with its own Index impl rejects usize subscripts, so raw positions go through raw()/raw_mut(). The Package columns are declared indexed by PackageID through multi_array_columns!, and buffers.dependencies / buffers.resolutions are IdVec<DependencyID, _>. Every x[id.index()] in install and the pm commands becomes x[id]; Lockfile::package(id) gathers a row; the remaining .index() calls feed bitsets and packages.len() checks.
e122818 to
bf1a583
Compare
robobun
changed the title
|
Both review items are in, plus the typed indexing from the follow-up comment; the description now describes the branch as it stands.
|
Checks that PackageID / DependencyID are newtypes, that the package columns and the dependency buffers are declared id-indexed, and that no buffer[id.index()] or buffer.get(id.index()) subscript is left in the install crate or the pm commands (two documented exceptions).
… RawSlice / plain slices
…ef'd slices With the buffers as IdVec, .as_mut_ptr() resolved through Deref to the slice's, whose pointer only covers len (0 here) while the migration fills the reserved capacity through it; raw_mut().as_mut_ptr() is Vec's pointer again, as the set_len below already does.
There was a problem hiding this comment.
The yarn.rs provenance fix in e97a91b looks correct — raw_mut().as_mut_ptr() now reaches Vec::as_mut_ptr so the pointer covers the reserved capacity, and the IdVec doc comment now warns about this pitfall. This run found no further issues.
Given the size (71 files across the install/lockfile core, new unsafe in IdSlice::from_raw{,_mut}, and the bun.lockb on-disk layout being held byte-identical only by repr(transparent) + the padding_checker pins), a final human look from alii — who has been driving the design here — is still the right call before merge.
Extended reasoning...
Overview
This PR converts PackageID and DependencyID from u32 type aliases to #[repr(transparent)] newtypes and threads them through the entire install subsystem: 71 files including the lockfile core (lockfile.rs, Buffers.rs, Tree.rs, Package.rs, bun.lock.rs, bun.lockb.rs), both installers (hoisted/isolated), the package manager, migrators (npm/pnpm/yarn), dedupe, audit, prune, and every bun pm command. It adds IdSlice/IdVec in bun_collections and an indexed by clause to multi_array_columns! so the ~600 buffer[id.index()] sites become buffer[id] and cross-type indexing is a compile error.
Changes since my last review
My 02:27 inline comment flagged that yarn.rs:794-795's .as_mut_ptr() now resolved through DerefMut → [T]::as_mut_ptr (provenance limited to len, not capacity) instead of Vec::as_mut_ptr. Commit e97a91b fixes this: both sites now use .raw_mut().as_mut_ptr(), the SAFETY comment explains why, and the IdVec doc comment (id_slice.rs:143-146) now documents the hazard for future callers. I grepped the rest of the install crate for other as_mut_ptr on the retyped IdVec fields and found none — the remaining .as_mut_ptr() calls in yarn.rs are on the local &mut [T] slices (dependencies_buf, resolutions_buf) which already have full-range provenance from ffi::slice_mut. The three earlier stale-RawSlice comments and the Default = INVALID review item were already confirmed fixed.
Security risks
None identified. The change is type-level; no new external input parsing, no auth/permission surface. The new unsafe in IdSlice::from_raw{,_mut} is a repr(transparent) pointer cast with a SAFETY comment stating the layout-equality justification, which is the standard pattern.
Level of scrutiny
High. The bun.lockb binary format serializes resolutions, hoisted_dependencies, and the 20-byte Tree records as raw bytes; correctness depends on repr(transparent) + bytemuck::Pod keeping the wire format identical (pinned in padding_checker.rs and covered by the lockfile round-trip suites). Default changed from 0 (root) to INVALID, which alii audited but is a semantic shift. The multi_array_columns! macro grew three new arms and helper macros. This is well beyond the "simple, mechanical, obvious" bar for auto-approval.
Other factors
alii has been actively reviewing and shaping this PR (requested Default=INVALID, requested the typed-indexing be done here rather than a follow-up) but has not yet approved. The bug-hunting system found nothing this run. The author ran ~2,450 install tests locally with the same pre-existing failure set before and after. Deferring so alii can give the final sign-off on a change they have been directing.
…lumn traits These run under the cargo miri test job, so the from_raw / from_raw_mut casts and the IdSlice-wrapped split_mut columns get interpreted there rather than only being compiled as part of the install crate.
|
Nothing outstanding from the review passes; the three inline threads are answered and resolved. One more follow-up pushed (72b03b5): the two residual-risk points the last pass named (the |
There was a problem hiding this comment.
Re-reviewed after e97a91b / a13e861 / 72b03b5 — both earlier findings are addressed (yarn.rs now takes the capacity pointers via raw_mut().as_mut_ptr() with an updated SAFETY note; the stale RawSlice comments are gone) and this pass found nothing further. Given the scope — 71 files across the lockfile buffers, package columns, hoisting, and both migrators — a final look from alii on the typed-indexing commit would still be worthwhile.
Checked this run: the IdSlice repr(transparent) casts and the multi_array_columns! indexed by expansion; that Default for both ids is INVALID and the remaining get_or_put fills overwrite the slot on every path; the bun.lockb layout pins in padding_checker.rs and Tree::EXTERNAL_SIZE; and swept src/install for other .as_mut_ptr() / .as_ptr() on IdVec fields — the two in yarn.rs were the only ones.
Extended reasoning...
Overview
This PR converts PackageID and DependencyID from type X = u32 aliases into #[repr(transparent)] newtypes and adds IdSlice<I, T> / IdVec<I, T> so the lockfile's parallel buffers and MultiArrayList<Package> columns are subscripted only by their own id type. It touches 71 files (~490 KB diff): the new collection types and the multi_array_columns! indexed by clause in src/collections/, the id newtypes in src/install_types/resolver_hooks.rs, and every consumer across src/install/, the resolver hooks, and the bun pm CLI commands. On-disk layout is preserved via repr(transparent) + bytemuck::Pod and pinned in padding_checker.rs.
Security risks
None identified. This is a type-tightening refactor of internal indexing; no new user input parsing, no auth/crypto/permissions surface. The one memory-safety regression it introduced (slice-provenance as_mut_ptr() in the yarn migrator) was caught in the previous pass and is fixed in e97a91b.
Level of scrutiny
High. The change is intended to be behavior-preserving, but it rewrites indexing across the lockfile serializer, both migrators (npm/pnpm/yarn), the hoist tree builder, the isolated installer, dedupe, audit-fix, and every bun pm command — all production-critical paths where an off-by-one or a wrong-buffer subscript would corrupt a lockfile or misinstall. The IdVec deref-to-slice design also silently changes method resolution for anything not explicitly forwarded (which is exactly how the yarn.rs bug arose). alii has been actively reviewing and drove two of the design decisions (Default = INVALID, typed indexing in this PR rather than a follow-up); a final sign-off from them on the current head is appropriate before merge.
Other factors
- Both prior inline findings from this bot are resolved and verified in the tree at HEAD (72b03b5):
src/install/yarn.rs:796-797now goes throughraw_mut().as_mut_ptr()with a corrected SAFETY comment, and noRawSlice<Resolution>string remains inPackageInstaller.rs. - The
IdVecdoc comment now explicitly warns about theas_ptr/as_mut_ptrderef hazard, and a grep ofsrc/installconfirms no otherIdVecfield has.as_mut_ptr()/.as_ptr()called on it. - 72b03b5 adds unit tests for
IdSlice/IdVecand bothmulti_array_columns!forms;test/internal/source-lints/install-id-indexing.test.tsguards against regressions. - The bug-hunting system found nothing on this head.
|
Nothing further to change from this pass. For the record on 72b03b5: the |
|
This has gotten too big and too laser focused on a particular solution that I think we've lost the point of the original change. Could we instead make a larger refactor elsewhere that eliminates this problem entirely? What other options are there? Going to close this one for now. Feel free to open a followup if there's a better design you can find please @robobun. |
|
Agreed on closing it. The size was structural rather than a choice of container: on main, 58 files reach straight into Options I can see, honestly weighed:
My suggestion would be 1 now, since it is small and closes out the finding, and 2 as the direction if eliminating the class is worth it. Happy to open either; say which. |
|
Measured the options rather than estimating them. Every state below is built from the same merge base (a42889a), so the deltas are only the change in question. How it was measured
Results
Reading the numbers
Conclusion: by the stated criterion, option 1, and it is not close: it is the only option whose cost is small on either axis, and it closes out the finding this started from (branch above, ready to open as a PR). Option 2 is worth doing only if the shorter consumers are wanted for their own sake. If the class of bug is ever worth eliminating, that costs ~63 files whichever way it is sliced; the closed branch's first commit is that change and can be reused. Say the word and I will open option 1. |
2 participants
Problem
PackageIDandDependencyIDwere bothtype X = u32;aliases (src/install_types/resolver_hooks.rs), so a package id used where a dependency id belongs compiled silently. mordant'sinterchangeable_aliasesflagged one such site,ROOT_DEP_ID: DependencyID = invalid_package_id - 1insrc/install/lockfile/Tree.rs.PackageManager::FailFntook aPackageID; its only implementation (fail_root_resolution) and every caller pass a dependency id.lockfile::PendingResolution::resolve_idwas aPackageID; it indexesbuffers.resolutions, i.e. it is a dependency id.tree::Builder::resolution_listsandTree::hoist_dependency's range parameter were typed asDependencyIDSlicebut are fedPackage.resolutions(PackageIDSlice);verify_resolutionsannotated the same column the same way;yarn.rsbuiltPackage.resolutionsasDependencyIDSlice::new(..)in four places;Tree::EXTERNAL_SIZEsized thedependency_idslot withsize_of::<PackageID>().INVALID_PACKAGE_IDserved as the dependency-id sentinel inprinter/tree_printer.rs(4 sites) and in the resolver'sPendingResolution::default().hooks::TaskCallbackContext::root_request_idwas a bareu32while thebun_installside is aPackageID.Lockfile::loaded_package_countand a few local counters were typed asPackageIDbut are counts.Buffers::load's legacy-lockfile path reads package ids out ofDependencyIDslots; that reinterpretation is now explicit.column[id.index()]subscripts accepted anyusize, so the types guarded parameters and fields but not the indexing itself (review feedback on the first version of this PR).Fix
Commits, in order (plus three small follow-ups from review: stale comments, the yarn capacity pointers described under typed indexing, and unit tests for
IdSlice/IdVecand theindexed bycolumn traits inbun_collections, which thecargo miri testjob runs, so thefrom_rawcasts and the wrappedsplit_mutcolumns are interpreted under Tree Borrows in CI rather than only compiled):PackageIDandDependencyIDbecome#[repr(transparent)]newtypes overu32withnew,from_index(truncating, like theascasts it replaces),TryFrom<usize>(for the existing checked sites),get,index,INVALID, andPackageID::ROOT.Debug/Displayprint the bare number, so log lines,bun pm ls-style output and the debug lockfile JSON are unchanged. They arebytemuck::Podso thebun.lockbbuffers andindex_sortkeep working on them, andArrayIdentityContextis implemented for them so the identity-hashed maps keyed by ids keep their context.INVALID_PACKAGE_ID/invalid_package_idetc. keep their names.DependencySliceandResolutionSlice(the two slices whose positions are dependency ids) gaindependency_ids()and a typedcontains(DependencyID); the untypedExternalSlice::contains(u32)had no other callers and is removed. The mislabeled declarations above get the type their values actually have; on-disk layout is unaffected (padding_checker.rspins both types at 4 bytes next to the other serialized leaf types).mordant-baseline.tomlloses theinterchangeable_aliases:src/install/lockfile/Tree.rsentry; the lint only looks at integer aliases, so the newtypes make that site (and any future one) impossible to write.bun run rust:mordanton this branch reports nothing over the baseline.Defaultfor both ids isINVALID, likeNewIdin the isolated installer andbun_ast::Index, instead of the derived 0 (which is the root package). The only consumers ofDefaultare theget_or_putfills in the bun.lock / pnpm parsers, which every path overwrites or abandons on error; the scratch buffers that used to be zero-filled now sayinvalid_package_idexplicitly, and the two structs that derivedDefaultonly to hold an id did not use it (derives dropped).bun_collections::IdSlice<I, T>/IdVec<I, T>are a slice and a vector subscripted with an id typeI: Idx(implemented by both ids). They deref to[T]for everything that is not subscripting, solen/iter/&[T]parameters/byte casts need no changes, but because the wrapper has its ownIndeximpl,s[0],s[some_usize]ands[a..b]are compile errors; raw positions go throughraw()/raw_mut()(24 sites: sub-ranges being filled, sorted or walked, theVecbeing handed to the capacity-growing helpers, and the yarn migration's capacity fill, which takes its base pointers from theVecs because the slice'sas_mut_ptrreached throughDerefwould only coverlen; that last point is also called out inIdVec's docs).multi_array_columns!acceptsfor Package<_>, indexed by PackageID, so everyitems_*()accessor (andsplit_mut) hands outIdSlice<PackageID, _>;buffers.dependencies/buffers.resolutionsareIdVec<DependencyID, _>; the resolver hooks,tree::Builder,reachable::Walk, the dedupe pass, the printers,PackageInstaller's column back-references (nowBackRef<IdSlice<..>>instead ofRawSlice), and the per-package / per-dependency scratch vectors (preinstall_state, the clone mapping, dedupe'sgroup_of, audit'sparent_of, ...) follow.x[id.index()]no longer occurs anywhere insrc/installor thebun pmcommands; loops that counted0..lenand converted useids()/iter_enumerated();Lockfile::package(id)gathers a row (the one remainingpackages.get(id.index())); the.index()calls left feed bitsets andpackages.len()bounds checks.has(id)replaces theid.index() < col.len()checks against typed slices.test/internal/source-lints/install-id-indexing.test.tskeeps this in place: it checks that the two ids are newtypes, that the package columns and the dependency buffers are declared id-indexed, and that nobuffer[id.index()]/buffer.get(id.index())subscript is left in the install crate or the pm commands (the two exceptions above are allow-listed with exact counts). It fails on main's sources and passes here.id_mappingslice passed toDiff::generatestores positions within the old root package's dependency list in a[PackageID], andinstall_with_managerreads it back with.index()into a sub-slice. It is internally consistent and a separate cleanup.cargo check --workspace; clippy onbun_collections,bun_install_types,bun_resolver,bun_install,bun_jsc,bun_runtime,bun_ast,bun_bundler(the othermulti_array_columns!users);cargo fmt --all;bun run rust:mordant;cargo test -p bun_collectionsandbun run rust:miri -p bun_collections(42 tests, 7 of them new).bun bd teston about 2,450 tests: the lockfile suites (bun-lockb,bun-lock,migrate-bun-lockb-v2,lockfile-version-2,lockfile-only),test/cli/install/migration/,bun-install,bun-pm,bun-pm-why,bun-pm-licenses,bun-audit,bun-dedupe,bun-prune,bun-add,bun-update-transitive,bun-workspaces,hoist,isolated-install,catalogs,overrides,nested-overrides,frozen-lockfile-pruned,bun-add-filter,bun-install-security-provider,bun-install-registry,bun-update,bun-patch,bun-install-patch,bun-remove,bun-link, andtest/cli/update_interactive_*. The failures are the same set before and after every step of this PR and are unrelated to it: tests needing network access (git/bitbucket/gitlab clones, remote tarballs), two migration tests that sit at the 5 s limit on an unoptimized build when run in a batch and pass alone (4.6 s / 2.6 s, same as before this change), and onebun-linkexpectation that does not account for the#[cfg(bun_debug)]failure trace debug builds print (file untouched here). Details in the block below.Background
Lockfile.packagesis a struct-of-arrays of packages (MultiArrayList, one column per field), indexed byPackageID; package 0 is always the root project.buffers.dependenciesholds every declared dependency edge, indexed byDependencyID, andbuffers.resolutionsis parallel to it:resolutions[dep_id]is thePackageIDthe edge resolved to (orINVALID_PACKAGE_ID). Each package stores aDependencySlice/PackageIDSlicepair of(offset, len)ranges into those two buffers, which is why the positions covered by either slice are dependency ids, and why a package's own sub-slice of either buffer is position-indexed rather than id-indexed (those stay plain slices).bun.lockbserializesbuffers.resolutionsandbuffers.hoisted_dependenciesas rawu32arrays, andTree(one pernode_modulesfolder) as 20 raw bytes including aDependencyID;repr(transparent)keeps those byte-for-byte identical. Very old lockfiles stored package ids in the tree's dependency-id slots, which is the legacy conversion inBuffers::load.ROOT_DEP_IDis a sentinel dependency id given to the rootnode_modulestree node, which has no dependency edge of its own;Tree::process_subtreemaps it to package 0. Its value (u32::MAX - 1) is unchanged; it is now derived from the dependency-id sentinel rather than the package-id one.IdSlicecan deref to[T]and still rejects[0]: indexing stops at the first type in the auto-deref chain that implementsIndexat all and then requires the subscript to match one of that type's impls; it does not keep dereferencing to find an impl for a different index type. SoIdSlicegets the whole slice API throughDerefwhile subscripting is only possible with its id.multi_array_columns!is the macro that generates theitems_<field>()column accessors from a struct's fields; the newindexed byclause wraps each accessor's result inIdSlice::from_raw, which is a pointer cast made sound by#[repr(transparent)].mordant-baseline.toml;interchangeable_aliasesreports a value declared under one integer alias flowing into a slot declared under another.Local test runs (unoptimized debug build)
[review] gate passed · iteration 1 · 71 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 1
evidence per changed file