Skip to content

Fix #6542: Pickle line sizes in TASTy #10363

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 3 commits into from
Nov 30, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
16 changes: 5 additions & 11 deletions compiler/src/dotty/tools/dotc/core/tasty/DottyUnpickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,17 @@ object DottyUnpickler {
/** Exception thrown if classfile is corrupted */
class BadSignature(msg: String) extends RuntimeException(msg)

class TreeSectionUnpickler(posUnpickler: Option[PositionUnpickler], lineSizesUnpickler: Option[LineSizesUnpickler], commentUnpickler: Option[CommentUnpickler])
class TreeSectionUnpickler(posUnpickler: Option[PositionUnpickler], commentUnpickler: Option[CommentUnpickler])
extends SectionUnpickler[TreeUnpickler](ASTsSection) {
def unpickle(reader: TastyReader, nameAtRef: NameTable): TreeUnpickler =
new TreeUnpickler(reader, nameAtRef, posUnpickler, lineSizesUnpickler, commentUnpickler)
new TreeUnpickler(reader, nameAtRef, posUnpickler, commentUnpickler)
}

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

class LineSizesSectionUnpickler extends SectionUnpickler[LineSizesUnpickler]("LineSizes") {
def unpickle(reader: TastyReader, nameAtRef: NameTable): LineSizesUnpickler =
new LineSizesUnpickler(reader)
}

class CommentsSectionUnpickler extends SectionUnpickler[CommentUnpickler](CommentsSection) {
def unpickle(reader: TastyReader, nameAtRef: NameTable): CommentUnpickler =
new CommentUnpickler(reader)
Expand All @@ -50,18 +45,17 @@ class DottyUnpickler(bytes: Array[Byte], mode: UnpickleMode = UnpickleMode.TopLe

val unpickler: TastyUnpickler = new TastyUnpickler(bytes)
private val posUnpicklerOpt = unpickler.unpickle(new PositionsSectionUnpickler)
private val lineSizesUnpicklerOpt = unpickler.unpickle(new LineSizesSectionUnpickler)
private val commentUnpicklerOpt = unpickler.unpickle(new CommentsSectionUnpickler)
private val treeUnpickler = unpickler.unpickle(treeSectionUnpickler(posUnpicklerOpt, lineSizesUnpicklerOpt, commentUnpicklerOpt)).get
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
*/
def enter(roots: Set[SymDenotation])(using Context): Unit =
treeUnpickler.enter(roots)

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

protected def computeRootTrees(using Context): List[Tree] = treeUnpickler.unpickle(mode)

Expand Down
35 changes: 0 additions & 35 deletions compiler/src/dotty/tools/dotc/core/tasty/LineSizesPickler.scala

This file was deleted.

45 changes: 0 additions & 45 deletions compiler/src/dotty/tools/dotc/core/tasty/LineSizesUnpickler.scala

This file was deleted.

16 changes: 15 additions & 1 deletion compiler/src/dotty/tools/dotc/core/tasty/PositionPickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,21 @@ class PositionPickler(
(addrDelta << 3) | (toInt(hasStartDelta) << 2) | (toInt(hasEndDelta) << 1) | toInt(hasPoint)
}

def picklePositions(roots: List[Tree], warnings: mutable.ListBuffer[String]): Unit = {
def picklePositions(source: SourceFile, roots: List[Tree], warnings: mutable.ListBuffer[String]): Unit = {
/** Pickle the number of lines followed by the length of each line */
def pickleLineOffsetts(): Unit = {
val content = source.content()
buf.writeInt(content.count(_ == '\n') + 1) // number of lines
var lastIndex = content.indexOf('\n', 0)
buf.writeInt(lastIndex) // size of first line
while lastIndex != -1 do
val nextIndex = content.indexOf('\n', lastIndex + 1)
val end = if nextIndex != -1 then nextIndex else content.length
buf.writeInt(end - lastIndex - 1) // size of the next line
lastIndex = nextIndex
}
pickleLineOffsetts()

var lastIndex = 0
var lastSpan = Span(0, 0)
def pickleDeltas(index: Int, span: Span) = {
Expand Down
13 changes: 13 additions & 0 deletions compiler/src/dotty/tools/dotc/core/tasty/PositionUnpickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,20 @@ import Names.TermName
class PositionUnpickler(reader: TastyReader, nameAtRef: NameRef => TermName) {
import reader._

private var myLineSizes: Array[Int] = _
private var mySpans: util.HashMap[Addr, Span] = _
private var mySourcePaths: util.HashMap[Addr, String] = _
private var isDefined = false

def ensureDefined(): Unit = {
if (!isDefined) {
val lines = readInt()
myLineSizes = new Array[Int](lines)
var i = 0
while i < lines do
myLineSizes(i) += readInt()
i += 1

mySpans = util.HashMap[Addr, Span]()
mySourcePaths = util.HashMap[Addr, String]()
var curIndex = 0
Expand Down Expand Up @@ -60,6 +68,11 @@ class PositionUnpickler(reader: TastyReader, nameAtRef: NameRef => TermName) {
mySourcePaths
}

private[tasty] def lineSizes: Array[Int] = {
ensureDefined()
myLineSizes
}

def spanAt(addr: Addr): Span = spans.getOrElse(addr, NoSpan)
def sourcePathAt(addr: Addr): String = sourcePaths.getOrElse(addr, "")
}
26 changes: 6 additions & 20 deletions compiler/src/dotty/tools/dotc/core/tasty/TastyPrinter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,6 @@ class TastyPrinter(bytes: Array[Byte]) {
case _ =>
}
sb.append("\n\n")
unpickle(new LineSizesSectionUnpickler) match {
case Some(s) => sb.append(s)
case _ =>
}
sb.append("\n\n")
unpickle(new CommentSectionUnpickler) match {
case Some(s) => sb.append(s)
case _ =>
Expand Down Expand Up @@ -144,9 +139,14 @@ class TastyPrinter(bytes: Array[Byte]) {
private val sb: StringBuilder = new StringBuilder

def unpickle(reader: TastyReader, tastyName: NameTable): String = {
val posUnpickler = new PositionUnpickler(reader, tastyName)
sb.append(s" ${reader.endAddr.index - reader.currentAddr.index}")
val spans = new PositionUnpickler(reader, tastyName).spans
sb.append(" position bytes:\n")
val lineSizes = posUnpickler.lineSizes
sb.append(s" lines: ${lineSizes.length}\n")
sb.append(posUnpickler.lineSizes.mkString(" line sizes: ", ", ", "\n"))
sb.append(" positions:\n")
val spans = posUnpickler.spans
val sorted = spans.toSeq.sortBy(_._1.index)
for ((addr, pos) <- sorted) {
sb.append(treeStr("%10d".format(addr.index)))
Expand All @@ -156,20 +156,6 @@ class TastyPrinter(bytes: Array[Byte]) {
}
}

class LineSizesSectionUnpickler extends SectionUnpickler[String]("LineSizes") {

private val sb: StringBuilder = new StringBuilder

def unpickle(reader: TastyReader, tastyName: NameTable): String = {
sb.append(" ").append(reader.endAddr.index - reader.currentAddr.index)
sb.append(" line sizes bytes:\n")
val lineSizes = new LineSizesUnpickler(reader)
sb.append(" sizes: ")
sb.append(lineSizes.sizes.mkString(", "))
sb.result
}
}

class CommentSectionUnpickler extends SectionUnpickler[String](CommentsSection) {

private val sb: StringBuilder = new StringBuilder
Expand Down
14 changes: 6 additions & 8 deletions compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,13 @@ import scala.annotation.constructorOnly
import scala.annotation.internal.sharable

/** Unpickler for typed trees
* @param reader the reader from which to unpickle
* @param posUnpicklerOpt the unpickler for positions, if it exists
* @param lineSizesUnpicklerOpt the unpickler for line sizes, if it exists
* @param commentUnpicklerOpt the unpickler for comments, 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
*/
class TreeUnpickler(reader: TastyReader,
nameAtRef: NameTable,
posUnpicklerOpt: Option[PositionUnpickler],
lineSizesUnpicklerOpt: Option[LineSizesUnpickler],
commentUnpicklerOpt: Option[CommentUnpickler]) {
import TreeUnpickler._
import tpd._
Expand Down Expand Up @@ -1366,9 +1364,9 @@ class TreeUnpickler(reader: TastyReader,
val path = sourcePathAt(addr)
if (path.nonEmpty) {
val sourceFile = ctx.getSource(path)
lineSizesUnpicklerOpt match
case Some(lineSizesUnpickler) =>
sourceFile.setLineIndices(lineSizesUnpickler.lineIndices)
posUnpicklerOpt match
case Some(posUnpickler) =>
sourceFile.setLineIndicesFromLineSizes(posUnpickler.lineSizes)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a potential issue here: the relative path of a file is not a unique identifier of the file, there might be path conflicts in the Scala ecosystem.

If we also store the hash of source files, that would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you use that hash?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe ctx.getSource(path) can take hash as an argument? I haven't thought through the details.

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 hash would be on the contents of the source? Then we would need to read all the sources eagerly when we add them to the context. That may be quite expensive.

Copy link
Contributor

Choose a reason for hiding this comment

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

When we compile files, we already have the file contents in memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it is only loaded when we first access something the depends on the source https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/util/SourceFile.scala#L43.

Copy link
Contributor

Choose a reason for hiding this comment

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

When we compile Scala files, the contents for the Scala files will be forced.

case _ =>
pickling.println(i"source change at $addr: $path")
ctx.withSource(sourceFile)
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/quoted/PickledQuotes.scala
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ object PickledQuotes {
if tree.span.exists then
val positionWarnings = new mutable.ListBuffer[String]()
new PositionPickler(pickler, treePkl.buf.addrOfTree, treePkl.treeAnnots)
.picklePositions(tree :: Nil, positionWarnings)
.picklePositions(ctx.compilationUnit.source, tree :: Nil, positionWarnings)
positionWarnings.foreach(report.warning(_))

val pickled = pickler.assembleParts()
Expand Down
4 changes: 1 addition & 3 deletions compiler/src/dotty/tools/dotc/transform/Pickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,7 @@ class Pickler extends Phase {
treePkl.compactify()
if tree.span.exists then
new PositionPickler(pickler, treePkl.buf.addrOfTree, treePkl.treeAnnots)
.picklePositions(tree :: Nil, positionWarnings)
new LineSizesPickler(pickler)
.pickleLineNumbers(unit.source)
.picklePositions(unit.source, tree :: Nil, positionWarnings)

if !ctx.settings.YdropComments.value then
new CommentPickler(pickler, treePkl.buf.addrOfTree, treePkl.docString)
Expand Down
10 changes: 9 additions & 1 deletion compiler/src/dotty/tools/dotc/util/SourceFile.scala
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,15 @@ class SourceFile(val file: AbstractFile, computeContent: => Array[Char]) extends
if lineIndicesCache eq null then
lineIndicesCache = calculateLineIndicesFromContents()
lineIndicesCache
def setLineIndices(indices: Array[Int]): Unit =
def setLineIndicesFromLineSizes(sizes: Array[Int]): Unit =
val lines = sizes.length
val indices = new Array[Int](lines + 1)
var i = 0
val penultimate = lines - 1
while i < penultimate do
indices(i + 1) = indices(i) + sizes(i) + 1 // `+1` for the '\n' at the end of the line
i += 1
indices(lines) = indices(penultimate) + sizes(penultimate) // last line does not end with '\n'
lineIndicesCache = indices

/** Map line to offset of first character in line */
Expand Down
1 change: 1 addition & 0 deletions project/scripts/cmdTests
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ cp tests/neg/i6371/B_2.scala $OUT/B.scala
rm $OUT/A.scala
"$SBT" "scalac -classpath $OUT1 -d $OUT1 $OUT/B.scala" > "$tmp" 2>&1 || echo "ok"
grep -qe "B.scala:2:7" "$tmp"
grep -qe "This location contains code that was inlined from A.scala:3" "$tmp"

echo "testing -Ythrough-tasty"
clear_out "$OUT"
Expand Down
13 changes: 5 additions & 8 deletions tasty/src/dotty/tools/tasty/TastyFormat.scala
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,9 @@ Note: Tree tags are grouped into 5 categories that determine what follows, and t
Category 5 (tags 128-255): tag Length <payload>


Standard-Section: "Positions" Assoc*
Standard-Section: "Positions" LinesSizes Assoc*

LinesSizes = Int Int* // Number of lines followed by the size of each line not counting the trailing `\n`

Assoc = Header offset_Delta? offset_Delta? point_Delta?
| SOURCE nameref_Int
Expand All @@ -247,11 +249,6 @@ Standard-Section: "Positions" Assoc*
All elements of a position section are serialized as Ints


Standard-Section: "LineSizes" LineSize*

LineSize = Int // Size the i-th line not counting the trailing `\n`


Standard Section: "Comments" Comment*

Comment = Length Bytes LongInt // Raw comment's bytes encoded as UTF-8, followed by the comment's coordinates.
Expand All @@ -262,8 +259,8 @@ Standard Section: "Comments" Comment*
object TastyFormat {

final val header: Array[Int] = Array(0x5C, 0xA1, 0xAB, 0x1F)
val MajorVersion: Int = 25
val MinorVersion: Int = 1
val MajorVersion: Int = 26
val MinorVersion: Int = 0

final val ASTsSection = "ASTs"
final val PositionsSection = "Positions"
Expand Down