Skip to content

install: name satisfies_dependency_version's range-side arguments after the dependency - #39127

Merged
alii merged 2 commits into
mainfrom
farm/b7e84246/resolution-satisfies-buf-names
Aug 15, 2026
Merged

install: name satisfies_dependency_version's range-side arguments after the dependency#39127
alii merged 2 commits into
mainfrom
farm/b7e84246/resolution-satisfies-buf-names

Conversation

@robobun

@robobun robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • mordant's arg_named_like_other_param flags src/install/resolution.rs:34: version_buf is passed as Group::satisfies's group_buf parameter, and satisfies also has a version_buf parameter of the same type (satisfies(version, group_buf, version_buf) in src/semver/SemverQuery.rs:556).
  • The call is not transposed. In Resolution::satisfies_dependency_version, version is a dependency::Version (the declared range, which is the group), so its buffer belongs in group_buf; self is the resolution holding the concrete version, so resolution_buf belongs in satisfies's version_buf. The git and github arms agree (self is lhs with resolution_buf, the dependency is rhs with its own buffer). The two vocabularies just collide on the word version.
  • All three callers (PackageManagerEnqueue.rs:2915, :2933, lockfile/bun.lock.rs:3414) pass the same lockfile string buffer in both slots, so a swap would not have been observable there either way.

Fix

  • Rename the parameters to dep_version / dep_version_buf. resolution_buf is unchanged. No behavior change, so no new test.
  • Regenerate the [bun_install] section of mordant-baseline.toml (MORDANT_BASELINE_WRITE=1 cargo dylint --all -p bun_install --no-deps). Besides the cleared arg_named_like_other_param:src/install/resolution.rs entry, this drops always_unwrapped_option:src/install/PackageInstall.rs: the walker: Option<Walker> field it counted was removed in install: let the walker own the cache dir it walks and build InstallDirState in one go #38271, which landed after the baseline was recorded in ci: bump the mordant pin (sixteen new lints) and record their baseline #38846.
  • Verified:
    • cargo dylint --all -p bun_install --no-deps (the pinned mordant) with this baseline: no findings. With the rename reverted and the same baseline, it reports exactly the resolution.rs:34 finding as over the baseline, so the baseline edit and the rename match.
    • bun bd builds; the three callers' paths pass on it: bun bd test test/cli/install/test-dev-peer-dependency-priority.test.ts (4/4), test/cli/install/bun-lock.test.ts (40/40, peer resolution while loading bun.lock), test/cli/install/bun-install-patch.test.ts (20/20, patched package lookup).

Background

  • Group::satisfies(version, group_buf, version_buf) tests a concrete semver version against a parsed range (the "group"). Both sides store their string pieces (prerelease and build tags) as offsets into a byte buffer, which is why each side passes the buffer that backs it.
  • mordant-baseline.toml is the ratchet for the mordant lint pack: it records the per-(lint, file) finding counts that predate the job, so CI fails only on new findings. Fixing a site means its entry goes away; entries whose findings were fixed without touching the file (like the PackageInstall.rs one) are harmless but stale until the section is regenerated.

…er the dependency

Resolution::satisfies_dependency_version took `version` (a dependency::Version,
the declared range) and `version_buf`, and forwarded `version_buf` into
Group::satisfies's `group_buf` slot while `resolution_buf` went into its
`version_buf` slot. The call is correct: the dependency's range is the group
and the resolution holds the concrete version. Only the names disagreed, so
rename them to `dep_version` / `dep_version_buf`.

Regenerate the bun_install section of mordant-baseline.toml. That drops the
cleared arg_named_like_other_param entry for resolution.rs and the
always_unwrapped_option entry for PackageInstall.rs, whose Option field was
removed in #38271 after the baseline was recorded.

No behavior change.
@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 45 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5a1cd328-6b41-4c32-8d50-f88fb1cdac50

📥 Commits

Reviewing files that changed from the base of the PR and between 28a438d and 5d12bf3.

📒 Files selected for processing (2)
  • mordant-baseline.toml
  • src/install/resolution.rs

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review.

  • Reproduced the finding with the pinned mordant (cargo dylint --all -p bun_install --no-deps): with the rename reverted and this PR's baseline it reports the one resolution.rs:34 site as over the baseline; with the rename it reports nothing.
  • The call was right as written (the dependency range is the group, the resolution is the version), so this is a rename plus the regenerated [bun_install] baseline section. No behavior change.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — pure parameter rename to clear a mordant arg_named_like_other_param finding, plus baseline regen.

