Promote `WhileDo` loops to typed trees. #5113

sjrd · 2018-09-15T21:35:59Z

They remain as such, without desugaring, until the back-end. We also use them to desugar DoWhile loops, instead of label-defs.

This is the second step towards getting rid of label-defs.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

Separate subject from body with a blank line

When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:

Limit the subject line to 72 characters

Capitalize the subject line

Do not end the subject line with a period

Use the imperative mood in the subject line ("Add" instead of "Added")

Wrap the body at 80 characters

Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

sjrd · 2018-09-15T21:39:22Z

Everything seems to work fine, except the source display. One command that reproduces the issue:

dotty-compiler-bootstrapped/testCompilation tests/run-with-compiler/quote-inline-function

Apparently the typer (or some other early phase) collapses nested Blocks, which means that my efforts in TreeOpsImpl.scala to recover the shapes of do..while loops are useless.

@nicolasstucki Any idea to fix this?

nicolasstucki · 2018-09-16T04:52:23Z

There is some logic taking care of the collapsed Blocks in normalizedLoops, you probably shoul addapt it. Maybe you just need to change this line to case _: tpd.WhileDo => true.

sjrd · 2018-09-16T11:12:38Z

Hum, I don't think that's going to help me. normalizeLoops can recover lost Blocks because it can infer their presence from the call to the label-def. But the blocks in the WhileDo that come from a desugared DoWhile leave no such trace. They're just normal Blocks.

I wonder if maybe the best thing to do would simply be to remove DoWhile from tasty-reflect. Don't pretend they exist, but rather always expose While loops instead. We would then formalize the desugaring of do { body } while (cond) into while ({ { body }; { cond } }) {} in the specification of the language/tasty-reflect.

allanrenucci · 2018-09-16T11:16:58Z

compiler/src/dotty/tools/dotc/ast/Desugar.scala

+        // while ({ { body }; { cond } }) { () }
+        // the inner blocks are there to protect the scopes of body and cond from each other
+        val protectedBody = Block(Nil, body).withPos(body)
+        val protectedCond = Block(Nil, cond).withPos(cond)


Is the block for cond needed?

I am not 100% sure. I can't really come up with a problematic source code, but at the same time I cannot convince myself that there are no problematic source code. It is definitely possible to come up with a problematic AST (with cond being a ValDef for example), but the source syntax doesn't allow that, AFAICT. IMO it's better to play it safe.

smarter · 2018-09-16T12:02:51Z

compiler/src/dotty/tools/dotc/ast/Desugar.scala

-        labelDefAndCall(nme.DO_WHILE_PREFIX, rhs, call)
+        // while ({ { body }; { cond } }) { () }
+        // the inner blocks are there to protect the scopes of body and cond from each other
+        val protectedBody = Block(Nil, body).withPos(body)


The withPos calls should be unnecessary: a tree position is the union of the positions of its children.

nicolasstucki · 2018-09-16T18:06:17Z

We can remove the do while from tasty reflect. We would then simply do a best effort in the decompiler to recover do while.

sjrd · 2018-09-17T10:03:24Z

We would then simply do a best effort in the decompiler to recover do while.

I'm already doing the best-effort to recover the DoWhile at the tasty-reflect level, and it's not enough even for the most basic do..while loops. The decompiler won't be able to do any better, I'm afraid.

The only approach I can see involves deep inspection: if the cond is a Block(stats, expr), then

Shallow-inspect stats to find the symbols it defines (vals, defs, etc.)
Deep-inspect expr to test whether it refers to any of those symbols.

If it doesn't, we can display a do { stats } while (expr).

nicolasstucki · 2018-09-17T11:32:32Z

I was also considering the deep inspection. I would say we should just decompile it as a while loop. If someone actually cares about them in the future, at that moment we would implement the best effort decompilation of do while loops. Let's not waste time more time now and keep it simple.

sjrd · 2018-09-17T18:10:57Z

So, turns out I was wrong: Blocks are not flattened by typer, and my efforts to recover DoWhiles were fruitful. It just doesn't work when showing quoted trees. But for decompilation, it works like a charm.

sjrd · 2018-09-17T18:29:52Z

It's green now :) Ready for a full review.

nicolasstucki · 2018-09-18T08:08:11Z

compiler/src/dotty/tools/dotc/tastyreflect/TreeOpsImpl.scala

-        case Trees.Block((ddef: tpd.DefDef) :: Nil, expr) if expr.symbol.is(Flags.Label) && expr.symbol.name == nme.WHILE_PREFIX =>
-          val Trees.If(cond, Trees.Block(bodyStats, _), _) = ddef.rhs
-          Some((cond, loopBody(bodyStats)))
+        case x: tpd.WhileDo => Some((x.cond, x.body))


Before this case we should check that this is not a do while.

