Create DecompilerPrinter and ReplPrinter #3691

nicolasstucki · 2017-12-22T08:41:21Z

Enable flag by default for the decompiler

nicolasstucki · 2017-12-22T08:42:33Z

From this

to this

allanrenucci · 2017-12-22T09:31:50Z

Not sure it is the correct solution. It prints invalid trees after lambda lift:

class Test {
  def test = {
    val y = 2
    (x: Int) => x + y
  }
}

package <empty> {
  @scala.annotation.internal.SourceFile("tests/allan/Test.scala") class Test
     extends
   Object { 
    def <init>(): Unit = 
      {
        super()
        ()
      }
    def test(): Function1 = 
      {
        val y: Int = 2
        {
          
        }
      }
    (y$1: Int, x: Int) => 
      {
        x.+(y$1)
      }
  }
}

scalac:

    def test(): Function1 = {
      val y: Int = 2;
      {
        ((x: Int) => Test.this.$anonfun$test$1(y, x))
      }
    };
    final <artifact> private[this] def $anonfun$test$1(y$1: Int, x: Int): Int = x.+(y$1)

nicolasstucki · 2017-12-22T09:38:54Z

This option is not supposed to be used after lambda lift. In fact it is only intended for pickling level of tree abstractions. I could make it a Y setting.

allanrenucci · 2017-12-22T09:47:43Z

Can't we have a solution that works across all phases? The way we currently print closures doesn't feel natural to me:

    def test(): Function1 = 
      {
        val y: Int = 2
        {
          closure(y | <empty>.this.test$$anonfun$1:JFunction1$mcII$sp)
        }
      }
    private <static> def test$$anonfun$1(y$1: Int, x: Int): Int = 
      {
        x.+(y$1)
      }

It would be great to print them like scalac does (at least under an option).

nicolasstucki · 2017-12-22T09:52:13Z

It should print the normal output if we are in a transformation phase. No normal user should ever see the trees in those.

allanrenucci · 2017-12-22T10:53:52Z

Don't you don't think this:

    def test(): Function1 = 
      {
        val y: Int = 2
        {
          closure(y | <empty>.this.test$$anonfun$1:JFunction1$mcII$sp)
        }
      }

would be better printed as:

    def test(): Function1 = 
      {
        val y: Int = 2
        {
          (x: Int) => <empty>.this.test$$anonfun$1(y: Int, x: Int)
        }
      }

It is still the same tree, just a different way to print it

nicolasstucki · 2017-12-22T13:02:44Z

@allanrenucci changing printers in transforms is outside of the scope of this PR

allanrenucci · 2017-12-22T13:26:35Z

If the scope of this PR is only the decompiler then I don't think it is worth the added complexity to RefinedPrinter. I would rather create a special printer for the decompiler (a subclass of RefinedPrinter) and not introduce a new setting.

nicolasstucki · 2017-12-22T14:28:35Z

There would be a lot of duplication if we do not put it in the RefinedPrinter. And we would have to maintain any change in two parallel printers.

smarter · 2017-12-24T17:34:50Z

