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