Skip to content

fix: reject Symbols in template substitutions - #10995

Closed
proggeramlug wants to merge 2 commits into
mainfrom
fix/10609-template-symbol
Closed

proggeramlug wants to merge 2 commits into
mainfrom
fix/10609-template-symbol

Conversation

@proggeramlug

Copy link
Copy Markdown
Contributor

Fixes #10609.

Template substitutions now use an implicit string conversion that rejects Symbols. The native path carries that check through an object's Symbol.toPrimitive and toString results. The JS and WASM backends emit template interpolation with the same behavior. The concat optimization keeps the check when a declared number contains a Symbol at runtime. Explicit String(symbol) and symbol.toString() retain their existing behavior.

Added test_gap_10609_template_symbol.ts for direct, multi-part, declared-number, and object-returned Symbols, plus the explicit conversion controls.

Validation:

  • cargo check --profile perry-dev -p perry-hir -p perry-codegen -p perry-codegen-js -p perry-codegen-wasm -p perry-runtime
  • cargo build --profile perry-dev -p perry -p perry-runtime-static -p perry-stdlib-static
  • PERRY_SKIP_BUILD=1 ... ./run_parity_tests.sh --filter test_gap_10609_template_symbol — 1/1 pass
  • cargo fmt --all --check, scripts/check_file_size.sh, node --check crates/perry-codegen-wasm/src/wasm_runtime.js

@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 2 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 8 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: ce418bf4-e98a-4f28-8c9f-2c302fb995ec

📥 Commits

Reviewing files that changed from the base of the PR and between c7cbc3c and 4e2edcd.

📒 Files selected for processing (43)
  • changelog.d/10995-template-symbol.md
  • crates/perry-codegen-js/src/emit/exprs_more.rs
  • crates/perry-codegen-wasm/src/emit/compile.rs
  • crates/perry-codegen-wasm/src/emit/expr/strings_json.rs
  • crates/perry-codegen-wasm/src/emit/js_fallback.rs
  • crates/perry-codegen-wasm/src/emit/runtime_imports.rs
  • crates/perry-codegen-wasm/src/emit/string_collection.rs
  • crates/perry-codegen-wasm/src/wasm_runtime.js
  • crates/perry-codegen/src/collectors/escape_arrays.rs
  • crates/perry-codegen/src/collectors/escape_check.rs
  • crates/perry-codegen/src/collectors/escape_news.rs
  • crates/perry-codegen/src/collectors/escape_objects.rs
  • crates/perry-codegen/src/collectors/hot_callees.rs
  • crates/perry-codegen/src/collectors/i32_locals.rs
  • crates/perry-codegen/src/collectors/index_uses.rs
  • crates/perry-codegen/src/collectors/local_refs.rs
  • crates/perry-codegen/src/collectors/mutation.rs
  • crates/perry-codegen/src/collectors/not_bigint_locals.rs
  • crates/perry-codegen/src/collectors/refs.rs
  • crates/perry-codegen/src/collectors/this_as_value.rs
  • crates/perry-codegen/src/expr/dispatch.rs
  • crates/perry-codegen/src/expr/logical_collections.rs
  • crates/perry-codegen/src/expr/slot_rep.rs
  • crates/perry-codegen/src/lower_string_concat.rs
  • crates/perry-codegen/src/runtime_decls/strings.rs
  • crates/perry-codegen/src/stmt/loops.rs
  • crates/perry-codegen/src/type_analysis/numeric.rs
  • crates/perry-codegen/src/type_analysis/refine.rs
  • crates/perry-codegen/src/type_analysis/strings.rs
  • crates/perry-hir/src/analysis/value_types.rs
  • crates/perry-hir/src/dynamic_import.rs
  • crates/perry-hir/src/dynamic_import/worker_paths.rs
  • crates/perry-hir/src/ir/expr.rs
  • crates/perry-hir/src/js_transform/imports.rs
  • crates/perry-hir/src/js_transform/local_natives.rs
  • crates/perry-hir/src/lower/expr_misc.rs
  • crates/perry-hir/src/monomorph/substitute_expr.rs
  • crates/perry-hir/src/stable_hash/expr.rs
  • crates/perry-hir/src/walker/expr_mut.rs
  • crates/perry-hir/src/walker/expr_ref.rs
  • crates/perry-runtime/src/builtins/numbers.rs
  • crates/perry-runtime/src/value/to_string.rs
  • test-files/test_gap_10609_template_symbol.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Held from merge train 257 (#11039) by the file-size cap: on current main this takes crates/perry-runtime/src/value/to_string.rs to 2001 lines, one over scripts/check_file_size.sh's 2000 limit, which fails lint for the whole train.

Split it into topical sub-modules — extract a function group into a sibling file and re-export with explicit named use statements (the script names the v0.5.1019/v0.5.1020 commits as the recipe). Everything else about the PR was fine in the train: gates clean and no conflict. Ping me when it is under the cap.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Assembled this into train 261 and had to pull it back out — it now trips a required lint step, through no fault of the change itself.

crates/perry-runtime/src/value/to_string.rs is 1999 lines on current main; this PR adds 2, giving 2001, one over scripts/check_file_size.sh's 2000-line cap:

The following files are too large:
   2001  crates/perry-runtime/src/value/to_string.rs

Your own CI passed that gate because main was shorter when your branch was cut. Dropping just these two commits from the train put it back to OK: no Rust source files exceed 2000 lines. — nothing else in the train touches the file.

I have a separate PR going up that splits to_string.rs into topical sub-modules, since at 1999/2000 it currently blocks any PR that touches it, not only this one. Once that lands, rebase onto main and this should go green with no change to your fix. Reproduce locally meanwhile with bash scripts/check_file_size.sh.

Everything else here is green: all 6 gap shards, cargo-test, warnings, e2e-scoped, gc-stress.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed on main in merge train 265 (#11108), released as v0.5.1648 at 9d26936298.

Cherry-picked from this PR's head 4e2edcd291 and validated as one tree with 15 other PRs — CI 22/22 green, all 6 gap-suite shards. A train rebase gives the commits new SHAs, so GitHub cannot auto-close the source PR; closing by hand.

Nothing needed from you. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Template literal interpolating a Symbol stringifies instead of throwing TypeError

1 participant