Add support for raw docstrings in ASTs #1151

felixmulder · 2016-03-07T15:34:24Z

Hello, as some of you might know (@heathermiller, @odersky) - I'm coming to EPFL in a couple of weeks to help out on dottydoc 🎉

As such, I wanted to get a head start on the implementation. I've perused the code and started an implementation for docstring parsing.

In this PR the scanner has been modified to save the docstrings according to the following heuristic:

In the scanner, parse all comments
~~If the comment is a docstring, save it in a closestDocString: Option[Comment] var, replacing the old value~~
If the comment is a docstring, save it in a closestDocString: mutable.Queue[Comment] val
If keepComments is set to true also save it to revComments (not sure if this is necessary, but didn't want to ruin someone else's possibly ongoing implementation)
In the parser, put the saved docstring into the MemberDef tree if applicable

I'm not sure if this is the optimal implementation, but it seems to eliminate the need for positional checking and instead follows "if the docstring is the closest docstring to an entity, it is the entity's docstring." The closestDocString var would in this approach, however, have to be emptied at the end of parsing bodies (not done yet).

There are two questions I'm looking to have answered:

Am I on the right track, or is there a better way to go about this?
I want to add tests that check the resulting AST using some example code - where should I place these? Do we have similar tests somewhere I can have a look at?

Sidenote: in a separate branch I've also started the implementation of a dottydoc tool which uses only the FrontEnd phase to get an AST. As such, finding the correct solution to adding comments to the AST will be a requirement for moving forward with said branch.

DarkDimius · 2016-03-07T18:06:16Z

./src/dotty/tools/dotc/typer/Typer.scala:551: error: ')' expected but '}' found.
  }

does not seem to compile.

felixmulder · 2016-03-07T18:18:03Z

Thanks @DarkDimius, it should compile - even so, this PR does not touch the typer.

EDIT: but it does touch the scanner. I can see that Martin has a PR open for the successive opening of comments as well. So I'll lay low :)

EDIT2: there was an off by one error inherited from previous code that showed when saving the comments, fixed it! 😅

smarter · 2016-03-08T14:02:52Z

I want to add tests that check the resulting AST using some example code - where should I place these? Do we have similar tests somewhere I can have a look at?

Look in https://github.com/lampepfl/dotty/tree/master/test/test, you might need to extend DottyTest, note that most of these tests haven't been maintained in a long time and may be broken in subtle ways, feel free to clean up/refactor things if it helps you.

felixmulder · 2016-03-08T14:24:35Z

@smarter thanks! I'll have a look later today.

felixmulder · 2016-03-08T20:25:37Z

Traits without a body caused the structure of only keeping one docstring to fail (since the parser would gobble them up before finding out that the trait had no body). So the docstrings are now saved in a queue. Maybe that's what the revComments list was for.

EDIT: fixed 👍

felixmulder · 2016-03-10T15:16:42Z

Good afternoon, I now feel that this PR is nearing a state where it should be evaluated on a first review basis. When first opened, it did not meet the quality criteria and for this I apologize (@DarkDimius).

The tests now show what this implementation can handle, please see: https://github.com/lampepfl/dotty/pull/1151/files#diff-a8ce7031a634f23d40bb1c04c8aeaf38R57

These are the tests performed, and should help you with an overview:

NoComment
SingleClassInPackage
MultipleOpenedOnSingleClassInPackage
MultipleClassesInPackage
SingleCaseClassWithoutPackage
SingleTraitWihoutPackage
MultipleTraitsWithoutPackage
MultipleMixedEntitiesWithPackage
NestedClass
NestedClassThenOuter
Objects
ObjectsNestedClass
PackageObject
MultipleDocStringsBeforeEntity
MultipleDocStringsBeforeAndAfter
ValuesWithDocString
VarsWithDocString
DefsWithDocString
TypesWithDocString

The current heuristic for associating comments is simply selecting the closest docstring (preceding the entity) from a List[List[Comment]] (where the outer list represents the block scope).

Entering and leaving a block scope will, push respectively pop the docstring comments var.

Speaking with Heather, I've been told to ping @odersky (since he is responsible for this part of the compiler).

I hope I'm not stepping on any more toes. Sincerely,
Felix

DarkDimius · 2016-03-10T15:19:16Z

it did not meet the quality criteria and for this I apologize (@DarkDimius).

It's not it wasn't meeting quality criteria. It's that it was failing a specific test so I've tried to tell you what file to try to compile locally to reproduce the issue.

I'll have a look at the PR, though getting feedback from @odersky would be great.

DarkDimius · 2016-03-10T15:20:57Z

src/dotty/tools/dotc/ast/Trees.scala

@@ -317,6 +317,10 @@ object Trees {
    private[ast] def rawMods: Modifiers[T] =
      if (myMods == null) genericEmptyModifiers else myMods

+    private[this] var myComment: Option[String] = None


hmmm, adding new fields in trees is likely to increase memory footprint.
As most trees do not have comments,@felixmulder, would you consider putting comments into attachments of the tree?

Of course - didn't know about attachments. Will look into it 👍

felixmulder · 2016-03-10T18:51:10Z

@DarkDimius the two latest commits contain your suggested changes 👍 thanks for taking a look!

DarkDimius · 2016-03-14T11:54:12Z

Otherwise LGTM. Good job!

felixmulder · 2016-03-14T19:16:03Z

Rebased and fixupped the latest commit to include the change "back" to setComment according to your previous suggestion @DarkDimius.

Let me know if this needs to be squished in order to be merged. Thanks!

DarkDimius · 2016-03-31T12:53:35Z

@felixmulder is this still a WIP branch, or is it ready for review?

felixmulder · 2016-03-31T13:03:24Z

@DarkDimius - ready for review!

This commit fixes errors that would've occurred in this situation: ``` /** Docstring 1 */ <- this one would've been chosen /** Docstring 2 */ /** Docstring 3 */ class Class ``` And this situation: ``` /** Docstring 1 */ trait Trait /** Docstring 2 */ <- this one would've been chosen ```

This commit also adds a printer for use by dottydoc.

odersky · 2016-04-08T14:55:14Z

LGTM, Thanks!

felixmulder force-pushed the topic/wip-docstrings branch from 212c756 to 5d4199a Compare March 7, 2016 18:13

felixmulder force-pushed the topic/wip-docstrings branch 2 times, most recently from 7d7bfa1 to 23529fd Compare March 8, 2016 13:55

felixmulder force-pushed the topic/wip-docstrings branch 2 times, most recently from 4ed74a0 to 8028ec4 Compare March 10, 2016 14:35

DarkDimius reviewed Mar 10, 2016
View reviewed changes

felixmulder force-pushed the topic/wip-docstrings branch from e563b43 to adf0f2e Compare March 10, 2016 21:22

felixmulder force-pushed the topic/wip-docstrings branch from adf0f2e to c0ae291 Compare March 14, 2016 18:17

felixmulder force-pushed the topic/wip-docstrings branch from c0ae291 to 7853ce6 Compare March 20, 2016 20:54

felixmulder changed the title ~~[WIP] Add initial support for raw docstrings in ASTs~~ Add initial support for raw docstrings in ASTs Mar 31, 2016

felixmulder force-pushed the topic/wip-docstrings branch from cdeb00a to f4a543d Compare April 7, 2016 06:39

felixmulder added 3 commits April 7, 2016 08:40

Add initial support for raw docstrings in ASTs

0e6cce6

Add tests for classes and traits

3b4906d

Fix traits w/o bodies not getting the correct docstring

7f20aef

felixmulder added 6 commits April 7, 2016 08:40

Add ability to parse nested classes/traits docstrings

ee54137

Add package object support

3aab803

Add docstring support for types, vals, vars and defs

6c8265c

Change tests to be based on JUnit, rename comments variable

8513dd6

Implement comments as attachments instead of tree members

844683e

felixmulder force-pushed the topic/wip-docstrings branch from f4a543d to f43f520 Compare April 7, 2016 06:56

felixmulder added 2 commits April 7, 2016 08:57

Add binding between Symbol and Untyped tree in base context

f43f520

This commit also adds a printer for use by dottydoc.

Add commandline argument -Ykeep-comments to remove hardcoding

c7730ad

felixmulder changed the title ~~Add initial support for raw docstrings in ASTs~~ Add support for raw docstrings in ASTs Apr 7, 2016

Remove old datastructure for comments

60379c2

dottybot added the reviewed label Apr 8, 2016

odersky merged commit fcf0efe into scala:master Apr 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for raw docstrings in ASTs #1151

Add support for raw docstrings in ASTs #1151

Uh oh!

felixmulder commented Mar 7, 2016

Uh oh!

DarkDimius commented Mar 7, 2016

Uh oh!

felixmulder commented Mar 7, 2016

Uh oh!

smarter commented Mar 8, 2016

Uh oh!

felixmulder commented Mar 8, 2016

Uh oh!

felixmulder commented Mar 8, 2016

Uh oh!

felixmulder commented Mar 10, 2016

Uh oh!

DarkDimius commented Mar 10, 2016

Uh oh!

DarkDimius Mar 10, 2016

Uh oh!

felixmulder Mar 10, 2016

Uh oh!

felixmulder commented Mar 10, 2016

Uh oh!

DarkDimius commented Mar 14, 2016

Uh oh!

felixmulder commented Mar 14, 2016

Uh oh!

DarkDimius commented Mar 31, 2016

Uh oh!

felixmulder commented Mar 31, 2016

Uh oh!

odersky commented Apr 8, 2016

Uh oh!

Uh oh!

Add support for raw docstrings in ASTs #1151

Add support for raw docstrings in ASTs #1151

Uh oh!

Conversation

felixmulder commented Mar 7, 2016

Uh oh!

DarkDimius commented Mar 7, 2016

Uh oh!

felixmulder commented Mar 7, 2016

Uh oh!

smarter commented Mar 8, 2016

Uh oh!

felixmulder commented Mar 8, 2016

Uh oh!

felixmulder commented Mar 8, 2016

Uh oh!

felixmulder commented Mar 10, 2016

Uh oh!

DarkDimius commented Mar 10, 2016

Uh oh!

DarkDimius Mar 10, 2016

Choose a reason for hiding this comment

Uh oh!

felixmulder Mar 10, 2016

Choose a reason for hiding this comment

Uh oh!

felixmulder commented Mar 10, 2016

Uh oh!

DarkDimius commented Mar 14, 2016

Uh oh!

felixmulder commented Mar 14, 2016

Uh oh!

DarkDimius commented Mar 31, 2016

Uh oh!

felixmulder commented Mar 31, 2016

Uh oh!

odersky commented Apr 8, 2016

Uh oh!

Uh oh!