WASM: implement str() and repr(), fix print()'s float formatting - #332
martin-henz wants to merge 7 commits into
Conversation
repr() was a copy of print() that returned None instead of a string;
str() didn't exist at all (NameError). Both are now implemented via
new TO_STR_FX/TO_REPR_FX WASM functions that reuse LOG_FX's formatting
by calling back into the exported log() and allocating the rendered
text as a new WASM string (mirroring the tokenize host import's
malloc/encode/write/makeString sequence).
Two bugs surfaced along the way, both fixed:
- log_float shared its (module, field) import key with log_int
("console"/"log" for both), so both silently resolved to the same
JS console.log at instantiation. print(5.0) rendered "5" instead of
"5.0", and large floats never got exponential notation. Renamed to
distinct log_int/log_float import fields and gave log_float its own
Python-formatting implementation (reusing toPythonFloat, now
exported from stdlib/utils.ts).
- The first str()/repr() implementation leaked a shadow-stack entry:
GET_LEX_ADDR_FX already pushes a GC'able argument onto the shadow
stack when fetching it, and the newly allocated result string pushed
a second entry, but nothing popped the first. That stray entry
corrupted whatever GC'able-operand accounting the surrounding
expression did next (e.g. string concatenation reading garbage).
Fixed by popping the input's shadow-stack entry before returning,
mirroring IS_INT_FX/MAKE_PAIR_FX's existing per-argument convention.
Also updates operator-conformance-wasm.test.ts to drop its now-obsolete
numeric-tolerance workaround for float results (verified via a fresh
3933-case run with exact-text comparison) and fixes three
wasm-modules.test.ts expectations that had baked in the old float-
formatting bug (imported numeric module values are Python floats, so
print() on one now correctly renders "42.0" instead of "42").
Closes #323.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KrwJGug3rCXijRseRjys9V
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KrwJGug3rCXijRseRjys9V
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughWASM now supports ChangesWASM string conversion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Library as miscLib.str/repr
participant Runtime as TO_STR_FX/TO_REPR_FX
participant Host as stringify.to_str/to_repr
participant Output as runtime.output
participant Memory as allocateWasmString
Library->>Runtime: pass tagged WASM value
Runtime->>Host: invoke host conversion
Host->>Output: render and retrieve text
Host->>Memory: encode and allocate result
Memory-->>Library: return WASM string
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 review |
✅ Action performedReview finished.
|
![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 `@src/engines/wasm/hostImports.ts`:
- Around line 144-150: Update the to_repr host import and its
collection-rendering path so nested string elements use repr semantics,
producing quoted values such as ['hi'] while preserving str output such as [hi].
Propagate repr awareness through log_list and child log_string calls, and add
regression coverage for repr and str on collections.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: da813468-5f8a-4d11-b359-0ed6aee78214
📒 Files selected for processing (9)
src/engines/wasm/builderGenerator.tssrc/engines/wasm/hostImports.tssrc/engines/wasm/library.tssrc/engines/wasm/runtime/index.tssrc/engines/wasm/runtime/stdlib.tssrc/stdlib/utils.tssrc/tests/operator-conformance-wasm.test.tssrc/tests/wasm-modules.test.tssrc/tests/wasm/wasm-misc.spec.ts
| to_repr: (tag: number, value: bigint): [number, bigint] => { | ||
| if (!runtime.wasmExports) throw new Error("WASM exports not initialised"); | ||
| runtime.wasmExports.log(tag, value); | ||
| const rendered = runtime.output.pop(); | ||
| if (rendered === undefined) throw new Error("repr() logging did not produce rendered text"); | ||
| const text = tag === TYPE_TAG.STRING ? escape(rendered) : rendered; | ||
| return allocateWasmString(runtime.wasmExports, memory, text); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Render collection elements with repr semantics.
Line 149 only quotes a top-level string. repr(["hi"]) and str(["hi"]) flow through log_list, whose child log_string calls produce [hi] rather than Python’s ['hi']. Add recursive repr-aware collection rendering and regression coverage.
🤖 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 `@src/engines/wasm/hostImports.ts` around lines 144 - 150, Update the to_repr
host import and its collection-rendering path so nested string elements use repr
semantics, producing quoted values such as ['hi'] while preserving str output
such as [hi]. Propagate repr awareness through log_list and child log_string
calls, and add regression coverage for repr and str on collections.
This branch has not been deployed
1 participant
Summary
Closes #323, which was originally filed as "repr() returns None instead of a string" but turned out to be larger in scope on re-investigation:
str()didn't exist on WASM at all (NameError: name 'str' is not defined), andrepr()was literally a copy ofprint()'s body — same void return, no string produced.TO_STR_FX/TO_REPR_FX(new WASM functions,src/engines/wasm/runtime/stdlib.ts) that reuseLOG_FX's existing type-dispatch formatting by calling back into the exportedlog()and popping the rendered text — the same tricklog_listalready uses for its element rendering. The formatted text is then allocated as a real WASM string via the same malloc/encode/write/makeStringsequence thetokenizehost import uses (factored out into a sharedallocateWasmStringhelper inhostImports.ts, also now used bytokenizeitself instead of its own inline copy).str/reprintosrc/engines/wasm/library.ts'smiscLib, replacing the brokenreprentry.repr()applies Python's exact string-quoting rules (single-quote-preferring, switches to double quotes when the string itself contains a') by reusingescape()fromsrc/stdlib/utils.ts(now exported) instead of reinventing it.Two bugs found and fixed along the way
log_floatformatting.wasm.import("console", "log").func("$_log_int")andwasm.import("console", "log").func("$_log_float")declared the same(module, field)import key, so both silently resolved to the same JSconsole.logat instantiation regardless of which local alias a call site referenced.$_log_floatended up bound to the int-formatting implementation (value.toString()), soprint(5.0)rendered"5"instead of"5.0", and large floats never got exponential notation. Renamed the imports to distinctlog_int/log_floatfields and gavelog_floatits own implementation usingtoPythonFloat(the same function CSE's owntoPythonStringuses, now exported fromstdlib/utils.ts), so WASM's float formatting now matches CSE exactly.Shadow-stack leak (first implementation attempt).
GET_LEX_ADDR_FXalready pushes a fetched argument onto the shadow stack when it's GC'able (protecting it during any subsequent allocation before it's consumed). The first version ofTO_STR_FX/TO_REPR_FXdidn't account for this: it read the argument, computed a result, and returned a new string (itself pushed byMAKE_STRING_FXas a side effect) — leaving the original argument's shadow-stack entry never popped. That stray entry sat underneath the new result and threw off whatever GC'able-operand accounting the surrounding expression did next. Concretely,"[" + repr("hi") + "]"produced garbage (unrelated static string-table bytes) instead of"['hi']", because the+operator's string-concat branch unconditionally pops exactly two shadow-stack entries for its two operands and got the wrong ones. Fixed by popping the input's shadow-stack entry (if GC'able) before computing, mirroring the existingIS_INT_FX/IS_PAIR_FX/MAKE_PAIR_FXper-argument convention.Test changes
src/tests/wasm/wasm-misc.spec.ts— newstr() and repr()describe block: per-type formatting, quoting rules, and a regression test for the shadow-stack leak (string concatenation of astr()/repr()result).src/tests/operator-conformance-wasm.test.ts— dropped the numeric-tolerance workaround for float results (previously needed only because of thelog_floatbug above); floats are now compared as exact text like every other type. Verified with a full 3933-case run.src/tests/wasm-modules.test.ts— updated 3 expectations that had baked in the old float-formatting bug: imported numeric module values (DataType.NUMBER) map to Python floats at the WASM boundary (seemoduleInterop.ts), soprint()on an imported42now correctly renders"42.0", not"42".Test plan
npx tsc --noEmityarn lint(targeted eslint on changed files)yarn test— full suite, twice, both green (19234 passed / 3166 skipped)operator-conformance-wasm.test.tsrun standalone (3933 cases) with the tightened exact-text float comparison🤖 Generated with Claude Code
https://claude.ai/code/session_01KrwJGug3rCXijRseRjys9V