Conversation
There was a problem hiding this comment.
Pull request overview
Implements Wasm backend support for class instance methods by lowering them to standalone Wasm functions that take an explicit this parameter, plus a prescan/predeclaration step so methods can be referenced before their bodies are emitted.
Changes:
- Add prescan predeclaration and emission/overwrite logic for class methods in the Wasm WAT builder, including implicit-call behavior for getter-style methods.
- Extend Wasm codegen to recognize and compile method calls/selections into direct Wasm calls with
thisas the first operand. - Add Wasm test coverage for class methods (including recursion/mutual recursion and chaining) and extend lowering to support assignment targets using
SelProj.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| hkmc2/shared/src/test/mlscript/wasm/ClassMethods.mls | New Wasm diff tests covering method calls, getters, recursion, chaining, and an expected error case. |
| hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala | Adds method predeclaration and direct-call lowering with explicit this, plus updates function setup to support an optional this param. |
| hkmc2/shared/src/main/scala/hkmc2/codegen/Lowering.scala | Adds assignment lowering support for SelProj(...) LHS into AssignField. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Resolves `sym` to a predeclared class method symbol, if any. */ | ||
| private def predeclaredClassMethodSym(sym: DefinitionSymbol[?])(using Ctx): Opt[BlockMemberSymbol] = | ||
| sym.asBlkMember.filter: methodSym => | ||
| methodSym.asTrm.exists(_.owner.exists(_.asCls.isDefined)) && ctx.getFunc(methodSym).nonEmpty |
There was a problem hiding this comment.
predeclaredClassMethodSym currently treats any resolved DefinitionSymbol whose asBlkMember has a predeclared function as a “class method”. This can misclassify field selections in the presence of overloaded members (e.g., a val and fun sharing the same BlockMemberSymbol) and route them through the method-call path. Consider additionally checking that the resolved symbol’s defn is a TermDefinition with k == syntax.Fun (and ideally that it refers to the correct overload) before treating it as a method.
| methodSym.asTrm.exists(_.owner.exists(_.asCls.isDefined)) && ctx.getFunc(methodSym).nonEmpty | |
| sym.defn match | |
| case trmDefn: syntax.TermDefinition => | |
| trmDefn.k == syntax.Fun && | |
| methodSym.asTrm.exists(_.owner.exists(_.asCls.isDefined)) && | |
| ctx.getFunc(methodSym).nonEmpty | |
| case _ => false |
| clsLikeDefn.methods.foreach: | ||
| case methodDefn @ FunDefn(_, sym, _, Nil, bod) => | ||
| overwriteMethod(sym, Seq(clsLikeDefn.isym), PlainParamList(Nil), bod) | ||
| case methodDefn @ FunDefn(_, sym, _, _ :: _ :: _, _) => | ||
| break(errUnimplExpr("multi-parameter-list method")) |
There was a problem hiding this comment.
clsLikeDefn.methods may contain multiple FunDefns with the same BlockMemberSymbol when a method name is overloaded (the symbol model explicitly supports overload sets). Iterating and overwriting the Wasm function by sym will silently keep only the last overload, so calls to other overloads will compile but invoke the wrong body. Please either (a) reject overloaded methods in the Wasm backend with a clear error, or (b) predeclare/emit distinct Wasm functions per overload (e.g., key by the TermSymbol/disambiguated symbol or mangle by signature) and adjust call lowering accordingly.
f5b9c17 to
323afbd
Compare
This PR implements wasm class methods, which are lowered as standalone direct-call wasm functions with an explicit
thisparameter.This PR supersedes #429.