refactor(stdlib): remove cron/exponential-backoff/moment/node-forge native bindings - #10795
proggeramlug wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (57)
💤 Files with no reviewable changes (38)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe PR removes bundled ChangesBundled binding removal
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~60 minutes Change: Refactor 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 68.42% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 13 files. (6 skipped: 6 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
4aeb292 to
ad7c072
Compare
|
Queued as the next train, behind #10793+#10787 which are validating now. Reading it, the structure is right and worth saying so explicitly, because it is the shape the campaign needed after #10765. Probing nine and removing four is the correct order. #10765 had to pull Three details I noticed that suggest the fixtures were built by someone who had thought about reproducibility, and which I would rather not see simplified away later:
On What the train will check, flagged now so none of it is a surprise:
|
…ative bindings
Removes the perry-ext-cron, perry-ext-exponential-backoff, perry-ext-moment,
and perry-ext-node-forge crates (and each package's hidden perry-stdlib-side
duplicate: cron.rs, exponential_backoff.rs, moment.rs), plus their
well_known_bindings.toml rows, NATIVE_MODULE_TABLE dispatch rows, HIR
special-casing, FFI declarations, and API-manifest entries.
Real npm source for all four now compiles cleanly via perry.compilePackages
and matches node --experimental-strip-types byte-for-byte on a fixture
exercising each package's primary documented use:
- cron 4.4.0: CronJob tick scheduling + CronTime pattern parsing (2 modules)
- exponential-backoff 3.1.3: backOff retry/backoff/predicate paths (1 module)
- moment 2.30.1: parse/format/arithmetic/diff/duration (1 module)
- node-forge 1.4.0: RSA keygen + X.509 build/sign/verify + PEM round-trip
(1 module)
cron's two event-loop liveness FFI symbols (js_cron_timer_tick /
js_cron_timer_has_pending) stay as unconditional 0-returning stubs -- the
generated entry loop calls them every iteration regardless of whether a
program uses cron at all.
Five other probed packages in the same 9-package batch are NOT removed:
- cheerio 1.2.0: cheerio.load() returns a callable object decorated with
static/prototype properties (Object.assign(initialize, staticMethods,
{...})); calling it as a function ($('h2')) throws
'TypeError: string "h2" is not a function' under Perry, even though
'typeof $' correctly reports "function". Real compiler defect, not
attempted here.
- ioredis 5.11.1: functionally correct (stdout byte-for-byte identical to
node; two Perry-only stderr warnings about a circular '.default'
access that node's loader doesn't hit for the same import -- benign,
stdout-only comparison is this repo's convention). NOT removed: the
literal string "ioredis" is shared infrastructure, not just an npm
package name. It is (a) the well_known_bindings.toml lookup key that
EXT_PREFIX_REGISTRY's 'js_ioredis_*' prefix resolves through for
iovalkey and redis's own static-lib auto-linking (see
perry-codegen/src/ext_registry.rs's own comment: "ioredis, iovalkey,
and valkey all share this wrapper... so the single 'ioredis' binding
key covers every RESP package"), and (b) the internal canonical
dispatch key that iovalkey/redis's bare 'Redis' type-name resolution
routes through in 8 HIR/codegen files (static_and_instance.rs,
native_new.rs, module_decl.rs, local_natives.rs, misc.rs, fn_decl.rs,
expr_function.rs, ir/module.rs). Deregistering it would break iovalkey
and redis, which are staying.
- nodemailer 9.0.3: confirms the known #10649 gap -- nodemailer's
lib/xoauth2/index.js does 'class XOAuth2 extends Stream' against the
BARE node:stream Stream (not Readable/Writable/Duplex/Transform, which
#10649 fixed). 'new XOAuth2(...)' throws 'TypeError: is not a
constructor'. This is loaded unconditionally on require("nodemailer")
(nodemailer.js -> smtp-transport -> xoauth2), so any OAuth2 SMTP auth
(e.g. Gmail) is broken under a real-source compile, even though the
narrower JSONTransport/sendMail smoke path compiles and runs correctly.
- undici 8.9.0: real source compiles (112 modules) but request() throws
"TypeError: Cannot read properties of undefined (reading 'length')"
before attempting a TCP connect -- minimal repro:
import { request } from "undici"; await request("http://127.0.0.1:1/x")
- ws 8.21.1: real source compiles and completes the WebSocket handshake
correctly (verified byte-for-byte against a raw curl Upgrade request,
matching RFC 6455's example Sec-WebSocket-Accept), but the compiled
WebSocketServer then throws "TypeError: Cannot read properties of
undefined (reading 'destroy')" immediately after the handshake, before
the user-level 'connection' event ever fires.
Counts re-derived from the resolved tree, not carried forward: workspace
members / API-manifest NATIVE_MODULES / crates all agree at 66;
scripts/native_result_ledger.py: 356->349 rows, 307->300 providers;
scripts/unrooted_local_shape.py --update-baseline; docs/api/perry.d.ts and
docs/src/api/reference.md regenerated from a fresh release build
(2041->2026 entries, 122->118 modules); docs/src/native-libraries/governance.md
regenerated via binding_governance.py --table.
Not run: the compile tier of run_lint_gates.sh (known-red on this Linux
host per CLAUDE.md); a full gap sweep (fixed port, not available on this
shared host). run_lint_gates.sh with SKIP_COMPILE_GATES=1: 78 of 79 passed,
1 pre-existing failure (Public benchmark evidence freshness, #10707/#10573,
unrelated to this change).
# Conflicts:
# Cargo.lock
ad7c072 to
4c7996d
Compare
… orphaned decimal.js parity fixture Removes the stray perry-ext-decimal crate entry that survived the rebase's auto-merge in workspace-architecture.json, resyncs Cargo.lock, and recomputes native_result_ledger EXPECTED_ROWS/PROVIDERS (344 rows, 295 providers), the unrooted-local-shape baseline, the generated binding-governance table, and docs/api/perry.d.ts + docs/src/api/reference.md from a fresh perry-dev build. crates/perry-hir/src/lower_patterns.rs: detect_native_instance_expr's new-expression arm went dead. Its match on class_name used to have five live arms (Big/Decimal/BigNumber from this PR, LRUCache/Command from the already- landed #10708/#10712) -- with all five gone the fallback-only match triggered rustc's unreachable_code lint under -D warnings. Simplified the arm to what it now always evaluates to (None after the local-class shadow check), and rewrote the function doc comment to explain why the stub is kept rather than deleted. This is a sequencing interaction the brief calls out explicitly: this file wasn't touched by mysql2/pg/cron's diffs, but decimal.js landing after commander/lru-cache emptied a match neither PR could see on its own. test-files/test_parity_decimal.ts + its test-parity/known_failures.json entry: the original PR left this fixture behind (unlike #10795, which deleted its own moment/cron/backoff test files as part of the same removal). The fixture is now double-dead: decimal.js has no Perry-specific behavior left to validate, and the file was already skip-listed as a broken oracle (node itself can't resolve decimal.js post-npm-ci, #8271) before this PR.
|
Landed via merge train 240 (#10819) as v0.5.1619 — Probing nine packages and removing only the four that passed is the right shape, and it is worth saying so plainly: #10765 had to pull The fixtures earned their keep too — Two things the train adds, both found in review. The release provider broke again. I restored It is also a fixture with real thought in it: #10581 rewrote its wait as a barrier rather than a deadline after the original raced a 10-second wall clock against a one-per-second schedule, and its header explains why no fallback bound is permitted. That reasoning is not recoverable from a deleted file. It passes — Counts were re-derived on the assembled tree rather than carried: ledger 349/300, governance 24 crates, pins 20, workspace policy OK. Validation: ten cheap gates, |
2 participants
Summary
Phase 1 of the 9-package native-binding-removal batch probed
cheerio,cron,exponential-backoff,ioredis,moment,node-forge,nodemailer,undici,wsat each binding's pinned upstream version (fromwell_known_bindings.toml), forcing real-source compilation viaperry.compilePackagesand diffing stdout byte-for-byte againstnode --experimental-strip-types(Node 26.5.1) on a fixture exercising each package's primary documented use.Phase 2 (this PR) removes the bindings for the four packages that passed: cron, exponential-backoff, moment, node-forge. The other five stay, each with a concrete blocker below.
Phase 1 results
compilePackages)Removed (real npm source now compiles via
perry.compilePackages)CronJobtick scheduling (asserted via guaranteed count bounds, not an exact tick count, since the phase offset within the first wall-clock second is not reproducible across two separate process launches) +CronTime/nextDate()pattern parsing. 2 modules (cron,luxon). Byte-for-byte identical to node.backOff()retry-until-success, theretrypredicate callback, and the exhausted-attempts rejection path. 1 module. Byte-for-byte identical.moment()with no args, so output is reproducible across separate runs). 1 module. Byte-for-byte identical.Removed per package: the
perry-ext-<pkg>crate, each package's hidden perry-stdlib-side duplicate (perry-stdlib/src/cron.rs,exponential_backoff.rs,moment.rs— every binding audited in this campaign so far has had one, per #10678), thewell_known_bindings.tomlbinding + upstream-pin rows,NATIVE_MODULE_TABLEdispatch rows (native_table/dates.rsdeleted outright — after date-fns/dayjs's earlier removal it held only moment's 27 rows), HIR special-casing (node-forge's dedicatedtry_node_forge_namespacesub-namespace flattener; single-line dispatch-hint arms for cron/moment elsewhere),js_<pkg>_*FFI declarations, and API-manifest entries (methods/classes +NATIVE_MODULES).cron's two event-loop liveness FFI symbols (
js_cron_timer_tick/js_cron_timer_has_pending) are not removed — the generated entry loop calls them unconditionally every iteration regardless of whether a program uses cron, so they stay as the permanent 0-returning stub (previously the fallback only when theschedulerfeature was off).Not removed, with each blocker
cheerio 1.2.0 —
cheerio.load(html)returns a callable object built viaObject.assign(initialize, staticMethods, {...})(a plain function decorated with static/prototype properties — the standard cheerio$factory shape).typeof $correctly reports"function", but calling it —$('h2')— throwsTypeError: string "h2" is not a function. Minimal repro:Real compiler defect (a function value carrying extra properties isn't invocable), not attempted here.
ioredis 5.11.1 — functionally correct: stdout is byte-for-byte identical to node (set/get/callback-get/zadd+zrange WITHSCORES/incr against a real local redis, db 15). Two Perry-only stderr lines diverge (
Warning: Accessing non-existent property 'default' of module exports inside circular dependency) — a real but stdout-invisible divergence in Perry's CJS circular-require init order; this repo's own parity harness diffs stdout only (run_parity_tests.sh's own comment: "the harness only diffed stdout"), so this doesn't block on this convention, and I'm not otherwise fixing it here.Not removed despite passing:
perry-ext-ioredisis shared byioredis,iovalkey, andredis(three separate npm packages, one crate) — the brief only flagged iovalkey, butredisshares it too, perwell_known_bindings.toml's own rows. More importantly, the literal string"ioredis"is load-bearing shared infrastructure, not just an npm package name:crates/perry-codegen/src/ext_registry.rs'sEXT_PREFIX_REGISTRYroutes everyjs_ioredis_*emitted symbol back to thewell_known_bindings.tomlbinding key"ioredis"to resolve which.ato auto-link — its own comment: "ioredis, iovalkey, and valkey all share this wrapper... so the single 'ioredis' binding key covers every RESP package that lowers here." Deregistering[bindings.ioredis]breaks this lookup foriovalkey/redistoo, not just for the npm package literally namedioredis.Redistype-name resolves to the canonical module string"ioredis"in 8 HIR/codegen files (static_and_instance.rs,native_new.rs,module_decl.rs,local_natives.rs,misc.rs,fn_decl.rs,expr_function.rs,ir/module.rs) — this isiovalkey/redis's own dispatch path for their (identically-named)Redisclass, not something specific to theioredisnpm package.iovalkeyis explicitly staying (open cosmetic issue Spurious 'non-existent property default inside circular dependency' warnings on stderr for iovalkey — Node emits none; the emulated-warning trigger over-fires #10760) andredisisn't part of this batch either, so I stopped rather than working around this — per the brief's instruction.nodemailer 9.0.3 — confirms the brief's stated known expectation.
nodemailer.jsunconditionally requires./smtp-transport, which unconditionally requires../xoauth2, whoselib/xoauth2/index.jsdoesclass XOAuth2 extends Streamagainst the barenode:streamStream(destructuredconst { Stream } = require("stream")) — notReadable/Writable/Duplex/Transform, which fix(runtime): dispatch node:stream super() through any bound-export heritage shape #10649 fixed.new XOAuth2(...)throwsTypeError: is not a constructor. A narrower fixture (createTransport({ jsonTransport: true })+sendMail(), which never touches XOAuth2) compiles and matches node byte-for-byte, but real OAuth2 SMTP auth (Gmail, etc.) is broken. Per the brief: "nodemailer simply does not get removed — do not try to fix it."undici 8.9.0 — real source compiles cleanly (112 modules).
request()throws before even attempting a TCP connection. Minimal, package-independent-shaped repro:ws 8.21.1 — real source compiles. The server side of the WebSocket handshake is actually correct: a raw
curlUpgrade request against a Perry-compiledWebSocketServergets backHTTP/1.1 101 Switching ProtocolswithSec-WebSocket-Accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo=— the exact RFC 6455 example value for that example key, so the handshake math is right. A realwsclient (either the compiled one, or an unmodified Nodewsclient run under Node) successfully reachesopen. But the compiled server then throwsTypeError: Cannot read properties of undefined (reading 'destroy')before the user-level'connection'event ever fires — logged in the server process, not surfaced to the client (the client instead just hangs / the client-and-server-in-one-process variant surfaced it asUnexpected server response: 426, presumably because the crash truncated the response mid-write in that ordering).Counts (re-derived from the resolved tree)
crates/directories /workspace-architecture.jsonpolicy entries / sum ofdecision_counts: 66, all four agree (was 70).scripts/native_result_ledger.py: 356 → 349 rows, 307 → 300 providers.scripts/unrooted_local_shape.py --update-baselineandscripts/string_payload_access_inventory.py --write-baselinere-run (baselines regenerated, not hand-edited).docs/api/perry.d.ts/docs/src/api/reference.mdregenerated from a freshcargo build --release -p perry(2041 → 2026 entries, 122 → 118 modules);docs/src/native-libraries/governance.md's generated table regenerated viapython3 scripts/binding_governance.py --table. All three diffs are pure deletions with intact tails — not the destructive-truncation shape.test_gap_backoff_options.ts,test_gap_cron_cronjob.ts,test_gap_moment_methods.ts,test_parity_moment.ts) — each imported its removed npm package directly with nocompilePackages, so its only subject was the deleted native stub. Also removed the now-staletest_parity_momententry fromtest-parity/known_failures.json(it documented a bug in the shim being deleted).What was and wasn't run
cargo fmt --all -- --check: clean.cargo check --workspace --all-targets --exclude perry-ui-gtk4underRUSTFLAGS="-D warnings"on the defaultdevprofile: clean.cargo test -p perry-hir -p perry-api-manifest -p perry-codegen: all pass exceptevery_dispatch_entry_has_manifest_counterpart, which is pre-existing onorigin/mainbefore this branch's changes (verified viagit stash) — 15 unrelatednet/httpdispatch↔manifest drift entries, nothing to do with this PR.cargo test -p perry --bin perry: 1139 passed, 0 failed, including the three tests that directly named the removed packages (shipped_subset_bindings_are_partial,ext_prefix_binding_keys_resolve_to_wrapper_crates,ext_binding_build_routing_split), all updated to dropnode-forgeand re-verified green.run_lint_gates.shwithSKIP_COMPILE_GATES=1: 78 of 79 passed. The one failure, "Public benchmark evidence freshness," is the pre-existing red documented in CLAUDE.md/ci: two reds on main fail every PR — gap-suite shard 5 parity regression (test_gap_10430) and a stale public benchmark baseline #10707 — not chased.node scripts/binding_pins.mjs --checkunder Node 26.5.1: OK, 20 pinned.run_lint_gates.sh(documented known-red on this Linux host); a full gap sweep (fixed port 17891, unavailable on this shared host, per the standing guidance for this host).Branched from
origin/mainat8e9f6f09e(v0.5.1616);mainadvanced one merge train to1a4fa6507e(v0.5.1617) while this was in flight — checked, no file overlap with anything this PR touches.Summary by CodeRabbit
Changes
cron,exponential-backoff,moment, andnode-forge.Documentation
Rebase note (2026-09-20)
Rebased onto
main@b9ba951ff861c61afb845bfbdfa574cb0fa4080e(train 239) as part of a4-PR sequential rebase campaign together with #10677, #10680, #10704 — all four
independently rebased onto this same main SHA and pushed together so they are mutually
consistent as of this snapshot. This PR's own base was already
main(3 commits behind);only
Cargo.lockconflicted, resolved by taking main's side and resyncing viacargo metadata --offline(dropped 109 further stale transitive-dep lines).Recomputed triple (workspace-architecture.json baseline / native_result_ledger.py /
unrooted-local-shape), derived from the resolved tree via the actual tools, not carried
over:
workspace_members=66(decision_counts: externalize=17, keep=44, merge=1, remove=1,review=3) — already correct as auto-merged, no edit needed;
native_result_ledger: 349rows / 300 providers — already correct as committed, no edit needed;
unrooted-local-shapetotal: 527 — unchanged (none of cron/exponential-backoff/moment/node-forge's files carried findings).
These numbers assume main is still at the stated SHA. #10677/#10680/#10704 remove
different crates from the same starting point — if any of them lands before this one, these
counts (and the other three PRs') need re-deriving against the tree as it then stands.
Shared-crate hazard check: grepped
ext_registry.rsandwell_known_bindings.tomlforcron/exponential-backoff/moment/node-forge.node-cron's own binding was alreadyremoved separately (#466, confirmed via
stdlib_features.rs's comment) — onlycronitselfstill routed through
perry-ext-cron, so no shared-crate breakage.Gates run on this branch:
cargo fmt --all -- --checkOK;cargo check --workspace --all-targetsunder-D warningson the defaultdevprofile (excl.perry-ui-gtk4) —clean, 0 warnings;
run_lint_gates.sh SKIP_COMPILE_GATES=1— 78 of 79 passed (1pre-existing: "Public benchmark evidence freshness", #10707, not chased, per the campaign
brief);
binding_governance.py --checkOK;binding_pins.mjs --checkunder Node 26.5.1 OK;check_file_size.shOK.docs/api/perry.d.ts/docs/src/api/reference.mdregenerated froma fresh
perry-devbuild and diffed — zero drift (already correct as auto-merged).Compile tier of
run_lint_gates.shnot run (Linux compile tier is known-red per the brief).No gap sweep run (fixed port 17891, per the brief). No acceptance re-run — the packages'
behavior is unchanged by this rebase, only the base commit moved.