RFR [15] 8238598: Tighten up com.sun.source.doctree.DocTree API

Jonathan Gibbons jonathan.gibbons at oracle.com
Fri Feb 7 23:15:37 UTC 2020


Pavel,

Thanks for looking at this.

First, some general comments.

1. There is no description in the JBS entry. It should have one, 
especially for a change like this.

2. There's 3 aspects to the changes here.

     i) Fixing bugs or otherwise improving the existing specification.
         This is always welcome, assuming the changes are correct. More 
details later.

     ii) Providing new specification for what was previously missing.
         This is generally welcome, but there may be more than one way 
to do it.

     iii) Moving the implementation of some methods.
         This is the most questionable part of the proposal.


I agree it is an issue that we do not specify the getKind or accept 
methods very well,
but it is not clear to me that adding overriding methods is the best way 
to provide
the documentation, since the new methods "bloat" the overall generated 
documentation
for most of the classses in the package.

For getKind(), the kind of each subtype of DocTree could be narratively 
specified in
the class-level doc comment for each class. For example, for AuthorTree, 
you could add

     The {@link #getKind()} of an {@code AuthorTree} is {@link 
Kind.AUTHOR AUTHOR}.

That would extend to the "combo nodes" like ThrowsTree as follows:

     The {@link #getKind()} of an {@code ThrowsTree} is either
     {@link Kind.EXCEPTION EXCEPTION} or {@link Kind.THROWS THROWS}.

I would add such text after the example, before the @since tag.

You could also add more text to DocTree.getKind:

     For each concrete subtype of {@code DocTree}, {@code getKind} 
returns a value that is
     specific to the subtype.
     For example, {@link AuthorTree#getKind()} returns {@link 
DocTree.Kind.AUTHOR}.

For the "accept" methods, I would suggest that we just provide a more 
detailed
specification on the base method DocTree.accept. I think you could add 
something
like the following, to DocTree.accept:

     For each concrete subtype of {@code DocTree}, the implementation of 
{@code accept}
     calls the appropriate <i>visit</i> method on the provided visitor, 
for which the
     type of the first argument is assignable from the subtype.
     For example, {@link AuthorTree#accept} will return the result of 
calling
         {@link DocTreeVisitor#visitAuthor(AuthorTree, P) 
visitor.visitAuthor(this, data) }.

It is not uncomon in Java/JDK API for a package or type to provide 
high-level general
comments, to impose requirements on types, subtypes, and overriding methods.
For example, some packages state that, "Unless otherwise specified, all 
methods
in all classes in this package will throw NPE if given null arguments."  
Or, look at the
Collections methods to see how they impose requirements on impl classes.

In terms of your specific proposal, I do not particularly like the 
inconsistency in
the documentation of the getKind methods: some use @implSpec, some don't.
I realize the correlation with the "combo types", but splitting the impl 
of the
"combo types" to make the documentation consistent is definitely the 
tail wagging
the dog.

FInally, for this part of the feedback, we could give more narrative 
specification of the
use of the visitor pattern in either the package description for 
com.sun.source.doctree,
or in the class-level docs for DocTree itself, which would further 
reduce the need
for any additional specification on each subtype's getKind and accept 
methods.

------

As for moving the implementations ... the code is not wrong the way it 
is, and
adding default methods to an interface is a potentially incompatible change.
JLS 13.5.6 
https://docs.oracle.com/javase/specs/jls/se13/html/jls-13.html#jls-13.5.6
says this:

> Adding a |default| method, or changing a method from |abstract| to 
> |default|, does not break compatibility with pre-existing binaries, 
> but may cause an |IncompatibleClassChangeError| if a pre-existing 
> binary attempts to invoke the method. This error occurs if the 
> qualifying type, T, is a subtype of two interfaces, I and J, where 
> both I and J declare a |default| method with the same signature and 
> result, and neither I nor J is a subinterface of the other. 

I have some sympathy for declaring a default method for the accept methods,
because that is the only reasonable default implementation in all 
instances,
and if we were talking about writing a new class hierarchy, it might be 
reasonable
to use default methods for accept.  But as much as we might want to go 
back and
rewrite JDK history when new language feature arrive, that's just not 
realistic.
Although default interface methods were added in JDK 8, javac and javadoc
are always constrained to use JDK N-1 language features. and so could not
use default methods when the API was created.

I have less sympathy for moving the impl of the getKind() methods, if only
because they cannot *all* be moved ... some of them depend on impl state.

For me, the bottom line is that if we can fix the *spec* issues, the *impl*
issues are not significant enough to warrant the change, including the 
suggested
need for a CSR. If it ain't broke, don't fix it.

------

Looking at the individual changes ...

I can't decide how much we should improve the examples to be more realistic.
for example, "{@docroot}" is normally only used inside <a 
href="{@docroot}/...">
but maybe this is not the place to get into that level of detail.

Maybe at some point, as a separate pass, we link each node page to the 
corresponding
place in the doc comment spec. All the tags on that page should have 
suitable ids
defined.  This will be easier if/when we get a custom '{@spec}' tag.

DocTree.Kind, ... I see we already link each Kind to its subtype (just, 
not the other
way around!)

DocTreeVisitor ... there are lots of type names that should be in {@code}.
e.g.  "Visits a <type> node".


ErroneousTree: remove "a" in "stand in for a malformed text".

DocSourcePositions:57  "CompilationUnit" -> "the compilation unit"
DocSourcePositions:60 add "the":  "the tree for which a position is sought"

DocTreePath:37  use {@code} around the DocCommentTree type name
DocTreePath:97,98 more type names; use {@code} or convert to English words

(minor, style)
These days, I use IntelliJ to reformat doc comments, to align the 
params, etc

DocTreePath:167  good catch :-)

DCTree, suggest reverting this file and removing the default methods in 
the interfaces.


----------

Summary:

I agree the specification can be improved, but I think it is better to 
drop all the
default methods and fix the specification issues in some other more concise
and/or centralized way that does not require overriding methods in each
subinterface of DocTree.  I think a paragraph in the package doc or DocTree
doc about using the "visitor pattern", switch on "getKind()", or using 
instanceof
would go a long way to reduce any confusion.  I note that you can search
online for info about the visitor pattern in general, so I wouldn't go 
into excess
detail on the basic mechanics of the visitor pattern.

If you'd like me to propose high-level spec updates, I can do that.

I agree with all of your typographic changes, and suggested a few more 
for you.

-- Jon


On 02/07/2020 04:18 AM, Pavel Rappo wrote:
> Hello,
>
> Please review the change for https://bugs.openjdk.java.net/browse/JDK-8238598:
>
>    http://cr.openjdk.java.net/~prappo/8238598/webrev.00/
>
> The crux of the proposed change is implementing and specifying semantically
> dependent methods across the DocTree hierarchy of interfaces.
>
> The design of the com.sun.source.doctree.DocTree API intentionally provides some
> flexibility. Tree nodes can be interacted-with using an OOP style (the Visitor
> design pattern), a structured programming style (boolean expressions on enum
> constants), or a mixed style of `instanceof` checks.
>
> Each subtype of the DocTree interface has these two methods:
>
>      <R, D> R accept(DocTreeVisitor<R, D> visitor, D data);
>      Kind getKind();
>
> To process a tree node, you can implement visitor's visitXYZ methods of interest
> and then pass that visitor to this node's `accept` method. Or, you can first
> check this node's "flavor" explicitly by switching the result returned by
> `getKind()` on a set of enum constants or using a series of `instanceof `
> checks, and then based on this information, cast the node to a more specific
> type.
>
> For this to work, the kind and the type of a node must be firmly linked.
> The expectation is that if there's the `XYZ` type of a tree node,
> it has to return the `XYZe` enum constant from its `getKind` method
> and vice versa [*].
>
> That said, the implementations of the `accept` and `getKind` methods are
> currently deferred until concrete classes, com.sun.tools.javac.tree.DCTree, and
> their specification is absent. The proposed patch tries to address both issues
> by moving the said methods' implementation to the most specific interfaces and
> specifying their behavior. This collocates and ties both methods in a more
> explicit way.
>
> I cannot think of any likely compatibility risks caused by introducing default
> methods in this particular case. Still, if this is approved I will file a CSR.
> I'm also aware that if approved the DocTree API will become less consistent with
> the com.sun.source.tree.Tree API which it has been modeled after. It would be
> nice to get feedback from the original designers of those two APIs.
>
> I couldn't help cleaning up some of the javadoc comments issues along the way.
> This includes typos, formatting, and non-normative clarifications.
>
> Thanks,
> -Pavel
>
> -------------------------------------------------------------------------------
> [*] The mapping from the Kind enum to the subtype of the DocTree interface is
> not bidirectional. Some of the tags are modeled using the same interface.\
> For example, @throws and @exception, @link and @linkplain, @code and @literal,
> share the ThrowsTree, LinkTree, and LiteralTree interfaces respectively. When
> dispatching the call to the visitor, sharing tags use the same method.
> The visitor then might need to resolve the ambiguity by additionally querying
> the kind of the node it visits. For example,
>
>      @Override
>      public Boolean visitLiteral(LiteralTree node, Content c) {
>          String s = node.getBody().getBody();
>          Content content = new StringContent(utils.normalizeNewlines(s));
>      *   if (node.getKind() == CODE)
>              content = HtmlTree.CODE(content);
>          result.add(content);
>          return false;
>      }
>
> Thus, the visitor or the `instanceof` alone is not enough. I'm not sure why it
> was done like that, but as a thought experiment, we could consider what it would
> take to split those 3 pairs of shared tags in a compatible way.
>
> If we were to introduce, say, the `CodeTree` type, we would have to do the
> following (most of the javadoc comments are not included for brevity).
>
> 1. Create a new subinterface of `LiteralTree`:
>
>      public interface CodeTree extends LiteralTree {
>          /**
>           * Returns {@link Kind#CODE}.
>           * @return {@link Kind#CODE}
>           */
>          @Override
>          default Kind getKind() {
>              return Kind.CODE;
>          }
>
>          /**
>           * @implSpec This implementation calls {@code visitor.visitCode(this, data)}.
>           */
>          @Override
>          default <R, D> R accept(DocTreeVisitor<R, D> visitor, D data) {
>              return visitor.visitCode(this, data);
>          }
>      }
>
> Why is `CodeTree` a subinterface of `LiteralTree` rather than a sibling
> (i.e. `CodeTree extends InlineTagTree`)? Because of compatibility with existing
> implementations that might use `instanceof` instead of `getKind()`.
>
> 2. Introduce a new method in `DocTreeVisitor`, which by default forwards the
> calls to the old path:
>
>      @Override
>      default R visitCode(CodeTree node, P data) {
>          return visitLiteral(node, data);
>      }
>
>      

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20200207/cdd000a2/attachment-0001.htm>


More information about the compiler-dev mailing list