Note that we also have a https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/printing/UserFacingPrinter.scala (which I'd like to get rid of if possible), so calling this option -Yprint-user is confusing.

nicolasstucki · 2017-12-24T17:40:56Z

We should probably move that logic from UserFacingPrinter to PlainPrinter as well.

What about -Yprint-decompile?

nicolasstucki · 2017-12-28T17:01:41Z

@smarter all branches of the code in UserFacingPrinter were covered by tests.

liufengyun

I've a general concern to make RefinedPrinter use-scenario-aware: repl, decompile are both specific usage of RefinedPrinter.

I think it's better to add functional options (usage-neutral), like printPos, printLines, printTypes, etc. In current PR, maybe something like printFriendly to print friendly syntax for While/DoWhile and function literals?

For the case of Repl, I think it's better to create a separate Printer for it, as it's really usage-specific.

liufengyun · 2018-01-10T13:15:13Z

compiler/src/dotty/tools/dotc/printing/RefinedPrinter.scala

@@ -14,7 +14,8 @@ import TypeApplications._
 import Decorators._
 import config.Config
 import util.Positions._
-import transform.SymUtils._
+import dotty.tools.dotc.transform.SymUtils._
+import dotty.tools.dotc.transform.FirstTransform


Remove the prefix dotty.tools.dotc ?

No, confused the IDE

liufengyun · 2018-01-10T15:19:29Z

compiler/src/dotty/tools/dotc/printing/RefinedPrinter.scala

@@ -27,6 +28,10 @@ class RefinedPrinter(_ctx: Context) extends PlainPrinter(_ctx) {
  private[this] var myCtx: Context = _ctx
  private[this] var printPos = ctx.settings.YprintPos.value
  private[this] val printLines = ctx.settings.printLines.value
+  private[this] val YprintDecompile =
+    ctx.settings.YprintDecompile.value && !ctx.phases.dropWhile(!_.isInstanceOf[FirstTransform]).contains(ctx.phase)


Add a comment to explain the latter condition?

allanrenucci · 2018-01-10T15:54:46Z

I've a general concern to make RefinedPrinter use-scenario-aware: repl, decompile are both specific usage of RefinedPrinter.

I tend to agree with @liufengyun here. Should we discuss this PR during the next Dotty meeting?

nicolasstucki · 2018-01-17T08:17:11Z

First 3 commits from #3701

nicolasstucki · 2018-01-17T08:34:40Z

@liufengyun the last commit is ready for review. The first 3 are based on another PR.

liufengyun

LGTM

BTW, other things that can be handled as well (possibly in a different PR):

While/DoWhile encoding
String Interpolator encoding
Anonymous class encoding

liufengyun · 2018-01-17T11:01:55Z

compiler/src/dotty/tools/dotc/decompiler/DecompilationPrinter.scala

@@ -14,23 +18,41 @@ class DecompilationPrinter extends Phase {
  override def phaseName: String = "decompilationPrinter"

  override def run(implicit ctx: Context): Unit = {
-    val unit = ctx.compilationUnit
+    val outputDir = ctx.settings.outputDir.value
+    if (outputDir == ".") printToOutput(System.out)


So here we assume print to current folder means printing to standard console?

Yes, as it is the default it is practical.

liufengyun · 2018-01-17T11:04:54Z

compiler/src/dotty/tools/dotc/decompiler/DecompilationPrinter.scala

+      var os: OutputStream = null
+      var ps: PrintStream = null
+      try {
+        os = File(outputDir + ".decompiled").outputStream()


dir + .decompiled: filename missing?

Oh, I use the outputDir as a file. Just like we do with JARs, where we write -d Foo.jar. We can change the format later if needed.

This mode is mostly intended for testing

liufengyun · 2018-01-17T11:11:11Z

compiler/src/dotty/tools/dotc/printing/DecompilerPrinter.scala

+
+class DecompilerPrinter(_ctx: Context) extends RefinedPrinter(_ctx) {
+
+  override protected def modText(mods: untpd.Modifiers, kw: String): Text = { // DD


It's better to have a comment to document what's changed/overriden. The same for other overwritten methods.

Or, can we share more code with base class to reduce the overriding changes? The same for other overwritten methods.

I will go for the second. Documenting all changes might end in the large and unmaintainable list of features when we fix more stuff. I would rely more on the commit history and the tests added for that change.

liufengyun · 2018-01-17T12:31:46Z

compiler/src/dotty/tools/dotc/printing/RefinedPrinter.scala

@@ -64,7 +78,9 @@ class RefinedPrinter(_ctx: Context) extends PlainPrinter(_ctx) {
  protected val PrintableFlags = (SourceModifierFlags | Label | Module | Local).toCommonFlags

  override def nameString(name: Name): String =
-    if (ctx.settings.YdebugNames.value) name.debugString else name.toString
+    if (name.isReplAssignName) name.decode.toString.takeWhile(_ != '$')


Override nameString in ReplPrinter?

Yes, missed it.

nicolasstucki changed the title ~~Add -XprintUser and pretty print lambdas with it~~ Add -XprintUser to pretty print for the decompiler Dec 22, 2017

nicolasstucki force-pushed the print-lambdas branch from 209ae55 to 2166012 Compare December 24, 2017 17:30

nicolasstucki mentioned this pull request Dec 24, 2017

Add decomplilation tests #3701

Closed

nicolasstucki force-pushed the print-lambdas branch 2 times, most recently from d34b0dc to 3d9e71d Compare December 28, 2017 17:00

nicolasstucki requested a review from smarter January 2, 2018 11:03

nicolasstucki added the stat:needs review label Jan 2, 2018

nicolasstucki changed the title ~~Add -XprintUser to pretty print for the decompiler~~ Add -YprintDecompile and -YprintRepl to pretty print for the decompiler and repl Jan 5, 2018

nicolasstucki assigned smarter Jan 8, 2018

nicolasstucki mentioned this pull request Jan 8, 2018

Implement Expr.{run|show} #3685

Merged

nicolasstucki requested review from liufengyun and removed request for smarter January 10, 2018 12:21

nicolasstucki assigned liufengyun and unassigned smarter Jan 10, 2018

liufengyun reviewed Jan 10, 2018

View reviewed changes

nicolasstucki added 2 commits January 16, 2018 15:31

Add decompiler tests

b80d85e

Output to file and not show diff in terminal

bfb38d3

Cleanup

8ae50fe

nicolasstucki force-pushed the print-lambdas branch 5 times, most recently from f77825b to 65cb973 Compare January 17, 2018 08:09

nicolasstucki changed the title ~~Add -YprintDecompile and -YprintRepl to pretty print for the decompiler and repl~~ Create DecompilerPrinter and ReplPrinter Jan 17, 2018

Create DecompilerPrinter and ReplPrinter

6f81983

nicolasstucki force-pushed the print-lambdas branch from 65cb973 to 6f81983 Compare January 17, 2018 08:16

liufengyun approved these changes Jan 17, 2018

View reviewed changes

nicolasstucki added 2 commits January 17, 2018 18:22

Move repl logic to ReplPrinter

241bd91

Simplify mods.annotations filtering

a5f967e

liufengyun merged commit 4b75d3c into scala:master Jan 18, 2018

liufengyun deleted the print-lambdas branch January 18, 2018 09:13


		class DecompilerPrinter(_ctx: Context) extends RefinedPrinter(_ctx) {

		override protected def modText(mods: untpd.Modifiers, kw: String): Text = { // DD

Create DecompilerPrinter and ReplPrinter #3691

Create DecompilerPrinter and ReplPrinter #3691

Uh oh!

Conversation

nicolasstucki commented Dec 22, 2017

Uh oh!

nicolasstucki commented Dec 22, 2017

Uh oh!

allanrenucci commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolasstucki commented Dec 22, 2017

Uh oh!

allanrenucci commented Dec 22, 2017

Uh oh!

nicolasstucki commented Dec 22, 2017

Uh oh!

allanrenucci commented Dec 22, 2017

Uh oh!

nicolasstucki commented Dec 22, 2017

Uh oh!

allanrenucci commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolasstucki commented Dec 22, 2017

Uh oh!

smarter commented Dec 24, 2017

Uh oh!

nicolasstucki commented Dec 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolasstucki commented Dec 28, 2017

Uh oh!

liufengyun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanrenucci commented Jan 10, 2018

Uh oh!

nicolasstucki commented Jan 17, 2018

Uh oh!

nicolasstucki commented Jan 17, 2018

Uh oh!

liufengyun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

allanrenucci commented Dec 22, 2017 •

edited

Loading

allanrenucci commented Dec 22, 2017 •

edited

Loading

nicolasstucki commented Dec 24, 2017 •

edited

Loading