fix(hir,stdlib): narrow the #7726 spread bail and complete the querystring bridge - #7734
Conversation
📝 WalkthroughWalkthroughThe change narrows native node-core sub-namespace dispatch to supported buckets and completes dynamic ChangesQuerystring native dispatch
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant NativeDispatcher
participant Querystring
Test->>NativeDispatcher: invoke dynamic querystring method
NativeDispatcher->>Querystring: route method or alias
Querystring-->>NativeDispatcher: return result
NativeDispatcher-->>Test: return value or undefined
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fced7b7 to
405a00c
Compare
Validation complete — flipping out of draftGap suite, both arms, same host (511 tests;
The 20-mismatch sets are byte-identical — all pre-existing, and none of those 20 files contains a spread call argument at all (only rest-param declarations and comments), so the guard cannot reach them. They are this host's known profile: npm-package tests needing The single crash-count difference is Final A/B matrix (32 cases, base vs this branch vs node 26.5.1): 17 fixed / 14 same / 1 wrong→wrong / 0 regressed. The one wrong→wrong is the Other checks: |
…tring bridge Follow-up to #7726, which routed node-core module spread calls through the variadic runtime dispatch. A 32-case A/B matrix (this tree vs. the same tree with the guard forced to `false`, both against node 26.5.1) found two calls that were CORRECT before and `undefined` after, plus two wrong-to-differently-wrong conversions. All four are addressed here; the matrix is now 17 fixed / 13 same / 2 wrong-to-wrong / 0 regressed. 1. `querystring.escape(...args)` regressed to `undefined`. Root cause is older and wider than the spread bail: `nm_dispatch_querystring` advertises escape/unescape/stringify/encode/parse/decode, but the stdlib bridge it calls (`js_querystring_native_dispatch`) implemented ONLY `unescapeBuffer` and fell to `_ => undefined` for everything else. So on main every indirect form was already silently undefined — `const d: any = qs; d.escape("a b")`, `const e = qs.escape; e("a b")` — while the statically dispatched `qs.escape("a b")` was correct. #7726 merely routed spread calls onto that hole. Wire the remaining six names to the `js_querystring_*` entry points that already existed (`encode`/`decode` are Node's aliases for `stringify`/`parse`), which fixes the regression and the pre-existing captured/dynamic forms together. 2. `fs.promises` / `dns.promises` are not dispatch buckets. The predicate recursed through any sub-namespace receiver and treated any `<module>/<export>` that happened to be a node-core module name as a namespace. `nm_module_index` has DOTTED tags only for `path.posix`, `path.win32`, `util.types`, `crypto.subtle`/`webcrypto` and `punycode.ucs2`; there is no `fs.promises` bucket. Diverting the bucket-less ones produced a silent `undefined` (`dns.promises.lookup(...args)`) and a synchronous `TypeError: value is not a function` where a rejected promise used to arrive (`import { promises } from "node:fs"`). Replace the derivation with an explicit allowlist, and reject the slash sub-module tags: the direct import (`import fsp from "node:fs/promises"`) already reaches the generic tail without the bail, measured identical on both arms, so excluding them costs nothing. Known and deliberate: `events.listenerCount(...args)` still changes a bogus `ERR_INVALID_ARG_TYPE` throw into `undefined` — `nm_dispatch_events` implements only `init` and `EventEmitterAsyncResource`, so the dispatcher has no arm to reach. Both forms are wrong (node returns a count); completing that dispatcher is its own change. Tests: `sub_namespace_allowlist_is_the_runtime_bucket_set` pins the allowlist against exactly the re-derivation that shipped, `bucketless_sub_namespaces_keep_ their_lowering` pins the two HIR verdicts that changed, and `node-suite/querystring/aliases/dynamic-dispatch.ts` byte-compares the static, captured, dynamic and spread forms of six querystring methods against node.
405a00c to
8af631f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Cargo.toml`:
- Line 318: Revert the workspace package version change at Cargo.toml lines
318-318 and restore the prior value. Also revert the Current Version change in
CLAUDE.md lines 11-11; both release metadata updates must be removed while
retaining the PR-keyed changelog fragment.
In `@crates/perry-hir/src/lower/expr_call/native_module_spread_tests.rs`:
- Around line 142-148: Add the ("crypto", "webcrypto") pair to the
module/submodule cases in sub_namespace_has_dispatch_bucket, preserving the
existing cases so the test covers every allowed dispatch bucket.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 12b4c5d2-8b60-481c-8c46-dc2ebac00476
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
CLAUDE.mdCargo.tomlchangelog.d/7734-narrow-spread-bail-querystring-bridge.mdcrates/perry-hir/src/lower/expr_call/native_module.rscrates/perry-hir/src/lower/expr_call/native_module_spread_tests.rscrates/perry-stdlib/src/querystring.rstest-parity/node-suite/querystring/aliases/dynamic-dispatch.ts
|
|
||
| [workspace.package] | ||
| version = "0.5.1425" | ||
| version = "0.5.1426" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep release metadata out of the contributor change.
The PR-keyed changelog fragment is present, so the contributor change should not also update release/version metadata. Maintainers must own the merge or release bump.
Cargo.toml#L318-L318: Revert the[workspace.package].versionchange.CLAUDE.md#L11-L11: Revert theCurrent Versionchange.
As per coding guidelines, external contributors must not modify the workspace version. Based on learnings, maintainers own the corresponding CLAUDE.md and Cargo.toml release metadata when a PR-keyed changelog fragment is present.
📍 Affects 2 files
Cargo.toml#L318-L318(this comment)CLAUDE.md#L11-L11
🤖 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 `@Cargo.toml` at line 318, Revert the workspace package version change at
Cargo.toml lines 318-318 and restore the prior value. Also revert the Current
Version change in CLAUDE.md lines 11-11; both release metadata updates must be
removed while retaining the PR-keyed changelog fragment.
Sources: Coding guidelines, Learnings
| for (module, sub) in [ | ||
| ("path", "posix"), | ||
| ("path", "win32"), | ||
| ("util", "types"), | ||
| ("crypto", "subtle"), | ||
| ("punycode", "ucs2"), | ||
| ] { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover crypto.webcrypto in the allowlist test.
sub_namespace_has_dispatch_bucket allows ("crypto", "webcrypto"), but this test omits it. Add the pair so the test covers the complete allowlist.
Proposed fix
("util", "types"),
("crypto", "subtle"),
+ ("crypto", "webcrypto"),
("punycode", "ucs2"),📝 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.
| for (module, sub) in [ | |
| ("path", "posix"), | |
| ("path", "win32"), | |
| ("util", "types"), | |
| ("crypto", "subtle"), | |
| ("punycode", "ucs2"), | |
| ] { | |
| for (module, sub) in [ | |
| ("path", "posix"), | |
| ("path", "win32"), | |
| ("util", "types"), | |
| ("crypto", "subtle"), | |
| ("crypto", "webcrypto"), | |
| ("punycode", "ucs2"), | |
| ] { |
🤖 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 `@crates/perry-hir/src/lower/expr_call/native_module_spread_tests.rs` around
lines 142 - 148, Add the ("crypto", "webcrypto") pair to the module/submodule
cases in sub_namespace_has_dispatch_bucket, preserving the existing cases so the
test covers every allowed dispatch bucket.
Merging as v0.5.1426 — and I should have caught these before merging #7726I merged #7726 on its own validation plus my verification of its vacuity finding. I did not run the node-core spread surface myself, and your A/B matrix found two The matrix is the right instrument, and it isolates the variable properly — same stdlib in both arms, guard forced
Holding the 17 fixes while taking the regressions to zero is the outcome that matters. The querystring finding is much older than #7726, and I verified it
match name {
"unescapeBuffer" => { … }
_ => undefined,
}One arm. So every indirect form — The
|
1 participant
Follow-up to #7726 (issue #7720). Opening as a draft until the baseline gap-suite arm finishes; the substance below is already measured.
#7726 was merged before my validation finished. It carried two regressions. This corrects them at the root and narrows the predicate to what is measured, not derived.
How this was measured
A 32-case A/B matrix over the node-core spread surface, run on one host, three ways per case:
false(byte-identical to pre-fix(hir): route node-core module spread calls through the variadic dispatch #7726 lowering, same stdlib in both arms so the comparison isolates the HIR change)1.
querystring.escape(...args)— regressed toundefinedThe root cause is older and wider than the spread bail.
nm_dispatch_querystringadvertises the whole set:…but the stdlib bridge it calls,
js_querystring_native_dispatch, implemented onlyunescapeBufferand fell to_ => undefinedfor the other six. So onmain, before any of this, every indirect form was already silentlyundefined:#7726 only routed spread calls onto that pre-existing hole. Every
js_querystring_*entry point already existed; only the bridge was missing. Wiring the six names up (encode/decodeare Node's aliases forstringify/parse) fixes the regression and the captured/dynamic forms in one go.2.
fs.promises/dns.promisesare not dispatch bucketsThe predicate recursed through any sub-namespace receiver and accepted any
<module>/<export>that happened to be a node-core module name. Butnm_module_indexhas dotted tags only forpath.posix,path.win32,util.types,crypto.subtle/webcrypto,punycode.ucs2. There is nofs.promisesbucket, so diverting them produced:dns.promises.lookup(...args)→ silentundefined(was a throw)import { promises } from "node:fs"; promises.readFile(...args)→ synchronousTypeError: value is not a functionwhere a rejected promise used to arrive — the worst of the four, since a.catch()no longer catches itReplaced the derivation with an explicit allowlist, and rejected the slash sub-module tags. The direct import (
import fsp from "node:fs/promises") needs no help from the bail — it already reached the generic tail before #7726 (measured: identical HIR andENOENTon both arms) — so excluding the slash tags costs nothing.Known and deliberate
events.listenerCount(...args)still turns a bogusERR_INVALID_ARG_TYPEthrow intoundefined.nm_dispatch_eventsimplements onlyinitandEventEmitterAsyncResource, so there is no arm to reach; both the old and new behaviour are wrong (node returns a count). Completing that dispatcher is the same shape of change as the querystring bridge here and deserves its own PR. Excludingeventsfrom the bail would be a one-line denylist entry if you would rather have the loud failure in the meantime — say the word.Tests
sub_namespace_allowlist_is_the_runtime_bucket_set— pins the allowlist against exactly the re-derivation that shipped (assertsfs.promises/dns.promises/stream.webare not in it)bucketless_sub_namespaces_keep_their_lowering— the two HIR verdicts that changed.fs.promises.readFile(...)is deliberately absent: it already reached the generic tail before fix(hir): route node-core module spread calls through the variadic dispatch #7726, so asserting on it would test nothingnode-suite/querystring/aliases/dynamic-dispatch.ts— static / captured / dynamic / spread forms of six querystring methods, byte-compared against node; verified identicalcargo test -p perry-hir --lib— 9/9 in the spread suite, 287 in the crateSummary by CodeRabbit
Bug Fixes
escape,unescape,stringify/encode, andparse/decode.Tests
Documentation