Add Comments section in TASTY
#4461
Conversation
|
What do you think about reversing the flag, |
| ctx0.setSettings(summary.sstate) | ||
|
|
||
| val ctx = | ||
| if (shouldAddDocContext(ctx0)) ctx0.setProperty(ContextDoc, new ContextDocstrings) |
There was a problem hiding this comment.
I don't think you need ctx0, Context.setProperty mutates the context
| import org.junit.Test | ||
| import org.junit.Assert.{assertEquals, assertFalse, fail} | ||
|
|
||
| class CommentPicklingTest extends ParallelTesting { |
There was a problem hiding this comment.
Why does this extends ParallelTesting?
| maxDepth: Int = Int.MaxValue): Seq[Path] = { | ||
| val out = collection.mutable.ListBuffer.empty[Path] | ||
| val matcher = FileSystems.getDefault.getPathMatcher(pattern) | ||
| val visitor = new FileVisitor[Path] { |
There was a problem hiding this comment.
Something like?
val paths = Files.walk(base)
try paths.filter(matcher.matches).iterator().asScala.toList
finally paths.close()|
|
||
| private def getAll(base: Path, | ||
| pattern: String, | ||
| maxDepth: Int = Int.MaxValue): Seq[Path] = { |
There was a problem hiding this comment.
maxDepth is not used. Remove?
| compileAndCheckComment(sources, "buzz".toTermName, Some("/** foo */")) | ||
| } | ||
|
|
||
| private def compileAndCheckComment(sources: Seq[String], treeName: Name, expectedComment: Option[String]): Unit = { |
There was a problem hiding this comment.
I would use List instead of Seq across the file. I don't think you gain anything by using Seq
There was a problem hiding this comment.
Do we optimise the API for the most common use case where you want to check for the presence of a comment? Option[String] => String.
Also maybe add an overload for one source
| } | ||
| } | ||
|
|
||
| private def findTreeNamed(name: Name)(trees: Seq[tpd.Tree], ctx: Context): Option[tpd.NameTree] = { |
There was a problem hiding this comment.
We have find on Tree in tpd.TreeOps
| @@ -0,0 +1,178 @@ | |||
| package dotty.tools.dotc | |||
There was a problem hiding this comment.
Move to tasty package?
|
@OlivierBlanvillain I've tried switching
I'd prefer to leave the switching of the flag for a later PR, when it's clearer to me what needs to be fixed in order to keep the comments by default. |
Duhemm
force-pushed
the
topic/tasty-doc
branch
2 times, most recently
from
18b0230 to
13476fd
Compare
The comments are pickled when `-Ykeep-comments` is set.
This commit removes `-Ykeep-comments` and adds a new `-Ydrop-comments` flag. `-Ydrop-comments` is used to prevent the parser from keeping the comments, and defaults to `false`.
| import java.nio.charset.Charset | ||
|
|
||
| class CommentPickler(pickler: TastyPickler, addrOfTree: tpd.Tree => Option[Addr])(implicit ctx: Context) { | ||
| val buf = new TastyBuffer(5000) |
| // -- Output routines -------------------------------------------- | ||
|
|
||
| /** Write a boolean value. */ | ||
| def writeBoolean(b: Boolean): Unit = |
There was a problem hiding this comment.
I would write the byte explicitly pickleComment and read the byte in CommentUnpickler. This abstraction is only used once and hides the underlying representation. It would make it too easy to write two calls to writeBoolean instead of one to writeByte (with bitset representation).
| buf.writeAddr(addr) | ||
| buf.writeNat(length) | ||
| buf.writeBytes(bytes, length) | ||
| buf.writeBoolean(cmt.isExpanded) |
There was a problem hiding this comment.
This seems to be missing from the documentation.
| new TastyReader(bytes, index(start), index(end), base) | ||
|
|
||
| /** Read a boolean value. */ | ||
| def readBoolean(): Boolean = |
|
The |
|
@nicolasstucki I've addressed your comments and modified the |
|
Maybe print the first |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4461/ to see the changes. Benchmarks is based on merging with master (78a5e09) |
| * 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 = { |
There was a problem hiding this comment.
Why protected? It seems to be only used here
There was a problem hiding this comment.
Because it is likely to be used in setup or an override of setup, and everything in this class is protected.
There was a problem hiding this comment.
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
| 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.
@nicolasstucki Does it make sense to pickle comment for quoted trees?
There was a problem hiding this comment.
I do not have a use case in mind. We should not add it.
| sym.registerIfChild(late = true) | ||
|
|
||
| if (ctx.mode.is(Mode.ReadComments)) { | ||
| for { docCtx <- ctx.docCtx |
There was a problem hiding this comment.
If the mode is ReadComments, then I suppose both docCtx and commentUnpickler should not be empty
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
No idea, but what you said sounds reasonable
There was a problem hiding this comment.
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.
| * 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.
Why do we have both a new mode and a compiler option?
Isn't !ctx.settings.YdropComments <=> Mode.ReadComments.
There was a problem hiding this comment.
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.
| def pickleComment(root: tpd.Tree): Unit = | ||
| new Traverser().traverse(root) | ||
|
|
||
| def pickleComment(addrOfTree: Option[Addr], comment: Option[Comment]): Unit = (addrOfTree, comment) match { |
| * 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 = { |
There was a problem hiding this comment.
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
| * unpickle the doc comments. | ||
| */ | ||
| protected def shouldAddDocContext(implicit ctx: Context): Boolean = { | ||
| !ctx.settings.YdropComments.value || ctx.mode.is(Mode.ReadComments) |
| () | ||
| } | ||
|
|
||
| private class Traverser extends tpd.TreeTraverser { |
There was a problem hiding this comment.
Make the traverser take the docCtx and only create the traverser in pickleComment if it exists
| sym.registerIfChild(late = true) | ||
|
|
||
| if (ctx.mode.is(Mode.ReadComments)) { | ||
| for { docCtx <- ctx.docCtx |
There was a problem hiding this comment.
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
| } | ||
|
|
||
| private def findTreeNamed(name: Name)(trees: List[tpd.Tree], ctx: Context): Option[tpd.NameTree] = { | ||
| val acc = new tpd.TreeAccumulator[Option[tpd.NameTree]] { |
There was a problem hiding this comment.
why not use TreeOps.find from tpd.scala?
| Files.createDirectories(out) | ||
|
|
||
| val options = compileOptions.and("-d", out.toAbsolutePath.toString).and(sourceFiles: _*) | ||
| val driver = new Driver |
There was a problem hiding this comment.
dotty.tools.package.dotc.Main?
| } | ||
|
|
||
| private def compileAndUnpickle[T](sources: List[String])(fn: (List[tpd.Tree], Context) => T) = { | ||
| inTempDirectory { tmp => |
There was a problem hiding this comment.
A nice to have (that shouldn't block this PR): a driver that compiles from virtual files. I think it would be fairly easy: override doCompile and call compileSources(List[SourceFile]) instead of compile(List[String]). Creating a virtual source file is straightforward: new SourceFile("<fake name>", content.toCharArray)
Now that we check that the `DocContext` is set, we need to set it in the tests (or add `-Ydiscard-comments`, but this better reflects how Dotty is going to be used).
6 participants
The comments are pickled when
-Ykeep-commentsis set.