Scaladoc diagrams (again) #816

Merged
merged 7 commits into from Jul 7, 2012

Conversation

Projects
None yet
5 participants
@VladUreche
Member

VladUreche commented Jul 3, 2012

Review by:
@kzys, @heathermiller

VladUreche and others added some commits Jun 13, 2012

Scaladoc class diagrams part 1
This commit contains model changes required for adding class diagrams
to scaladoc. It also contains an improved implicit shadowing
computation, which hides the shadowed implicitly inherited members
from the main view and gives instructions on how to access them.

This is joint work with Damien Obrist (@damienobrist) on supporting
diagram generation in scaladoc, as part of Damien's semester project
in the LAMP laborarory at EPFL.

The full history is located at:
https://github.com/damienobrist/scala/tree/feature/diagrams-dev

Commit summary:

 - diagrams model
   - diagram settings (Settings.scala, ScalaDoc.scala)
   - diagram model object (Entity.scala, Diagram.scala)
   - model: tracking direct superclasses and subclasses,
     implicit conversions from and to (ModelFactory.scala)
   - diagram object computation (DiagramFactory.scala, DocFactory.scala)
   - capacity to filter diagrams (CommentFactory.scala,
     DiagramDirectiveParser.scala)
   - diagram statistics object (DiagramStats.scala)
   - delayed link evaluation (Body.scala, Comment.scala)
   - tests

 - improved implicits shadowing information
   - model shadowing computation (ModelFactoryImplicitSupport.scala,
     Entity.scala)
   - html generation for shadowing information (Template.scala)
   - tests

Also fixes an issue reported by @dragos, where single-line comment
expansion would lead to the comment disappearing.

Review by @kzys, @pedrofurla.

Adapted to the new model and fixed a couple of problems:
 - duplicate implicit conversions in StringAdd/StringFormat
 - incorrect implicit conversion signature (from X to X)

Conflicts:

	src/compiler/scala/tools/nsc/doc/Settings.scala
	src/compiler/scala/tools/nsc/doc/html/page/Template.scala
	src/compiler/scala/tools/nsc/doc/model/Entity.scala
	src/compiler/scala/tools/nsc/doc/model/ModelFactory.scala
	src/compiler/scala/tools/nsc/doc/model/ModelFactoryImplicitSupport.scala
Documented SyncVar
Since we used it in the DocRunner and noticed it could have better
documentation.

Review by @heathermiller.
Scaladoc class diagrams part 2
This commit contains the svg diagram generation using the graphviz
package, the template changes, the css styling and javascript code
that enables displaying and interacting with the diagrams.

The full history is located at:
https://github.com/damienobrist/scala/tree/feature/diagrams-dev

The diagrams are included as svg markup inside the html code. This
enables interacting with the image beyond what would be possible
with a static image (highlighting, scaling, tooltips, links to
nodes, etc).

The svg generation has four main phases: model => dot,
dot => svg (using the graphviz package), svg postprocessing,
inclusion in the html page.

This commit also fixes SI-5212 - links to individual pages
automatically load the left navigation panel of the website.

Commit summary:

 - diagram generation
   - model => dot (DotDiagramGenerator.scala, DiagramGenerator.scala)
   - dot => svg (DotRunner.scala)
   - svg post-processing (DotDiagramGenerator.scala)
   - svg inclusion in the html (Template.scala)

 - diagram interaction
   - css, js and image files

Review by @heathermiller, @kzys.

Also fixed the memory leak that was causing the testsuite to timeout.
Scaladoc diff-firendly output
Scaladoc can create raw content files that we can easily diff and spot
any modifications. There is a cool project by Stefan Zeiger to export
the scaladoc model in JSON, but with the language and scaladoc being so
quick to evolve, it'll be a pain to properly maintain. In the long-run,
the plan is to sample a couple of raw files on each build and email me
the diff. If I spot anything that may be wrong I can fix it, revert the
commit or at least file a bug.

