chore: merge train 242 (v0.5.1621) - #10830
Merged
Merged
Conversation
…mplete, net Socket surface)
…osures URLSearchParams's `#10555` arm in populate_builtin_prototype_methods only installed a Symbol.toStringTag descriptor, on the assumption that its methods are always reached through type-directed static dispatch or the small-int/handle dispatch tables. That assumption breaks for a method read AS A VALUE: `URLSearchParams.prototype.append`, `.prototype["has"]`, or a Proxy `get` trap indirection all returned `undefined` instead of a callable, name-carrying closure. node-fetch@3.3.2's `Headers extends URLSearchParams` returns a Proxy from its constructor whose `get` trap does exactly this (`URLSearchParams.prototype[p].call(target, ...)`), and `headers.has(...)` is reached on every `fetch()` call before the request is sent, so this threw "Function.prototype.call was called on a value that is not a function" on the very first fetch. Install the same no-op-backed reified-closure set the neighboring Headers/ URLPattern/Request/Response arms already use, dispatched by name through try_url_search_params_dynamic_dispatch. The toStringTag install is kept so reflection on URLSearchParams.prototype itself doesn't regress. Added test-files/test_gap_10759_urlsearchparams_prototype_method_value.ts, verified byte-identical against node --experimental-strip-types (v26.5.1).
…base
class X extends Stream (the bare node:stream base that Readable/Writable/
Duplex/Transform themselves derive from) never installed the EventEmitter
listener/emit surface, the pipe() method, or the instanceof Stream class
edge, for ANY heritage shape reaching it (bare import, namespace member,
or CJS destructured require('stream')) -- #10649's dynamic bound-export
dispatch in js_fetch_or_value_super stopped short of Stream, and Stream
was never in canonical_native_parent_name's static recognition list
either.
Stream carries no hidden per-instance state (unlike Readable/Writable/
Duplex/Transform's _readableState/_writableState): in Node it is
EventEmitter plus one added prototype method, pipe(). Reuse the existing
EventEmitter-shaped install (a new js_node_stream_legacy_subclass_init,
which is js_event_emitter_subclass_init plus pipe) rather than adding a
stream-state shim that would duplicate work Stream doesn't need.
instanceof Stream needed its own hop in the class-id chain rather than
collapsing onto EventEmitter's id (which #10430 already registers for
`new X() instanceof EventEmitter`): js_instanceof walks the full chain,
so class_id -> CLASS_ID_STREAM -> CLASS_ID_EVENT_EMITTER keeps
`instanceof EventEmitter` true transitively while making
`instanceof Stream` true ONLY for a genuine extends-Stream subclass --
collapsing both onto one id would have made a plain `extends EventEmitter`
class wrongly satisfy `instanceof Stream` too.
PassThrough (#10745) is a separate, deeper HIR-level gap and is
unaffected by this change, as expected -- canonical_native_parent_name
still doesn't recognize it, so the hidden _transform field its shim
reads is still never pre-seeded.
Investigation note: the literal "is not a constructor" TypeError #10798
describes does not reproduce for genuine `extends Stream` usage (bare,
namespace-member, or CJS-destructured, with or without an explicit
constructor) -- confirmed against a pristine build before this fix, and
via nodemailer 9.0.3's own real internal usage (smtp-transport.js's
`new XOAuth2(authData, this.logger)`, intra-package). It DOES reproduce,
identically, for a plain class with NO heritage at all, when a compiled
package's internal file is require()'d directly from OUTSIDE that
package (e.g. require("nodemailer/lib/xoauth2") from a top-level
project file) -- a pre-existing, heritage-independent bug in
perry.compilePackages's cross-module class export, unrelated to Stream
and out of scope here.
MAP_STRING_INDEX's inner table (map_ptr -> content_hash -> Vec<entry_idx>) hashed its `u64` key with std::collections::HashMap's default SipHash. That key is not raw input -- it is already the FNV-1a content hash computed one layer up, so every string-keyed Map.get/set/has/delete past SIDE_TABLE_THRESHOLD paid for a second, unrelated hash of an already-mixed value. Switched the inner table to PtrHasher (one multiply by the Fibonacci constant plus an xorshift avalanche), the same treatment the sibling NumericIndex.hashed table already gets for hashing a computed, non- adversarial u64. Not a HashDoS regression: two colliding u64 FNV-1a hashes land in the same Vec<u32> bucket under any hasher, so SipHash on the outer table could never have defended against a crafted FNV-1a collision -- the attack surface is FNV-1a itself, unchanged here. HashMap<K, V, S>::get also re-checks K: Eq on every candidate regardless of S, so a hasher swap cannot change which entry a lookup resolves to, only how fast it gets there. New test `string_index_resolves_every_key_correctly_past_the_hashed_threshold` inserts 10,000 distinct string keys (forcing real bucket collisions at both the outer map-pointer and inner content-hash levels) and checks reverse-order reads, content-equal-but-freshly-allocated keys, adversarial shared-prefix misses, and delete-half-then-verify. Measured as a flat constant-factor win across an N-sweep from 16 to 4,096 map entries (well past the growth threshold): 115-121 instructions saved per lookup at every size tested, for interned hits, dynamically-built hits, and misses alike -- not a small-map-only effect. A first pass on one shape (dynamically-rebuilt keys) showed an apparent ~38-instruction-per- doubling growth with N; that was a probe artifact (the decimal key suffix grew a digit as N grew, correlating length with N) and vanished under a length-controlled follow-up. Not a fix for #10697 (string-keyed Map slow on small maps): that turned out to be a codegen type-proof gap in the generic dispatch path (find_key_index_cold / jsvalue_eq / string_view_from_bits re-deriving a key already proven to be a string), tracked and owned separately. Gap test: test_gap_map_get_string_key_perf.ts, byte-identical to node 26.5.1 -- SameValueZero (NaN, +0/-0), insertion-order iteration, has vs get for a stored undefined, delete-then-reinsert ordering, non-ASCII and lone-surrogate keys, content-equal-but-distinct-identity keys, a map past the growth threshold.
…t N calls
const {a, b, ...rest} = obj built the excluded-key array via one
js_array_alloc_with_length call plus one js_array_set_f64_unchecked call
per excluded key. Each per-key call re-derives and re-bounds-checks a
receiver the site just allocated itself, so every check inside (frozen?,
has index descriptors?, index in range?) was statically true. The
excluded keys are compile-time-known string literals, so route them
through lower_array_literal instead -- the same inline bump-allocation
path an ordinary [k1, k2, ...] literal already takes, previously reused
only for the rest/arguments call-bundle case. Also reorders so the
source object's pointer is derived after that allocation instead of
cached across it.
Found and documented (not fixed, unrelated to this change): {...rest}
drops Symbol-keyed properties from the source object instead of copying
them through -- reproduces identically on unmodified main.
Linking `regex-engine` replaces `String.prototype.split` wholesale (see
`string::mod`), so a program that uses a regex anywhere ran every split through
the engine's per-UTF-16-unit subject reader -- even `"a b".split(" ")`, where the
engine has nothing to contribute. Measured on one source compiled twice:
split(" "), engine linked 37,558 instructions
split(" "), engine absent 3,662
node 26.5.1 2,714
Both arms auto-optimized, so that is the implementation swap rather than the
build mode; specialization accounts for about 5% of it. Roughly 48% of the
engine path is `Units::at`, `BoundSpan::retarget`, `Cursor::next_unit` and
`copy_units`.
The plain algorithm now answers the call when it provably agrees, below the
`@@split` check so a custom splitter still wins. Three input classes are
excluded rather than repaired, because the two implementations genuinely differ
on them:
* a separator holding a lone surrogate, which the engine matches against one
half of a valid pair and a WTF-8 byte scan cannot;
* a separator that is not already a string, whose ToString can run user code
or throw -- a Symbol must raise TypeError;
* a `limit` that is not already a number or undefined, whose ToNumber can
throw.
Everything excluded takes the engine path, so this only narrows what the fast
path answers. The plain algorithm also reports failure by throwing where this
module returns Err, so the call is wrapped in `api::caught`.
split(" "): 37,558 -> 5,081 instructions, -86.5%, 13.84x -> 1.87x node
Answers are identical to Node on 27 cases where a byte scan and a unit scan can
disagree -- empty separator, separator longer than the subject, every `limit`
form, lone surrogates, an astral pair split by units, a separator that is a
prefix of itself at the tail -- and on 12 non-string separator forms including
`@@split` callable and not callable.
…s bound functions dispatch_bound_function unconditionally built a Vec<f64>, copied the call-time args into it, and freed it before every Function.prototype.bind call -- even for the overwhelmingly common .bind(thisArg) method-reference shape with no partial-applied arguments, where js_function_bind already leaves the bound-args capture null. That shape is exactly what direct.rs's per-loop dispatch hoisting excludes (BoundFunction is deliberately not resolved by resolve_direct_func_ptr), so a bound method used as an arr.forEach callback paid this allocation on every element.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (35)
✨ 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 |
This was referenced Sep 20, 2026
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge train 242 — eight PRs, released as v0.5.1621.
Contents
fix(runtime): dispatchnode:streamsuper()through the legacy Stream pathfix(runtime): install URLSearchParams prototype methods as reified propertiesperf(runtime): stop double-hashing Map's string content-hash side tableperf(codegen): build ObjectRest's excluded-key array as a literalperf(regex): answer a plain-string split without the enginefix(api-manifest): add 15 missing dispatch-table entriesperf(runtime): skip the arg-combine Vec for no-partial-args bound callsdocs(runtime): narrow the diagnostics feature comment30 files, +1,514 / −101. Eight PRs travel together because their file sets are disjoint — checked before assembly, not discovered during it. They span genuinely separate subsystems, so bundling costs nothing in attributability.
Two of the eight carried defects, and the train fixed both
The first validation aborted at lint with two unexpected failures. Neither PR's own CI caught either, and neither is the kind of thing a reviewer would spot by reading.
#10816 — an unused variable that only exists in some builds.
string/split.rsboundsep_jvunconditionally while reading it only inside#[cfg(feature = "regex-engine")], soRUSTFLAGS="-D warnings" cargo check -p perry --binsfailed.Why review missed it is the interesting part: a one-invocation whole-workspace build unifies cargo features, so the regex engine is always on and the binding is always read. Only the per-package
-p perry --binscommand — one of the sixrun_lint_gates.shderives — lacks that unification. Same shape ascargo check --libnot compilingcfg(test)code: the narrower command is the one that tells the truth. Gated behind the feature that reads it; the exact failing command now returns rc=0 with zero warnings. Verifiedlim_jvon the following line is genuinely used outside the block rather than assuming it shared the problem.#10817 — 15 manifest entries added without regenerating the docs.
docs/src/api/reference.mdstill read 2855. Regenerated from a built binary: 2855 → 2870, exactly the 15 the PR adds, withdocs/api/perry.d.tscorrectly unchanged at 2026 because those rows are dispatch-table entries rather than public API surface.Checked against that script's own failure mode:
regen_api_docs.shhardcodes<worktree>/target/release/perry, and with the binary absent it regenerates from nothing and leaves both files truncated. A real regeneration moves the header counts and leaves the tail intact. Both tails were verified present, andperry.d.tsholding at 2026 is the corroboration — a truncating run would have emptied it too.it_manifest_consistencypasses on the assembled tree, which is stronger than the drift check alone: a green drift check only proves the files match the binary, that suite proves the manifest is internally consistent.A third defect, in the validation driver itself
The first run's abort left the worktree dirty and blocked every subsequent run, presenting as "the tree is dirty" rather than "lint failed" — two steps from the cause.
scripts/run_lint_gates.shis not read-only:regen_api_docs.shrewrites both docs files as a side effect. The driver restores them — but that restore sat after the lint assertions, so an aborting lint never reached it. A cleanup that only runs on the success path is not cleanup. Moved to immediately afterlint, before anything can abort.One representation absence, correct
verify()flagged 11 missing insertions in #10817 — anexponential-backoffmanifest entry. Merge train 240 removed that binding, and it is now absent from both the manifest andwell_known_bindings.tomlonmain. Restoring it would have re-added a row for a binding that no longer exists and failed manifest sync. The PR was simply cut against an older base.Validation
Assembled on
80434ce650; all eight source heads asserted fresh; no attribution trailers. Ten cheap gates,cargo check --workspace --all-targetsunder-D warnings, all five pinned artifacts byte-identical before and after, seven unit suites with an empty failing set, andlintcomplete at 6-of-6 with nothing outside the known-red public-baseline step on the re-run.Both compiler-output suites at
failed_workloads=[];repsel_census rc=0 wasted_promotion=False.Gap sweep at
PERRY_RUN_TIMEOUT=30, eight areas chosen one-per-PR, 250 fixtures, every area asserted live, zero unexplained regressions: