fix(wasm): six codegen divergences from the native backend, found porting a 3D game#6524
Merged
Conversation
added 3 commits
July 17, 2026 11:46
Found porting a full 3D game (Bloom shooter) to --target web; each was invisible on native and produced maddeningly partial failures on wasm. 1. `new Array(n)` fell through to the generic class_new path and built a plain object: element writes landed as properties, but Array.isArray was false so `.length` read 0 forever. New "Array" case in the New emitter routes no-args to array_new, one arg to a new array_constructor_single bridge (ES2015 22.1.1: single number = length after validation, anything else = one element), and >=2 args to the array-literal lowering. 2. Namespace imports (`import * as W from "./mod"`) resolved to nothing: every `W.member` read undefined, and `W.fn(args)` fell to the class-dispatch fallback with an undefined receiver -- returning undefined WITHOUT executing fn. Named imports of the same symbols worked, so a program could load a world yet see every count as undefined. Member reads now resolve through dotted-key entries in imported_var_globals (same promoted-let globals the Named arm uses); member calls take a direct-call fast path via the new imported_ns_funcs map; members used as values wrap in a zero-capture closure, mirroring ExternFuncRef. 3. JS bitwise ops emitted trapping i32.trunc_f64_s, so `NaN | 0` -- which the spec defines as 0 (ToInt32) -- crashed the module with "float unrepresentable in integer range". Now i64.trunc_sat_f64_s + i32.wrap_i64: exact ToInt32 semantics (NaN->0, modular wrap) across the whole i64 range. nontrapping-fptoint is baseline WebAssembly in every browser since 2020.
resolution, ToInt32 bitwise
Second batch from porting a full 3D game to --target web (first batch:
new Array(n) / namespace member basics / trunc-sat groundwork).
- Named imports now resolve through re-export chains: Export::Named
whose local is itself an import binding, Export::ReExport, and
export-* star chains, with directory specifiers ("./core") mapping
to their index module and Windows path separators normalized. A
library facade like bloom's index.ts re-exporting `Key` from
core/keys.ts is three hops from the consumer; stopping at the first
module made every re-exported const OBJECT read undefined while
same-named scalars sometimes survived other paths - maddeningly
partial failures.
- Function calls/values resolve per-consumer (imported_func_indices,
built from each module's own imports through the same chains) BEFORE
the whole-program func_name_map. Bare names collide the moment two
modules define the same function name - a serializer's local
`vec3(v): string` captured the math library's `vec3(x,y,z)` for
every caller in the program, which is why spawn positions read as
NaN in a game whose world data was perfectly fine. func_name_map
keeps exported-wins/or_insert ordering as the fallback.
- Namespace member calls (`import * as W; W.fn(args)`) lower to a
direct wasm call; `W.fn` as a value wraps in a zero-capture closure.
Previously the callee fell to class-dispatch on an undefined
receiver and silently returned undefined WITHOUT executing fn.
- JS bitwise ops emit i64.trunc_sat_f64_s + i32.wrap_i64 instead of
the trapping i32.trunc_f64_s: exact ToInt32 semantics (NaN -> 0,
modular wrap) rather than "float unrepresentable in integer range"
crashes on NaN.
- PERRY_WASM_DEBUG_IMPORTS=1 dumps the per-import resolution table.
Third batch from the 3D-game web port. JS defines unsigned right shift as producing a ToUint32 result, but the codegen widened the i32 back to f64 SIGNED. Invisible for any shift >= 1 (shifting in a zero clears the sign bit, so signed and unsigned agree) — and wrong for exactly the canonical `x >>> 0` "reinterpret as unsigned" idiom, which handed back the negative input unchanged. Found via a game engine packing colours as `(a|r|g|b) >>> 0` — its own comment reads "use unsigned-shift-zero to keep the value positive when stored as f64". The negative f64 crossed the FFI into a Rust `as u32`, whose saturating cast floored it to 0: every model tint became transparent black, so alpha-cutout foliage discarded its entire canopy. `>>>` now widens with f64.convert_i32_u; every other bitwise operator keeps the signed conversion, which is correct for them.
📝 WalkthroughWalkthroughWASM emission now follows multi-hop module re-exports for imported globals and functions, prioritizes consumer-scoped function mappings, and supports namespace member access. Bitwise conversion distinguishes unsigned results, while ChangesCross-module import resolution
Bitwise and Array semantics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ModuleCompiler
participant ExportResolvers
participant ImportedFunctionMaps
participant NamespaceCallEmitter
ModuleCompiler->>ExportResolvers: resolve re-exported symbol
ExportResolvers->>ImportedFunctionMaps: record consumer-scoped function index
NamespaceCallEmitter->>ImportedFunctionMaps: look up namespace member
ImportedFunctionMaps-->>NamespaceCallEmitter: return wasm function index
NamespaceCallEmitter->>NamespaceCallEmitter: emit argument handling and wasm Call
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: 6
🤖 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 `@crates/perry-codegen-wasm/src/emit/binary.rs`:
- Around line 42-63: Replace the direct integer conversion logic in
emit_bitwise_binary_impl with calls to the appropriate runtime helpers:
js_dynamic_bitand, js_dynamic_bitor, js_dynamic_bitxor, js_dynamic_shr, and
js_dynamic_ushr, preserving each operation’s result handling. In
crates/perry-codegen-wasm/src/emit/binary.rs lines 42-63, route all binary
bitwise operators through these helpers; in
crates/perry-codegen-wasm/src/emit/expr/literals_vars.rs lines 387-388, route
bitwise NOT through js_dynamic_bitnot.
In `@crates/perry-codegen-wasm/src/emit/compile.rs`:
- Around line 959-971: Set self.current_mod_idx = mod_idx in both the
global-initializer loop and static-field loop before emitting their initializer
expressions, alongside the existing func_map updates. Ensure every initializer
emission context uses its own consumer module index so imported function
resolution remains consumer-scoped.
- Around line 974-1052: Update the namespace-import handling around the
Namespace arm to recursively enumerate the source module’s transitive public
exports, including ExportAll entries, and register bindings only for that
resolved export surface. Remove the src_lets fallback so private facade locals
are not exposed, while preserving let and function resolution through
resolve_export_to_let and resolve_export_to_func.
In `@crates/perry-codegen-wasm/src/emit/expr/calls.rs`:
- Around line 20-44: The direct imported-call lowering in calls.rs must also
support function-valued globals stored in imported_var_globals. At the
namespace-member path around lines 20-44, add a fallback that loads the resolved
imported global and dispatches it dynamically when imported_ns_funcs has no
match; apply the same named-import fallback around lines 187-194. Preserve
existing compiled-function dispatch and argument handling, and update both sites
in crates/perry-codegen-wasm/src/emit/expr/calls.rs.
In `@crates/perry-codegen-wasm/src/emit/locals.rs`:
- Around line 140-169: Update resolve_module_idx_by_source to accept the
exporting module’s path or index, then resolve source relative to that module’s
directory before applying extension and index fallbacks. Match the normalized
relative candidate against module names first, and retain suffix matching only
as a fallback so re-exports such as a/index.ts → "./util" cannot select an
unrelated longer module path.
- Around line 245-265: Update the fallback lookup in resolve_export_to_let so it
only returns bindings explicitly registered as exports, not arbitrary same-named
locals in src_let_names. Apply the same restriction to the corresponding
fallback at the second referenced location, preserving export-star traversal so
later sources can resolve the actual exported name.
🪄 Autofix (Beta)
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: b7f8aae6-e0cb-41f0-b9f2-2ffcf3f8e52a
📒 Files selected for processing (11)
crates/perry-codegen-wasm/src/emit/binary.rscrates/perry-codegen-wasm/src/emit/compile.rscrates/perry-codegen-wasm/src/emit/expr/calls.rscrates/perry-codegen-wasm/src/emit/expr/classes.rscrates/perry-codegen-wasm/src/emit/expr/literals_vars.rscrates/perry-codegen-wasm/src/emit/expr/objects.rscrates/perry-codegen-wasm/src/emit/locals.rscrates/perry-codegen-wasm/src/emit/mod.rscrates/perry-codegen-wasm/src/emit/module_emitter.rscrates/perry-codegen-wasm/src/emit/string_collection.rscrates/perry-codegen-wasm/src/wasm_runtime.js
Comment on lines
+42
to
+63
| fn emit_bitwise_binary_impl( | ||
| &mut self, | ||
| func: &mut Function, | ||
| left: &Expr, | ||
| right: &Expr, | ||
| op: Instruction<'static>, | ||
| result_unsigned: bool, | ||
| ) { | ||
| self.emit_expr(func, left); | ||
| func.instruction(&Instruction::F64ReinterpretI64); | ||
| func.instruction(&Instruction::I32TruncF64S); | ||
| func.instruction(&Instruction::I64TruncSatF64S); | ||
| func.instruction(&Instruction::I32WrapI64); | ||
| self.emit_expr(func, right); | ||
| func.instruction(&Instruction::F64ReinterpretI64); | ||
| func.instruction(&Instruction::I32TruncF64S); | ||
| func.instruction(&Instruction::I64TruncSatF64S); | ||
| func.instruction(&Instruction::I32WrapI64); | ||
| func.instruction(&op); | ||
| func.instruction(&Instruction::F64ConvertI32S); | ||
| if result_unsigned { | ||
| func.instruction(&Instruction::F64ConvertI32U); | ||
| } else { | ||
| func.instruction(&Instruction::F64ConvertI32S); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== binary.rs ==\n'
sed -n '1,220p' crates/perry-codegen-wasm/src/emit/binary.rs
printf '\n== literals_vars.rs (around bitnot) ==\n'
sed -n '330,430p' crates/perry-codegen-wasm/src/emit/expr/literals_vars.rs
printf '\n== search for dynamic bitwise helpers and ToInt32/ToUint32 ==\n'
rg -n "js_dynamic_bit|dyn_to_int32|ToInt32|ToUint32|I64TruncSatF64S|I32WrapI64|BitNot|bitnot|bitand|bitor|bitxor|ushr|shr" crates -g '!**/target/**'Repository: PerryTS/perry
Length of output: 38945
🏁 Script executed:
set -euo pipefail
printf '\n== outline: binary.rs ==\n'
ast-grep outline crates/perry-codegen-wasm/src/emit/binary.rs --view expanded || true
printf '\n== outline: literals_vars.rs ==\n'
ast-grep outline crates/perry-codegen-wasm/src/emit/expr/literals_vars.rs --view expanded || trueRepository: PerryTS/perry
Length of output: 588
🏁 Script executed:
set -euo pipefail
printf '\n== callers of emit_bitwise_binary* in wasm emitter ==\n'
rg -n "emit_bitwise_binary(_u)?\(" crates/perry-codegen-wasm/src -g '!**/target/**'
printf '\n== wasm emitter expr handling around binary/unary ==\n'
sed -n '1,260p' crates/perry-codegen-wasm/src/emit/mod.rs
printf '\n== runtime dynamic bitwise helpers ==\n'
sed -n '540,700p' crates/perry-runtime/src/value/dynamic_arith.rs
printf '\n== wasm runtime declarations for dynamic helpers ==\n'
sed -n '1180,1215p' crates/perry-codegen/src/runtime_decls/strings.rsRepository: PerryTS/perry
Length of output: 13577
🏁 Script executed:
set -euo pipefail
printf '\n== wasm emitter binary + literals_vars context around BitNot/bitwise ==\n'
sed -n '1,220p' crates/perry-codegen-wasm/src/emit/binary.rs
printf '\n---\n'
sed -n '360,420p' crates/perry-codegen-wasm/src/emit/expr/literals_vars.rs
printf '\n== search for bitwise-related type assumptions in wasm codegen ==\n'
rg -n "UnaryOp::BitNot|BinaryOp::BitAnd|BinaryOp::BitOr|BinaryOp::BitXor|BinaryOp::Shr|BinaryOp::UShr|ToInt32|ToUint32|js_dynamic_bitnot|js_dynamic_bitand|js_dynamic_bitor|js_dynamic_bitxor|js_dynamic_shr|js_dynamic_ushr" crates/perry-codegen-wasm/src crates/perry-codegen/src -g '!**/target/**'Repository: PerryTS/perry
Length of output: 27170
🏁 Script executed:
set -euo pipefail
printf '\n== bitwise emission call sites in codegen-wasm ==\n'
rg -n "emit_bitwise_binary(_u)?\(" crates/perry-codegen-wasm/src -g '!**/target/**'
printf '\n== top-level expr emission in wasm ==\n'
sed -n '1,220p' crates/perry-codegen-wasm/src/emit/mod.rs
printf '\n== runtime dynamic arithmetic around bitwise helpers ==\n'
sed -n '540,700p' crates/perry-runtime/src/value/dynamic_arith.rs
printf '\n== runtime declarations for dynamic bitwise helpers ==\n'
sed -n '1188,1210p' crates/perry-codegen/src/runtime_decls/strings.rsRepository: PerryTS/perry
Length of output: 12665
🏁 Script executed:
set -euo pipefail
printf '\n== wasm codegen BigInt support / tags ==\n'
rg -n "BigInt|bigint|TAG_BIGINT|tag.*bigint|js_bigint|dynamic_bigint|bitnot" crates/perry-codegen-wasm crates/perry-runtime/src/value crates/perry-hir/src -g '!**/target/**'
printf '\n== emit_expr and value representation in wasm codegen ==\n'
sed -n '1,260p' crates/perry-codegen-wasm/src/emit/mod.rsRepository: PerryTS/perry
Length of output: 49322
🏁 Script executed:
set -euo pipefail
printf '\n== literals_vars.rs around binary op emission ==\n'
sed -n '160,220p' crates/perry-codegen-wasm/src/emit/expr/literals_vars.rs
printf '\n== literals_vars.rs around BigInt/native handling ==\n'
sed -n '430,500p' crates/perry-codegen-wasm/src/emit/expr/literals_vars.rs
printf '\n== wasm emitter helpers for Expr::BigIntCoerce ==\n'
sed -n '900,980p' crates/perry-codegen-wasm/src/emit/string_collection.rs
printf '\n== search for runtime helper imports in wasm emitter ==\n'
rg -n "declare_function\\(\"js_dynamic_|emit_memcall\\(|memcall.*js_dynamic_|BitNot|BigIntCoerce" crates/perry-codegen-wasm/src -g '!**/target/**'Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
set -euo pipefail
printf '\n== HIR typing for bitwise and BitNot ==\n'
sed -n '400,500p' crates/perry-hir/src/lower_types.rs
printf '\n---\n'
sed -n '600,640p' crates/perry-hir/src/analysis/value_types.rs
printf '\n== wasm emitter expression variants around BigInt ==\n'
rg -n "Expr::BigInt|Expr::BigIntCoerce|UnaryOp::BitNot|BinaryOp::BitAnd|BinaryOp::UShr" crates/perry-codegen-wasm/src/emit -g '!**/target/**'Repository: PerryTS/perry
Length of output: 6882
🏁 Script executed:
set -euo pipefail
printf '\n== typed ABI / wasm codegen comparison for bitwise ops ==\n'
sed -n '1188,1355p' crates/perry-codegen/src/codegen/typed_abi.rs
printf '\n== wasm runtime JSValue numeric coercion helpers ==\n'
sed -n '1,120p' crates/perry-runtime/src/value/dynamic_arith.rsRepository: PerryTS/perry
Length of output: 11283
🏁 Script executed:
set -euo pipefail
printf '\n== Expr::Binary handling in wasm literals_vars ==\n'
sed -n '130,215p' crates/perry-codegen-wasm/src/emit/expr/literals_vars.rs
printf '\n== Expr::BigInt / BigIntCoerce handling in wasm literals_vars ==\n'
sed -n '440,480p' crates/perry-codegen-wasm/src/emit/expr/literals_vars.rs
printf '\n== any wasm fallback logic for unsupported expressions ==\n'
rg -n "fallback|js_fallback|unsupported|return false|try_emit_expr_literals_vars" crates/perry-codegen-wasm/src/emit -g '!**/target/**'Repository: PerryTS/perry
Length of output: 9017
🏁 Script executed:
python3 - <<'PY'
import math
x = 1e20
# JS ToInt32: ToNumber -> trunc -> modulo 2**32 -> signed int32
u = math.trunc(x) % (2**32)
i = u if u < 2**31 else u - 2**32
print("ToInt32(1e20) =", i)
print("ToUint32(1e20) =", u)
PYRepository: PerryTS/perry
Length of output: 206
Route bitwise ops through the runtime helpers
I64TruncSatF64S/I32WrapI64 does not match JS ToInt32/ToUint32 for out-of-range numbers, and it skips ToNumeric/BigInt handling. Use js_dynamic_bitand/js_dynamic_bitor/js_dynamic_bitxor/js_dynamic_shr/js_dynamic_ushr in crates/perry-codegen-wasm/src/emit/binary.rs and js_dynamic_bitnot in crates/perry-codegen-wasm/src/emit/expr/literals_vars.rs.
📍 Affects 2 files
crates/perry-codegen-wasm/src/emit/binary.rs#L42-L63(this comment)crates/perry-codegen-wasm/src/emit/expr/literals_vars.rs#L387-L388
🤖 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 `@crates/perry-codegen-wasm/src/emit/binary.rs` around lines 42 - 63, Replace
the direct integer conversion logic in emit_bitwise_binary_impl with calls to
the appropriate runtime helpers: js_dynamic_bitand, js_dynamic_bitor,
js_dynamic_bitxor, js_dynamic_shr, and js_dynamic_ushr, preserving each
operation’s result handling. In crates/perry-codegen-wasm/src/emit/binary.rs
lines 42-63, route all binary bitwise operators through these helpers; in
crates/perry-codegen-wasm/src/emit/expr/literals_vars.rs lines 387-388, route
bitwise NOT through js_dynamic_bitnot.
Comment on lines
+974
to
+1052
| // Namespace import (`import * as W from "./mod"`): | ||
| // register every exported module-level let under a | ||
| // DOTTED key ("W.MESH_COUNT"), so PropertyGet on the | ||
| // namespace ident resolves to the source module's | ||
| // promoted-let global — the same mechanism the Named | ||
| // arm above uses. Without this, every `W.member` read | ||
| // emitted a class_get_field on an undefined receiver | ||
| // and produced undefined (functions kept working via | ||
| // the whole-program name map, which made the failure | ||
| // maddeningly partial). | ||
| if let perry_hir::ir::ImportSpecifier::Namespace { local } = spec { | ||
| let src_module = &modules[src_idx].1; | ||
| let mut resolved_local: Option<&str> = None; | ||
| for export in &src_module.exports { | ||
| if let perry_hir::ir::Export::Named { | ||
| local: src_local, | ||
| let exported = match export { | ||
| perry_hir::ir::Export::Named { exported, .. } => exported, | ||
| perry_hir::ir::Export::ReExport { exported, .. } => exported, | ||
| _ => continue, | ||
| }; | ||
| if let Some(gidx) = resolve_export_to_let( | ||
| modules, | ||
| &src_let_names, | ||
| &name_to_idx, | ||
| src_idx, | ||
| exported, | ||
| } = export | ||
| { | ||
| if exported == imported { | ||
| resolved_local = Some(src_local.as_str()); | ||
| break; | ||
| } | ||
| 8, | ||
| ) { | ||
| self.imported_var_globals.insert( | ||
| (consumer_idx, format!("{}.{}", local, exported)), | ||
| gidx, | ||
| ); | ||
| } | ||
| } | ||
| // Direct fall-through: if no Export::Named matched | ||
| // but a Let with the imported name exists, use it. | ||
| // (Some HIR lowering shapes register exports out-of- | ||
| // band; this keeps `export const X = ...` robust.) | ||
| let key = resolved_local.unwrap_or(imported.as_str()); | ||
| if let Some(&gidx) = src_lets.get(key) { | ||
| // Fall-through parity with the Named arm: exports | ||
| // registered out-of-band still have their let — | ||
| // expose those by name unless an Export::Named | ||
| // already claimed the key. | ||
| for (name, &gidx) in src_lets.iter() { | ||
| self.imported_var_globals | ||
| .insert((consumer_idx, local.clone()), gidx); | ||
| .entry((consumer_idx, format!("{}.{}", local, name))) | ||
| .or_insert(gidx); | ||
| } | ||
| // Exported FUNCTIONS: `W.fn(args)` must call the | ||
| // source module's compiled function, `W.fn` as a | ||
| // value must wrap it in a closure. Resolved with | ||
| // the same chain-following helper the named arm | ||
| // uses. | ||
| for (exp_name, _) in &src_module.exported_functions { | ||
| if let Some(fidx) = resolve_export_to_func( | ||
| modules, | ||
| &self.module_func_maps, | ||
| &name_to_idx, | ||
| src_idx, | ||
| exp_name, | ||
| 8, | ||
| ) { | ||
| self.imported_ns_funcs.insert( | ||
| (consumer_idx, format!("{}.{}", local, exp_name)), | ||
| fidx, | ||
| ); | ||
| } | ||
| } | ||
| for export in &src_module.exports { | ||
| let exported = match export { | ||
| perry_hir::ir::Export::Named { exported, .. } => exported, | ||
| perry_hir::ir::Export::ReExport { exported, .. } => exported, | ||
| _ => continue, | ||
| }; | ||
| if let Some(fidx) = resolve_export_to_func( | ||
| modules, | ||
| &self.module_func_maps, | ||
| &name_to_idx, | ||
| src_idx, | ||
| exported, | ||
| 8, | ||
| ) { | ||
| self.imported_ns_funcs | ||
| .entry((consumer_idx, format!("{}.{}", local, exported))) | ||
| .or_insert(fidx); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Populate namespaces from the transitive export surface.
ExportAll entries are skipped, so a facade containing only export * from "./impl" produces no W.member bindings. Conversely, the src_lets fallback exposes private facade locals. Recursively enumerate exported names and register only that resulting public surface.
🤖 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 `@crates/perry-codegen-wasm/src/emit/compile.rs` around lines 974 - 1052,
Update the namespace-import handling around the Namespace arm to recursively
enumerate the source module’s transitive public exports, including ExportAll
entries, and register bindings only for that resolved export surface. Remove the
src_lets fallback so private facade locals are not exposed, while preserving let
and function resolution through resolve_export_to_let and
resolve_export_to_func.
Comment on lines
+20
to
+44
| if let Expr::ExternFuncRef { name, .. } = object.as_ref() { | ||
| let key = ( | ||
| self.emitter.current_mod_idx, | ||
| format!("{}.{}", name, property), | ||
| ); | ||
| if let Some(&idx) = self.emitter.imported_ns_funcs.get(&key).copied().as_ref() { | ||
| for arg in args { | ||
| self.emit_expr(func, arg); | ||
| } | ||
| // Pad-up / drop-excess — see the FuncRef arm below (#183). | ||
| if let Some(&expected) = self.emitter.func_param_counts.get(&idx) { | ||
| for _ in args.len()..expected { | ||
| func.instruction(&Instruction::I64Const(TAG_UNDEFINED as i64)); | ||
| } | ||
| for _ in expected..args.len() { | ||
| func.instruction(&Instruction::Drop); | ||
| } | ||
| } | ||
| func.instruction(&Instruction::Call(idx)); | ||
| if self.emitter.void_funcs.contains(&idx) { | ||
| func.instruction(&Instruction::I64Const(TAG_UNDEFINED as i64)); | ||
| } | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Support direct calls through imported function-valued globals.
Direct imported-call lowering only consults compiled-function maps, even though exported arrow/function-valued constants are represented by closures in imported_var_globals.
crates/perry-codegen-wasm/src/emit/expr/calls.rs#L20-L44: if the namespace member resolves to an imported global, load it and dispatch it dynamically.crates/perry-codegen-wasm/src/emit/expr/calls.rs#L187-L194: apply the same fallback for named imported globals.
📍 Affects 1 file
crates/perry-codegen-wasm/src/emit/expr/calls.rs#L20-L44(this comment)crates/perry-codegen-wasm/src/emit/expr/calls.rs#L187-L194
🤖 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 `@crates/perry-codegen-wasm/src/emit/expr/calls.rs` around lines 20 - 44, The
direct imported-call lowering in calls.rs must also support function-valued
globals stored in imported_var_globals. At the namespace-member path around
lines 20-44, add a fallback that loads the resolved imported global and
dispatches it dynamically when imported_ns_funcs has no match; apply the same
named-import fallback around lines 187-194. Preserve existing compiled-function
dispatch and argument handling, and update both sites in
crates/perry-codegen-wasm/src/emit/expr/calls.rs.
Comment on lines
+140
to
+169
| pub(super) fn resolve_module_idx_by_source( | ||
| modules: &[(String, perry_hir::ir::Module)], | ||
| source: &str, | ||
| ) -> Option<usize> { | ||
| let src = source | ||
| .trim_start_matches("./") | ||
| .trim_start_matches("../") | ||
| .replace('\\', "/"); | ||
| // A directory specifier ("./core") resolves to its index module. | ||
| let src_index = format!("{}/index", src); | ||
| let mut best: Option<(usize, usize)> = None; | ||
| for (i, (_, m)) in modules.iter().enumerate() { | ||
| // Module names are project-relative paths with the platform's | ||
| // separators ("engine\src\core\keys.ts" on Windows) — normalize. | ||
| let mn = m.name.replace('\\', "/"); | ||
| let stem = mn.rsplit_once('.').map(|(s, _)| s.to_string()).unwrap_or_else(|| mn.clone()); | ||
| let hit = stem == src | ||
| || mn == src | ||
| || stem.ends_with(&format!("/{}", src)) | ||
| || stem.ends_with(&format!("/{}", src_index)) | ||
| || stem == src_index; | ||
| if hit { | ||
| let n = mn.len(); | ||
| if best.map(|(_, bn)| n > bn).unwrap_or(true) { | ||
| best = Some((i, n)); | ||
| } | ||
| } | ||
| } | ||
| best.map(|(i, _)| i) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Resolve re-export specifiers relative to the exporting module.
This globally suffix-matches source and selects the longest module name. For a/index.ts → "./util", the presence of a longer b/nested/util.ts can resolve the re-export to the wrong module. Pass the originating module path/index and resolve the normalized relative candidate before extension/index fallbacks.
🤖 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 `@crates/perry-codegen-wasm/src/emit/locals.rs` around lines 140 - 169, Update
resolve_module_idx_by_source to accept the exporting module’s path or index,
then resolve source relative to that module’s directory before applying
extension and index fallbacks. Match the normalized relative candidate against
module names first, and retain suffix matching only as a fallback so re-exports
such as a/index.ts → "./util" cannot select an unrelated longer module path.
- Set current_mod_idx in the global-initializer and class-method emission loops. Per-consumer import resolution (imported_var_globals / imported_func_indices / imported_ns_funcs) is keyed by current_mod_idx; a module-scope initializer that called an imported symbol resolved against a stale consumer index and could bind another module's like-named export. (The exported-wins func_name_map fallback masked it for single-export names, but two modules exporting the same name would misbind.) - Gate the resolve_export_to_let / resolve_export_to_func fallbacks on a new module_exports_name() check. The tail fallback returned any same-named module-LOCAL, so during `export *` recursion a private `foo` in an early source masked a genuine exported `foo` in a later one. It now returns a local only when the name is actually part of that module's public surface. - Register namespace-import members from the module's PUBLIC export surface (collect_exported_names: named/re-export/function/object exports + a recursive walk of `export * from`), replacing a blanket "register every module-level let" loop that both leaked PRIVATE locals as `W.private` and skipped `export *` re-exports entirely. Verified: the 3D game this port was built for still boots, renders, and plays with 0 console errors after the tightened resolver.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/perry-codegen-wasm/src/emit/locals.rs`:
- Around line 215-219: Update collect_exported_names and the related
resolve_export_to_* logic so names re-exported by multiple export * sources are
tracked as ambiguous rather than deduplicated and resolved to the first match.
Preserve names explicitly re-exported by the current module as valid, while
leaving ambiguous star-exported names unresolved.
- Around line 222-228: Update the namespace import resolution flow, especially
resolve_export_to_let and resolve_export_to_func, to handle NamespaceReExport
names collected by collect_exported_names. Ensure a lookup such as W.ns resolves
to a namespace-object binding and is registered instead of falling through as
undefined; alternatively, stop collect_exported_names from exposing
NamespaceReExport until that binding can be represented.
🪄 Autofix (Beta)
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: f0785150-debe-4d23-91dc-612d00fe86ff
📒 Files selected for processing (3)
crates/perry-codegen-wasm/src/emit/compile.rscrates/perry-codegen-wasm/src/emit/locals.rscrates/perry-codegen-wasm/src/emit/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-codegen-wasm/src/emit/mod.rs
Comment on lines
+215
to
+219
| perry_hir::ir::Export::ExportAll { source } => { | ||
| if let Some(si) = resolve_module_idx_by_source(modules, source) { | ||
| if si != mod_idx { | ||
| collect_exported_names(modules, si, depth - 1, out); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'ExportAll|collect_exported_names|resolve_export_to_(let|func)' \
crates/perry-codegen-wasm/src/emit/locals.rs \
crates/perry-codegen-wasm/src/emit/compile.rs
rg -n -C2 'export\s+\*.*from|ambiguous' \
crates/perry-codegen-wasm crates/perry/testsRepository: PerryTS/perry
Length of output: 19340
🏁 Script executed:
#!/bin/bash
sed -n '190,460p' crates/perry-codegen-wasm/src/emit/locals.rsRepository: PerryTS/perry
Length of output: 10392
🏁 Script executed:
#!/bin/bash
rg -n -C3 'ambiguous|duplicate.*export|export \*.*export \*|exported_names|resolve_export_to_let|resolve_export_to_func' \
crates/perry-codegen-wasm crates/perry/testsRepository: PerryTS/perry
Length of output: 14652
Reject ambiguous export * names
collect_exported_names dedupes star-exported names, and resolve_export_to_* then returns the first matching source. That can expose an ambiguous name that should stay unresolved unless an explicit export disambiguates it. crates/perry-codegen-wasm/src/emit/locals.rs:215-219
🤖 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 `@crates/perry-codegen-wasm/src/emit/locals.rs` around lines 215 - 219, Update
collect_exported_names and the related resolve_export_to_* logic so names
re-exported by multiple export * sources are tracked as ambiguous rather than
deduplicated and resolved to the first match. Preserve names explicitly
re-exported by the current module as valid, while leaving ambiguous
star-exported names unresolved.
Comment on lines
+222
to
+228
| // `export * as ns from "..."` binds the whole namespace under one | ||
| // name; the namespace object itself is not a promoted let we can | ||
| // resolve here, so expose the name (resolution is a no-op) rather | ||
| // than recurse into the source's members. | ||
| perry_hir::ir::Export::NamespaceReExport { name, .. } => { | ||
| out.insert(name.clone()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 'NamespaceReExport|imported_ns_funcs|collect_exported_names' \
crates/perry-codegen-wasm/srcRepository: PerryTS/perry
Length of output: 10506
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,320p' crates/perry-codegen-wasm/src/emit/locals.rs
printf '\n--- compile excerpt ---\n'
sed -n '980,1035p' crates/perry-codegen-wasm/src/emit/compile.rs
printf '\n--- module emitter excerpt ---\n'
sed -n '70,95p' crates/perry-codegen-wasm/src/emit/module_emitter.rsRepository: PerryTS/perry
Length of output: 17394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '320,520p' crates/perry-codegen-wasm/src/emit/locals.rs
printf '\n--- NamespaceReExport search ---\n'
rg -n -C3 'NamespaceReExport|imported_var_globals|imported_ns_funcs|resolve_export_to_let|resolve_export_to_func' crates/perry-codegen-wasm/srcRepository: PerryTS/perry
Length of output: 26485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'imported_var_globals|Namespace import|namespace re-export|\.ns\b|MemberExpression|class_get_field|GlobalGet' crates/perry-codegen-wasm/src/emitRepository: PerryTS/perry
Length of output: 28744
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline crates/perry-codegen-wasm/src/emit/expr/objects.rs --view expanded
printf '\n--- member/namespace handling ---\n'
rg -n -C3 'Member|imported_ns_funcs|imported_var_globals|namespace' crates/perry-codegen-wasm/src/emit/expr/objects.rs crates/perry-codegen-wasm/src/emit/expr/calls.rsRepository: PerryTS/perry
Length of output: 2808
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'ImportSpecifier::Namespace|Namespace \{ local \}|PropertyGet \{ object, property \}|import \* as|namespace import' crates/perry-codegen-wasm/src/emitRepository: PerryTS/perry
Length of output: 8758
Handle NamespaceReExport in namespace import lookup
collect_exported_names now includes ns, but resolve_export_to_let/resolve_export_to_func never return a binding for it. W.ns is therefore skipped during namespace import registration and reads fall through as undefined. Add a namespace-object binding path here or omit NamespaceReExport from the collected surface until it can be represented.
🤖 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 `@crates/perry-codegen-wasm/src/emit/locals.rs` around lines 222 - 228, Update
the namespace import resolution flow, especially resolve_export_to_let and
resolve_export_to_func, to handle NamespaceReExport names collected by
collect_exported_names. Ensure a lookup such as W.ns resolves to a
namespace-object binding and is registered instead of falling through as
undefined; alternatively, stop collect_exported_names from exposing
NamespaceReExport until that binding can be represented.
Contributor
Author
|
Triage note (automated PR pass). All six CodeRabbit findings here are substantive |
proggeramlug
added
the
ready
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.
The
--target wasmbackend had drifted from the native backend in ways that only surface at the scale of a full application. All six were found porting a real 3D game (a third-person shooter on a wgpu/WebGPU engine) to the browser; each was invisible on native and produced a maddeningly partial failure on wasm — some things worked, which is what made them hard to spot.Codegen-only: 11 files, all in
crates/perry-codegen-wasm(10 Rust + thewasm_runtime.jsbridge). Native codegen is untouched.The six
new Array(n)fell through to the genericclass_newpath and built a plain object — element writes landed as properties, butArray.isArraywas false so.lengthread 0 forever. New"Array"case in the New emitter: no args →array_new; one arg → a newarray_constructor_singlebridge (ES2015 §22.1.1: a single number is a length after validation, anything else is a one-element array); ≥2 args → the array-literal lowering.Namespace imports (
import * as W from "./mod") resolved to nothing: everyW.memberreadundefined, andW.fn(args)fell to class-dispatch on an undefined receiver and returnedundefinedwithout executingfn. Named imports of the same symbols worked, so a program could load its data yet see every derived value asundefined. Member reads now resolve through dotted-key entries inimported_var_globals(the same promoted-let globals the named arm uses); calls take a direct-call fast path via a newimported_ns_funcsmap; members-as-values wrap in a zero-capture closure.Re-export chains were never followed.
import { X } from "lib"wherelib/index.tsdoesexport { X } from "./core"(andcoremay itself re-export) resolvedXtoundefined— only direct exports worked.resolve_export_to_let/resolve_export_to_funcnow chaseExport::Named-of-an-import,Export::ReExport, andexport *star chains (depth-capped), with directory specifiers ("./core"→core/index) and Windows path separators normalized.Per-consumer function resolution.
func_name_mapwas keyed by bare name across the whole program, so two modules defining a same-named function collided: a serializer's localfunction vec3(v): stringcaptured a math library'svec3(x,y,z)for every caller. Function imports now resolve per-consumer (through the re-export chains above), consulted before the global name map.NaN | 0trapped the module (i32.trunc_f64_son a NaN) instead of producing0as JS's ToInt32 requires. All bitwise ops now usei64.trunc_sat_f64_s+i32.wrap_i64— exact ToInt32 (NaN→0, modular wrap) across the whole i64 range. (nontrapping-fptointis baseline WebAssembly since 2020.)x >>> 0returned the signed value. JS defines unsigned right shift as producing a ToUint32 result, but the codegen widened the i32 back to f64 signed. Invisible for any shift ≥ 1 (shifting in a zero clears the sign bit, so signed and unsigned agree) — and wrong for exactly the canonicalx >>> 0"reinterpret as unsigned" idiom. The game packed ARGB colours with(a|r|g|b) >>> 0; the negative f64 crossed the FFI into a Rust saturatingas u32that floored it to 0, so every model tint became transparent black and alpha-cutout foliage discarded its whole canopy.>>>now widens withf64.convert_i32_u; every other bitwise operator keeps the signed conversion, which is correct for them.Verification
cargo build --release -p perry-codegen-wasm— clean..tscompiled with--target wasm, asserted in a headless browser) that fails before and passes after; the shooter it was found on now boots, renders, and plays in the browser with 0 console errors.PERRY_WASM_DEBUG_IMPORTS=1dumps the per-import resolution table (added with linux compilation and README #2/Using the fetch API #3).Summary by CodeRabbit
New Features
new Array(...)lowering, including runtime bridge support fornew Array(n)and argument-length validation.Bug Fixes
~xto match expected JS semantics.