What was reviewed:

  • Confirmed Group::satisfies(&self, version, group_buf, version_buf) at src/semver/SemverQuery.rs:556dep_version_buf correctly lands in group_buf and resolution_buf in version_buf; argument order is unchanged.
  • All three callers (PackageManagerEnqueue.rs:2915, :2933, lockfile/bun.lock.rs:3414) pass positionally with the same buffer in both slots, so no caller updates needed.
  • Verified walker: Option<...> no longer exists in PackageInstall.rs, so the dropped always_unwrapped_option baseline entry is stale as described.
Extended reasoning...

Overview

This PR renames two parameters of Resolution::satisfies_dependency_version in src/install/resolution.rs from version/version_buf to dep_version/dep_version_buf, and updates the doc comment and function body accordingly. It also removes two entries from the [bun_install] section of mordant-baseline.toml: the arg_named_like_other_param:src/install/resolution.rs entry cleared by this rename, and a stale always_unwrapped_option:src/install/PackageInstall.rs entry whose underlying field was removed in a prior PR.

Security risks

None. This is a local identifier rename in a semver comparison helper plus a lint-baseline edit. No user input handling, no I/O, no allocation, no control-flow change.

Level of scrutiny

Low. The diff is a mechanical rename with zero behavior change: every call site inside the function body passes the renamed locals in exactly the same positional order as before (.satisfies(self.npm().version, dep_version_buf, resolution_buf) vs the old .satisfies(self.npm().version, version_buf, resolution_buf); likewise for the git/github .eql arms). I cross-checked the callee signature at SemverQuery.rs:556 and confirmed the mapping in the PR description is correct — the dependency's buffer is the group buffer, the resolution's buffer is the version buffer — so the lint was a naming collision, not a transposition bug.

Other factors

The three external callers all pass arguments positionally (and pass the same lockfile string buffer for both buffer slots), so the parameter rename cannot affect them. The second dropped baseline entry was verified stale by grepping PackageInstall.rs for walker.*Option — no matches. The PR description explicitly justifies the absence of a new test (no behavior change), which matches the repo's review guidance for pure renames. No prior human review comments to address; the only timeline entry is a coderabbit rate-limit notice.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:05 AM PT - Aug 15th, 2026

@robobun, your commit 39a754d has some failures in Build #98343 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 39127

That installs a local version of the PR into your bun-39127 executable, so you can run:

bun-39127 --bun

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:05 AM PT - Aug 15th, 2026

@robobun, your commit 39a754d is building: #98343

@alii
alii merged commit cff3cd0 into main Aug 15, 2026
9 of 10 checks passed
@alii
alii deleted the farm/b7e84246/resolution-satisfies-buf-names branch August 15, 2026 18:30

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — pure parameter rename plus the matching mordant baseline entry removal; no behavior change.

What was reviewed

  • Confirmed Group::satisfies at src/semver/SemverQuery.rs:556 takes (version, group_buf, version_buf); dep_version_bufgroup_buf and resolution_bufversion_buf preserves the pre-rename argument order in all three arms.
  • All three callers (PackageManagerEnqueue.rs:2915, :2933, lockfile/bun.lock.rs:3414) pass positional args with the same buffer in both slots, so the rename is invisible to them.
  • The dropped arg_named_like_other_param:src/install/resolution.rs baseline entry corresponds exactly to the renamed site.
Extended reasoning...

Overview

Two files touched: src/install/resolution.rs renames the version / version_buf parameters of Resolution::satisfies_dependency_version to dep_version / dep_version_buf (plus the doc comment), and mordant-baseline.toml drops the now-cleared arg_named_like_other_param:src/install/resolution.rs = 1 entry from the [bun_install] section. The function body is byte-for-byte identical modulo the identifier rename and rustfmt reflow of the git arm — every call passes the same expressions in the same positions as before.

Security risks

None. This is an identifier rename in a semver/git-repo comparison helper and a lint-baseline count file; no input handling, allocation, or control-flow changes.

Level of scrutiny

Low. Mechanical rename to satisfy a lint that flags same-typed adjacent parameters where a caller-side name matches the other parameter. I verified against Group::satisfies's actual signature that the pre-existing argument order was already correct (the dependency's range buffer goes to group_buf, the resolution's buffer to version_buf), so the rename clarifies without swapping. The three call sites are all positional and unaffected. This matches the pattern of several recently-merged mordant cleanups (#39117, #39118, #39130, #39137).

Other factors

The PR description mentions a second baseline entry (always_unwrapped_option:src/install/PackageInstall.rs) also being dropped, but the final diff after merging main only removes the one line — that stale entry appears to have been cleared on main already, so the description is slightly out of date but the diff is consistent. No new test is expected per REVIEW.md since there is no behavior change to observe. The bug hunting system found nothing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants