-
Notifications
You must be signed in to change notification settings - Fork 28
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
base: hkmc2
Are you sure you want to change the base?
Type Classes #256
Conversation
14e02f5
to
9b9f4be
Compare
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))))) |
There was a problem hiding this 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()()()
!
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/ImplicitResolver.scala
Outdated
Show resolved
Hide resolved
hkmc2/shared/src/main/scala/hkmc2/semantics/ImplicitResolver.scala
Outdated
Show resolved
Hide resolved
hkmc2/shared/src/main/scala/hkmc2/semantics/ImplicitResolver.scala
Outdated
Show resolved
Hide resolved
hkmc2/shared/src/main/scala/hkmc2/semantics/ImplicitResolver.scala
Outdated
Show resolved
Hide resolved
hkmc2/shared/src/main/scala/hkmc2/semantics/ImplicitResolver.scala
Outdated
Show resolved
Hide resolved
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) |
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 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. |
Most comments addressed, except: Symbols as Context KeysI tried, but it seems that it is not possible to use pure enum Type:
case Any
case Sym(sym: TypeSymbol)
case App(t: Sym, typeArgs: Ls[Type])
I'm also aware that a Partial Application RestrictionsIt 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 |
All tests should pass now, except for this one
The ident |
There was a problem hiding this 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:
mlscript/hkmc2/shared/src/main/scala/hkmc2/semantics/Importer.scala
Lines 63 to 71 in 8aed187
// * 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 definedOrd[Int]
after defining aOrd[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 aTypeAliasSymbol
. 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.
hkmc2/shared/src/main/scala/hkmc2/semantics/ImplicitResolver.scala
Outdated
Show resolved
Hide resolved
PS:
Just rename the |
7052f6d
to
211db7c
Compare
This PR is ready for review. The history is also clean. Main changes since last review: Light ElaborationI fixed the light elaboration according to the suggestion. There is a problem that there are two kinds of imports - one in the I added a field |
Oh wait I forgot to resolve the conflicts |
211db7c
to
105afbe
Compare
Thanks, I will try to review it soon. But I already notice some problems. First, I just realized that I wanted to comment on this last time but somehow forgot: It seems you didn't understand the reason why we need to do resolution in a pass that's separate from elaboration. Indeed, what you implemented is not a separate pass: you do resolution on the fly, as elaboration progresses. So examples like this don't work: :r
fun main() =
use Bar = new Bar
Test.test()
class Bar
module Test with
fun test()(using bar: Bar): Bar = bar
//│ Resolved: { ‹› fun member:main() = { ‹› use member:instance$Ident_Bar_: globalThis:block#11#666(.)Bar‹member:Bar› = new SynthSel(Ref(globalThis:block#11),Ident(Bar))(); globalThis:block#11#666(.)Test‹member:Test›.test‹member:test›() }; Cls Bar { }; Mod Test { ‹module› fun member:test()ctx (Param(‹›,bar,Some(SynthSel(Ref(globalThis:block#11),Ident(Bar)))), ): globalThis:block#11#666(.)Bar‹member:Bar› = bar#666; }; }
main()
//│ = [Function (anonymous)] Notice that the This test also exemplifies a critical flaw in your way of writing code: instead of reporting errors when things fail to elaborate, you silently generate wrong code! This sort of behavior is really to be avoided at all costs. Again, the default should be rejection, and you should only accept the code when it's 100% sure to be good. Even with a compiler crash is by far preferable to a miscompilation. Notice that the following works and returns a different result. We don't what that changing the order of module and method definitions cause this sort of semantic discrepancies! class Bar
module Test with
fun test()(using bar: Bar): Bar = bar
fun main() =
use Bar = new Bar
Test.test()
main()
//│ = Bar {} |
Thanks for the comments! You are right that this doesn't work, but I think the reason is not really related to the resolution pass itself. Prior to resolution, the elaborator identifies all module method applications and "marks" replaces any missing contextual parameters as some placeholders. What I was thinking is that it can be performed during elaboration to speed up the process, but it turns out that it indeed requires all symbols to be resolved, so I'll move it to the resolution pass. |
Ah you're right, I read your diff too fast. You indeed have a properly separated phase, which is not the same as the |
This (draft) PR introduces type classes by a minimal implementation of contextual parameters, including:
Parser rules for parsing
use
andusing
keywords.Elaborator rules for elaborating
use
andusing
keywords.A
use
definition is elaborated into aTermDefinition
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
. TheseElem
s are populated later in the new implicit resolution pass.New pass: Implicit Resolution
A new pass called
ImplicitResolution
is introduced to resolve implicit arguments after elaboration.It traverses the elaboration tree, find:
TODO: Move rules of module methods to this pass.