Transformation for return keyword #923
Conversation
There was a problem hiding this comment.
This is great, thanks for the quick implementation!
One small nit is that the extra let/match construct which extracts the return value from topLevelCF makes the transformation result a bit less readable. WDYT of adding some simple inlining logic that matches this exact structure and pushes the return value extraction into the leaf match expressions in the transformed body?
| with utils.SyntheticSorts { self => | ||
|
|
||
| val s: Trees | ||
| val t: s.type |
There was a problem hiding this comment.
Your transformation isn't checking all trees (e.g. a return inside a type refinement wouldn't be rejected).
In general, it's safer to keep s and t types potentially distinct so the (Scala) type checker will make sure you visit all trees in your transformation.
| ValDef.fresh("cf", ControlFlowSort.controlFlow(retType, firstType)).setPos(e) | ||
| val transformedRest = processBlockExpressions(rest) | ||
|
|
||
| if (rest.exists(tc.exprHasReturn)) { |
There was a problem hiding this comment.
Nit: this would probably be more readable if you pushed the if-expression down to the ControlFlowSort.andThen lambda argument.
| } | ||
|
|
||
| case e +: rest => | ||
| val unusedVal = ValDef.fresh("unused", e.getType) |
There was a problem hiding this comment.
You could preserve the Block, something like
case es =>
val nonReturnEs = es.takeWhile(e => !tc.hasExprReturn(e))
Block(nonReturnEs, processBlockExpressions(es.drop(nonReturnEs.size()))).copiedFrom(expr)You'll probably need to handle the case where es.drop(nonReturnEs.size()) is empty specially.
| * @param expr The expression to return | ||
| */ | ||
| sealed case class Return(expr: Expr) extends Expr with CachingTyped { | ||
| override protected def computeType(implicit s: Symbols): Type = NothingType() |
There was a problem hiding this comment.
You should check that expr.isTyped here and return Untyped otherwise.
|
|
||
|
|
||
| // ControlFlowSort represents the following class: | ||
| // sealed abstract class ControlFlow[Ret, Cur] { |
|
Wow, great job @jad-hamza! Would be great if we could also add a benchmark which involves mutation, to ensure that the transformation plays nicely with the imperative elimination phase. I know you have a benchmark which involves mutation and loops this in #925, but if local mutation already works with this PR it might be good to include it here as well. |
Thanks! I've added a simple example with mutations (and no loops) in the other PR, I can port the changes here if you prefer we merge this separately. |
This implements @romac transformation idea (with suggestions from @samarion) for the
returnkeyword.Feedback is welcome! (Loops are not supported yet, I can add them to this PR or a different one)
Here are the examples we discussed (added to imperative benchmarks in this PR):
SimpleReturn.scala
ControlFlow2.scala