chore: merge train 228 (v0.5.1607) - #10764
Merged
Merged
Conversation
added 5 commits
September 20, 2026 00:25
A top-level CommonJS `require()` in a conditional position never ran its target when that target carried no CommonJS export marker. The branch was taken, the shim was reached, and the module body never executed — silently, with no crash and no diagnostic. #10754 reports this as a call-site-shape problem (`if` works, `&&` / `?:` / `try` / `switch` / loop-body do not). That is a confound in the reproducer: its `if` case required a module with `module.exports = 1` while the other five required side-effect-only modules. Crossing the two axes against Node 26.5.1 on a release build of main @ 91a566c shows the discriminator is the TARGET's export shape — all six shapes fail with a side-effect-only target, all six pass with a value-returning one. #10674 defers a conditional require correctly: the target stays `ModuleInitKind::Deferred` and the shim returns the `_lazyreq_N` import binding, with codegen firing `<S>__init()` at the binding read. That init call is gated on the binding being a known imported FUNCTION (`ctx.import_function_prefixes`), which a target with no default export never is — so nothing fires. - `cjs_wrap/deferred_requires.rs`: an AST visitor replaces the text-scanning deferral classifier, which missed `if (cond) x = require('S')` (still eagerly hoisted on main, a residual #10437 shape), concise arrows, the ternary ALTERNATE arm, both halves of a `do`/`while`, and `&&=`/`||=`/`??=`. `extract_requires::function_local_specs` stays as the parse-failure fallback. - `cjs_wrap/wrap.rs`: a deferred specifier resolves through the path registry rather than its import binding, so initialization no longer depends on the target's export shape; the registry record is memoized per call site once `loaded === true`, because re-entering the registry on every call measured 3.4x on a hot require. - `cjs_wrap/wrap.rs`: the registry only holds EXPORTS for a target that publishes them, which is every CJS-wrapped module and no other. A file with no CommonJS marker is not CJS-wrapped, so it registers an initializer and never any exports, and the registry returned `undefined` where Node returns `{}`. The arm falls back to the import binding on a genuine registry miss, discriminated by `__perry_has_path_module` — the same guard the generic runtime-`require(path)` arm in the same wrapper already uses. - `perry-codegen/src/expr/dyn_extern_i18n.rs`: fire the deferred `__init()` before the imported-class and namespace fast paths, which can themselves depend on module initialization. The implementation is PR #10285's, rebased onto current main; the registry-miss fallback and the gap fixture are new here. `test_gap_10754_cjs_conditional_require_shapes.cts` crosses all six shapes with three cells each — taken/side-effect-only, taken/value-returning and not-taken — because a fix that loads the module unconditionally is #10437 again, not a fix. On unfixed main it differs from the Node oracle on 12 lines (six side-effect-only targets never load; three value-returning targets load before the program's first statement instead of at their call site) and the harness reports parity_fail; with this change it matches Node byte-for-byte and the harness reports PASS. Closes #10754
customAlphabet(alphabet, size) is documented to return a generator function; native customAlphabet instead returns the generated id string directly (js_nanoid_custom's own doc comment: "For simplicity, we combine this into one call"), so the only documented usage -- const gen = customAlphabet(...); gen(); -- crashes with TypeError: value is not a function. Per #10678 (duplicate extern "C" exports across perry-ext-*/perry-stdlib pairs), this binding existed twice: crates/perry-ext-nanoid/ (governance- tracked) and crates/perry-stdlib/src/nanoid.rs (a second, independent implementation behind the default-on bundled-nanoid feature, exporting the same js_nanoid/js_nanoid_sized/js_nanoid_custom symbols). customAlphabet is declared to codegen (data_stores.rs's js_nanoid_custom) but has no call-site wiring anywhere in perry-codegen -- no NativeModSig row, no lower_call special case. Only plain nanoid(size) had a dispatch row (native_table/utils_crypto.rs, routing to js_nanoid_sized), consistent with customAlphabet not being a first-class compiled call at all. Removed both crates, the 1-entry NativeModSig dispatch row, the 2 js_nanoid* FFI declarations, the well_known_bindings.toml entry, the "nanoid" NATIVE_MODULES entry + manifest row, the bundled-nanoid stdlib feature, and 2 Android stub exports. DELETED the shipped_unproven_bindings_are_partial test rather than emptying it. Its subject population was exactly the hand-written wrappers shipped without proven upstream parity: uuid (#10701), dotenv (#10691, landed in train 227) and nanoid (here). With the last one gone the array would read `for name in []` -- a test that compiles, runs, asserts nothing and reports green forever, which is failure mode #4 in CLAUDE.md's "four ways a gate can be unable to fail". Coverage is not lost: shipped_subset_bindings_are_partial is a separate test and still asserts the same property for undici, node-forge, lru-cache and qs. Verified the deletion orphans nothing -- the test module is `use super::*`, lookup_well_known has 9 other callers, BindingCompat::Partial has 5 other uses, and nothing in the tree keys on the test's name. The "ids" umbrella is now EMPTY. It was retargeted to ["bundled-nanoid"] when #10701 removed uuid (train 225); removing bundled-nanoid leaves it with no members, so it is kept as `ids = []` -- an intentionally harmless no-op that preserves `--features ids` for existing callers rather than breaking them. The stale comments that described the two-member split (perry-stdlib/Cargo.toml, perry-stdlib/src/lib.rs, stdlib_features.rs) are rewritten to say so. perry-stdlib's `uuid` crate dependency is untouched: #10701 already made it non-optional because crypto/random.rs calls it unconditionally. test-files/test_parity_nanoid.ts (the exact customAlphabet(...)(); reproduction) is already excluded from the parity gate -- known_failures.json classifies it "ci-env": Node's own oracle fails with ERR_MODULE_NOT_FOUND in CI because nanoid was never added to the repo's root package.json, so npm ci never installs it. Not touched -- provisioning a real npm dependency in the root package.json is out of scope for a binding-removal PR, and the test remains excluded before and after this change for the same underlying reason. Every absolute count re-derived from its own script against the resolved tree, never carried across the rebase and never hand-merged: workspace-architecture.json 77 -> 76 members, externalize 28 -> 27, keep 44 (scripts/workspace_architecture.py; git auto-merged this file with NO conflict and left the stale 77/28, which the script caught); docs/api/perry.d.ts 2065 entries/131 modules -> 2064/130 and docs/src/api/reference.md 3007/133 -> 3006/132, both regenerated by running the built binary's --print-api-manifest rather than editing the headers; Cargo.lock regenerated with `cargo metadata`. native_result_ledger (376 rows/326 providers), unrooted_local_shape (578) and string_payload_access (perry-stdlib inline-offset 37) confirmed unchanged by running them, not by assuming. Rebased onto v0.5.1606 (train 227). Conflicts were all of the form "main deleted dotenv, this branch deleted nanoid, in one hunk"; every one was resolved to main's current content minus nanoid's own entries, which is neither side.
This was referenced Sep 19, 2026
|
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 (46)
✨ 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 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
1 participant
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 228 — two PRs validated together as one tree, released as v0.5.1607.
Trains land as their own PR, so the source PRs are closed, not merged, and their close-keywords never fire.
Contents
fix(cjs): evaluate a conditionalrequire()whatever the target's export shaperefactor(stdlib): remove the nanoid native binding, compile the real package from source#10756 — the bug was not what its issue originally said
Reported as "a conditional
require()in short-circuit, ternary,try,switchor loop position is never evaluated, whileifworks". That framing was wrong, and the reproducer that produced it was confounded: theifcase required a target withmodule.exports, the other five required side-effect-only targets.Crossing both axes properly — 6 call-site shapes × {side-effect-only, exporting} × {taken, not taken}, 24 cells, two release arms — gives the real rule: all six shapes fail for a target with no default export, and all six work for one that has it. The discriminator is the target, not the syntax.
That matters because it relocates the defect. There is exactly one evaluation site per specifier —
wrap.rsemits a singleif (specifier === 'S')arm in the syntheticrequire()shim, reached whatever syntax surrounds the call — so "a site forifbut not the others" was never structurally possible. What varied is that the arm fires<S>__init()only when the binding is inctx.import_function_prefixes, i.e. only when the target has a recognized default export.The fix routes through
__perry_require_path_module, which is export-shape-independent, with a registry-miss fallback: a file carrying no CommonJS marker is never CJS-wrapped, so it registers an initializer and never exports, and routing it through the registry alone returnedundefinedwhere Node returns{}. Nothing in the predecessor's own 11-test suite caught that; the new gap fixture did.Both directions are asserted for all six shapes — loads when taken, does not load when not — because the defect this replaces (#10437) was eager loading, and a fix that always loads would satisfy a naive test while reintroducing it. Against the Node oracle: 12 lines differ on unfixed main, byte-for-byte match with the fix, in both default and
--no-auto-optimizemodes. Verified through the gate rather than only by hand —parity_failand harness exit 1 on main,PASSwith the fix.#10693 — and the silent auto-merge this train caught
workspace-architecture.jsoncame through the rebase asM, notUU: git merged it with zero conflict markers and preserved main's77/28/44intact. That value is wrong — the script says 76/27/44. Nothing in the diff, the conflict list, orMERGEABLEsignalled it; only runningworkspace_architecture.py --checkdid, which exited 1.This is the mechanism described in #10739 firing in practice: absolute counts sit on different JSON lines from the crate entry a removal deletes, so both sides merge cleanly. The defence cannot be "check whether the sides disagree" — there is nothing to compare. Every absolute count in this train was re-derived from its own script against the assembled tree, never carried or adjusted.
shipped_unproven_bindings_are_partialis deleted, not emptied. Its subject population was exactly uuid, dotenv and nanoid; all three are now gone, and the natural "take both deletions" resolution would have producedfor name in []— a test that compiles, runs, asserts nothing and reports green forever.shipped_subset_bindings_are_partialstill covers undici, node-forge, lru-cache and qs.Acceptance: real nanoid 5.1.16 (resolving the PR's declared
^5.0.7, which is a different major from the6.0.0the deleted registry entry pinned), nocompilePackagesentry, byte-for-byte identical to Node 26.5.1. Liveness asserted rather than assumed —nmshows zerojs_nanoid*symbols in both archives and the produced binary.Validation
Assembled on
7fe8009492; source heads asserted unchanged; both PRs proven fully represented — zero dropped. Counts on the assembled tree: workspace 76/27/44, ledger 376 rows / 326 providers,unrooted_local_shape578,check_file_sizeOK, zeroperry-ext-nanoidin the regenerated lock, andfor name in []confirmed absent.All nine cheap gates,
cargo check --workspace --all-targetsunder-D warnings, the release build of all five pinned artifacts, every unit suite, and a 6-area gap sweep with zero unexplained regressions and every area asserted to have run a non-zero number of tests, atPERRY_RUN_TIMEOUT=30.lintcompleted its full 6-of-6 compile tier with nothing outside the known-red public-baseline step.Issues resolved
Closes #10754