Class Lifter #266

CAG2Mark · 2025-01-27T08:01:31Z

Lifts classes and nested functions to the top-level.

Method

General idea: when functions have to capture local variables, we need to rewrite their parameter lists. For example:

fun f(arg) =
  let unused = 0
  fun g(arg2) = set arg1 = 1
  let foo = g
  foo(unused)
  arg

is transformed into

fun g(f_capture)(arg2) =
  set f_capture.arg1 = 1 
class f$capture(arg1)
fun f(arg) =
  let capture = f$capture(arg)
  let foo = g(capture)
  foo(unused)
  arg

Variables which are not mutated will be passed as a parameter directly.

TODO

Lift functions.
Lift classes:
- Instantiate does not work at all
- Functions within functions within classes are lifted out of the class. This is technically fine, but it's better to have them as class methods. However, we need private methods for this, which is not yet implemented.
- Private variables can't be accessed by nested classes/functions after being lifted out.
- Modules are not lifted properly
Only pass mutated variables through the capture class. Move other variables by passing them as an argument directly. Note that there are some subtle cases where this causes problems, such as
```
fun f() =
  let x = g
  let y = 2
  fun g() = y
  x()
```
so for now, we only do this when the function is not passed around as a first-class object. More thorough analysis could be done in another PR.
- More advanced analysis is work-in-progress.

Replace calls with a single call where we can. For example

fun f(arg) =
  let unused = 0
  fun g(arg2) = set arg1 = 1
  g(unused)
  arg

could be rewritten as

fun g(f_capture, arg2) =
  set f_capture.arg1 = 1
fun g$(f_capture)(arg2) = g(f_capture, arg2)
class f$capture(arg1)
fun f(arg) =
  let capture = f$capture(arg)
  g(capture, unused)
  arg

where the call g(unused) is symbolically replaced.
("lifting" curried functions is much easier than the lifting in general, so it can be done later)

Auxiliary Parameters

Classes can now have auxiliary parameters (IR only currently) with curried constructors.

LPTK · 2025-01-27T09:29:59Z

What's the intended scheme regarding mutable local variables?

FWIW, we should distinguish mutable from immutable variables. I intend to let mut be used to annotate pattern variables, like in Rust, and to reject re-assignments to non-mut variables. Note that this would still leave let x; if b do x = 1 well-formed.

When there are only immutable variables being captured, the scheme can be simpler.

CAG2Mark · 2025-01-27T10:14:52Z

What's the intended scheme regarding mutable local variables?

FWIW, we should distinguish mutable from immutable variables. I intend to let mut be used to annotate pattern variables, like in Rust, and to reject re-assignments to non-mut variables. Note that this would still leave let x; if b do x = 1 well-formed.

When there are only immutable variables being captured, the scheme can be simpler.

I don't distinguish between local variables that are mutated / not mutated for now. I intend to move every local variable that's referenced inside a nested class / function into a closure.

Would it be better to add the immutable local variables to each class/function directly?

P.S. seems I accidentally marked this PR as ready earlier, but it's definitely not. It's now marked as a draft.

…tion

CAG2Mark · 2025-01-28T10:47:08Z

Added much more into to the the first comment.

…ses that don't capture variables

hkmc2/shared/src/main/scala/hkmc2/codegen/Lifter.scala

CAG2Mark · 2025-02-12T14:59:26Z

hkmc2/shared/src/test/mlscript/codegen/ReboundLet.mls

@@ -15,7 +15,7 @@ foo()

 let x = 1
 let f = () => x
-//│ f = [function f]
+//│ f = [function lambda]


Hm. Did this actually rewrite it as a proper function before? Or is this just due to the way it's printed?

What do you mean? The name is taken from the name of the binding, because we generate things like f = function f ....

CAG2Mark · 2025-02-12T15:07:28Z

OK, I think it's ready for review.

hkmc2/shared/src/main/scala/hkmc2/codegen/Block.scala

LPTK · 2025-02-13T01:20:57Z

hkmc2/shared/src/main/scala/hkmc2/codegen/BlockTransformer.scala

