Skip to content

Add Comments section in TASTY #4461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jun 26, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions compiler/src/dotty/tools/dotc/Driver.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ package dotty.tools.dotc

import dotty.tools.FatalError
import config.CompilerCommand
import core.Comments.{ContextDoc, ContextDocstrings}
import core.Contexts.{Context, ContextBase}
import core.Mode
import util.DotClass
import reporting._
import scala.util.control.NonFatal
Expand Down Expand Up @@ -40,10 +42,22 @@ class Driver extends DotClass {

protected def sourcesRequired = true

/**
* Should the `ContextDocstrings` be set for this context? The `ContextDocstrings` is used
* to store doc comments unless `-Ydrop-comments` is set, or when TASTY is configured to
* unpickle the doc comments.
*/
protected def shouldAddDocContext(implicit ctx: Context): Boolean = {
Copy link
Contributor

@allanrenucci allanrenucci Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why protected? It seems to be only used here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it is likely to be used in setup or an override of setup, and everything in this class is protected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely or not, this is not currently the case. I would just inline it into setup. In the future, if there is a use case, one can always extract the wanted bits into a protected def

!ctx.settings.YdropComments.value || ctx.mode.is(Mode.ReadComments)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have both a new mode and a compiler option?
Isn't !ctx.settings.YdropComments <=> Mode.ReadComments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, ReadComments configures whether the unpickler should unpickle comments. You may want to keep comments that are in your code, but don't want to unpickle comments from your dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough

}

def setup(args: Array[String], rootCtx: Context): (List[String], Context) = {
val ctx = rootCtx.fresh
val summary = CompilerCommand.distill(args)(ctx)
ctx.setSettings(summary.sstate)

if (shouldAddDocContext(ctx)) ctx.setProperty(ContextDoc, new ContextDocstrings)

val fileNames = CompilerCommand.checkUsage(summary, sourcesRequired)(ctx)
(fileNames, ctx)
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/config/ScalaSettings.scala
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ class ScalaSettings extends Settings.SettingGroup {
val YshowPrintErrors = BooleanSetting("-Yshow-print-errors", "don't suppress exceptions thrown during tree printing.")
val YtestPickler = BooleanSetting("-Ytest-pickler", "self-test for pickling functionality; should be used with -Ystop-after:pickler")
val YcheckReentrant = BooleanSetting("-Ycheck-reentrant", "check that compiled program does not contain vars that can be accessed from a global root.")
val YkeepComments = BooleanSetting("-Ykeep-comments", "Keep comments when scanning source files.")
val YdropComments = BooleanSetting("-Ydrop-comments", "Drop comments when scanning source files.")
val YcookComments = BooleanSetting("-Ycook-comments", "Cook the comments (type check `@usecase`, etc.)")
val YforceSbtPhases = BooleanSetting("-Yforce-sbt-phases", "Run the phases used by sbt for incremental compilation (ExtractDependencies and ExtractAPI) even if the compiler is ran outside of sbt, for debugging.")
val YdumpSbtInc = BooleanSetting("-Ydump-sbt-inc", "For every compiled foo.scala, output the API representation and dependencies used for sbt incremental compilation in foo.inc, implies -Yforce-sbt-phases.")
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/core/Comments.scala
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ object Comments {
}

object Comment {
def apply(pos: Position, raw: String, expanded: Boolean = false, usc: List[UseCase] = Nil)(implicit ctx: Context): Comment =
def apply(pos: Position, raw: String, expanded: Boolean = false, usc: List[UseCase] = Nil): Comment =
new Comment(pos, raw) {
val isExpanded = expanded
val usecases = usc
Expand Down
4 changes: 4 additions & 0 deletions compiler/src/dotty/tools/dotc/core/Mode.scala
Original file line number Diff line number Diff line change
Expand Up @@ -93,4 +93,8 @@ object Mode {

/** We are in the IDE */
val Interactive = newMode(20, "Interactive")

/** Read comments from definitions when unpickling from TASTY */
val ReadComments = newMode(21, "ReadComments")

}
10 changes: 5 additions & 5 deletions compiler/src/dotty/tools/dotc/core/quoted/TastyUnpickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import dotty.tools.dotc.core.tasty._
import dotty.tools.dotc.core.tasty.TastyUnpickler.NameTable

object TastyUnpickler {
class QuotedTreeSectionUnpickler(posUnpickler: Option[PositionUnpickler], splices: Seq[Any])
extends DottyUnpickler.TreeSectionUnpickler(posUnpickler) {
class QuotedTreeSectionUnpickler(posUnpickler: Option[PositionUnpickler], commentUnpickler: Option[CommentUnpickler], splices: Seq[Any])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolasstucki Does it make sense to pickle comment for quoted trees?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have a use case in mind. We should not add it.

extends DottyUnpickler.TreeSectionUnpickler(posUnpickler, commentUnpickler) {
override def unpickle(reader: TastyReader, nameAtRef: NameTable) =
new TreeUnpickler(reader, nameAtRef, posUnpickler, splices)
new TreeUnpickler(reader, nameAtRef, posUnpickler, commentUnpickler, splices)
}
}

Expand All @@ -19,6 +19,6 @@ class TastyUnpickler(bytes: Array[Byte], splices: Seq[Any]) extends DottyUnpickl
import DottyUnpickler._
import TastyUnpickler._

protected override def treeSectionUnpickler(posUnpicklerOpt: Option[PositionUnpickler]): TreeSectionUnpickler =
new QuotedTreeSectionUnpickler(posUnpicklerOpt, splices)
protected override def treeSectionUnpickler(posUnpicklerOpt: Option[PositionUnpickler], commentUnpicklerOpt: Option[CommentUnpickler]): TreeSectionUnpickler =
new QuotedTreeSectionUnpickler(posUnpicklerOpt, commentUnpicklerOpt, splices)
}
43 changes: 43 additions & 0 deletions compiler/src/dotty/tools/dotc/core/tasty/CommentPickler.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package dotty.tools.dotc.core.tasty

import dotty.tools.dotc.ast.tpd
import dotty.tools.dotc.core.Comments.{Comment, CommentsContext}
import dotty.tools.dotc.core.Contexts.Context
import dotty.tools.dotc.core.tasty.TastyBuffer.Addr

import java.nio.charset.Charset

class CommentPickler(pickler: TastyPickler, addrOfTree: tpd.Tree => Option[Addr])(implicit ctx: Context) {
private[this] val buf = new TastyBuffer(5000)
pickler.newSection("Comments", buf)

def pickleComment(root: tpd.Tree): Unit =
new Traverser().traverse(root)

def pickleComment(addrOfTree: Option[Addr], comment: Option[Comment]): Unit = (addrOfTree, comment) match {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private?

case (Some(addr), Some(cmt)) =>
val bytes = cmt.raw.getBytes(Charset.forName("UTF-8"))
val length = bytes.length
buf.writeAddr(addr)
buf.writeNat(length)
buf.writeBytes(bytes, length)
buf.writeByte(if (cmt.isExpanded) 1 else 0)
case other =>
()
}

private class Traverser extends tpd.TreeTraverser {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make the traverser take the docCtx and only create the traverser in pickleComment if it exists

override def traverse(tree: tpd.Tree)(implicit ctx: Context): Unit =
tree match {
case md: tpd.MemberDef =>
ctx.docCtx.foreach { docCtx =>
val comment = docCtx.docstring(md.symbol)
pickleComment(addrOfTree(md), comment)
}
traverseChildren(md)
case _ =>
traverseChildren(tree)
}
}

}
33 changes: 33 additions & 0 deletions compiler/src/dotty/tools/dotc/core/tasty/CommentUnpickler.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package dotty.tools.dotc.core.tasty

import dotty.tools.dotc.core.Comments.Comment
import dotty.tools.dotc.core.Symbols.Symbol
import dotty.tools.dotc.core.tasty.TastyBuffer.Addr
import dotty.tools.dotc.util.Positions

import scala.collection.mutable.HashMap

import java.nio.charset.Charset

class CommentUnpickler(reader: TastyReader) {
import reader._

private[tasty] lazy val comments = {
val comments = new HashMap[Addr, Comment]
while (!isAtEnd) {
val addr = readAddr()
val length = readNat()
if (length > 0) {
val bytes = readBytes(length)
val expanded = readByte() == 1
val rawComment = new String(bytes, Charset.forName("UTF-8"))
comments(addr) = Comment(Positions.NoPosition, rawComment, expanded = expanded)
}
}
comments.toMap
}

def commentAt(addr: Addr): Option[Comment] =
comments.get(addr)

}
16 changes: 11 additions & 5 deletions compiler/src/dotty/tools/dotc/core/tasty/DottyUnpickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,21 @@ object DottyUnpickler {
/** Exception thrown if classfile is corrupted */
class BadSignature(msg: String) extends RuntimeException(msg)

class TreeSectionUnpickler(posUnpickler: Option[PositionUnpickler])
class TreeSectionUnpickler(posUnpickler: Option[PositionUnpickler], commentUnpickler: Option[CommentUnpickler])
extends SectionUnpickler[TreeUnpickler]("ASTs") {
def unpickle(reader: TastyReader, nameAtRef: NameTable) =
new TreeUnpickler(reader, nameAtRef, posUnpickler, Seq.empty)
new TreeUnpickler(reader, nameAtRef, posUnpickler, commentUnpickler, Seq.empty)
}

class PositionsSectionUnpickler extends SectionUnpickler[PositionUnpickler]("Positions") {
def unpickle(reader: TastyReader, nameAtRef: NameTable) =
new PositionUnpickler(reader)
}

class CommentsSectionUnpickler extends SectionUnpickler[CommentUnpickler]("Comments") {
def unpickle(reader: TastyReader, nameAtRef: NameTable): CommentUnpickler =
new CommentUnpickler(reader)
}
}

/** A class for unpickling Tasty trees and symbols.
Expand All @@ -38,7 +43,8 @@ class DottyUnpickler(bytes: Array[Byte]) extends ClassfileParser.Embedded with t

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

/** Enter all toplevel classes and objects into their scopes
* @param roots a set of SymDenotations that should be overwritten by unpickling
Expand All @@ -52,8 +58,8 @@ class DottyUnpickler(bytes: Array[Byte]) extends ClassfileParser.Embedded with t
def unpickleTypeTree()(implicit ctx: Context): Tree =
treeUnpickler.unpickleTypeTree()

protected def treeSectionUnpickler(posUnpicklerOpt: Option[PositionUnpickler]): TreeSectionUnpickler = {
new TreeSectionUnpickler(posUnpicklerOpt)
protected def treeSectionUnpickler(posUnpicklerOpt: Option[PositionUnpickler], commentUnpicklerOpt: Option[CommentUnpickler]): TreeSectionUnpickler = {
new TreeSectionUnpickler(posUnpicklerOpt, commentUnpicklerOpt)
}

protected def computeTrees(implicit ctx: Context) = treeUnpickler.unpickle()
Expand Down
6 changes: 6 additions & 0 deletions compiler/src/dotty/tools/dotc/core/tasty/TastyFormat.scala
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,12 @@ Standard Section: "Positions" Assoc*
// same offset in the previously recorded node (or 0 for the first recorded node)
Delta = Int // Difference between consecutive offsets,

Standard Section: "Comments" Comment*

Comment = Length Bytes Byte // Raw comment's bytes encoded as UTF-8, plus a byte indicating
// whether the comment is expanded (1) or not expanded (0)


**************************************************************************************/

object TastyFormat {
Expand Down
14 changes: 14 additions & 0 deletions compiler/src/dotty/tools/dotc/core/tasty/TastyPrinter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class TastyPrinter(bytes: Array[Byte])(implicit ctx: Context) {
println("Trees:")
unpickle(new TreeSectionUnpickler)
unpickle(new PositionSectionUnpickler)
unpickle(new CommentSectionUnpickler)
}

class TreeSectionUnpickler extends SectionUnpickler[Unit]("ASTs") {
Expand Down Expand Up @@ -120,6 +121,19 @@ class TastyPrinter(bytes: Array[Byte])(implicit ctx: Context) {
}
}

class CommentSectionUnpickler extends SectionUnpickler[Unit]("Comments") {
def unpickle(reader: TastyReader, tastyName: NameTable): Unit = {
print(s" ${reader.endAddr.index - reader.currentAddr.index}")
val comments = new CommentUnpickler(reader).comments
println(s" comment bytes:")
val sorted = comments.toSeq.sortBy(_._1.index)
for ((addr, cmt) <- sorted) {
print(treeColor("%10d".format(addr.index)))
println(s": ${cmt.raw} (expanded = ${cmt.isExpanded})")
}
}
}

private def nameColor(str: String): String = Magenta(str).show
private def treeColor(str: String): String = Yellow(str).show
private def lengthColor(str: String): String = Cyan(str).show
Expand Down
19 changes: 16 additions & 3 deletions compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package dotc
package core
package tasty

import Comments.CommentsContext
import Contexts._, Symbols._, Types._, Scopes._, SymDenotations._, Names._, NameOps._
import StdNames._, Denotations._, Flags._, Constants._, Annotations._
import NameKinds._
Expand All @@ -25,13 +26,15 @@ import scala.quoted.Types.TreeType
import scala.quoted.Exprs.TastyTreeExpr

/** Unpickler for typed trees
* @param reader the reader from which to unpickle
* @param tastyName the nametable
* @param posUNpicklerOpt the unpickler for positions, if it exists
* @param reader the reader from which to unpickle
* @param posUnpicklerOpt the unpickler for positions, if it exists
* @param commentUnpicklerOpt the unpickler for comments, if it exists
* @param splices
*/
class TreeUnpickler(reader: TastyReader,
nameAtRef: NameRef => TermName,
posUnpicklerOpt: Option[PositionUnpickler],
commentUnpicklerOpt: Option[CommentUnpickler],
splices: Seq[Any]) {
import TastyFormat._
import TreeUnpickler._
Expand Down Expand Up @@ -820,6 +823,16 @@ class TreeUnpickler(reader: TastyReader,
// Child annotations for local classes and enum values are not pickled, so
// need to be re-established here.
sym.registerIfChild(late = true)

if (ctx.mode.is(Mode.ReadComments)) {
for { docCtx <- ctx.docCtx
Copy link
Contributor

@allanrenucci allanrenucci Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the mode is ReadComments, then I suppose both docCtx and commentUnpickler should not be empty

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same goes for posUnpicklerOpt: we check if the mode is ReadPositions and whether posUnpicklerOpt is set.

The TreeUnpickler can be constructed with a certain configuration in mind (i.e. not defining commentUnpicklerOpt), and then used with a different configuration (i.e. ctx.mode.is(Mode.ReadComments)), which would lead to crashes if we just used .get on the Option.

Do we prefer crashing? Should we add a warning?

Copy link
Contributor

@allanrenucci allanrenucci Jun 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really can't tell why the PositionUnpickler is taken as an option.

I don't see a good reason to do the same. It only makes thing more complicated. I think the TreeUnpickler does not need an optional CommentUnpickler. I think we should just look at Mode.ReadComments to decide whether or not we unpickle comments. And in the case we do, we should assert that we have a docCtx.

Maybe @smarter or @nicolasstucki can explain why it was done like this for PositionUnpickler

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea, but what you said sounds reasonable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually valid for the positionUnpickler or commentUnpickler to be None if there are no Positions or Comments sections in the TASTY file from which we're unpickling.

commentUnpickler <- commentUnpicklerOpt } {
val comment = commentUnpickler.commentAt(start)
docCtx.addDocstring(tree.symbol, comment)
tree.setComment(comment)
}
}

tree
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/parsing/Scanners.scala
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ object Scanners {
}

class Scanner(source: SourceFile, override val startFrom: Offset = 0)(implicit ctx: Context) extends ScannerCommon(source)(ctx) {
val keepComments = ctx.settings.YkeepComments.value
val keepComments = !ctx.settings.YdropComments.value

/** All doc comments kept by their end position in a `Map` */
private[this] var docstringMap: SortedMap[Int, Comment] = SortedMap.empty
Expand Down
4 changes: 4 additions & 0 deletions compiler/src/dotty/tools/dotc/transform/Pickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ class Pickler extends Phase {
if (tree.pos.exists)
new PositionPickler(pickler, treePkl.buf.addrOfTree).picklePositions(tree :: Nil)

if (!ctx.settings.YdropComments.value)
new CommentPickler(pickler, treePkl.buf.addrOfTree).pickleComment(tree)

// other pickle sections go here.
val pickled = pickler.assembleParts()
unit.pickled += (cls -> pickled)
Expand All @@ -84,6 +87,7 @@ class Pickler extends Phase {
.setPeriod(Period(ctx.runId + 1, FirstPhaseId))
.setReporter(new ThrowingReporter(ctx.reporter))
.addMode(Mode.ReadPositions)
.addMode(Mode.ReadComments)
.addMode(Mode.PrintShowExceptions))
result
}
Expand Down
Loading