Skip to content

ast: name BabyString::r#in's parameters for what they are - #39137

Merged
alii merged 2 commits into
mainfrom
farm/82374b2d/ast-in-arg-names
Aug 15, 2026
Merged

alii merged 2 commits into
mainfrom
farm/82374b2d/ast-in-arg-names

Conversation

@robobun

@robobun robobun commented Aug 15, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • mordant's arg_named_like_other_param reports src/ast/lib.rs:1651, in Log::add_resolve_error_with_level: the local text is passed as BabyString::r#in's parent parameter, and r#in also has a parameter named text of the same type (&[u8]), so the call reads as if its arguments were swapped.
  • The call is not swapped. BabyString::r#in(parent, text) (src/ast/lib.rs:1080) searches for text inside parent and records the offset and length it found there; the caller wants the specifier located inside the formatted message, which is what r#in(&text, specifier_arg) does. The other two callers (src/jsc/VirtualMachine.rs:4698, src/runtime/jsc_hooks.rs:5496) pass the same order. Only r#in's parameter names are misleading: its text is the needle, while every caller's text is the haystack.

Fix

  • Rename r#in's parameters to container and substring. container is the name BabyString::slice already uses for the same string (a BabyString only means something against the string it was built from), and substring says what the second argument is. Callers are unchanged. No behavior change: the body is the same code with the names substituted.
  • mordant-baseline.toml regenerated with bun run rust:mordant:baseline. It drops the arg_named_like_other_param:src/ast/lib.rs entry this change fixes, plus two entries that were already stale on main because the findings were removed by changes that landed after the baseline was recorded in ci: bump the mordant pin (sixteen new lints) and record their baseline #38846: always_unwrapped_option:src/install/PackageInstall.rs (the Option<Walker> removed in install: let the walker own the cache dir it walks and build InstallDirState in one go #38271) and narrowed_two_ways:src/runtime/node/node_crypto_binding.rs (the key length made usize in crypto: store PBKDF2's key length as usize #37648). Happy to drop those two hunks if you would rather keep this to the one line.
  • Verified:
    • bun run rust:mordant (cargo-dylint 6.0.3, the mordant revision pinned in Cargo.toml) over the whole workspace: no findings and no target/mordant/over-baseline.txt (the CI gate), both against the baseline on main and against the regenerated one.
    • bun bd test test/js/bun/resolve/resolve-error.test.ts test/js/node/missing-module.test.js test/js/bun/resolve/import-meta.test.js test/js/bun/resolve/import-meta-resolve.test.mjs test/regression/issue/29264.test.ts: 71 pass. resolve-error.test.ts reads .specifier off runtime ResolveMessages and off Bun.build logs, which is the value r#in computes; import-meta-resolve.test.mjs covers the empty-specifier branch.
    • bun bd test test/js/bun/http/serve.test.ts -t "dev error page": 2 pass (the dev error page is the other reader of the stored offset).
  • No new test: Rust parameter names are not visible to callers, so no test can tell this change apart from main. The check for it is the mordant job in the Rust lints workflow, which runs against the regenerated baseline.

Background

  • BabyString (src/ast/lib.rs) packs a 16-bit offset and a 16-bit length into a u32. A resolve error stores its specifier this way, as a position inside the message's own text, instead of keeping a second copy of the string; BabyString::r#in builds it and BabyString::slice reads it back (src/jsc/ResolveMessage.rs, src/runtime/server/DevErrorPage.rs). The method is spelled r#in because in is a keyword; the name is carried over from the logger this was ported from.
  • mordant is the lint pack bun run rust:mordant runs in the Rust lints workflow. mordant-baseline.toml is a ratchet: per (lint, file) counts of the findings that predate the job, so a PR fails only when it adds one. Fixing a finding means regenerating the file so its entry disappears, which is the second hunk here.

r#in(parent, text) locates `text` inside `parent`, so every caller passes
its message text as `parent`, which mordant's arg_named_like_other_param
flags as a possibly transposed call. The calls are in the right order;
only the parameter names were misleading. Rename them to `container` and
`substring`, matching BabyString::slice. No behavior change.

Regenerate mordant-baseline.toml. Besides the entry this fixes, it drops
two that were already stale: the PackageInstall.rs Option removed in
#38271 and the node_crypto_binding.rs narrowing removed in #37648, both
of which landed after the baseline was recorded.
@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: 6 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: e2079180-7f18-4e17-8aa4-06ba800c07b1

📥 Commits

Reviewing files that changed from the base of the PR and between bf1437a and b886315.

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

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

@robobun

robobun commented Aug 15, 2026 •

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

@alii, your commit b886315 is building: #98499

@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, no behavior change.