Or maybe it would be better to remove the do while extractor and place that condition in the decompiler.

nicolasstucki · 2018-09-18T08:13:40Z

library/src/scala/tasty/util/ShowSourceCode.scala

      case Term.While(cond, body) =>
        this += "while "
        inParens(printTree(cond)) += " "
        printTree(body)

-      case Term.DoWhile(body, cond) =>


This should not be changed. The order should not matter.

sjrd · 2018-09-18T09:09:40Z

@nicolasstucki I have added a commit that removes DoWhile from tasty-reflect, and enhances the decompiler instead, as suggested.

nicolasstucki · 2018-09-21T13:01:29Z

Submodule reference can be updated now that lampepfl/scala#38 is merged.

sjrd · 2018-09-21T13:03:46Z

Submodule reference can be updated now that lampepfl/scala#38 is merged.

Done.

nicolasstucki · 2018-09-21T13:28:28Z

@sjrd you may need to rebase

sjrd · 2018-09-21T13:30:08Z

Hum, I ... did rebase, without conflict, but apparently there was a semantic conflict. I'll fix it later today, or tomorrow.

They remain as such, without desugaring, until the back-end. We also use them to desugar `DoWhile` loops, instead of label-defs: do { body } while (cond) is now desugared as while { { body }; { cond } } () the inner blocks protecting the respective scopes of `body` and `cond` from each other. This is the second step towards getting rid of label-defs.

This avoids the awkward situation where `WhileDo` needs to be tested before `While` for it to meaningful, or suffering some more performance hit by rejecting `do-while`-like stuff in the `While` deconstructor. It will probably also simplify most usage patterns of the tasty-reflect API, since users will only have to worry about one kind of loop. The decompiler takes it upon itself to deconstruct `While` loops that look like the product of `do..while` to print a nice decompilation output.

sjrd · 2018-09-21T19:14:31Z

Back to green.

dottybot reviewed Sep 15, 2018

View reviewed changes

sjrd mentioned this pull request Sep 15, 2018

Add support for the WhileDo loops of dotty. lampepfl/scala#38

Merged

allanrenucci reviewed Sep 16, 2018

View reviewed changes

smarter reviewed Sep 16, 2018

View reviewed changes

sjrd force-pushed the real-while-loops branch 2 times, most recently from c59877a to 26c7fdc Compare September 17, 2018 18:09

sjrd mentioned this pull request Sep 17, 2018

Use Labeled blocks in TailRec, instead of label-defs. #5115

Merged

nicolasstucki suggested changes Sep 18, 2018

View reviewed changes

nicolasstucki approved these changes Sep 18, 2018

View reviewed changes

sjrd force-pushed the real-while-loops branch from f900fe0 to 58f0dda Compare September 21, 2018 13:03

sjrd added 2 commits September 21, 2018 20:55

sjrd force-pushed the real-while-loops branch from 58f0dda to 2acfbf8 Compare September 21, 2018 18:56

nicolasstucki merged commit a0b429b into scala:master Sep 21, 2018

nicolasstucki deleted the real-while-loops branch September 21, 2018 20:29

Promote WhileDo loops to typed trees. #5113

Promote WhileDo loops to typed trees. #5113

Uh oh!

Conversation

sjrd commented Sep 15, 2018

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

Uh oh!

sjrd commented Sep 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolasstucki commented Sep 16, 2018

Uh oh!

sjrd commented Sep 16, 2018

Uh oh!

allanrenucci Sep 16, 2018

Choose a reason for hiding this comment

Uh oh!

sjrd Sep 16, 2018

Choose a reason for hiding this comment

Uh oh!

smarter Sep 16, 2018

Choose a reason for hiding this comment

Uh oh!

nicolasstucki commented Sep 16, 2018

Uh oh!

sjrd commented Sep 17, 2018

Uh oh!

nicolasstucki commented Sep 17, 2018

Uh oh!

sjrd commented Sep 17, 2018

Uh oh!

sjrd commented Sep 17, 2018

Uh oh!

nicolasstucki Sep 18, 2018

Choose a reason for hiding this comment

Uh oh!

nicolasstucki Sep 18, 2018

Choose a reason for hiding this comment

Uh oh!

nicolasstucki Sep 18, 2018

Choose a reason for hiding this comment

Uh oh!

sjrd commented Sep 18, 2018

Uh oh!

nicolasstucki commented Sep 21, 2018

Uh oh!

sjrd commented Sep 21, 2018

Uh oh!

nicolasstucki commented Sep 21, 2018

Uh oh!

sjrd commented Sep 21, 2018

Uh oh!

sjrd commented Sep 21, 2018

Uh oh!

Uh oh!

Promote `WhileDo` loops to typed trees. #5113

Promote `WhileDo` loops to typed trees. #5113

sjrd commented Sep 15, 2018 •

edited

Loading