Join logic for unpickling top level and term tasty #4546

nicolasstucki · 2018-05-17T21:27:55Z

No description provided.

nicolasstucki · 2018-05-17T21:28:41Z

Improvement proposed in #4538

smarter · 2018-05-17T21:30:24Z

compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala

-    val rdr = new TreeReader(reader).fork
-    ownerTree = new OwnerTree(NoAddr, 0, rdr.fork, reader.endAddr)
-    rdr.readTerm()
+    if (isTopLevel)


Can't we infer whether this is top level or not based on the tag of the first tree node?

smarter · 2018-05-17T21:54:21Z

compiler/src/dotty/tools/dotc/core/quoted/PickledQuotes.scala

  private def unpickle(bytes: Array[Byte], splices: Seq[Any])(implicit ctx: Context): Tree = {
    val unpickler = new TastyUnpickler(bytes, splices)
-    val tree = unpickler.unpickleExpr()
+    unpickler.enter(Set(ctx.owner))


Is entering ctx.owner actually useful here? Otherwise you could just do Set.empty

I will have to try.

smarter · 2018-05-17T21:56:28Z

compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala

+
    def readTopLevel()(implicit ctx: Context): List[Tree] = {
      @tailrec def read(acc: ListBuffer[Tree]): List[Tree] = nextByte match {
        case IMPORT | PACKAGE =>


Could use isTopLevel here. Also I wonder if we couldn't simplify that to return a single Tree instead of a List[Tree] if we never pickle a list of trees.

We may have problems tasty files that have top-level imports

Do we even export those?

If I put a top level import I get

import scala.collection.mutable.TreeMap ...

0: PACKAGE(80) 2: TERMREFpkg 1 [<empty>] 4: IMPORT(8) 6: SELECT 2 [mutable] 8: SELECT 3 [collection] 10: TERMREFpkg 4 [scala] 12: IMPORTED 5 [TreeMap] 14: PACKAGE(66) 16: TERMREFpkg 6 [foo] ...

same with

package bar import scala.collection.mutable.TreeMap

0: PACKAGE(80) 2: TERMREFpkg 1 [bar] 4: IMPORT(8) 6: SELECT 2 [mutable] 8: SELECT 3 [collection] 10: TERMREFpkg 4 [scala] 12: IMPORTED 5 [TreeMap]

I think we may go further and do

def isTopLevel(implicit ctx: Context): Boolean = nextByte == PACKAGE

nicolasstucki · 2018-06-06T14:05:09Z

Rebased

smarter · 2018-06-12T16:25:03Z

compiler/src/dotty/tools/dotc/core/quoted/TastyUnpickler.scala

 *  @param splices       splices that will fill the holes in the quote
 */
-class TastyUnpickler(bytes: Array[Byte], splices: Seq[Any]) extends DottyUnpickler(bytes) {
+class TastyUnpickler(bytes: Array[Byte], splices: Seq[Any], isTypeTree: Boolean) extends DottyUnpickler(bytes) {


Woudln't it make more sense to call this class QuoteUnpickler ?

Yes. I will change it.

smarter · 2018-06-12T16:25:31Z

compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala


 object TreeUnpickler {

+  sealed trait UnpickleMode


Needs documentation for this trait and all its children.

smarter · 2018-06-12T16:26:17Z

compiler/src/dotty/tools/dotc/core/tasty/DottyUnpickler.scala

  private val posUnpicklerOpt = unpickler.unpickle(new PositionsSectionUnpickler)
  private val treeUnpickler = unpickler.unpickle(treeSectionUnpickler(posUnpicklerOpt)).get

+  protected def mode: TreeUnpickler.UnpickleMode = TreeUnpickler.UnpickleMode.TopLevel


This should be a val I think.

Yes, it should

smarter · 2018-06-12T16:26:38Z

compiler/src/dotty/tools/dotc/core/quoted/TastyUnpickler.scala

  import DottyUnpickler._
  import TastyUnpickler._

+  override protected def mode: TreeUnpickler.UnpickleMode =


protected override, like three lines below :)

nicolasstucki · 2018-07-11T08:34:10Z

Rebased

nicolasstucki · 2018-07-15T09:14:02Z

@smarter don't forget to review this one.

smarter · 2018-07-15T09:51:49Z

compiler/src/dotty/tools/dotc/core/tasty/DottyUnpickler.scala

  private val commentUnpicklerOpt = unpickler.unpickle(new CommentsSectionUnpickler)
  private val treeUnpickler = unpickler.unpickle(treeSectionUnpickler(posUnpicklerOpt, commentUnpicklerOpt)).get

+  protected val mode: TreeUnpickler.UnpickleMode = TreeUnpickler.UnpickleMode.TopLevel


Maybe instead of a field this could be a class parameter: class DottyUnpickler(bytes: Array[Byte], mode: UnpickleMode = UnpickleMode.TopLevel)

Good idea. Done.

allanrenucci · 2018-07-16T11:53:55Z

compiler/src/dotty/tools/dotc/core/tasty/DottyUnpickler.scala

 *  @param bytes         the bytearray containing the Tasty file from which we unpickle
 */
-class DottyUnpickler(bytes: Array[Byte]) extends ClassfileParser.Embedded with tpd.TreeProvider {
+class DottyUnpickler(bytes: Array[Byte], mode: UnpickleMode = UnpickleMode.TopLevel) extends ClassfileParser.Embedded with tpd.TreeProvider {


Can you update the class documentation?

nicolasstucki requested a review from smarter May 17, 2018 21:27

smarter reviewed May 17, 2018

View reviewed changes

nicolasstucki force-pushed the join-unpicklings branch from 7ccdee0 to b88ade9 Compare May 18, 2018 19:25

nicolasstucki requested a review from smarter May 18, 2018 19:56

nicolasstucki added the stat:needs review label May 18, 2018

nicolasstucki assigned smarter May 18, 2018

nicolasstucki force-pushed the join-unpicklings branch from b88ade9 to d1a55d9 Compare June 6, 2018 14:04

nicolasstucki force-pushed the join-unpicklings branch from d1a55d9 to fb56b95 Compare June 6, 2018 19:33

smarter reviewed Jun 12, 2018

View reviewed changes

nicolasstucki force-pushed the join-unpicklings branch from eb7649a to 2794be3 Compare June 13, 2018 05:29

nicolasstucki added 3 commits July 11, 2018 10:32

Join logic for unpickling top level and term tasty

8f5a8ab

Rename TastyUnpickler to QuoteUnpickler

cfac787

Add comments

ad111dd

nicolasstucki force-pushed the join-unpicklings branch from 2794be3 to ad111dd Compare July 11, 2018 08:33

smarter approved these changes Jul 15, 2018

View reviewed changes

Make DottyUnpickler mode a class parameter

68043eb

allanrenucci reviewed Jul 16, 2018

View reviewed changes

Add docs

0100c12

nicolasstucki merged commit bfbd1af into scala:master Jul 18, 2018

allanrenucci deleted the join-unpicklings branch July 18, 2018 11:10

Join logic for unpickling top level and term tasty #4546

Join logic for unpickling top level and term tasty #4546

Uh oh!

Conversation

nicolasstucki commented May 17, 2018

Uh oh!

nicolasstucki commented May 17, 2018

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!

nicolasstucki commented Jun 6, 2018

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!

nicolasstucki commented Jul 11, 2018

Uh oh!

nicolasstucki commented Jul 15, 2018

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants