Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type Classes #256

Draft
wants to merge 14 commits into
base: hkmc2
Choose a base branch
from

Conversation

FlandiaYingman
Copy link

@FlandiaYingman FlandiaYingman commented Dec 22, 2024

This (draft) PR introduces type classes by a minimal implementation of contextual parameters, including:

Parser rules for parsing use and using keywords.

abstract class Foo with
  fun foo(): Int

class FooImpl extends Foo with
  fun foo(): Int = 40

use Int as i = 2

use Foo = new FooImpl()
module M with
  fun f(using someInt: Int, someFoo: Foo): Int = someInt + someFoo.foo()

Elaborator rules for elaborating use and using keywords.

A use definition is elaborated into a TermDefinition of kind instance (Ins). If there is no explicitly specified identifier with the instance, a synthesized identifier is used.

If any parameter in the parameter list is modified by the using keyword, the whole parameter list becomes an "implicit parameter list".

Further elaboration on module methods

Module method applications (or implicit applications) are further elaborated with the type information.

In particular, if there is any missing contextual argument lists, the elaborator will unify the argument lists with the parameter lists, with some placeholder Elem. These Elems are populated later in the new implicit resolution pass.

module M with
  fun foo(using someInt: Int, someFoo: Foo): Int = someInt + someFoo.foo()
  fun bar()(using someInt: Int) = someInt

M.foo
// => M.foo(using Int ‹unpopulated›, using Foo ‹unpopulated›)

M.bar
// => elaboration error: mismatch

M.bar()
// => M.foo()(using Int ‹unpopulated›)

New pass: Implicit Resolution

A new pass called ImplicitResolution is introduced to resolve implicit arguments after elaboration.

It traverses the elaboration tree, find:

  • Instance definitions: add them into the context with their type signature as the key.
  • Contextual argument placeholder: resolve the contextual argument with the corresponding instance definition in the context.

TODO: Move rules of module methods to this pass.

@FlandiaYingman
Copy link
Author

FlandiaYingman commented Dec 22, 2024

Wow it works like magic 🫨🤓

// Monoid Example

abstract class Monoid[T] extends Semigroup[T] with
  fun combine(a: T, b: T): T
  fun empty: T

object IntAddMonoid extends Monoid[Int] with
  fun combine(a: Int, b: Int): Int = a + b
  fun empty: Int = 0

object IntMulMonoid extends Monoid[Int] with
  fun combine(a: Int, b: Int): Int = a * b
  fun empty: Int = 1

module M with
  fun foldInt(x1: Int, x2: Int, x3: Int)(using m: Monoid[Int]): Int =
    m.combine(x1, m.combine(x2, m.combine(x3, m.empty)))
  fun fold[T](x1: T, x2: T, x3: T)(using m: Monoid[T]): T =
    m.combine(x1, m.combine(x2, m.combine(x3, m.empty)))
    
use Monoid[Int] = IntAddMonoid
M.foldInt(2, 3, 4)
//│ = 9

use Monoid[Int] = IntMulMonoid
M.foldInt(2, 3, 4)
//│ = 24

use Monoid[Int] = IntAddMonoid
M.fold(1, 2, 3)
//│ FAILURE: Unexpected type error
//│ ═══[ERROR] Missing instance for context argument Param(‹›,m,Some(TyApp(SynthSel(Ref(globalThis:block#18),Ident(Monoid)),List(Ref(T)))))

Copy link
Contributor

@LPTK LPTK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, nice draft changes!

TODO: Instance identifiers should not be able to be referred directly in the code.

Edit: Wait. Do we really want to do that? I thought in Scala implicit instances are in another name scope, but seems that's not the case 🤓

Indeed, there need be no such restriction.

  • Instance definitions: add them into the context with their type signature as the key.

The key should be the type constructor of the type class being instantiated. It can be concrete or abstract (a type parameter or type member). As in fun foo[A](someA) = { use A = someA; ... }

BTW eventually we'll like to define things like:

module Foo[A](using Bar[A]) with
  ...
    [baz, Foo.baz] // equivalent to `[this.baz, Foo(using Bar[A].baz]`
    use Bar[Int] // possible to change the context here
    Foo.baz // equivalent to `Foo(using Bar[Int]).baz`
  ...

But for this we first need to add support for parameterized modules.

@@ -178,6 +178,9 @@ extends Importer:

def term(tree: Tree, inAppPrefix: Bool = false): Ctxl[Term] =
trace[Term](s"Elab term ${tree.showDbg}", r => s"~> $r"):
def maybeModuleMethod(t: Term): Ctxl[Term] =
if !inAppPrefix && ModuleChecker.isModuleMethodApp(t) then moduleMethod(t)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why !inAppPrefix?

Admittedly we should document what the purpose of inAppPrefix does: it's used to avoid generating undefined-checks when a field being accessed is actually called like a method, because in this case undefined will crash as needed already.

I don't think this has anything to do with module methods.

Copy link
Author

@FlandiaYingman FlandiaYingman Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As its name suggests and by looking at the implementation of term, I think this flag is true only if the elaborator is evaluating the LHS of an App. Without this flag check, if the elaborator sees some App like f()()(), it'll "further elaborate" the term 3 times, namely f(), f()(), f()()(), while it is expected to be further elaborated only once with f()()()!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sure, if we want to elaborate calls with multiple argument lists at once. Does this work well in your current implementation approach? What if only some of the argument lists are provided (ie it's partially applied)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't think of a way to have module methods partially applied. If we do that, the result may not be recognized as a module method and then cannot be further elaborated with type information. I remember we discussed this, and we ended up with to reject partial application on module methods. (not implemented yet)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the omitted parameter lists don't have any special feature in them (no using, etc.) then there's no reason to forbid it, as it's functionally equivalent to a module method returning a lambda. There would be no reason to reject that.

I remember we discussed this, and we ended up with to reject partial application on module methods.

IIRC I was just suggesting to have this arbitrary implementation restriction to ease progress on the first PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. Let's formally restrict partial application if the resulting function does require further elaboration with type information. Otherwise, let it behaves just like normal methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the resulting function does require further elaboration with type information

Note that we should also check that the result type also does not need module-method treatment. E.g., it shouldn't return a module!

hkmc2/shared/src/main/scala/hkmc2/semantics/Term.scala Outdated Show resolved Hide resolved
log(s"Resolving instance definition $t")
sign match
case N =>
raise(ErrorReport(msg"Missing signature for instance ${t.toString}" -> N :: Nil))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally all the errors raised in your PR need to have a test reproducing them. Usually in a file with a name like BadTypeClasses.mls.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically the error is never raised because the elaboration promised that an instance must have a type signature. I guess later we can have some term other than TermDefinition to describe an instance definition? I found it didn't fit our use case here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either it's "impossible" and you should make this report an internal error (e.g. by calling lastWords, although we do need a better way of reporting internal errors, without necessarily terminating compilation) or it's possible but an error will have already been reported about this, in which case the correct behavior is to silently return some error term (with a code comment explaining why that's fine).

@FlandiaYingman
Copy link
Author

The key should be the type constructor of the type class being instantiated. It can be concrete or abstract (a type parameter or type member). As in fun foo[A](someA) = { use A = someA; ... }

If only the type constructors are specified as keys, wouldn't the following application fail?

use Monoid[Int] = IntAddMonoid
use Monoid[Str] = StrConcatMonoid

M.foldInt(2, 3, 4)
// resolves to M.foldInt(2, 3, 4)(using StrConcatMonoid)

@LPTK
Copy link
Contributor

LPTK commented Dec 23, 2024

It does fail, indeed. I guess we could make the rules a wee bit more fine-grained: we could look for the innermost instance in scope that could match the type query, where only those types explicitly specified (like the Int of Ord[Int]) are taken into account – so, as expected, Ord[A] for some unspecified A will unconditionally use StrConcatMonoid.

In any case, the map should still use keys that are type constructor symbols and not signatures. You can't just use syntactic types as keys as several type expressions could be equivalent and represent the same semantic type.

@FlandiaYingman
Copy link
Author

FlandiaYingman commented Jan 3, 2025

Most comments addressed, except:

Symbols as Context Keys

I tried, but it seems that it is not possible to use pure Symbol as the keys, because by doing so I couldn't get a previously defined Ord[Int] after defining a Ord[Str] (or maybe I didn't get your point 🙋‍♂️). Currently, a enum Type is used as the keys:

enum Type:
  case Any
  case Sym(sym: TypeSymbol)
  case App(t: Sym, typeArgs: Ls[Type])

Any is a placeholder for type parameters that are not explicitly provided. It could unconditionally match the innermost instance.

Map is not working here because many "query" keys can map to the same value, so instead a List is used, and it performs linear search to find an instance that matches the query.

I'm also aware that a TypeSymbol might be a TypeAliasSymbol. Should we resolve a type alias' definition (possibly perform a "lightweight" elaboration if absent) and substitute?

Partial Application Restrictions

It is currently not enforced because I find it easier to implement it when we move all module methods related checks to the new pass. I also find this PR is becoming large so let's have the checks in another PR

@FlandiaYingman
Copy link
Author

All tests should pass now, except for this one

fun use: [Rg, Br] -> Reg[Rg, Br] ->{Rg} Int

The ident use is now a keyword, so the parsing fails. I have no idea on bbml. How could I fix it 🫨

Copy link
Contributor

@LPTK LPTK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, looks very promising!

Though the light elaboration logic seems inappropriate. It appears that you're re-elaborating term definitions every time they are accessed, and in the wrong context! What you should do instead is to elaborate (in light mode) things that are imported from other files. This should be done in Importer.scala. Notice the old comment:

// * Note: we don't even need to elaborate the block!
// * Though doing so may be needed later for type checking,
// * so we should probably do it lazily in the future.
/*
given Elaborator.State = new Elaborator.State
given Elaborator.Ctx = Elaborator.Ctx.init.nest(N)
val elab = Elaborator(tl, file / os.up)
val (blk, newCtx) = elab.importFrom(resBlk)
*/

(Forget about the laziness for now!)

Then, if we still find that a definition is missing, it means either there was an error while elaborating the thing (and trying to re-elaborate it won't help) or there is a bug in the compiler.

I tried, but it seems that it is not possible to use pure Symbol as the keys, because by doing so I couldn't get a previously defined Ord[Int] after defining a Ord[Str] (or maybe I didn't get your point 🙋‍♂️).

Instead of a list of all instances and plain linear searches, we should at least index these instances on the type symbol. Thus "using symbols as keys".

I'm also aware that a TypeSymbol might be a TypeAliasSymbol. Should we resolve a type alias' definition and substitute?

Yes, if the type alias is not abstract, this should be done.

(possibly perform a "lightweight" elaboration if absent)

No (for the reason mentioned above).

[Partial Application Restrictions] currently not enforced because I find it easier to implement it when we move all module methods related checks to the new pass. I also find this PR is becoming large so let's have the checks in another PR

Sure, that's fine. Let's get small PRs merged regularly rather than big ones including many loosely related changes.


Don't forget to update the branch and mark the PR ready when it's ready for a thorough review.

@@ -36,7 +36,8 @@ object Elaborator:

val reservedNames = binaryOps.toSet ++ aliasOps.keySet + "NaN" + "Infinity"

case class Ctx(outer: Opt[InnerSymbol], parent: Opt[Ctx], env: Map[Str, Ctx.Elem]):
case class Ctx(outer: Opt[InnerSymbol], parent: Opt[Ctx], env: Map[Str, Ctx.Elem],
mode: Mode = Mode.Full):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mode: Mode = Mode.Full):
mode: Mode = Mode.Full):

@@ -474,37 +480,63 @@ extends Importer:
// ???

/** Module method applications that requrie further elaboration. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** Module method applications that requrie further elaboration. */
/** Module method applications that require further elaboration. */

Comment on lines +911 to +915
if first then
true
else
raise(ErrorReport(msg"Unexpected `using` keyword." -> hd.toLoc :: Nil))
false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if first then
true
else
raise(ErrorReport(msg"Unexpected `using` keyword." -> hd.toLoc :: Nil))
false
if !first then
raise(ErrorReport(msg"Unexpected `using` keyword." -> hd.toLoc :: Nil))
first

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plz remove all the junk imports of hkmc2 stuff and just declare the package as

package hkmc2
package semantics

instead.

case _ => false

enum Type:
case Any
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
case Any
case Unspecified

t.subTerms.foreach(resolve(_))
ictx
case _ =>
// Default Case. Simply resovle all subterms.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Default Case. Simply resovle all subterms.
// Default Case. Simply resolve all subterms.

M.foo
//│ = 42

// should resolve to strFoo(2, new IntFoo())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// should resolve to strFoo(2, new IntFoo())
// should resolve to intFoo(2, new IntFoo())

@LPTK
Copy link
Contributor

LPTK commented Jan 6, 2025

PS:

The ident use is now a keyword, so the parsing fails. I have no idea on bbml. How could I fix it 🫨

Just rename the use function to something else! E.g., use_.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants