Skip to content

Port JS scoped-blocks and remove definedVars#400

Merged
LPTK merged 13 commits intohkust-taco:hkmc2from
b-temirov:scoped-blocks
Mar 9, 2026
Merged

Port JS scoped-blocks and remove definedVars#400
LPTK merged 13 commits intohkust-taco:hkmc2from
b-temirov:scoped-blocks

Conversation

@b-temirov
Copy link
Copy Markdown
Contributor

This PR ports JSBuilder scoped-block local allocation semantics to wasm backend. It removes definedVars local collection and switches to scope-driven allocation.

Copilot AI review requested due to automatic review settings February 27, 2026 14:28
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Ports JSBuilder-style scoped-block local allocation to the Wasm text backend by removing reliance on definedVars and adding new coverage for scoped locals behavior in the Wasm tests.

Changes:

  • Add ScopedLocals.mls Wasm tests covering local allocation across top-level lets, chaining, if/else branches, and match arms.
  • Update WatBuilder to (a) handle Assign(State.noSymbol, ...) by evaluating and dropping results, and (b) switch block-local allocation to scope-/Scoped-driven allocation via withLocalDelta + nonNestedScoped.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
hkmc2/shared/src/test/mlscript/wasm/ScopedLocals.mls Adds new Wasm test cases validating scoped-local allocation semantics (and some WAT snapshots).
hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala Implements scope-driven local allocation, removes definedVars dependency, and adds a noSymbol assignment handling path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala Outdated
Comment thread hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala Outdated
Copy link
Copy Markdown
Contributor

@LPTK LPTK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the change proposed by Copilot, I realize that there's a bunch of junk in the WatBuilder that was mindlessly copied from JSBuilder, such as def body now being unused. As a more serious example, it doesn't make any sense to use things like scope.nestRebindThis, as this deals with details of how JavaScript deals with the this keyword (which doesn't even exist in Wasm) in nested definitions (which aren't even possible in Wasm).

Please make a pass to remove and simplify this unnecessary logic.

@b-temirov
Copy link
Copy Markdown
Contributor Author

  1. Removed dead code:
  • Deleted unused body helper.
  • Removed unused name parameter from setupFunction.
  1. Removed JS-specific this rebinding flow:
  • Replaced scope.nestRebindThis(...) with scope.nest in Define handling.
  • Removed thisProxy / thisProxyDefined branch and error path.
  • Replaced scope.findThis_! usages with explicit scope/local lookup.
  1. Added explicit constructor self binding:
  • Introduced bindCtorThis(...) to bind constructor isym directly to local name "this" in wasm scope.

@LPTK
Copy link
Copy Markdown
Contributor

LPTK commented Mar 4, 2026

3. Added explicit constructor self binding:

There is no test to accompany this change. There should be at least one WASM test that uses this.

@b-temirov
Copy link
Copy Markdown
Contributor Author

Added two new test cases to Basics.mls.

Copy link
Copy Markdown
Contributor

@LPTK LPTK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM now. Except for this one small question.

Comment thread hkmc2/shared/src/test/mlscript/wasm/Basics.mls Outdated
@LPTK LPTK merged commit 0044359 into hkust-taco:hkmc2 Mar 9, 2026
1 check passed
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.

3 participants