For now, .html.raw files are generated on-demand, using
  ant -Dscaladoc.raw.output="yes" <targets>

Also added a script that will do the job of diff-ing.

Review by @jsuereth.

Conflicts:

	src/compiler/scala/tools/nsc/doc/Settings.scala
Reorganized scaladoc model
Since the old model was "interruptible", it was prone to something
similar to race conditions -- where the model was creating a template,
that template creating was interrupted to creat another template, and
so on until a cycle was hit -- then, the loop would be broken by
returning the originally not-yet-finished template.

Now everything happens in a depth-first order, starting from root,
traversing packages and classes all the way to members. The previously
interrupting operations are now grouped in two categories:
 - those that were meant to add entities, like inheriting a class from
a template to the other (e.g. trait T { class C }; trait U extends T)
=> those were moved right after the core model creation
 - those that were meant to do lookups - like finding the companion
object -- those were moved after the model creation and inheritance
and are not allowed to create new documentable templates.

Now, for the documentable templates we have:

DocTemplateImpl - the main documentable template, it represents a
                  Scala template (class, trait, object or package).
                  It may only be created when modelFinished=false by
                  methods in the modelCreation object
NoDocTemplateMemberImpl - a non-documented (source not present)
                          template that was inherited. May be used as
                          a member, but does not get its own page
NoDocTemplateImpl - a non-documented (source not present) template
                    that may not be used as a member and does not
                    get its own page

For model users: you can use anything in the ModelFactory trait at
will, but not from the modelCreation object -- that is reserved for the
core model creation and using those functions may lead to duplicate
templates, invalid links and other ugly problems.
Diagram tweaks #1
- relaxed the restrictions on nodes - nodes can be classes, traits and
objects, both stand-alone and companion objects -- all are added to the
diagram, but usually companion objects are filtered out as they don't
have any superclasses
- changed the rules for default diagram creation:
  - classes and traits (and AnyRef) get inheritance diagrams
  - packages and objects get content diagrams
(can be overridden by @contentDiagram [hideDiagram] and
@inheritanceDiagram [hideDiagram])
- tweaked the model to register subclasses of Any
- hardcoded the scala package diagram to show all relations
- enabled @contentDiagram showInheritedNodes by default and changed
the setting to hideInheritedNodes (and added a test for this)
- better node selection (can select nodes that don't have a
corresponding trait)
- fixed the docsite link in member selection, which was broken since
the first commit :))
Diagram tweaks #2
- fixed the AnyRef linking (SI-5780)
- added tooltips to implicit conversions in diagrams
- fixed the intermittent dot error where node images would be left out
(dot is not reliable at all -- with all the mechanisms in place to fail
gracefully, we still get dot errors crawling their way into diagrams -
and that usually means no diagram generated, which is the most
appropriate way to fail, I think...)
@VladUreche

This comment has been minimized.

Show comment
Hide comment
@heathermiller

heathermiller Jul 5, 2012

Collaborator

Just want to point out that the advice and explanation that you give here (especially the advice at the bottom) should be moved to the contributors page for Scaladoc (Manohar already wrote most of it up) to be included either as a separate page under /contribute on the new scala-lang, or included in the forthcoming Scaladoc guide.

Collaborator

heathermiller replied Jul 5, 2012

Just want to point out that the advice and explanation that you give here (especially the advice at the bottom) should be moved to the contributors page for Scaladoc (Manohar already wrote most of it up) to be included either as a separate page under /contribute on the new scala-lang, or included in the forthcoming Scaladoc guide.

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/401/

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment

jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/401/

@ghost ghost assigned heathermiller Jul 3, 2012

