fix(hir,runtime): late-assigned url.URL reads undefined; non-special URL href gains a trailing slash - #11325
Conversation
|
Ready to merge once CI is clean. Fixes #11322, which unblocks mongodb 7.0.0. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe change shares native-instance classification across initializers and later assignments. It also changes parsing for authority URLs without paths: only special schemes receive a default ChangesLate-assigned native instances
Authority URL paths
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The described URL fixes are mergeable after normal checks; no actionable issue is established by the supplied evidence. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The URL change is narrowly scoped, but newly recognized constructor assignments can retain a native identity after the variable receives a different value. That could send later method calls through the wrong dispatch path. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 27.59% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 8 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
|
Merge queue: this head has two CI runs. The cancelled duplicate (36141941822) accounts for the unexpanded gc-stress/gap-suite matrix names; the real run (36141945922) failed only the owner-grandfathered lint step plus pr-gate. Stacked on current main with #11331: lint, a strict workspace compile, tokio inventory and the perry suite (1203/0) are clean. |
Fixes #11322
Root causes
1.
let u; u = new URL(s)readsundefinedfor every property (withimport { URL } from "url").Native-instance tagging for
new C(...)existed twice: once forlet/const/varinitializers(
destructuring/var_decl/native_new.rs) and once for plain assignment (lower/expr_assign.rs).The initializer copy deliberately excludes the heap-object classes (
urlURL/URLSearchParams,utilTextEncoder/TextDecoder) and the handle-backed constructors (StringDecoder,DiffieHellman*), and resolves aliased imports. The assignment copy tagged anynewof animported native-module export.
uwas therefore registered as aurlnative instance, andu.hostnamelowered to a receiver-boundNativeMethodCall { module: "url", method: "hostname" }.That call returns
undefinedon the plain heap URL object. Theconstform lowers to a plainPropertyGetand works. This also brokelet u = new URL(a); u = new URL(b)(reassignment),var,let u: any, typedlet u: URL, branch assignment, and module-levellet x; x = new URL().Fix: one shared classifier,
native_instance_for_new(ctx, &NewExpr), now innative_new.rs.The initializer arm, the
await newarm and the assignment arm all use it, so the two bindingforms can no longer disagree. Side effects:
await new C()arm now gets alias resolution, thenew mod.Class()callee form, and theDisposableStackfallback, matching the non-await arm.new mod.Class()callee and the global-name fallback(
EventEmitter,DisposableStack, …), exactly as declarations do.2.
new URL("iLoveJS://127.0.0.1:1234").hrefgets a trailing/.parse_urlseeded the pathname with/for every authority-bearing URL that has no path. UnderWHATWG, only special schemes (
http,https,ws,wss,ftp;filehas its own branch) dothat. A non-special scheme such as
foo://a:5ormongodb://db:27017has an empty path:pathname === "", andhref/toString()have no trailing slash. The search/hash settersrebuild
hreffrom the same fields, so they are fixed too (foo://a:5?z=1, notfoo://a:5/?z=1).Tests
test-files/test_gap_url_late_assign_non_special_path.tshas two parts. The first is alate-assignment matrix: URL via
let/var/let: any/let: URL/reassign/try/branch/namespaceimport/module-level, plus URLSearchParams, TextEncoder/Decoder, StringDecoder, Map, Date, RegExp,
Buffer, a class ctor local and a class field. The second is a URL serialization matrix of 22
special and non-special inputs plus the setters.
PERRY_SKIP_BUILD=1 PERRY_NO_AUTO_OPTIMIZE=1)gives PARITY_FAIL on the branch point (182e375) and PASS with the fix.
crates/perry-hir/tests/late_assigned_native_new.rs(new suite): the late-assigned URL,URLSearchParams and TextEncoder must not lower to
url/utilNativeMethodCalls. It fails onthe branch point and passes with the fix. A control checks that a genuine native class (aliased
net.BlockList) is still tagged.perry-runtimeurl::unit tests: 17 passed, including the newpathnamespecial/non-special test.-pset for both arms: 35 related non-ext-routed gap tests(url, assign/uninit/late, disposable, async_local/asyncresource, …). Only the new test changed
(DIFF → PASS); the other 34 pass on both arms. EventEmitter late assignment (plain and
events.namespace) was checked by hand against Node on the fix build.
cargo test -p perry-hir: everything passes exceptunimplemented_api_check's twoevery_supported_module_rejects_bogus_*tests (sqlite/test/sea). Those fail identically withthe branch-point HIR sources, so they are pre-existing and unrelated.
cargo check -p perry-hir -p perry-runtime --all-targets: no warnings.cargo fmt --checkpasses.SKIP_COMPILE_GATES=1 scripts/run_lint_gates.sh(Linux): 89 of 91 script gates pass; thecompile tier was not run. The 2 failures:
cargo xwin check:cargo-xwinis not installed on the Linux host. This diff has nocfg(windows)code.Not run
events/net/http/…) gap tests under the harness: its prebuilt-archive modecan't serve them for a baseline arm.
the URL constructor's parse.
Summary by CodeRabbit
/.