Improve module method checks

[FlandiaYingman]

This PR improves the module method checks introduced in #233, and makes preparation in order to move all the checks to the resolution stage. It also fixes:

the problem that "module" parameters were not treated as modules

:e
fun f8(module m: M) = m

the problem that self referencing modules were not treated as modules

module Foo with
  val self = this

the problem that term definitions returning a module were not treated as modules

module A with
  fun f() = 2

module B with
  val a: module A = A

print(B.a) // B.a should be a module, and should be rejected passing to a regular parameter

[FlandiaYingman]

class Foo(val z) with
  blah

should now be elaborated into

class Foo(z‹123›) with
  val member:z‹124› = z‹123›
  blah

I didn't differentiate class Foo(val z) and class Foo(z) (they all elaborated to an additional val binding member:z) because all current tests seem to use the latter. But I think this behavior can be easily modified if we want.

[FlandiaYingman]

I changed a lot of tests to replace all class definitions that have parameters with data class definitions.

I also checked them so to not add data modifier to some class definitions (especially those in mlscript/basics) that obviously do not require the data semantic.

[LPTK]

Nice change, thanks!

Maybe @CAG2Mark you could take a quick look at the lifter changes?

[CAG2Mark]

LGTM with regards to the lifter. I've left one minor comment and a reply to another comment.
(p.s. the lifter logic is quite complicated, so good job on figuring it out :P)

BTW @LPTK it seems that VarSymbols are often created inside later IR passes because it's unavoidable, but you mentioned a while ago that VarSymbols all come from user defined variables. I think that in the future, it would be good to add a field to VarSymbol such as isUserDefined to make this distinction more explicit.

[CAG2Mark]

@AnsonYeung This PR may be of interest to you since it might change the way you need to call super in the handler lowering. I've described the changes here #283 (comment)

[AnsonYeung]

so preCtor is also for generating the field assignment, is there any reason for handling aux param differently? I don’t think it causes issue with handler lowering. In fact I think if preCtor and ctor is combined into a single block then it would be trivial to fix one of the current broken case with super call being effectful.

[CAG2Mark]

so preCtor is also for generating the field assignment, is there any reason for handling aux param differently? I don’t think it causes issue with handler lowering. In fact I think if preCtor and ctor is combined into a single block then it would be trivial to fix one of the current broken case with super call being effectful.

Probably because it's currently assumed to be handled by the front-end (but we don't have aux params in the front-end yet) but you're right. Though doing this will require even more changes in the lifter.

[LPTK]

is there any reason for handling aux param differently?

They should definitely be handled the same. The only difference is that thesae params won't get val semantics implicitly when using data class, unlike the main parameters.

In fact I think if preCtor and ctor is combined into a single block then it would be trivial to fix one of the current broken case with super call being effectful.

Weren't you the one who introduced the preCtor field in the first place? 😛

In e37d8f8#diff-72bbb753659dd44a5e47f3cd0b3954e4ee95fda127ca491a2c267461779c60fbR126

What was the purpose of using a separate block, and does that still make sense now?

[AnsonYeung]

I'm not sure about it now. Does lifter need it now (as I think it may need to insert some extra fields)? The original intent was just a hack to insert super calls before JSBuilder emits the field initializations.

In any case, I am completely fine with it being removed now.

[LPTK]

@AnsonYeung Ok, we'll probably want to remove it in a later PR.

[CAG2Mark]

On another note (sorry for putting so many extra comments in this PR), I think the toString function for classes should also be generated in an earlier phase. This could, for example, allow toString to benefit from stack safe recursion (which was actually a problem we ran into in our benchmarking; it would stack overflow).

[LPTK]

@CAG2Mark Thanks for the suggestion. I intend for toString to be changed to a simpler call to some rendering function defined externally, so this should no longer be a problem. Note that we already have this problematic test case:

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

Lines 6 to 13 in 4e1d3cf

	let foo = Foo(null)
	//│ foo = Foo(null)
	set foo.x = foo
	:fixme // Support showing cyclic values
	foo
	//│ ═══[RUNTIME ERROR] RangeError: Maximum call stack size exceeded

[LPTK]

LGTM