Implement method parsing and type checking (closes #9)

[fo5for]

Implemented:

• Method declaration and definition processing, including typing and subsumption checking
• Explicit and implicit method calls
• Shadowing for implicit method calls in definitions
• this identifier with shadowing semantics in method definitions

Issues:

• ConstraintSolver.extrude does not handle the polarity of type variables in TypeRefs
• Type simplification ignores type parameters of classes
• DiffTests outputs the defined method type for methods declaration if the same method is both declared and defined in the class
• To prohibit construction of abstract classes

[LPTK]

This is amazing, thanks!

However, I've outlined several places where the code seems unclear or needs some documentation/refactoring. Let's make sure to fix these before the merge.

[LPTK]

How can it be okay to filter out only the cases where the mapped value is a TypeVariable? That's very fishy. Couldn't it be a ProxyType at some point (maybe not now, but if the code base changes in the future)? Or an application Id[X] of the type Id[T] = T type? If we're indeed only interested in type variables here, this should be expressed in the type, to make sure we're not messing up, and for documentation.

[fo5for]

targsMaps store the mapping from type parameters to the dummy TypeVariables, and to the rigidified TraitTaga when typing methods. It was originally just a map to TypeVariables before I realized the class type parameters should be rigidified when typing methods.

I couldn't come up with a way to express this invariant in the type. I tried to use TypeVariable \/ TraitTag, but it would require a use of asInstanceOf, as freshenAbove returns a SimpleType even when called directly with a TypeVariable. For now I have added 3 die statements before I can come up with a better solution.

I think your worry about the other cases came from nested TypeRefs or ProxyTypes in the body. They are in fact not handled by this map, but by the subsMap a few lines below. The TypeVariables we are collecting are the dummy type arguments created when typing the body of the TypeDef. The actually applied type arguments are on the rhs of subsMap. I believe it is handling nested AppliedTypes properly, as they would be typed as TypeRefs by targsAppOf (formerly targsMapOf. I am running out of names...).

However, as I was writing tests to test this out, I discovered that nested type arguments are not typed correctly, even in the mlscript branch:

class Foo[A]: { value: A }
class Bar[A]: Foo[Foo[A]]
//│ Defined class Foo

The TypeDef of Bar is not registered to the context, even though no error are emitted. I believe this is due to checkRegular returning false.

[LPTK]

Okay, thanks for the explanation. Now I understand what you are doing, although I don't really agree with how it's done. See my other comment #22 (comment) for an idea to make the whole thing more functional, relying on computing the relevant information once, at the right place. We'd simply pass that relevant info along with each type definition being processed (instead of storing it in the context through the weird targsMaps mutable map), so that we can later perform subsumption checking after typing methods. Does that make sense to you?

[LPTK]

I think we can merge this, after you've addressed #22 (comment) and #22 (comment). Not sure why the CI is failing, though.

[LPTK]

The CI was failing because of my parallel bug fix in 199e937

[LPTK]

Oh and also the selection-typing part should be improved as we discussed: https://github.com/hkust-taco/mlscript/pull/22/files#diff-09ce6a8849c3c07173c504d6966f025cfa3338fa80afedecb4e77c6c013d50d3R660
Notably, code like err(msg"Class ${name} has no method ${fieldName.name}", term.toLoc) should be deduplicated.

[fo5for]

Oh and also the selection-typing part should be improved as we discussed: https://github.com/hkust-taco/mlscript/pull/22/files#diff-09ce6a8849c3c07173c504d6966f025cfa3338fa80afedecb4e77c6c013d50d3R660 Notably, code like err(msg"Class ${name} has no method ${fieldName.name}", term.toLoc) should be deduplicated.

Addressed in bc397a9

[LPTK]

Okay thanks, that's really good work! The processTypeDefs implementation is a lot clearer now. I just pushed a few improvements, and we're ready to merge.