+ // if there are too many super / sub / implicit nodes, represent
+ // them by on node with a corresponding tooltip
+ superClasses = if (_superClasses.length > settings.docDiagramsMaxNormalClasses.value) {
+ val superClassesTooltip = Some(limitSize(_superClasses.map(_.tpe.name).mkString(", ")))

This comment has been minimized.

@heathermiller

heathermiller Jul 4, 2012

Member

Note to self: we should change this to a modal box with links instead.

@heathermiller

heathermiller Jul 4, 2012

Member

Note to self: we should change this to a modal box with links instead.

+ if (dotRestarts == settings.docDiagramsDotRestart.value) {
+ settings.printMsg("\n")
+ settings.printMsg("**********************************************************************")
+ settings.printMsg("Diagrams will be disabled for this run beucause the graphviz dot tool")

This comment has been minimized.

@heathermiller

heathermiller Jul 5, 2012

Member

beucause? (Sorry for commenting on typos, but this one actually gets printed)

@heathermiller

heathermiller Jul 5, 2012

Member

beucause? (Sorry for commenting on typos, but this one actually gets printed)

This comment has been minimized.

@VladUreche

VladUreche Jul 5, 2012

Member

Fixed, will be there in the next pull request batch.

@VladUreche

VladUreche Jul 5, 2012

Member

Fixed, will be there in the next pull request batch.

@heathermiller

This comment has been minimized.

Show comment
Hide comment
@heathermiller

heathermiller Jul 5, 2012

Collaborator

Ok, really sorry to nitpick here, but please name it inOriginalOwner (correctly) instead! Not everyone uses an IDE and this could be annoying :(

Ok, really sorry to nitpick here, but please name it inOriginalOwner (correctly) instead! Not everyone uses an IDE and this could be annoying :(

This comment has been minimized.

Show comment
Hide comment
@VladUreche

VladUreche Jul 5, 2012

Owner

Done :)

Owner

VladUreche replied Jul 5, 2012

Done :)

-trait Constraint {
- // /** The implicit conversion during which this constraint appears */
- // def conversion: ImplicitConversion
+/** Shadowing captures the information that the member is shadowed by some other members

This comment has been minimized.

@heathermiller

heathermiller Jul 5, 2012

Member

Excellent!!

@heathermiller

heathermiller Jul 5, 2012

Member

Excellent!!

@@ -72,25 +75,28 @@ class SyncVar[A] {
}
// TODO: this method should be private
- // [Heather] the reason why: it doesn't take into consideration

This comment has been minimized.

@heathermiller

heathermiller Jul 5, 2012

Member

Thanks for fixing the whitespace problems in this file :)

@heathermiller

heathermiller Jul 5, 2012

Member

Thanks for fixing the whitespace problems in this file :)

@heathermiller

This comment has been minimized.

Show comment
Hide comment
@heathermiller

heathermiller Jul 5, 2012

Member

Thanks a lot Vlad! LGTM
We still have TODOs before 2.10, mostly regarding usability, but this looks really excellent! Nice going on better-organizing the model, the way you organized it makes a lot of sense.

Member

heathermiller commented Jul 5, 2012

Thanks a lot Vlad! LGTM
We still have TODOs before 2.10, mostly regarding usability, but this looks really excellent! Nice going on better-organizing the model, the way you organized it makes a lot of sense.

@heathermiller

This comment has been minimized.

Show comment
Hide comment
@heathermiller

heathermiller Jul 5, 2012

Member

Out of curiosity, could you point me to what you did to solve ticket SI-5212? (I ask because if there's an automatic js redirect, one could wind up in a mess where there are iframes embedded within iframes embeded within iframes, etc...)

Member

heathermiller commented Jul 5, 2012

Out of curiosity, could you point me to what you did to solve ticket SI-5212? (I ask because if there's an automatic js redirect, one could wind up in a mess where there are iframes embedded within iframes embeded within iframes, etc...)

@heathermiller

This comment has been minimized.

Show comment
Hide comment
@heathermiller

heathermiller Jul 5, 2012

Member

Also, sure, the raw output looks like a more desirable way to test. However, could you clarify what workflow-change that you mean in the commit message (in 44ec110) when you say,

"There is a cool project by Stefan Zeiger to export
the scaladoc model in JSON, but with the language and scaladoc being so
quick to evolve, it'll be a pain to properly maintain. In the long-run,
the plan is to sample a couple of raw files on each build and email me
the diff. If I spot anything that may be wrong I can fix it, revert the
commit or at least file a bug."

Member

heathermiller commented Jul 5, 2012

Also, sure, the raw output looks like a more desirable way to test. However, could you clarify what workflow-change that you mean in the commit message (in 44ec110) when you say,

"There is a cool project by Stefan Zeiger to export
the scaladoc model in JSON, but with the language and scaladoc being so
quick to evolve, it'll be a pain to properly maintain. In the long-run,
the plan is to sample a couple of raw files on each build and email me
the diff. If I spot anything that may be wrong I can fix it, revert the
commit or at least file a bug."

@VladUreche

This comment has been minimized.

Show comment
Hide comment
@VladUreche

VladUreche Jul 5, 2012

Member

Regarding ticket SI-5212: Damien fixed it, the change is here: https://github.com/damienobrist/scala/commit/5efb003605bbe86d91a424e96b88cbe901655f4e

Member

VladUreche commented Jul 5, 2012

Regarding ticket SI-5212: Damien fixed it, the change is here: https://github.com/damienobrist/scala/commit/5efb003605bbe86d91a424e96b88cbe901655f4e

@VladUreche

This comment has been minimized.

Show comment
Hide comment
@VladUreche

VladUreche Jul 5, 2012

Member

Raw output has 2 main drawbacks:

  • it depends on the html page that gets generated, so any layout change significantly changes the raw output (remember the HTML tests? They test the raw output, and any layout change breaks them)
  • it won't capture all the aspects of the model, such as the signatures that entities receive or the diagram contents. (as they're embedded in html elements)

On the other hand, hard-coding an export in JSON is not going to work well either, as there's no automated way of outputting JSON from the entities -- and doing it by hand is error-prone and hard to maintain.

All in all, outputting the raw pages helps a lot when you make model changes without touching the layout -- you'll easily spot a missing/extra member or a missing class, which otherwise would have gone unnoticed, but unfortunately that's the only scenario where it's really useful.

Member

VladUreche commented Jul 5, 2012

Raw output has 2 main drawbacks:

  • it depends on the html page that gets generated, so any layout change significantly changes the raw output (remember the HTML tests? They test the raw output, and any layout change breaks them)
  • it won't capture all the aspects of the model, such as the signatures that entities receive or the diagram contents. (as they're embedded in html elements)

On the other hand, hard-coding an export in JSON is not going to work well either, as there's no automated way of outputting JSON from the entities -- and doing it by hand is error-prone and hard to maintain.

All in all, outputting the raw pages helps a lot when you make model changes without touching the layout -- you'll easily spot a missing/extra member or a missing class, which otherwise would have gone unnoticed, but unfortunately that's the only scenario where it's really useful.

@heathermiller

This comment has been minimized.

Show comment
Hide comment
@heathermiller

heathermiller Jul 6, 2012

Member

That wasn't the question I asked. I asked about the workflow. You're proposing ambiguous changes to the workflow-- you want to "sample raw files" and hope that people will manually email you diffs? Sounds like it wouldn't really work in practice if arbitrary people (in LAMP/Typesafe or the community) are supposed to know that they're supposed to be personally emailing you diffs from time to time on some arbitrary sampling of files. And it doesn't sound very thorough if you expect sampling a few files to find errors for you. (Think about the errors you found which lead to you wanting to add DocTemplateImpl, NoDocTemplateMemberImpl and NoDocTemplateImpl. There were only a handful of places where some synthetically-generated entity didn't have a page that should've had one, right? I don't think that sampling a small number of files would have ever enabled you to find these.)

It's also just wrong, in the interest of a productive and healthy workflow, to have to update a bunch of failing tests each time you make a trivial change to anything in scaladoc.

That said, I'm still fully against scaladoc tests, and I don't think this solution really solves much (it helps in probably 10% of cases, max). It's also not what we all discussed and agreed to a while back. I still think that the way tests are handled doesn't make sense at all-- this is evident in the somewhat accepted behavior of just disabling or commenting-out failing scaladoc tests when they pop up.

You also didn't explain how you believe JSON is error prone. I don't agree at all (you stated it twice, but didn't validate why you think it's more error-prone than plain text). If you're saying that the language is changing too fast, and that we might add a keyword or something that would require someone to have to change the JSON parser in scaladoc, then that's not a very strong argument. This is actually a scenario where it's not totally blasphemous to manually update three dozen failing tests by hand (rather than how we currently do it-- manually update three dozen failing tests by hand each time you touch scaladoc).

All in all, outputting the raw pages helps a lot when you make model changes without touching the layout -- you'll easily spot a missing/extra member or a missing class, which otherwise would have gone unnoticed, but unfortunately that's the only scenario where it's really useful.

How realistic is this to have to run every night and on every check-in build? It sounds worthwhile only when you, Vlad, are fixing the model (as I'm unaware of anyone else these days who touches the model). It sounds like a really bad idea to run these so often, though, as you're essentially forcing everyone who might touch scaladoc to have to deal with tests that are all-too-sensitive about anything else except for the model that only you touch. That doesn't make sense. This makes sense for manual testing (you on your own), but not nightly and frequent automatic tests.

You're shooting yourself in the foot if you want scaladoc contributors from the community. Why? 95% of people contributing to scaladoc will end up having to touch the front-end, where you'll force people to have to deal with a billion inflexible tests failing.

Member

heathermiller commented Jul 6, 2012

That wasn't the question I asked. I asked about the workflow. You're proposing ambiguous changes to the workflow-- you want to "sample raw files" and hope that people will manually email you diffs? Sounds like it wouldn't really work in practice if arbitrary people (in LAMP/Typesafe or the community) are supposed to know that they're supposed to be personally emailing you diffs from time to time on some arbitrary sampling of files. And it doesn't sound very thorough if you expect sampling a few files to find errors for you. (Think about the errors you found which lead to you wanting to add DocTemplateImpl, NoDocTemplateMemberImpl and NoDocTemplateImpl. There were only a handful of places where some synthetically-generated entity didn't have a page that should've had one, right? I don't think that sampling a small number of files would have ever enabled you to find these.)

It's also just wrong, in the interest of a productive and healthy workflow, to have to update a bunch of failing tests each time you make a trivial change to anything in scaladoc.

That said, I'm still fully against scaladoc tests, and I don't think this solution really solves much (it helps in probably 10% of cases, max). It's also not what we all discussed and agreed to a while back. I still think that the way tests are handled doesn't make sense at all-- this is evident in the somewhat accepted behavior of just disabling or commenting-out failing scaladoc tests when they pop up.

You also didn't explain how you believe JSON is error prone. I don't agree at all (you stated it twice, but didn't validate why you think it's more error-prone than plain text). If you're saying that the language is changing too fast, and that we might add a keyword or something that would require someone to have to change the JSON parser in scaladoc, then that's not a very strong argument. This is actually a scenario where it's not totally blasphemous to manually update three dozen failing tests by hand (rather than how we currently do it-- manually update three dozen failing tests by hand each time you touch scaladoc).

All in all, outputting the raw pages helps a lot when you make model changes without touching the layout -- you'll easily spot a missing/extra member or a missing class, which otherwise would have gone unnoticed, but unfortunately that's the only scenario where it's really useful.

How realistic is this to have to run every night and on every check-in build? It sounds worthwhile only when you, Vlad, are fixing the model (as I'm unaware of anyone else these days who touches the model). It sounds like a really bad idea to run these so often, though, as you're essentially forcing everyone who might touch scaladoc to have to deal with tests that are all-too-sensitive about anything else except for the model that only you touch. That doesn't make sense. This makes sense for manual testing (you on your own), but not nightly and frequent automatic tests.

You're shooting yourself in the foot if you want scaladoc contributors from the community. Why? 95% of people contributing to scaladoc will end up having to touch the front-end, where you'll force people to have to deal with a billion inflexible tests failing.

@VladUreche

This comment has been minimized.

Show comment
Hide comment
@VladUreche

VladUreche Jul 6, 2012

Member

All I'm saying is I want a separate jenkins job that periodically generates the raw output, does a diff for a couple of important pages (like List) and emails it to me. It won't fail any tests on anyone, it will just enable me to check that the model changes didn't break anything. One example: when Simon added backticks everywhere, he completely broke the usecase types and linking -- if I could have seen the extent of the damage I could have reacted quicker to either adapt the model or revert his commit. But if I can't get a bird's eye view of what changed you can't expect me to properly fix it, no? And for you not to worry about it: If layout changes between two job runs, I can just ignore the diff, as long as no model changes took place.

Regarding JSON -- how would you do it? Would you use an automated tool based on reflection, or would you code it up yourself? If (1), you'd be exposing internal fields of the _Impl classes, that's not what you want to test. If (2), you'd need to keep the export code in sync with the model _by hand*, and that's error prone -- who can check you exported all the fields correctly?

Regarding commenting out scaladoc tests:

  • the html tests will be gone at some point, I just don't want to spend two days on rewriting them now (If you have such strong feelings about them, why not spend some time translating a couple of them to model tests, I did that whenever I had failing html tests)
  • commenting out a test, either in scaladoc or anywhere else should not be done. That's not to say it hasn't been done before, because people just don't want to spend the time figuring out what's failing, but that's not something we should accept, especially since we have the pull request mechanism.
Member

VladUreche commented Jul 6, 2012

All I'm saying is I want a separate jenkins job that periodically generates the raw output, does a diff for a couple of important pages (like List) and emails it to me. It won't fail any tests on anyone, it will just enable me to check that the model changes didn't break anything. One example: when Simon added backticks everywhere, he completely broke the usecase types and linking -- if I could have seen the extent of the damage I could have reacted quicker to either adapt the model or revert his commit. But if I can't get a bird's eye view of what changed you can't expect me to properly fix it, no? And for you not to worry about it: If layout changes between two job runs, I can just ignore the diff, as long as no model changes took place.

Regarding JSON -- how would you do it? Would you use an automated tool based on reflection, or would you code it up yourself? If (1), you'd be exposing internal fields of the _Impl classes, that's not what you want to test. If (2), you'd need to keep the export code in sync with the model _by hand*, and that's error prone -- who can check you exported all the fields correctly?

Regarding commenting out scaladoc tests:

  • the html tests will be gone at some point, I just don't want to spend two days on rewriting them now (If you have such strong feelings about them, why not spend some time translating a couple of them to model tests, I did that whenever I had failing html tests)
  • commenting out a test, either in scaladoc or anywhere else should not be done. That's not to say it hasn't been done before, because people just don't want to spend the time figuring out what's failing, but that's not something we should accept, especially since we have the pull request mechanism.
@VladUreche

This comment has been minimized.

Show comment
Hide comment
@VladUreche

VladUreche Jul 7, 2012

Member

A note to scala gatekeepers (@adriaanm @jsuereth and @paulp, I guess) -- this pull request got it's confirmation from Heather and we should really include it in M5. So please MergeThxBye this patch before M5!
KThxBye!

Member

VladUreche commented Jul 7, 2012

A note to scala gatekeepers (@adriaanm @jsuereth and @paulp, I guess) -- this pull request got it's confirmation from Heather and we should really include it in M5. So please MergeThxBye this patch before M5!
KThxBye!

jsuereth added a commit that referenced this pull request Jul 7, 2012

@jsuereth jsuereth merged commit 7ca925e into scala:master Jul 7, 2012

VladUreche added a commit to VladUreche/scala that referenced this pull request Jul 16, 2012

Scaladoc minor fix: Typos in diagrams
As suggested by Heather in pull request #816.

@scabug scabug referenced this pull request in scala/bug Apr 7, 2017

Closed

Disappearing member descriptions #5916

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment