Defunkify lifter and related middle-IR code #290
Conversation
| @@ -347,12 +351,10 @@ final case class ClsLikeDefn( | |||
| parentPath: Opt[Path], | |||
| methods: Ls[FunDefn], | |||
| privateFields: Ls[TermSymbol], | |||
| publicFields: Ls[TermDefinition], | |||
| publicFields: Ls[BlockMemberSymbol], | |||
There was a problem hiding this comment.
The only reason we were storing TermDefinitions here was to remember which things are supposed to be public fields. But clearly it's wrong to store elaborated terms in a middle-IR data structure. All we needed here was just the symbol.
| @@ -147,14 +147,17 @@ class BlockTransformer(subst: SymbolSubst): | |||
| if (own2 is fun.owner) && (sym2 is fun.sym) && (params2 is fun.params) && (body2 is fun.body) | |||
| then fun else FunDefn(own2, sym2, params2, body2) | |||
|
|
|||
| def applyValDefn(defn: ValDefn): ValDefn = | |||
There was a problem hiding this comment.
(Only reason I extracted this is because it used to be needed in my previous refactoring; I kept it for consistency with FunDefn.)
There was a problem hiding this comment.
For consistency, we should also probably extract applyClsLikeDefn. Note that this may require some changes to overridden applyDefn methods.
There was a problem hiding this comment.
I know but ain't nobody got time for dat.
| @@ -10,9 +10,12 @@ import os.write.over | |||
| // These all work like BlockTransformer and its derivatives, but do not rewrite the block. See BlockTransformer.scala. | |||
| // Please use this instead of BlockTransformer for static analysis. | |||
|
|
|||
| class BlockTraverser(subst: SymbolSubst): | |||
There was a problem hiding this comment.
Why would you need a symbol substitution in a block traverser? 🤔
| ) | ||
| .toMap | ||
|
|
||
| val varsList = cap.toList |
There was a problem hiding this comment.
You can't toList a set to get some arbitrary order, because that order is not well-defined. It could change depending on the object's memory address next time the compiler runs after minor modifications. But we need stable and reproducible compilation outputs.
| @@ -234,55 +236,38 @@ class Lifter(handlerPaths: Opt[HandlerPaths])(using State, Raise): | |||
| val FreeVars(_, cap) = ctx.usedLocals(f.sym) | |||
|
|
|||
| val fresh = FreshInt() | |||
|
|
|||
| val varsMap = cap.map: s => | |||
There was a problem hiding this comment.
The creation and subsequent repeated querying of this intermediate map was entirely gratuitous and resulted in needless overhead.
| val sortedVars = cap.toArray.sortBy(_.uid).map: sym => | ||
| val id = fresh.make | ||
| val nme = sym.nme + id + "$" | ||
|
|
||
| val varSym = VarSymbol(Tree.Ident(nme)) | ||
| val fldSym = BlockMemberSymbol(nme, Nil) | ||
|
|
||
| val p = Param(FldFlags.empty.copy(value = true), varSym, None) | ||
| varSym.decl = S(p) // * Currently this is only accessed to create the class' toString method | ||
|
|
||
| val vd = ValDefn( | ||
| S(clsSym), | ||
| syntax.ImmutVal, | ||
| fldSym, | ||
| Value.Ref(varSym) | ||
| ) | ||
|
|
||
| (sym -> varSym, p, vd) |
There was a problem hiding this comment.
I now compute all the relevant things in one place that's much easier to inspect and understand.
| sym.decl = S(p) | ||
| p | ||
| )), | ||
| S(PlainParamList(sortedVars.iterator.map(_._2).toList)), |
There was a problem hiding this comment.
I use iterator here and below to avoid allocating intermediate collections.
There was a problem hiding this comment.
Looks good. There's just some small things you may have overlooked in addition to @FlandiaYingman's comments.
| @@ -147,14 +147,17 @@ class BlockTransformer(subst: SymbolSubst): | |||
| if (own2 is fun.owner) && (sym2 is fun.sym) && (params2 is fun.params) && (body2 is fun.body) | |||
| then fun else FunDefn(own2, sym2, params2, body2) | |||
|
|
|||
| def applyValDefn(defn: ValDefn): ValDefn = | |||
There was a problem hiding this comment.
For consistency, we should also probably extract applyClsLikeDefn. Note that this may require some changes to overridden applyDefn methods.
| @@ -120,12 +127,12 @@ class BlockTraverser(subst: SymbolSubst): | |||
| applySubBlock(lam.body) | |||
|
|
|||
| def applyTermDefinition(td: TermDefinition): Unit = | |||
There was a problem hiding this comment.
Good catch, I missed that one!
There was a bunch of funky stuff happening both in the lifter and in related middle-IR code.
The only reason I made this a PR is for @CAG2Mark to have a chance to review it and learn a bit in the process.