RFR: [15] JDK-8235947: Cleanup/simplify Utils.getBlockTags

[Pavel Rappo]

Jon,

> On 16 Dec 2019, at 19:56, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
> 
> On 12/16/19 11:45 AM, Pavel Rappo wrote:
>> Jon,
>> 
>>> On 16 Dec 2019, at 19:00, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
>>> 
>>> On 12/16/2019 10:44 AM, Pavel Rappo wrote:
>>>> Jon,
>>>> 
>>>> The idea of the fix looks reasonable. Since I'm new to javadoc, I have more questions than would
>>>> probably be expected during a typical review.
>>>> 
>>>> 1. Could you help me understand the asymmetry in BaseTaglet?
>>>> 
>>>>     public boolean accepts(DocTree tree) {
>>>>         return (tree.getKind() == DocTree.Kind.UNKNOWN_BLOCK_TAG
>>>>                     && tagKind == DocTree.Kind.UNKNOWN_BLOCK_TAG)
>>>>                 ? ((UnknownBlockTagTree) tree).getTagName().equals(name)
>>>>                 : tree.getKind() == tagKind;
>>>>     }
>>>> 
>>>> Why does this method check the pair (kind, name) for the case of UNKNOWN_BLOCK_TAG, but only checks
>>>> the name in any other case, including the case where kind == UNKNOWN_INLINE_TAG?
>>> The DocTree API has specific nodes, with a specific kind, for all "standard" nodes, but the API also
>>> has to cope with additional/unknown/non-standard nodes.  These are (all) handled by
>>> UnknownBlockTagTree, which contains the name of the non-standard tag.
>>> 
>>> Thus, for standard tags, it is sufficient to check the kind; for non-standard tags, represented by
>>> instances of UnknownBlockTagTree, you need to check the name stashed in the UnknownBlockTagTree
>>> node.
>> Thanks, I got the idea. Still, I'll have to read the code further to understand the behavior
>> specific to UNKNOWN_INLINE_TAG.
> 
> 
> UNKNOWN_INLINE_TAG obviously exists for the same reason. Ideally, there would just be UNKNOWN_TAG,
> but for better or worse, we went down the path of supertypes for BlockTagTree and InlineTagTree.
> With 20/20 hindsight, it's not clear those supertypes have been very helpful, compared to (say)
> having methods "isInline" and "isBlock".

Okay, thanks.

>>>> 2. I think we could use contravariant generic parameter in the predicate in Utils:
>>>> 
>>>> -    public List<? extends DocTree> getBlockTags(Element element, Predicate<DocTree> filter) {
>>>> +    public List<? extends DocTree> getBlockTags(Element element, Predicate<? super DocTree> filter) {
>>> Since there are no useful/interesting supertypes of the DocTree interface, I don't see that it is
>>> necessary or helpful to use super-wildcards.
>> Fair enough. I think it was a knee-jerk reaction on my part. I usually add things like this because
>> it costs nothing to the API client, but adds some extra comfort to user experience.
> ... except that later on, you're also suggesting we avoid them ;-)

That's different! It shifts the burden of dealing with the consumed generics from the client to the
implementor of the method. Since you just pass (delegate) those generics on to Stream.filter, you
don't have to do anything. The ultimate recipient is the java.util.stream API.

>>>> On a related note. I noticed that com.sun.source.doctree.DocCommentTree uses bounded wildcards in
>>>> the return types of its methods (e.g. List<? extends DocTree>). From a caller's perspective,
>>>> List<DocTree> has the same usability as List<? extends DocTree>, if the caller agrees to only
>>>> consume the elements from the list and to never add them there. Which I think was the intent here.
>>>> 
>>>> Unless I'm mistaken, the only reason to use bounded wildcards in the return types is to design for
>>>> covariant returns in subtypes. For example,
>>>> 
>>>>     interface MyDocTree extends DocTree {
>>>>         ...
>>>>         List<? extends BlockTagTree> getBlockTags();
>>>>         ...
>>>>     }
>>>> 
>>>> We don't seem to have those in javadoc. At the same time we're paying a small boilerplate fee for
>>>> this--unused flexibility--each and every time one of those methods is called. What's worse is that
>>>> this has a ripple effect, causing long generified lines to appear far beyond those methods' call
>>>> sites.
>>>> 
>>>> I guess what I'm saying is that we could look into simplifying that on the javadoc side (not the
>>>> javac side).
>>> a) we do use the ability to return lists of subtypes
>> Do we use it in the javadoc?
> Probably only indirectly, in cases where we want to propagate results from public API that does
> use the wildcard.  I know there have been times when I've tried to go simple, and the compiler
> has rejected the code.
> 
> In terms of code-style for internal API, I don't think it's necessary to always use wildcards without
> due consideration of the need and context.

I agree on that last bit.

>>> b) this is a public API that now cannot reasonably be changed
>> Which API is that? I might not have expressed myself clearly. I'm talking about the javadoc code
>> only. Not about the contents of the com.sun.source.doctree package in jdk.compiler.
> OK, sorry, misunderstood a bit, here.
>> 
>> Anyhow, I guess some of those overly verbose call sites could be treated with "var".
> 
> Yeah, I haven't yet bought in to using "var" but that may just be a Luddite reaction.
> I find it messy/inconsistent when you have nearby code some of which can use var
> and some of which cannot.

It's a separate concern for now.

Thanks for patient explanations! Looks good to me.

>>>>> On 14 Dec 2019, at 02:17, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
>>>>> 
>>>>> Please review a moderately simple cleanup to the implementation(s) of Utils.getBlockTags.
>>>>> 
>>>>> The existing code is unnecessarily string-oriented, and can be improved by better leveraging DocTree.Kind, especially by updating each subtype of BaseTaglet to know its associated DocTree.Kind.
>>>>> 
>>>>> The core of the fix is Utils, with additional support in BaseTaglet and SimpleTaglet. The other changes are derivative changes using the new API.
>>>>> 
>>>>> There are more changes possible in this (general) area. For example, there are similar methods such as Utils.hasBlockTag, and methods like CommentHelper.getTagName. At a minimum, it may be reasonable to co-locate all these methods in a new "Tags" utility class, but it is also worth investigating what additional simplifications can be made. But for now, this is a good checkpoint.
>>>>> 
>>>>> The old code accidentally covered up a pre-existing bug, which was exposed in the replacement code. The old code did not return @uses and @provides from getBlockTags, and so they did not not to be skipped as part of the main comment in ModuleWriterImpl.  Now they are returned by getBlockTags, and so need to be skipped in TagletWriter.
>>>>> 
>>>>> This is all cleanup with no changes in the generated output. There are no new tests and no changes needed to any existing tests. A full comparison against a reference JDK was done with the standard JDK docs (make docs) and with all the output from all the jtreg javadoc tests.
>>>>> 
>>>>> -- Jon
>>>>> 
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8235947
>>>>> Webrev:  http://cr.openjdk.java.net/~jjg/8235947/webrev.00/
>>>>>