-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Changes from 7 commits
2c0d7d9
dbd6749
905a178
afbc23f
deb9dc8
f29e03b
236b931
4eed3f1
3f15ee0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 = { | ||
!ctx.settings.YdropComments.value || ctx.mode.is(Mode.ReadComments) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we have both a new mode and a compiler option? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nicolasstucki Does it make sense to pickle comment for quoted trees? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
} | ||
|
||
|
@@ -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) | ||
} |
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make the traverser take the |
||
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) | ||
} | ||
} | ||
|
||
} |
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) | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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._ | ||
|
@@ -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._ | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the mode is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same goes for The Do we prefer crashing? Should we add a warning? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really can't tell why the I don't see a good reason to do the same. It only makes thing more complicated. I think the Maybe @smarter or @nicolasstucki can explain why it was done like this for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No idea, but what you said sounds reasonable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's actually valid for the |
||
commentUnpickler <- commentUnpicklerOpt } { | ||
val comment = commentUnpickler.commentAt(start) | ||
docCtx.addDocstring(tree.symbol, comment) | ||
tree.setComment(comment) | ||
} | ||
} | ||
|
||
tree | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 hereThere was a problem hiding this comment.
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 ofsetup
, and everything in this class isprotected
.There was a problem hiding this comment.
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