size: compile the URL-shape probes only under url-engine - #7435
Merged
Conversation
proggeramlug
force-pushed
the
size/gate-url-shape-probes
branch
from
August 5, 2026 10:36
47af9e4 to
6fa926b
Compare
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe runtime now compiles URL-specific coercion, serialization, entry conversion, and setter paths only with ChangesURL probe feature gating
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test-files/test_gap_url_shape_probe_gate.ts`:
- Around line 18-19: Update the test around the existing JSON.stringify probes
to add one compact call that supplies a replacer and one pretty-print call with
a space argument, covering both changed implementation paths while preserving
the current outputs. Run the parity/gap check using the Node version specified
by .node-version.
🪄 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: d8f6a6d6-06a6-4d21-9e04-cc4b0c28abc3
📒 Files selected for processing (7)
changelog.d/7435-gate-url-shape-probes.mdcrates/perry-runtime/src/json/replacer.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry-runtime/src/object/object_ops/from_entries.rscrates/perry-runtime/src/value/dynamic_arith.rscrates/perry-runtime/src/value/to_string.rstest-files/test_gap_url_shape_probe_gate.ts
Comment on lines
+18
to
+19
| console.log(JSON.stringify({ u, sp: String(sp) })); | ||
| console.log(JSON.stringify(Object.fromEntries(sp))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files of interest =="
git ls-files | rg '(^\.node-version$|^test-files/test_gap_url_shape_probe_gate\.ts$|^crates/perry-runtime/src/json/replacer\.rs$)' || true
echo
echo "== .node-version =="
if [ -f .node-version ]; then cat -n .node-version; fi
echo
echo "== test file =="
if [ -f test-files/test_gap_url_shape_probe_gate.ts ]; then cat -n test-files/test_gap_url_shape_probe_gate.ts; fi
echo
echo "== replacer relevant snippets =="
if [ -f crates/perry-runtime/src/json/replacer.rs ]; then
wc -l crates/perry-runtime/src/json/replacer.rs
sed -n '240,300p' crates/perry-runtime/src/json/replacer.rs | cat -n
echo "---"
sed -n '800,845p' crates/perry-runtime/src/json/replacer.rs | cat -n
fiRepository: PerryTS/perry
Length of output: 7774
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== JSON.stringify implementation references =="
rg -n "stringify|JSON\.stringify|fn stringify|stringify_pretty|fn stringify_object\(|stringify_object_inner|write_url_href_json" crates/perry-runtime/src test-files -T ts -T rs
echo
echo "== stringify.rs relevant sections =="
fd '^stringify\.rs$' crates/perry-runtime/src && fd '^stringify\.rs$' crates/perry-runtime/src -x sh -c 'echo "--- $1"; wc -l "$1"; sed -n "1,220p" "$1" | cat -n' sh {}Repository: PerryTS/perry
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== JSON.stringify implementation references =="
rg -n "stringify|JSON\.stringify|fn stringify|stringify_pretty|fn stringify_object\(|stringify_object_inner|write_url_href_json" crates/perry-runtime/src test-files -g '*.ts' -g '*.rs'
echo
echo "== stringify.rs relevant sections =="
fd '^stringify\.rs$' crates/perry-runtime/src -x sh -c 'echo "--- $1"; wc -l "$1"; sed -n "1,220p" "$1" | cat -n' sh {}Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Node availability =="
if command -v node >/dev/null 2>&1; then
node -e "console.log(process.version)"
else
echo "node not available"
fi
echo
echo "== JSON.stringify branches from Node 26.5.1 if available =="
if command -v node >/dev/null 2>&1; then
node - <<'JS'
const cases = {
compact_replacer_url: JSON.stringify({ value: new URL("https://host/path?a=1") }, (k, v) => v),
pretty_url: JSON.stringify({ value: new URL("https://host/path?a=1") }, null, 2),
compact_default_url: JSON.stringify({ value: new URL("https://host/path?a=1") }),
};
for (const [name, value] of Object.entries(cases)) {
console.log(name, value);
}
JS
fiRepository: PerryTS/perry
Length of output: 404
Cover the compact replacer and pretty JSON.stringify paths.
The current test uses no replacer and no space argument, so it does not hit the changed compact replacer implementation path or the pretty-print implementation path.
Add one compact JSON.stringify call with a replacer and one pretty call.
Proposed test additions
console.log(JSON.stringify({ u, sp: String(sp) }));
+const identity_replacer = (_key: string, value: unknown) => value;
+console.log(JSON.stringify({ u }, identity_replacer));
+console.log(JSON.stringify({ u }, null, 2));Run this parity/gap check against the Node version in .node-version.
📝 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.
Suggested change
| console.log(JSON.stringify({ u, sp: String(sp) })); | |
| console.log(JSON.stringify(Object.fromEntries(sp))); | |
| console.log(JSON.stringify({ u, sp: String(sp) })); | |
| const identity_replacer = (_key: string, value: unknown) => value; | |
| console.log(JSON.stringify({ u }, identity_replacer)); | |
| console.log(JSON.stringify({ u }, null, 2)); | |
| console.log(JSON.stringify(Object.fromEntries(sp))); |
🤖 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 `@test-files/test_gap_url_shape_probe_gate.ts` around lines 18 - 19, Update the
test around the existing JSON.stringify probes to add one compact call that
supplies a replacer and one pretty-print call with a space argument, covering
both changed implementation paths while preserving the current outputs. Run the
parity/gap check using the Node version specified by .node-version.
Source: Coding guidelines
Contributor
Author
|
Blocked on the file-size cap, not on the change itself.
That is #7402's fuse doing what it was filed to warn about. The gating change itself looks right; it just cannot land until the file is split. I am putting an agent on the split now. Once that lands, rebase and this should go straight in. |
added 2 commits
August 5, 2026 14:05
Six sites on generic value paths carry a runtime "is this object
URL-shaped?" probe:
value/to_string.rs ToString(obj)
value/dynamic_arith.rs obj + x coercion
json/replacer.rs (x2) JSON.stringify compact + pretty walks
object/object_ops/from_entries.rs (x2) Object.fromEntries
object/field_set_by_name.rs obj.<name> = v (8 js_url_set_* entry points)
The probes are guarded at RUNTIME, which does nothing for binary size: the
static references keep the URL class and parser alive in every program.
`-why_live` on hello world showed the parser reached through the plainest
possible chain — `js_object_set_field_by_name` -> `js_url_set_href` ->
`create_url_object` — so any program that assigns an object field pinned it.
Compile the probes under `url-engine`. `ctx.uses_url` is what enables that
feature and is zero-false-negative by construction (it matches the lowered
`UrlNew`/`UrlParse`/`UrlGet`/`UrlSet`/`UrlInstance`/`UrlSearchParams`/
`UrlPattern` nodes and `module: "url"`), so a program that cannot name the
URL API cannot own a URL or URLSearchParams for these probes to find.
hello world: 4,708,736 -> 4,675,672 bytes (-33,064).
Note this is narrower than `crate::url` as a whole: `crate::url::abort`
(AbortController/AbortSignal, used by dgram / fs watch / child_process /
node_stream) and the URL constructor thunks in
`object/global_this/install_static.rs` are untouched.
Behavior change, deliberate and stated: in a binary built WITHOUT
`url-engine`, a plain object that coincidentally matches the URL shape
(class_id 0, >= URL_FIELD_COUNT fields, and a field that parses as an
absolute URL) no longer gets URL treatment on these paths. It cannot be a
real URL there, so this only affects a pathological look-alike, where
falling through to ordinary object semantics is the more correct answer.
Verified:
- `cargo check -p perry-runtime` on both cfg paths; `cargo fmt` clean.
- New gap test exercises ALL six sites (String(u), "" + u, three setters,
JSON.stringify({u}), Object.fromEntries(sp)) and matches
`node --experimental-strip-types` byte for byte.
- hello world still runs.
The gates left `boxed`/`obj` bound outside the `#[cfg]` block that was their only consumer, so a url-engine-off build gained three `unused variable` warnings. Moving each binding inside its gate restores the off-arm warning set to byte-identical with main.
proggeramlug
force-pushed
the
size/gate-url-shape-probes
branch
from
August 5, 2026 12:15
6fa926b to
960cae2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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.
What
Six sites on generic value paths carry a runtime "is this object URL-shaped?" probe:
value/to_string.rsToString(obj)value/dynamic_arith.rsobj + xcoercionjson/replacer.rs(×2)JSON.stringifycompact + pretty walksobject/object_ops/from_entries.rs(×2)Object.fromEntriesobject/field_set_by_name.rsobj.<name> = v— statically references eightjs_url_set_*entry pointsThe probes are guarded at runtime, which does nothing for binary size — the static references keep the URL class and parser alive in every program.
-why_liveon hello world found the parser through about the plainest chain there is:So any program that assigns an object field pinned the URL parser.
Fix
Compile the probes under
url-engine.ctx.uses_urlis what enables that feature and is zero-false-negative by construction — it matches the loweredUrlNew/UrlParse/UrlGet/UrlSet/UrlInstance/UrlSearchParams/UrlPatternnodes andmodule: "url". A program that cannot name the URL API cannot own a URL or URLSearchParams for these probes to find.Size
hello world: 4,708,736 → 4,675,672 bytes (−33,064).
This is narrower than all of
crate::url, deliberately:crate::url::abort(AbortController/AbortSignal, used by dgram / fs watch / child_process / node_stream) and the URL constructor thunks inglobal_this/install_static.rsare untouched.Behavior change, deliberate and stated
In a binary built without
url-engine, a plain object that coincidentally matches the URL shape (class_id == 0,>= URL_FIELD_COUNTfields, and a field that parses as an absolute URL) no longer receives URL treatment on these paths. It cannot be a real URL in such a binary, so this only affects a pathological look-alike — where falling through to ordinary object semantics is the more correct answer anyway.Verification
cargo check -p perry-runtimeon both cfg paths.cargo fmt --all -- --checkclean.String(u),"" + u,u.pathname=/u.search=/u.href=,JSON.stringify({u}),Object.fromEntries(sp), plusURLSearchParamsstringification — and matchesnode --experimental-strip-typesbyte for byte.Summary by CodeRabbit
Performance
Compatibility
URLSearchParamsbehavior when URL support is enabled, including coercion, serialization, property updates, and entry conversion.Tests