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.
HTH,
-Joe
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
> 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 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