What was reviewed:

  • BabyString::r#in body is byte-identical after the rename; index_of(container, substring) preserves the (haystack, needle) order and substring.len() replaces text.len().
  • All three callers (src/ast/lib.rs:1653, src/jsc/VirtualMachine.rs:4698, src/runtime/jsc_hooks.rs:5496) already pass (haystack, needle) — no call sites need updating, and the new container name matches BabyString::slice's existing parameter.
  • mordant-baseline.toml only drops entries (one fixed here, two stale from #38271/#37648); the ratchet tightens, never loosens.
Extended reasoning...

Overview

This PR renames the two parameters of BabyString::r#in in src/ast/lib.rs from (parent, text) to (container, substring) and adds a one-line doc comment. The function body is unchanged apart from the substituted identifiers: index_of(parent, text) → index_of(container, substring), text.is_empty() → substring.is_empty(), text.len() → substring.len(). The second file, mordant-baseline.toml, is the regenerated lint ratchet: it drops the arg_named_like_other_param:src/ast/lib.rs entry this rename resolves plus two entries the PR description identifies as already stale on main.

Security risks

None. This is a local identifier rename inside a 9-line helper with no change to control flow, argument order, types, or return value. No user input handling, no FFI, no allocation, no auth/crypto surface is touched.

Level of scrutiny

Low. The change is mechanical and self-evidently behavior-preserving — the function body diffs to the same AST modulo local names. I confirmed via grep that all three call sites pass the haystack first and the needle second, matching both the old and new parameter order, and that BabyString::slice (the paired reader) already names its parameter container, so the rename improves internal consistency rather than introducing a new convention. The baseline file is tool-generated; dropping entries can only make the CI lint gate stricter, not laxer, so even if one of the two "stale" removals were wrong the mordant job would catch it (and the PR description reports a clean bun run rust:mordant against both baselines).

Other factors

No CODEOWNERS cover these paths. No prior human review comments to address. The PR correctly ships no new test because nothing observable changes — existing coverage (resolve-error.test.ts, import-meta-resolve.test.mjs, the dev-error-page tests) exercises the value r#in computes and was reported passing. The bug hunting system found nothing.

@alii
alii merged commit 7d50fe5 into main Aug 15, 2026
9 of 10 checks passed
Comment thread src/ast/lib.rs
Comment on lines +1080 to +1081
/// Locates `substring` inside `container` (the string later passed to
/// [`BabyString::slice`]) and records its offset and length.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

Comment thread src/ast/lib.rs
Comment on lines +1084 to +1085
// needle, but an empty `substring` reaches this path via resolve errors
// for `import ""`, so short-circuit it here to offset 0.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

@alii
alii deleted the farm/82374b2d/ast-in-arg-names branch August 15, 2026 18:07

@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 (parent/text → container/substring) with no behavior change, plus the corresponding one-line mordant baseline drop.

What was reviewed:

  • Confirmed the function body is byte-identical modulo the renamed identifiers; index_of(container, substring) argument order matches the old index_of(parent, text).
  • container matches the existing name in BabyString::slice, so the two methods now agree on vocabulary.
  • The comment-cop flags on lines 1081/1085 are false positives: 1081 is a two-line rustdoc (not a workaround justification), and 1085 is a pre-existing comment that was only reworded (text → substring) and rewrapped.
  • The baseline diff removes only arg_named_like_other_param:src/ast/lib.rs; the two stale entries mentioned in the description were already gone after the merge with main.
Extended reasoning...

Overview

This PR renames the two parameters of BabyString::r#in in src/ast/lib.rs from parent/text to container/substring, adds a two-line rustdoc, and drops one line from mordant-baseline.toml. The rename resolves a mordant arg_named_like_other_param finding: callers pass a local named text as the first (haystack) argument, which read as if swapped against a second parameter also named text. Callers are unchanged. I diffed against bf1437a1 and confirmed the function body is identical except for identifier substitution and a rewrapped pre-existing comment.

Security risks

None. Parameter names are not part of the compiled artifact; no control flow, allocation, or boundary handling changes.

Level of scrutiny

Low. This is a mechanical clarity rename in the class of the other mordant-cleanup PRs already on main (#39130, #39129, #39119, #39125). The only executable-adjacent change is the identifier substitution inside the function body, which preserves argument order to bun_core::strings::index_of and to BabyString::new. The baseline hunk is a single deletion of the entry this rename fixes.

Other factors

The PR description mentions two additional stale baseline entries being dropped, but the actual diff between base and head only removes one line — the merge commit b8863156 from main appears to have already reconciled those. The two comment-cop bot comments are canned heuristic messages ("paragraph-long comment to justify a workaround") that misfire here: line 1081 is a two-line rustdoc cross-referencing BabyString::slice, and line 1085 is a pre-existing three-line comment whose only change is text → substring plus rewrap. Neither is a workaround justification, so I don't consider them blocking. The bug-hunting pass 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