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

Joe Darcy joe.darcy at oracle.com
Fri Feb 14 02:43:59 UTC 2020

Hi Pavel,

For context, javax.lang.model and the tree API both date back to circa 
JDK 6 and operated under the same language constraints, being designed 
prior to default methods, unable to use default methods before JDK 9 
because of bootstrapping, and hesitant to aggressively retrofit default 
due to compatibility concerns.

The javax.lang.model package has Element and Type hierarchies each with 
visitors and getKind methods. The element/type to kind mapping is 
similar to the one in doctree, "usually one-to-one, but in a few cases 
one-to-many." We haven't find it necessary so far to explicitly document 
the element/type to kind mapping. I agree with Jon it is bulky to do so 
explicitly in each interface. I'd recommend a table or list documenting 
the mapping in the getKind definition in the root interface.

The accept method implementation would be reasonable in new code, but 
not strictly necessary for a retrofit.



On 2/7/2020 3:15 PM, Jonathan Gibbons wrote:
> 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 
> 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 forhttps://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/20200213/c74c171b/attachment-0001.htm>

More information about the compiler-dev mailing list