node:path: name relative()'s buffers by role instead of by position - #39125
Conversation
relative_posix_t and relative_windows_t resolve `from` into their second buffer and use the third as scratch for both resolve_*_t calls, so the calls passed `buf2` as the callee's `buf` parameter and read like a transposed argument (mordant's arg_named_like_other_param). The calls are correct: `buf` is taken by `to` and the returned slice. Rename the two buffers to from_buf and tmp_buf and drop the baseline entry. Regenerating the baseline for bun_runtime also drops the narrowed_two_ways entry for node_crypto_binding.rs, which #37648 fixed without removing it.
|
Warning Review limit reached
Next review available in: 31 minutes 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 selected for processing (2)
Comment |
There was a problem hiding this comment.
LGTM — pure local-variable/parameter rename to clear a lint finding, no behavior change.
What was reviewed:
- Confirmed every
buf2/buf3occurrence insiderelative_posix_t,relative_windows_t, their_js_twrappers, andrelative_js_twas renamed; remainingbuf2hits inpath.rsall belong to other functions. - Checked
resolve_posix_t/resolve_windows_tsignatures — new argument names no longer alias a differently-positioned callee parameter, so the baseline entry removal is correct. relative_posix_t/relative_windows_thave no other callers beyond the updated wrappers.
Extended reasoning...
Overview
This PR renames two &mut [T] parameters (buf2 → from_buf, buf3 → tmp_buf) in relative_posix_t, relative_windows_t, and the three thin wrappers that thread them through (relative_posix_js_t, relative_windows_js_t, relative_js_t) in src/runtime/node/path.rs. It updates a handful of comments that referenced the old names, and drops two entries from mordant-baseline.toml: the arg_named_like_other_param:src/runtime/node/path.rs entry this rename resolves, and a stale narrowed_two_ways:src/runtime/node/node_crypto_binding.rs entry the author explains was orphaned by a merge race between #37648 and #38875.
Security risks
None. This is a lexical rename of local buffer bindings with no change to types, ordering of arguments at call sites, buffer sizing, or control flow. No user-facing surface changes.
Level of scrutiny
Low. The diff is mechanical and self-contained: I grepped path.rs for residual buf2/buf3 references and confirmed none remain within the touched functions (lines 2395–2780); all other buf2 hits belong to join_*_t, resolve_*_t, and to_namespaced_path_*, which the PR intentionally leaves alone. The callee signatures (resolve_posix_t(paths, buf, buf2) / resolve_windows_t(paths, buf, buf2)) confirm the lint's original complaint and that the new names (from_buf, tmp_buf) no longer collide. Argument order is preserved at every call site — this compiles to identical code.
Other factors
- The only callers of
relative_posix_t/relative_windows_tare the two_js_twrappers updated in this diff, so the rename is complete. - The extra baseline deletion (
narrowed_two_ways:...node_crypto_binding.rs) is a ratchet-file cleanup the author regenerated and explicitly flagged; since the baseline is a per-(lint,file) tolerance count, removing a stale entry only tightens CI and cannot mask a regression. - Author reports
bun bd test test/js/node/path/passes (123 pass / 3 skip), which covers bothpath.posix.relativeandpath.win32.relative— appropriate coverage for a no-op rename.
Problem
bun run rust:mordantreports twoarg_named_like_other_paramfindings insrc/runtime/node/path.rs, carried inmordant-baseline.toml:relative_posix_tandrelative_windows_teach callresolve_*_t(&[from], buf2, buf3), passing theirbuf2as the callee'sbufparameter while the callee also has a same-typed parameter namedbuf2, so the call reads like a transposed argument.relative_*_tresolvestointo its ownbuf(which also backs the returned slice), sofromhas to be resolved into a different buffer, and the third buffer is lent to bothresolve_*_tcalls as scratch and then reused to accumulate the..segments. Only the positional names (buf2,buf3) made that look like a mistake.Fix
relative_posix_t,relative_windows_t, their_js_twrappers andrelative_js_ttofrom_bufandtmp_buf, and update the comments that named them. The calls now readresolve_*_t(&[from], from_buf, tmp_buf)andresolve_*_t(&[to], buf, tmp_buf).resolve_posix_t/resolve_windows_tand their other callers are untouched.mordant-baseline.toml: regenerated the[bun_runtime]section. This drops thearg_named_like_other_param:src/runtime/node/path.rsentry, and alsonarrowed_two_ways:src/runtime/node/node_crypto_binding.rs, whose finding crypto: store PBKDF2's key length as usize #37648 removed without touching the baseline (it merged a few minutes after ci: pin mordant at the renamed lints; rename the disabled list and baseline keys to match #38875 regenerated the file). Happy to drop that second line if this PR should stay strictly scoped.MORDANT_BASELINE_WRITE=1 cargo dylint --all -p bun_runtime --no-deps(therust:mordant:baselinescript scoped to the one crate this touches;bun_runtimehas no cargo features, so the crate is compiled the same way as in the workspace run) rewrites[bun_runtime]with exactly those two entries removed and nothing added.bun bd test test/js/node/path/: 123 pass, 3 skip (Windows-only), 0 fail.relative.test.jsexercises bothpath.posix.relativeandpath.win32.relative, so both renamed functions run on Linux.Background
bun run rust:mordantruns (pinned inCargo.tomlunder[workspace.metadata.dylint]).mordant-baseline.tomlis a ratchet: per (lint, file) counts of pre-existing findings that CI tolerates, so fixing a site means also deleting its baseline entry, and a stale entry silently absorbs one new finding of that kind in that file.arg_named_like_other_paramfires when an argument is spelled with the name of a callee parameter other than the one it is bound to, and the two parameters share a type (so an actual swap would compile). Renaming the value on either side so the names no longer cross is the intended way to clear a correct call.