Repository navigation
Conversation
| //│ dead-param-elim > <<< dead-param-elim results <<< | ||
| //│ ——————————————| Optimized IR |—————————————————————————————————————————————————————————————————————— | ||
| //│ @private define f⁰ as fun f¹(a) { return a }; return 42 | ||
| //│ @private define f⁰ as fun f¹(a) { return (a as! Int⁰) }; return (42 as! Int⁰) |
There was a problem hiding this comment.
Why are casts now inserted here?
There was a problem hiding this comment.
[Astra] These casts became redundant after propagation/inlining, but the simplifier did not reconsider them. Fixed in 1f5390b: it now removes a cast when the rewritten operand is provably below its target, including checked casts whose check is guaranteed to succeed. This example is back to return a and return 42. The full test suite passes.
There was a problem hiding this comment.
[Claude] Verified: at b87501d, this file's output is identical to hkmc2 again.
| main1() | ||
| //│ dead-param-elim > >>> dead-param-elim results >>> | ||
| //│ dead-param-elim > apply⁰#0 @ main1⁰.main⁰.apply¹ -> eliminable: {1} | ||
| //│ dead-param-elim > lambda⁰#0 @ main1⁰ -> eliminable: {0} |
There was a problem hiding this comment.
Why is this optimization lost?
There was a problem hiding this comment.
[Astra] Lowering assigned no erased representation to generated lambda functions. That introduced a checked cast around the closure, which made flow analysis treat it as escaping and prevented dead-parameter elimination. Fixed in f853f10 by preserving the generated function calling convention. The unused parameter is eliminated again; the full suite passes.
There was a problem hiding this comment.
[Claude] Verified: the parameter is eliminated again, and this file matches hkmc2.
f853f10 has a side effect in the compiled Predef.mjs and Runtime.mjs: closures passed as arguments now stay bound to a temporary. For example, Predef.equals now reads:
lambda$here = lambda$1(Predef, b);
return runtime.safeCall(a.every(lambda$here))where hkmc2 has a.every(lambda$1(Predef, b)).
Erased types do not block inlining here. In the IR, call arguments must be paths (Arg.value: Path), and BlockSimplifier only propagates pure calls into Result positions. On hkmc2, the generated lambda had no representation, so lowering wrapped the argument in a checked cast to Function, the erasure of the function type that the prelude declares for every's parameter. Cast is a Path whose operand is a Result, so the simplifier could substitute the call into it, and JS code generation drops the cast. The optimized IR was a.every((lambda$(P.this, b) as! Function)).
Now that generated lambdas have a function representation, that cast is correctly gone, and nothing can absorb the call. Getting the old output back would take either allowing pure calls as arguments or folding single-use temporaries into arguments in the JS backend. Neither is specific to this PR, so I've listed this as a known limitation in the PR description.
| //│ else | ||
| //│ set tmp = -⁰(n, 1); | ||
| //│ return countdown¹(tmp) | ||
| //│ return countdown¹((tmp as! Int⁰)) |
There was a problem hiding this comment.
Why is a cast now inserted here?
There was a problem hiding this comment.
[Astra] Shape inference now gives n the Int representation, while the lowered builtin subtraction still has an unspecified result representation. The cast bridges that physical result to the inferred parameter slot. It is not a cast from Int to Int at the IR level, so the redundant-cast cleanup deliberately preserves it. Giving arithmetic builtins precise result representations would allow this cast to disappear too.
There was a problem hiding this comment.
[Claude] Fixed in b87501d, using the workaround @LPTK suggested. Until the builtins are declared as overloaded functions, semantics.BuiltinOperatorResults approximates operator results from the primitive classes of their operands, on the JS target only. Resolution and IR erasure share these rules.
-⁰(n, 1) is now an Int, so the cast is gone and this file matches hkmc2 again. For the same reason, sumPlus in dead-param-elim/todos.mls is back to its baseline output (Nil => return 1), and so are checkArgs and showStackTrace in Runtime.mjs.
| //│ match xss | ||
| //│ Nil⁰ => | ||
| //│ return 0 | ||
| //│ return (len$ls_Nil⁰() as! Object⁰)(eta$0$0, eta$0$1, eta$0$2, eta$0$3, eta$0$4) |
There was a problem hiding this comment.
Can we avoid this unsightly case?
There was a problem hiding this comment.
[Astra] Fixed in 551e4a8. Generated branch functions now retain their two parameter lists, so supplying just the constructor fields produces Function rather than an unknown value. The Object cast disappears and the branch calls inline again. The fix also updates fused parameters, results, captured values, and locals together; regression tests cover monomorphic fusion and field projection. The full suite passes.
There was a problem hiding this comment.
[Claude] Verified: no as! Object casts remain in this file, and the branch calls are inlined again.
| //│ return flatten$flatten$worker⁰(tmp4, undefined, undefined, undefined) | ||
| //│ set tmp5 = flatten$flatten⁰((tmp4 as! Object⁰)); | ||
| //│ return tmp5(undefined, undefined, undefined, undefined, undefined) |
There was a problem hiding this comment.
This looks like a concerning regression.
There was a problem hiding this comment.
[Astra] Fixed the lost branch inlining in 551e4a8 and the remaining partial-call propagation regression in 3caffd0. Checked arguments are now evaluated at the original partial application site, leaving a pure call prefix that can combine with its later application. The enum/sum example is back to an inlined loop, and flatten/append no longer retain the branch closures shown here. New regressions check argument capture and preservation of Wasm cast failures; the full suite passes.
There was a problem hiding this comment.
[Claude] Verified: the call is back to flatten$flatten$worker⁰(tmp4, undefined, undefined, undefined), as on hkmc2. I also checked that the new simplifier rule that hoists checked arguments out of partial applications terminates.
| private fun roundtrip(value) = | ||
| let saved = value | ||
| Slot(saved).value | ||
| let input = wasm.i32.const(42) |
There was a problem hiding this comment.
Why not open wasm.i32 { const } to avoid all these repeated paths?
There was a problem hiding this comment.
[Astra] Done in 0629de9. Qualified open bases are currently rejected, so the test uses open wasm { i32 } followed by open i32 { const }, and then plain const(...) throughout. The regenerated erasure test passes.
There was a problem hiding this comment.
[Claude] Verified: open wasm.i32 { const } is rejected with "Illegal 'open' statement base.", so the two-step open is the best option for now.
| //│ set tmp1 = Predef⁰.hide⁰(4); | ||
| //│ set tmp2 = +⁰(tmp1, 1); | ||
| //│ return Predef⁰.print⁰(tmp2) | ||
| //│ set x = (tmp as! Int⁰); |
There was a problem hiding this comment.
[Astra] x is now inferred as Int, including its writes. hide remains a separately compiled generic function with an unknown physical result representation, even though resolution propagates the argument shape through it. The cast narrows that result before storing it in x; retaining it preserves the imported function’s calling convention. Casts exposed as redundant by propagation/inlining are now removed in 1f5390b.
There was a problem hiding this comment.
[Claude] The mechanism is right. hide is a separately compiled polymorphic function with an unknown physical result, while resolution sees the Int flowing through it. So x is inferred as Int, and each write narrows hide's result with a checked cast. The cast is not needed to preserve hide's calling convention, though: leaving x at the top representation would preserve it too.
The cost shows in this test. Checked casts are effectful, so the dead write set x = hide(3) survives as do (tmp1 as! Int⁰). b87501d removes the cast on the +⁰ result, since builtin results are now approximated, but two kinds of casts remain:
- the casts of
hide's results; do (old as! Int⁰)in thefinallyblock of theset … intest below, because theoldtemporary that lowering introduces has no representation.
The second kind could be fixed by giving such temporaries the representation of the variable they copy. The first needs representations instantiated per call for polymorphic functions. Both are listed as known limitations in the PR description.
There was a problem hiding this comment.
[Claude] Update: the old cast is fixed in d710d52. Erasure now infers the representation of the temporary that set … in declares, like a source variable, so the set … in test below is back to its hkmc2 output. (On Wasm, the same untyped temporary made set x = … in an error for an unboxed Int32 variable; newres/wasm/Erasure.mls now covers that.) The casts of hide's results remain.
| //│ (import "repl" "counter" (global $counter (mut (ref null any)))) | ||
| //│ (import "repl" "step" (global $step (mut (ref null any)))) | ||
| //│ (func $entry (export "entry") (type $entry) (result (ref null any)) | ||
| //│ (return | ||
| //│ (call $plus_impl | ||
| //│ (global.get $counter) | ||
| //│ (global.get $step)))) | ||
| //│ (import "repl" "counter" (global $counter (mut (ref null i31)))) | ||
| //│ (import "repl" "step" (global $step (mut (ref null i31)))) | ||
| //│ (func $entry (export "entry") (type $entry) (result (ref null i31)) | ||
| //│ (local $tmp (ref null any)) | ||
| //│ (block (result (ref null i31)) | ||
| //│ (local.set $tmp | ||
| //│ (call $plus_impl | ||
| //│ (global.get $counter) | ||
| //│ (global.get $step))) | ||
| //│ (return | ||
| //│ (ref.cast (ref null i31) | ||
| //│ (local.get $tmp))))) |
There was a problem hiding this comment.
Why is the generated code longer than it used to be?
There was a problem hiding this comment.
[Astra] The worksheet globals and entry result are now inferred as Int (Wasm (ref null i31)), while the imported plus_impl still returns (ref null any). The extra ref.cast is therefore required at that boundary. The extra local is the existing normalization of the call followed by its cast. Previously all three slots were anyref, so no narrowing was necessary.
There was a problem hiding this comment.
[Claude] Accurate. The cast is inherent to narrowing counter: once the write to counter is emitted, storing the anyref result of plus_impl into the i31 global needs the same cast.
Related, but it predates this PR: neither this branch nor hkmc2 emits global.set $counter for set counter += step. The block returns 3, but a later block still reads counter = 1. I've proposed fixing that separately.
| //│ (local $receiver (ref null any)) | ||
| //│ (return | ||
| //│ (block (result (ref $Child)) | ||
| //│ (block (result (ref null $Child)) |
There was a problem hiding this comment.
Why is this now nullable?
There was a problem hiding this comment.
[Astra] This is intentional: a source slot of type Child must also accept undefined, whose Wasm representation is a null reference. Hence source parameters/results use (ref null $Child), including this cast of a virtual call's result. Constructor results and the receiver passed to an actual method remain non-null. The undefined parameter/field/result regressions in newres/wasm/Erasure.mls cover the distinction.
There was a problem hiding this comment.
[Claude] Accurate, but this has a cost that the reply doesn't mention:
- Field reads through class-typed parameters and locals now need a
ref.cast (ref $C)first (seegetXinwasm/Basics.mls). wasm/ClassMethods.mlsgets a double cast:(ref.cast (ref $A) (ref.cast (ref null $A) …)).- Constructor results are non-null in constructor signatures, but the locals that receive them are nullable (
(local $tmp (ref null $Foo))).
@LPTK asked whether, since reading back undefined is undefined behavior, it would make sense to fill such a slot with an uninitialized object of the corresponding class. Yes. This is consistent with the semantics now documented under Runtime Representations in doc/reference.md, and it would let class slots be non-null (ref $C), which removes these casts. A sketch:
- Keep one placeholder instance per class that needs one, in an immutable global. Build it with
struct.newfrom the class's type info and from placeholders for its fields: zero for numeric fields,ref.i31 0for boxed integers, and the placeholder of the field's class for class-typed fields. Abstract classes work the same way, since Wasm does not distinguish them.Function,Array, andStrslots also need placeholder values. - A class whose non-null class-typed fields form a cycle with no abstract class in between, like
class Node(next: Node), has no finite placeholder, so its slots could stay nullable. Withoutundefined, such a class cannot be instantiated anyway.
The alternative is to make a slot nullable only when undefined can reach it. Resolution observes undefined like any other value, and dead-parameter elimination knows which parameters it fills with placeholders. But that requires tracking nullability in erased types, which placeholders avoid. I haven't implemented either; it seems best done in a follow-up.
| //│ this.#b = tmp; | ||
| //│ lambda$here = lambda$(this); | ||
| //│ this.f = lambda$here; | ||
| //│ this.f = lambda$(this); |
There was a problem hiding this comment.
Looks like this is no longer inlined.
There was a problem hiding this comment.
[Astra] This test explicitly uses :noInline, and the call was not inlined before either: the old output assigned lambda$(this) to lambda$here and then copied that temporary into this.f. The reviewed output assigned the same call directly to this.f, removing a temporary rather than losing inlining. After the generated-function representation fix in f853f10, this snapshot is back to its original form.
There was a problem hiding this comment.
[Claude] Verified: this hunk is back to its hkmc2 form. The file's other differences from hkmc2 are improvements, such as the p = tmp3 copies now being propagated.
149217f to
c6b0f63
Compare
Preserve delayed erasure for constructor and placeholder symbols alongside private constructor storage and placeholder source locations. Keep exhaustive loops non-terminating in the ScopedBlocks expectation, and add erasure regressions for private construction, private shadowing, implicit lambdas, and trailing arguments. Regenerate golden outputs. Validated with Metals compilation, ctest, focused diff tests, and hkmc2AllTests/test.
- Type generator calls as iterators: `@generator` functions return an `Iterator`, both in resolution (`listenDefinitionResult`) and in erasure (`callableResult`), instead of the payload of their final step. - Check overrides against the representations of the members they override: unannotated override parameters take the inherited representations, override results must fit the inherited result, and declared parameters must admit the inherited ones (and match them on Wasm, which calls overrides through the signature of the overridden method). - Require type annotations on open members whose parameters are exposed to other compilation units, since subclasses there must implement them with the same representations. Annotate such members in the library and the tests. - WORKAROUND: approximate the results of builtin operators from the primitive classes of their operands (`BuiltinOperatorResults`), in resolution and in IR erasure, until the builtins are declared as overloaded functions. This only applies to the JavaScript target, as the Wasm backend implements operators with dynamically typed intrinsics. - Locate representation-conversion diagnostics (`Lowering.castTo` and `WatBuilder.castConserve`). - Document runtime representations in `doc/reference.md`, including `undefined` as undefined behavior and the rejection of programs whose only problem is a misleading annotation. - Add regression tests for the inference of representations across worksheet blocks, with a :fixme for the Wasm REPL crash when reading a field whose representation is another session class. - Clear `WasmScratch.mls`, moving its unique tests to `BuiltinOperators.mls`, `Casts.mls`, and `ReplImports.mls`. Co-Authored-By: Claude Opus 5.5 <[email protected]>
There was a problem hiding this comment.
[Claude] This is my review of the PR at d41909e, updated with @LPTK's answers. The fixes are in b87501d. A follow-up commit, e5e6126, updates two line numbers in a golden output: comment edits in b87501d shifted them, and CI's no-changes check caught it. hkmc2AllTests/test passes at e5e6126 with no golden-output changes. The PR description now covers the design and the known limitations, and I've replied to the existing threads in place.
These are the findings that the existing threads didn't cover (details inline):
- Wasm virtual dispatch could trap at run time (at
Erasure.checkOverride): overrides are now checked against the members they override, and exposed open members must be annotated. - Without strict resolution, worksheet blocks fix the representations of public definitions (at
newres/loose/ErasureAcrossBlocks.mls): this only affects diff tests, so the new tests record the behavior. - Generators got the representation of their final payload (at
Erasure's callable results): fixed. - Builtin operator results had no representation (at
BuiltinOperatorResults): they are now approximated, as a marked workaround. - Closure arguments are no longer inlined in
Predef.mjsandRuntime.mjs: explained in thedead-param-elim/lambda.mlsthread. Erased types are not the cause, and this is now listed as a known limitation. - Every class-typed Wasm slot is nullable: discussed in the
wasm/VirtualMethods.mlsthread, including placeholder objects forundefined.
Also in b87501d:
- Conversion diagnostics have source locations (at
Lowering.castTo). - The design decisions are documented (at
doc/reference.md). wasm/WasmScratch.mlsis cleared. Its unique tests moved towasm/BuiltinOperators.mls,wasm/Casts.mls, andwasm/ReplImports.mls, and the duplicates were deleted.
One more note: the merge commit d41909e also contains real changes. codegen/ScopedBlocks.mls now expects an infinite loop instead of an error, and the commit adds erasure tests. The new expectation looks right (a while … else whose branches all repeat never exits), but changes like these are easier to review in their own commit.
| * fail at run time. Parameters without declared types already take the inherited representations (see | ||
| * `inheritedParams` in `run`), so only declared ones can fail the parameter checks. | ||
| */ | ||
| private def checkOverride(td: TermDefinition): Unit = |
There was a problem hiding this comment.
[Claude] Finding 1. On Wasm, an override is called through the virtual slot of the method that introduced it, but that slot's representations were inferred only from how the introducing method itself is used. Programs that work on hkmc2 trapped with RuntimeError: illegal cast, even within one block and with private classes:
private class P1() with
open fun get() = P1()
private class C1() extends P1() with
override fun get() = "str"
private fun read1(p) = p.get()
read1(C1())
Since inheritance is open-world, @LPTK asked to check overrides rather than merge their representations into the base. b87501d does the following:
- This check requires overrides to satisfy the erased signature of the member they override:
- An unannotated override parameter takes the inherited representation (
inheritedParamsinrun). - A declared parameter must admit the inherited representation, and on Wasm match it exactly.
- The result must fit the inherited result.
overrideResultTypecoerces a wider result of an unannotated override to the inherited one with checked casts. An unrelated result is reported here.
- An unannotated override parameter takes the inherited representation (
checkExposedOpenMember(below) requires an open member to annotate its parameters and result when its parameters receive values from the unit's interface. This is detected through the existing unknown propagation (UnknownValueShape.fromPublicInterface).
The tests are in wasm/VirtualMethods.mls (Producer/StrProducer is the program above with public classes, plus Consumer/EchoConsumer, O1/O2, and N1/N2) and in newres/Erasure.mls (JS). The library and the tests now annotate their exposed open members (Iter.mls, ObjectBuffer.mls, Runtime.mls, newres/Overrides.mls, newres/wasm/Super.mls).
| @@ -0,0 +1,34 @@ | |||
| :. | |||
|
|
|||
| // Worksheet blocks are compiled one after the other, and the representations erasure infers for a block's | |||
There was a problem hiding this comment.
[Claude] Finding 2. Without strictResolution, Elaborator.topLevel does not expose a block's public definitions to unknown callers, and each block's observations are sealed when the block completes. So later blocks must use the representations inferred from an earlier block's own uses:
ident("a")afterident(1)is rejected.set counter = "hello"afterlet counter = 1is rejected too, even under strict resolution, since top-levellets are not exposed to later writes.
Per @LPTK, this only matters for diff tests (compiled files expose their exports), so this file and wasm/ErasureAcrossBlocks.mls record the behavior rather than change it.
The Wasm file also has a :fixme for a crash that this behavior makes more likely. Reading b.v, where the representation of v is another session class, fails with Missing type definition for type index $Foo, because the session module does not import the types that its imported struct types refer to. A declared field type fails the same way.
| // Resolution gives calls of such functions the same wrapper shapes. | ||
| val callableResult = | ||
| if !config.language.useNewResolution then resultType | ||
| else td.resultWrapperAnnotation match |
There was a problem hiding this comment.
[Claude] Finding 3. Only @async results were special-cased, so @generator fun gen() = { yield 0; 1 } was lowered as gen(): Int, the representation of its final payload. This would mislead anything that trusts the IR's types. Fixed in b87501d:
TermDefinition.resultWrapperAnnotationcovers both annotations. Async generators are not modeled; they are treated as async.- Resolution's
listenDefinitionResultpublishes anIteratorfor generator calls, which resolves the:fixmeinnewres/GeneratorResults.mls. - Here, the callable returns an
Iterator, while returns in the body are still coerced to the payload's representation (bodyResultType).
|
|
||
| import hkmc2.utils.*, shorthands.* | ||
|
|
||
| /** WORKAROUND: the builtin operators are not declared functions, and several of them are overloaded |
There was a problem hiding this comment.
[Claude] Finding 4. Builtin operators had no representation, so every write of a builtin result such as n - 1 into an inferred Int slot got a checked cast. Checked casts are effectful, so they blocked dead-code elimination and branch pushing. Examples are dead-param-elim/recursive.mls, sumPlus in dead-param-elim/todos.mls, and the extra name and localsMsg variables in Runtime.mjs. All three are now back to their hkmc2 output.
@LPTK pointed out that the proper fix is overloaded builtins, which is out of scope here. This object is the requested workaround, and it is marked as such here and at its two uses (NewResolver.builtinApplication and ErasedType.builtinOperatorResult). Notes:
- Resolution and IR erasure share these rules, so the representation inferred for a slot agrees with the erased type of the application flowing into it.
- The rules apply to the JS target only. On Wasm, operators lower to dynamically typed intrinsics (
wasm.*_impl), and their results are unknown. - Like InvalML's prelude signatures, the rules assume that
Intarithmetic stays withinInt, so an overflow toInfinityis not detected. As a result,fun f(x: Int, y: Int): Int = x + yno longer gets a checked cast.codegen/CheckedCasts.mlsandcodegen/ErasedTypes.mlsnow useNumoperands so that they still test casts. - The casts that remain in
opt/DeadAssignmentRemoval.mlshave other causes (see that thread).
| */ | ||
| def castTo[R <: Result](r: R, expected: Opt[ErasedType], loc: Opt[Loc]): (R | Cast) = | ||
| r.coerceTo(expected.getOrElse(ErasedType.Unknown), loc) | ||
| r.coerceTo(expected.getOrElse(ErasedType.Unknown), loc.orElse(r.toLoc)) |
There was a problem hiding this comment.
[Claude] From my review: conflicts in inferred slots were reported without a source location (═══[COMPILATION ERROR] Types 'Int32' and 'Unknown' have no common representation). Fixed in b87501d:
- A coercion without a better location now points at the coerced value.
- A field synthesized from a constructor parameter points at the field's name (the
ValDefncase). - The errors of the Wasm backend's
castConservecarry the location of the value being cast.
| still reported. Dynamic values also do not remove the need for an unambiguous | ||
| class or type identity in constructor patterns, class projections, or type references. | ||
|
|
||
| ### Runtime Representations |
There was a problem hiding this comment.
[Claude] As @LPTK asked, the two design decisions from my review are now explicit:
undefinedis an unspecified value, and reading it back is undefined behavior. This is documented here, in the literal table above, and inBlock.isUndefined.- Programs whose only problem is a misleading annotation are now rejected. This is documented here and in the
Erasureclass documentation.
This section also covers interface exposure, worksheets, overrides, and the builtin-operator approximation.
| td.body.foreach(body => listenResult(body)(publish)) | ||
| // Mutable fields admit replacements not represented by initializer flow. | ||
| // Keep the resolver's unknown-write alternative when choosing their storage. | ||
| if (td.k is syntax.MutVal) || mutableField then |
There was a problem hiding this comment.
[Claude] Re @LPTK's question about my remark that mutable fields "stay untyped correctly": I meant that the PR handles them conservatively. Resolution does not observe writes through selections (set o.f = v; assignVariable only tracks variables), so an initializer's shapes do not cover later writes. This line adds an unknown input instead, so mutable fields keep the top representation, including when later blocks write to them.
Agreed that they should be typed from their shapes, and that open-ended mutable fields exposed through the unit's interface should require an annotation. For that, resolution needs to route each field write to the field's host, for every class the receiver can be an instance of, and add an unknown write for fields reachable from the interface. I'd do this as a follow-up.
There was a problem hiding this comment.
[Astra] The mutable-field inference described as a follow-up above is now implemented in this PR (8bfe6a3 and 1b878f2), and 081891b fixes the remaining exposure and variance problems.
Exposure now follows values assigned into escaping record fields as well as their initializers, including fields copied through nested mutable spreads and assignments discovered after exposure. It preserves the member scope paths and generic substitutions. Thus an assigned object with an unannotated mutable class field gets the required annotation error, and an assigned callback receives unknown external inputs. The three previously documented exposure failures are now ordinary passing regression tests.
The variance concern was correct: one substituted field type cannot represent both bounds. Structural fields now retain separate input/output types internally, and the syntax { mut f: in S out T } is supported. Inputs are contravariant and outputs covariant; they are interpreted separately before substitution. This also applies through aliases and enclosing negative positions. Omitted inputs are Nothing, omitted outputs are Any, and intersecting record interfaces unions shared field inputs while intersecting their outputs. Writes through both structural and nominal interfaces constrain assigned callbacks against the input bound rather than the read bound.
The regressions cover these cases, recursive fields, generic capture and factory-call isolation, and a control ensuring that exposing an immutable source does not expose writes to its mutable copy. ctest, dtest MutableField, and the full hkmc2AllTests/test pass at 081891b, with all golden outputs committed and pushed. The exposure merge blocker is addressed; the PR description and language reference now describe the implemented behavior.
Follow-up on ordinary mutable fields: before 081891b, the reverse constraint only reached concrete assignable fields; it was skipped when the source was a declared record interface. This was a bug even without wildcard syntax. The new listenFieldInput path makes { mut f: S } <: { mut f: T } propagate both S <: T for reads and T <: S for writes. mut { f: S } uses the same rule. Commit 632d227 adds six direct regressions covering both directions and immutable-destination controls. Removing only that new reverse path makes both write-direction tests fail; with it restored, the full hkmc2AllTests/test suite passes.
These are the resolver's constraints for inference and callback checking, rather than a complete structural subtype checker. New resolution does not diagnose all concrete subtype mismatches, and InvalML does not yet support structural record types. The reference and PR description now state that limitation explicitly.
The internal errors that `newres/Overrides.mls` records cite their location in NewResolver.scala, which moved when comments were added there. Co-Authored-By: Claude Opus 5.5 <[email protected]>
A backtracking assignment `set x = v in body` saves `x` in an `old` temporary and restores it in a `finally` block. The temporary had no representation, so saving and restoring it converted from and back to the representation of `x`. This was a checked cast that dead-code elimination must keep (on JS), and an error for an unboxed `Int32` variable (on Wasm). Erasure now treats the temporaries that elaboration declares with `LetDecl` (currently only this one) as slots like source variables, inferring their representations from the values flowing into them. Co-Authored-By: Claude Opus 5.5 <[email protected]>
- Route assignments to fields (`set o.f = v`, including compound and
temporary assignments) to the fields they assign: resolve the assigned
member as selections do, for every class that the receiver can be an
instance of (including through projections, wildcard opens, annotated
receivers, and captures), and record the assigned values in each field's
host (`NewResolver.assignField` and `writeField`).
- Infer the erased type of an unannotated mutable field from its
initializer and its assignments, replacing the blanket unknown input.
Record fields are included: a mutable copy `mut {...r}` assigns the
fields of `r`.
- Make reads of unannotated mutable fields observe their assignments.
A read used to see only the initializer, so a member could be resolved
statically in the initializer's class while an instance of another class
had been assigned (a silent miscompilation on Wasm).
- Interface exposure assigns unknown values to the mutable fields (and
mutable record fields) that a compilation unit exposes, and
`Erasure.checkExposedMutableField` requires exposed mutable fields to
declare their types. Annotate such fields in the library, the apps, and
the tests.
- Share the projection lookup of `newSel` (`projectedLookup`).
- Document mutable fields in Runtime Representations, and add tests, with
:fixme regressions for assignments through structural types and for
private classes used in later Wasm worksheet blocks.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
- Record fields can be declared mutable with `mut`, in record literals
(`{mut count: 0, label: "x"}`) and record types (`{mut item: T}`).
Other fields are immutable, unless their whole record is (`mut {...}`).
A record literal with a mutable field is allocated unfrozen; resolution
assumes its other fields are never assigned, which type checking will
ensure.
- Through a record type, the values it describes can be assigned values of
the types of its mutable fields: `constrainRecord` assigns such values to
the actual values' assignable fields, as `constrainFunction` feeds the
declared parameter types to callbacks. Assignments through record types'
immutable fields are assumed not to happen, so assignments through record
types need not be followed. Record type views carry their mutable fields.
- Reads of mutable record fields return their initializer and the values
assigned to them, instead of values of unknown shape.
- Interface exposure assigns unknown values to the mutable fields of exposed
records.
- Share the resolution of assignable fields between assignments and record
constraints (`assignableTargets`, `listenAssignedValues`).
- Update the reference and the tests: the structural-assignment :fixme is
resolved, and the record :fixmes now only reflect that resolution is not
flow-sensitive.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Diagnostics now cite definition headers, and internal-error line numbers shifted. Co-Authored-By: Claude Opus 5.5 <[email protected]>
| //│ set tmp1 = Predef⁰.hide⁰(4); | ||
| //│ set tmp2 = +⁰(tmp1, 1); | ||
| //│ return Predef⁰.print⁰(tmp2) | ||
| //│ set x = (tmp as! Int⁰); |
There was a problem hiding this comment.
[Claude] Update: the old cast is fixed in d710d52. Erasure now infers the representation of the temporary that set … in declares, like a source variable, so the set … in test below is back to its hkmc2 output. (On Wasm, the same untyped temporary made set x = … in an error for an unboxed Int32 variable; newres/wasm/Erasure.mls now covers that.) The casts of hide's results remain.
| td.body.foreach(body => listenResult(body)(publish)) | ||
| // Mutable fields admit replacements not represented by initializer flow. | ||
| // Keep the resolver's unknown-write alternative when choosing their storage. | ||
| if (td.k is syntax.MutVal) || mutableField then |
There was a problem hiding this comment.
[Astra] The mutable-field inference described as a follow-up above is now implemented in this PR (8bfe6a3 and 1b878f2), and 081891b fixes the remaining exposure and variance problems.
Exposure now follows values assigned into escaping record fields as well as their initializers, including fields copied through nested mutable spreads and assignments discovered after exposure. It preserves the member scope paths and generic substitutions. Thus an assigned object with an unannotated mutable class field gets the required annotation error, and an assigned callback receives unknown external inputs. The three previously documented exposure failures are now ordinary passing regression tests.
The variance concern was correct: one substituted field type cannot represent both bounds. Structural fields now retain separate input/output types internally, and the syntax { mut f: in S out T } is supported. Inputs are contravariant and outputs covariant; they are interpreted separately before substitution. This also applies through aliases and enclosing negative positions. Omitted inputs are Nothing, omitted outputs are Any, and intersecting record interfaces unions shared field inputs while intersecting their outputs. Writes through both structural and nominal interfaces constrain assigned callbacks against the input bound rather than the read bound.
The regressions cover these cases, recursive fields, generic capture and factory-call isolation, and a control ensuring that exposing an immutable source does not expose writes to its mutable copy. ctest, dtest MutableField, and the full hkmc2AllTests/test pass at 081891b, with all golden outputs committed and pushed. The exposure merge blocker is addressed; the PR description and language reference now describe the implemented behavior.
Follow-up on ordinary mutable fields: before 081891b, the reverse constraint only reached concrete assignable fields; it was skipped when the source was a declared record interface. This was a bug even without wildcard syntax. The new listenFieldInput path makes { mut f: S } <: { mut f: T } propagate both S <: T for reads and T <: S for writes. mut { f: S } uses the same rule. Commit 632d227 adds six direct regressions covering both directions and immutable-destination controls. Removing only that new reverse path makes both write-direction tests fail; with it restored, the full hkmc2AllTests/test suite passes.
These are the resolver's constraints for inference and callback checking, rather than a complete structural subtype checker. New resolution does not diagnose all concrete subtype mismatches, and InvalML does not yet support structural record types. The reference and PR description now state that limitation explicitly.
[Astra] Description originally drafted by Claude Code and updated following review at @LPTK's request.
Summary
With new resolution (
#lang(0.3.x)), erasure now infers the runtime representation (erased type) of unannotated slots (parameters, results,letbindings and other variables, and fields) from the shapes that resolution observes flowing into them. Previously only annotations gave slots a representation, and every other slot was unknown. Annotations still fix the representations of their slots.Lowering converts each value to the representation of the slot it flows into, using the existing
coerceTo. A value whose representation is already within the target's needs no cast, a downcast becomes a checked cast, and unrelated representations are a compilation error. On Wasm, the inferred representations select the physical types: anInt32slot is an unboxedi32, and a slot of class instances is a reference to the class's struct.The user-facing rules are documented under Runtime Representations in
doc/reference.md.Design
NewResolver.prepareErasureregisters one host per erasable source slot inErasureInputs, in the slot's defining compilation unit. The host collects the shapes flowing into the slot untilcompleteBlockseals it.ErasureDeclarationsenumerates the slots that are not reachable throughsubStatements: split bindings, handler parameters, auxiliary constructor parameters, and synthesized record fields. Observation and assignment share this enumeration, so every delayed slot is prepared before erasure reads it. Resolution stores no erased types on source symbols.Erasuremaps each observed shape to a value type, and the slot gets the union of these types (inferredType). This includes the temporary that lowering introduces forset … in.set o.f = v), routing each write to the possible receiver fields. Field reads and erasure include these assigned values. An unannotated mutable field can therefore have a precise representation, including an unboxed Wasm representation.{mut field: value}declares an individually mutable field;mut {...record}also makes its copied fields assignable. Record-field writes participate in resolution. Record fields cannot declare their types, so exposed mutable record fields admit unknown writes and keep the top representation.{ mut f: in S out T }acceptsSon writes and exposesTon reads; omitted inputs areNothing, and omitted outputs areAny. An ordinary{ mut f: A }interpretsAseparately in the two polarities before substitution, so substitutingin S out Tpreserves both bounds, including inside aliases and negative positions. Thus{ mut f: S } <: { mut f: T }contributes bothS <: T(reads) andT <: S(writes);mut { f: S }follows the same rule. Shared fields in record intersections union their input types and intersect their output types. Assignments through structural and nominal interfaces constrain callbacks against the write type, independently of the read type.InterfaceExposure), so the representations of their unannotated parameters stay general. Mutable class fields reachable through the unit's interface must declare their types (Erasure.checkExposedMutableField), because external writes cannot be inferred from local uses. Exposing a record follows both its field initializers and assigned values, including assignments arriving later and fields copied through nested mutable spreads. The traversal preserves member scope paths and generic substitutions, so assigned objects and callbacks receive the same exposure checks as initializers.Erasure.checkOverride).Erasure.checkExposedOpenMember).@asyncfunctions return aPromiseand@generatorfunctions anIterator. Their bodies produce only the payload (Erasure.bodyResultType).semantics.BuiltinOperatorResults). For example,Int + Intis anInt, and+with aStroperand is aStr. On Wasm, operators lower to dynamically typed intrinsics, and their results are unknown. This logic should be removed once the builtins are properly overloaded.undefined.undefinedis an unspecified value of any representation, and reading it back is undefined behavior. A backend may therefore materialize it as any value of the slot's representation: on Wasm,0for ani32slot, or a null reference. The placeholders that dead-parameter elimination passes rely on this.Optimizer and backend changes
BlockSimplifierremoves casts that propagation and inlining make redundant, including checked casts whose check is guaranteed to succeed. Narrowing casts and their evaluation order are kept. When it propagates a partial application, checked arguments are evaluated at the original site, leaving a pure call prefix.ref.cast. Lowering maps builtin operators to thewasm.*_implintrinsics, andWatBuilderrejects any remaining builtin calls. Slots of class instances are nullable so that they can holdundefined.Programs that are now rejected
Because inferred representations are as binding as declared ones, some programs that used to compile, and run on the JavaScript backend, are now rejected. This includes programs whose only problem is a misleading annotation. Fixing these errors uncovered bugs in the compiled test sources:
argument: Treeis declared (Tree.App(op, acc :: rhs :: Nil), nowTree.Tuple(...)), and built aTree.Errorwith the wrong arguments. These fixes change the app's printed output.nofib/fish.mlspassed a string as a list (showl(xs, ""), nowNil).Stringto meanList[Char]; they now declaretype String = List[Char].Exposed open members, including abstract ones, now have annotations in the library sources (
Iter.mls,ObjectBuffer.mls,Runtime.mls). Exposed mutable class fields also require annotations; private fields whose writes are confined to the unit can be inferred.Known limitations
Predef.hidehave unknown physical results. Binding such a result to a narrower inferred slot therefore needs a checked cast, and dead writes of these results survive dead-assignment removal because checked casts are effectful (opt/DeadAssignmentRemoval.mls).lambda$hereinPredef.mjsandRuntime.mjs). Call arguments must be paths, and the simplifier only propagates pure calls intoResultpositions. Onhkmc2, the call reached the argument only because it was wrapped in a checked cast toFunction, and that cast is no longer needed.undefined, but field reads can need extra non-null casts, including redundant nested casts. Non-null placeholders or tracking whetherundefinedcan reach a slot remain optimization follow-ups.Intarithmetic stays withinInt: an overflow toInfinityis not detected.#lang(strictResolution: true)infer the representations of public definitions from the uses in their own block. Later blocks must use these representations consistently (seenewres/loose/ErasureAcrossBlocks.mls). This affects only diff tests: compiled files expose their exports.:fixmeinwasm/ErasureAcrossBlocks.mls). A declared field type fails the same way, but inference makes such fields more common.Validation
ctestandhkmc2AllTests/testpass at632d227cb. All updated golden outputs are committed.newres/MutableFieldExposure.mlsnow checks the assigned-object, copied-field, and assigned-callback cases as ordinary regressions, without:breakmemarkers. It also covers nested spreads, writes discovered after exposure, generic captures, isolation between separate factory calls, and a control ensuring that exposing an immutable source does not expose writes to its mutable copy.newres/MutableFieldVariance.mlscovers split input/output bounds, positive and negative substitution through aliases, one-sided bounds, structural and nominal callback writes, intersections, whole-record mutability, and recursive fields. The existing bounded recursive-intersection test still passes.newres/MutableRecordConstraints.mlsseparately checks the read and reverse write constraints for both{ mut f: S }andmut { f: S }, with immutable-destination controls. Removing the reverse constraint added in081891b34makes the two write-direction regressions fail.