Annotate more core types in lifted #209
Conversation
Still missing: Cases where tparams are needed.
Will be needed for correct `freeVariables` with types.
Needed to correctly track the types of free variables in the body.
By threading a `ModuleContext` through the transformation
We do not want to take function definitions as parameters, only block parameters. We should have a better way to distinguish them than checking the type of the symbol, but this works for now.
This reverts commit e38f553. I don't know why I thought that this was necessary.
| chez.Let(List(Binding(nameDef(id), chez.Builtin("fresh", Variable(nameRef(region)), toChez(init)))), toChez(body)) | ||
|
|
||
| case Try(body, tpe, handler) => | ||
| val handlers: List[chez.Handler] = handler.map { h => | ||
| val names = RecordNames(h.id) | ||
| val names = RecordNames(h.id.symbol) |
There was a problem hiding this comment.
I previously tried to get rid of the dependency on symbols in all phases after core. Why is this necessary here?
| @@ -102,7 +100,7 @@ case object Hole extends Stmt | |||
| * | |||
| * Used to represent handlers / capabilities, and objects / modules. | |||
| */ | |||
| case class Implementation(id: symbols.Interface, operations: List[Operation]) extends Tree | |||
| case class Implementation(id: core.BlockType.Interface, operations: List[Operation]) extends Tree | |||
| @@ -120,72 +118,70 @@ def Here() = Evidence(Nil) | |||
|
|
|||
| class EvidenceSymbol() extends Symbol { val name = Name.local(s"ev${id}") } | |||
|
|
|||
| def freeVariables(d: Definition): Set[Symbol] = d match { | |||
| def freeVariables(d: Definition): Set[Param] = d match { | |||
There was a problem hiding this comment.
Not sure collecting params here is the right thing to do. Eventually, we might drop param and replace it by separate lists of value and block bindings (like in core). Is it because you want to collect (Set[ValueSymbol], Set[BlockSymbol])?
There was a problem hiding this comment.
On the other hand, also having the types does make sense.
There was a problem hiding this comment.
Yes, having the types is the main reason, and that there is no type for evidence. An alternative would be a List[(Id, Type)] (or similar), but that would require us to add a type for Evidence (which would then ideally be used in other places, too.
| case id: symbols.ResumeParam => Variable(transform(id), transform(tpe)) | ||
| case id: lifted.EvidenceSymbol => Variable(transform(id), builtins.Evidence) | ||
| case lifted.ValueParam(id, tpe) => Variable(transform(id), transform(tpe)) | ||
| case lifted.BlockParam(id: (symbols.BlockParam | symbols.ResumeParam), tpe) => |
There was a problem hiding this comment.
Beautiful, much better!
There was a problem hiding this comment.
Why do you need the union type here? Can't you just drop it? Is it that we only need to ignore toplevel functions or also local functions? Toplevel functions could be looked up in the list of definitions.
There was a problem hiding this comment.
Agreed, this is not the nicest solution. IIUC, we should collect only those freeVariables that are bound somewhere on the way to the toplevel, minus local functions (which will become toplevel definitions, too).
See also https://github.com/effekt-lang/effekt/pull/209/files#diff-2bc35700065c09eb82c800736c2625a2516ddb669bb20acbfce3d9af8dbca683R71
There was a problem hiding this comment.
I think this code here might be the reason that pos/defdef.effekt is failing for LLVM 🤔
| }.getOrElse{ Context.abort(s"Expected a data type, got ${id}") } | ||
| } | ||
| } | ||
| def ModuleContext(using MC: ModuleContext): ModuleContext = MC |
There was a problem hiding this comment.
Great, at some point we should start sharing this.
|
In a separate commit, we could start homogenizing lifted to core. I am out of this game ;) This is the job of you (@marzipankaiser), @mm0821 , and @phischu who are the owners of For now, we could even replace |
This annotates core types at some more positions in lifted,
and uses this to also compute types in
freeVariables.As a necessity for this, it also threads the
lifted.Decls through themachine.Transformation(so it does not inspect the symbol anymore in at least one case).Fixes #207.