Conversation
There was a problem hiding this comment.
Pull request overview
Adds initial WebAssembly class inheritance support in the MLscript-to-WAT backend by predeclaring class hierarchy metadata, emitting inherited struct layouts, splitting subclass construction into ctor/init, and extending method + pattern-match lowering to work with inherited members.
Changes:
- Prescans top-level classes to register parent links and allocate per-class type/tag/constructor metadata.
- Lowers inheritance as Wasm GC struct subtyping with parent-prefix field layouts and parent-first init sequencing.
- Adds method metadata + direct-call lowering (including inherited method lookup) and updates class-pattern tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/WatBuilder.scala | Implements class hierarchy prescan, ctor/init split, method emission/calls, and inheritance-aware class pattern tests. |
| hkmc2/shared/src/main/scala/hkmc2/codegen/wasm/text/Ctx.scala | Stores constructor/method metadata and registered class parent relationships for use during lowering. |
| hkmc2/shared/src/test/mlscript/wasm/Methods.mls | Adds/updates method call tests (getter vs nullary vs callable, chaining, mutation). |
| hkmc2/shared/src/test/mlscript/wasm/MethodInheritance.mls | Adds tests for inherited method access and calls across subclasses. |
| hkmc2/shared/src/test/mlscript/wasm/FieldInheritance.mls | Adds tests for inherited field layouts, access, and mutation behavior. |
| hkmc2/shared/src/test/mlscript/wasm/ClassInheritance.mls | Adds tests for inheritance-aware is checks and class-pattern matching across hierarchies. |
| hkmc2/shared/src/test/mlscript/wasm/Matching.mls | Updates golden WAT output to reflect direct constructor calls (no call_ref indirection). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val prescanDefn = defn.copy(parentPath = N)(defn.configOverride) | ||
| if isSupportedTopLevelClass(prescanDefn) && (defn.k is syntax.Cls) then |
There was a problem hiding this comment.
collectTopLevelClasses builds prescanDefn by forcibly clearing parentPath before calling isSupportedTopLevelClass, which bypasses the new inheritance/preCtor support checks. This can cause unsupported subclasses (e.g., with an inherited preCtor shape you don’t handle) to be collected and have type/ctor metadata registered anyway, leaving the context in a partially-initialized state and later failing in returningTerm with unrelated “not implemented” errors. Consider checking isSupportedTopLevelClass(defn) directly (or explicitly validating the inherited preCtor/parentPath instead of zeroing it).
| val prescanDefn = defn.copy(parentPath = N)(defn.configOverride) | |
| if isSupportedTopLevelClass(prescanDefn) && (defn.k is syntax.Cls) then | |
| if isSupportedTopLevelClass(defn) && (defn.k is syntax.Cls) then |
| source = Diagnostic.Source.Compilation, | ||
| ), | ||
| ) | ||
| break(Set.empty) |
There was a problem hiding this comment.
When a cycle is detected, cycleError raises an error but then break(Set.empty) only exits the prescan boundary; codegen continues with no parent metadata, which then triggers secondary internal errors like WatBackend::returningTerm ... parentPath.nonEmpty not implemented yet for the same classes. It’d be better to suppress/short-circuit subsequent class lowering after a cycle is reported (e.g., mark the involved classes as invalid in Ctx, or have callers stop further lowering when registerClassHierarchyMetadata breaks).
| break(Set.empty) | |
| break(ordered.iterator.map(_.sym).toSet) |
| localClassTypeScopes.iterator.collectFirst(Function.unlift(_.get(local))) | ||
|
|
||
| /** Updates the innermost scope with a class type for a local, or clears it if no class is known. */ | ||
| private def setlocalClassType(local: Local, clsSym: Opt[BlockMemberSymbol]): Unit = |
There was a problem hiding this comment.
Method name setlocalClassType looks like a typo/inconsistent casing compared to the rest of the file (e.g., withKnownLocalClassTypes, localClassType). Renaming to setLocalClassType would improve readability and avoid confusion.
| private def setlocalClassType(local: Local, clsSym: Opt[BlockMemberSymbol]): Unit = | |
| private def setLocalClassType(local: Local, clsSym: Opt[BlockMemberSymbol]): Unit = |
| /** Returns metadata for a method declared directly on `clsSym` with name `methodName`, if present. */ | ||
| def getDeclaredMethodInfo(clsSym: BlockMemberSymbol, methodName: Str): Opt[Ctx.MethodInfo] = | ||
| methodInfoBySymbol.valuesIterator.find: info => | ||
| info.dSym.nme == methodName && info.ownerIsym.asBlkMember.contains(clsSym) |
There was a problem hiding this comment.
getDeclaredMethodInfo performs a linear scan over methodInfoBySymbol.valuesIterator for every lookup, and getMethodInfoInHierarchy may call this repeatedly while walking ancestors. This can become a noticeable compile-time hot path with many methods/classes. Consider maintaining an indexed map keyed by (ownerClassSym, methodName) (or Map[ownerClassSym, Map[methodName, MethodInfo]]) when registering methods.
This PR adds basic class-inheritance support by predeclaring class hierarchy metadata, lowering subclasses as struct subtypes with parent-prefix field layouts, and splitting construction into a public constructor and an internal parent-first init; updates class pattern matching, adds inherited non-overriding methods.