@@ -238,3 +244,28 @@ class BlockTransformerShallow(subst: SymbolSubst) extends BlockTransformer(subst
        then b else HandleBlock(l2, res2, par2, args2, cls2, hdr2, bod, rst2)
    case _ => super.applyBlock(b)

+// does not traverse into any other block


The semantics of this transformer seems broken wrt Begin. If you have Begin{foo()}; bar() you're not going to change foo even though you would have for the equivalent foo(); bar().

The job of this transformer is just to modify the paths and results in a block which do not require traversing another block to reach -- it's the job of the person using this transformer to traverse the sub-blocks, probably used in combination with one of the other block transformers.

As for Begin, both of its sub-blocks are expected to be traversed by whoever is using this transformer, not the transformer itself.

Ok, then please pick a better name for this class and document in the code what you've just written here. The current comment is inadequate as documentation.

Also it feels like a better architecture might be to use a method called applySubBlock in BlockTransformer whenever a sub-block (not a top-level block) is traversed, which would mean you only have to override that here to return input unchanged, instead of all this boilerplate.

Good point, I think that will be a better solution.

LPTK · 2025-02-13T01:23:54Z

hkmc2/shared/src/test/mlscript/codegen/Getters.mls

-//│ Foo1 = function Foo(x1) { return new Foo.class(x1); };
+//│ Foo1 = function Foo(x1) {
+//│   return new Foo.class(x1);
+//│ };


Any idea why there are no longer on one line?

When there's parameter lists, we need to curry the ctor using lambdas, and it's quite ugly to have it all on one line. I decided it was not worth special-casing non-curried ctors to be on one line such it's purely a superficial change.

Oh I see, I didn't realize the line was previously missing the breaks.

Can this be marked as resolved?

hkmc2/shared/src/main/scala/hkmc2/Config.scala

LPTK · 2025-02-13T01:29:37Z

hkmc2/shared/src/test/mlscript/basics/BadMemberProjections.mls

 //│ undefined
-//│ ═══[RUNTIME ERROR] Error: Function expected at least 1 argument but got 0
+//│ ═══[RUNTIME ERROR] Error: Function 'lambda' expected at least 1 argument but got 0


It would be good if we could avoid making every lambda get the name "lambda", at least in such error messages. Ideally they'd be unnamed and we would still have the old message here.

Currently it seems BlockMemberSymbols must have names. Maybe we could add an flag which disables the name from being printed in error messages -- I don't know if it makes sense for BlockMemberSymbols to be unnamed.

LPTK · 2025-02-13T01:30:59Z

OK, I think it's ready for review.

Thanks, I'll try to properly review it tomorrow. In the meantime please fix the conflicts and comments.

…ut of ignored classes

LPTK · 2025-02-20T03:53:10Z

hkmc2/shared/src/main/scala/hkmc2/codegen/Block.scala

@@ -91,12 +92,37 @@ sealed abstract class Block extends Product with AutoLocated:
    case TryBlock(sub, finallyDo, rest) => sub.freeVars ++ finallyDo.freeVars ++ rest.freeVars
    case Assign(lhs, rhs, rest) => Set(lhs) ++ rhs.freeVars ++ rest.freeVars
    case AssignField(lhs, nme, rhs, rest) => lhs.freeVars ++ rhs.freeVars ++ rest.freeVars
+    case AssignDynField(lhs, fld, arrayIdx, rhs, rest) => lhs.freeVarsLLIR ++ fld.freeVarsLLIR ++ rhs.freeVarsLLIR ++ rest.freeVarsLLIR


Is this really meant to use freeVarsLLIR? 🤔

CAG2Mark added 3 commits January 27, 2025 01:04

infrastructure

66edd88

some variable analysis

464e5a2

Merge hkmc2

9444ad4

CAG2Mark self-assigned this Jan 27, 2025

Merge branch 'hkmc2' into class-lifter

bc172c6

create closure class

3e59e8e

CAG2Mark marked this pull request as draft January 27, 2025 10:12

CAG2Mark added 9 commits January 27, 2025 19:16

add infrastructure to later distinguish mutable variables

0d56edf

Function lifting working

2ff423b

fix typos

93eb9dc

update test

f57b038

fix log and add test

7aa6e0f

refactor

babd710

Add auxiliary parameters

19f43d1

small refactor

7a3ae4e

capture at BlockMemberSymbol refs instead of at the start of the func…

4e2f4a3

…tion

CAG2Mark added 11 commits January 28, 2025 18:54

small optimization

41bf24b

pass immutable variables as args

d7c7bef

fix bug

2a45a20

add broken test

f97182f

refactor and fix huge bug

9cbad67

small progress and fix typo

f3bf942

re-organization and some documentation

cc55d1e

progess on class lifting

f09b1a6

Fix ctor generation

a12869d

Fix ctor generation

27f390f

classes almost working, refactor

3982756

fix many problems with modules, basic extends working for nested clas…

761e686

…ses that don't capture variables

LPTK reviewed Feb 12, 2025

View reviewed changes

hkmc2/shared/src/main/scala/hkmc2/codegen/Lifter.scala Show resolved Hide resolved

LPTK reviewed Feb 12, 2025

View reviewed changes

hkmc2/shared/src/main/scala/hkmc2/codegen/Lifter.scala Outdated Show resolved Hide resolved

LPTK reviewed Feb 12, 2025

View reviewed changes

hkmc2/shared/src/main/scala/hkmc2/codegen/Lifter.scala Outdated Show resolved Hide resolved

CAG2Mark added 4 commits February 12, 2025 22:43

Prepare for PR merge

26ea02c

Merge branch 'hkmc2' into class-lifter

507344e

merge and update tests

52f8844

pr changes

70f673c

CAG2Mark commented Feb 12, 2025

View reviewed changes

CAG2Mark added 4 commits February 12, 2025 23:04

add comments and unused stuff

50f3de6

restore the test

fc8e400

move the test to the correct place

632ca98

Fix line numbers

e3e989e

CAG2Mark marked this pull request as ready for review February 12, 2025 15:07

update compile tests

b93112f

LPTK reviewed Feb 13, 2025

View reviewed changes

hkmc2/shared/src/main/scala/hkmc2/codegen/Block.scala Show resolved Hide resolved

LPTK reviewed Feb 13, 2025

View reviewed changes

hkmc2/shared/src/main/scala/hkmc2/Config.scala Show resolved Hide resolved

LPTK reviewed Feb 13, 2025

View reviewed changes

CAG2Mark added 7 commits February 13, 2025 12:49

address pr comments

3ee9e4c

Merge branch 'hkmc2' into class-lifter

22bb3bf

Update

5099394

Don't lift classes directly in a module, fix defns not being lifted o…

b9e30e6

…ut of ignored classes

don't rewrite ignored objects

ca2cdfc

remove unneeded case

d47ce44

fix imports

19cab27

LPTK reviewed Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Class Lifter #266

Class Lifter #266

CAG2Mark commented Jan 27, 2025 •

edited

Loading

LPTK commented Jan 27, 2025 •

edited

Loading

CAG2Mark commented Jan 27, 2025 •

edited

Loading

CAG2Mark commented Jan 28, 2025

CAG2Mark Feb 12, 2025

LPTK Feb 13, 2025

CAG2Mark commented Feb 12, 2025

LPTK Feb 13, 2025

CAG2Mark Feb 13, 2025

LPTK Feb 15, 2025

LPTK Feb 15, 2025 •

edited

Loading

CAG2Mark Feb 17, 2025

LPTK Feb 13, 2025

CAG2Mark Feb 13, 2025

LPTK Feb 15, 2025

CAG2Mark Feb 17, 2025

LPTK Feb 13, 2025

CAG2Mark Feb 17, 2025

LPTK commented Feb 13, 2025

LPTK Feb 20, 2025

Class Lifter #266

Are you sure you want to change the base?

Class Lifter #266

Conversation

CAG2Mark commented Jan 27, 2025 • edited Loading

Method

TODO

Auxiliary Parameters

LPTK commented Jan 27, 2025 • edited Loading

CAG2Mark commented Jan 27, 2025 • edited Loading

CAG2Mark commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CAG2Mark commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LPTK Feb 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LPTK commented Feb 13, 2025

Choose a reason for hiding this comment

CAG2Mark commented Jan 27, 2025 •

edited

Loading

LPTK commented Jan 27, 2025 •

edited

Loading

CAG2Mark commented Jan 27, 2025 •

edited

Loading

LPTK Feb 15, 2025 •

edited

Loading