RFR: JDK-8309566: Migrate away from TagletWriter and TagletWriterImpl [v3]

Wed Jul 12 14:09:17 UTC 2023

On Tue, 11 Jul 2023 17:01:38 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

>> Please review a medium-substantial cleanup of the internal `Taglet` world.
>> 
>> The initial motivation was to move tag-specific functionality from `TagletWriter[Impl]` to HTML-specific new subtypes of the individual `Taglet` classes, so that taglets are now better represented by a format-neutral base type and an HTML-specific subtype. The new subtypes are collected in a new `formats.html.taglets` package, and for the most part, they are accessed via their `Taglet` API.
>> 
>> A secondary motivation is to clean up handling of inline tags. Not all inline tags had corresponding taglets: most notably, `{@link}` was not modeled by a taglet.  Introducing `[Html]LinkTaglet` allowed more code to move from `TagletWriterImpl` to `HtmlLinkTaglet` ... and `HtmlSeeTaglet` and `HtmlSnippetTablet` now delegate to `HtmlLinkTaglet` to generate links. Also, in `HtmlDocletWriter` a notable visitor, in `commentTags toContent` had explicit `visit...` methods that effectively duplicated the functionality of `defaultAction`, so those methods can be and have been deleted.
>> 
>> Taglets used to be stateless, even though they are generally created once per doclet. (That was fixed, now, they _are_ created just once per doclet.) They now contain the standard long-lived members, like `configuration`, `utils`, `messages`, etc for the convenience of subtypes.  `TagletWriterImpl.Context` has always been effectively format-neutral and has been moved up to `Taglet.Context`.  
>> 
>> It had been hoped to replace the `TagletWriter` parameter to `Taglet::getInlineTagOutput` and `Taglet::getAllBlockTagOutput` with `Taglet.Context` perhaps calling with a HTML-subtype instance. But there is still enough useful functionality on `TagletWriter` that that is not practical at this time.
>> 
>> Taglets vary greatly in size, from small/trivial to large/complex. While it might seem unnecessary to use top-level classes for the small case, it seems better to go with a consistent uniform design rather than try and reduce any perceived overhead, perhaps by selectively using nested classes, as is often the case elsewhere in `jdk.javadoc` and `jdk.compiler`. Grouping the new `HTML...Taglet` classes in a new `formats.html.taglets` package seems like a good compromise.
>> 
>> **Note**: this is just "cleanup" and refactoring. There is no intentional change to any functionality, nor any added or removed.  If code appears to have been "deleted" it has either been moved elsewhere, or was effectively unused anyway. No test...
>
> Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improve comments
>   Convert `InheritableTaglet` to just a marker interface.

I started adding "nits" on extra or missing newlines, but then realised it will introduce much noise. Could you please inspect the changed files for those newlines and also update the copyright years as appropriate? Thanks!

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlConfiguration.java line 440:

> 438:             }
> 439:         }
> 440: 

Nit: delete this line.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 1391:

> 1389:                 @Override
> 1390:                 protected Boolean defaultAction(DocTree node, Content content) {
> 1391:                     if (node instanceof InlineTagTree) {

As far as I understand, the deleted `visit{DocRoot, Index, Link, Literal, Summary, SystemProperty}` are routed here (i.e. `defaultAction`).

While I'm not against it, that `instanceof` seems unnecessary; did you add it for readability?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/TagletWriterImpl.java line 1:

> 1: /*

It's nice to see TagletWriterImpl go! It's not that I didn't like this class, it's that I didn't like the fact that the processing logic was haphazardly spread between this class and the taglets.

That said, there's more cleanup that needs to be done in TagletWriter, later.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/taglets/BaseTaglet.java line 81:

> 79:         this.inline = inline;
> 80:         this.sites = sites;
> 81: 

Nit: delete this line.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/taglets/DeprecatedTaglet.java line 93:

> 91:     }
> 92: 
> 93: }

Nit: add newline.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/taglets/IndexTaglet.java line 67:

> 65:         return tagletWriter.createAnchorAndSearchIndex(element, tagText, descText, tag);
> 66:     }
> 67: 

Nit: delete this line.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/taglets/InheritDocTaglet.java line 57:

> 55:      * Construct a new InheritDocTaglet.
> 56:      */
> 57:     public InheritDocTaglet(HtmlConfiguration config) {

Could have package access instead.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/taglets/SummaryTaglet.java line 44:

> 42: public class SummaryTaglet extends BaseTaglet {
> 43: 
> 44:     public SummaryTaglet(HtmlConfiguration config) {

This constructor could have package access similarly to those of other taglets.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/taglets/ValueTaglet.java line 127:

> 125:     }
> 126: 
> 127: 

Nit: delete this line.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/taglets/package-info.java line 37:

> 35:  * context. In general, inline tags always generate some (non-empty) output,
> 36:  * even if the output is some form indicating an error. It is almost never
> 37:  * correct to not generate any output to fill the between the parts of the

> fill the between the parts

Is it just me or it reads weird?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/doclets.properties line 27:

> 25: 
> 26: doclet.Generating_0=Generating {0}...
> 27: doclet.Toolkit_Usage_Violation=The Doclet Toolkit can only be used by {0}

Should we remove this key for other locales too?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 2120:

> 2118: 
> 2119:     public List<? extends DocTree> getBlockTags(Element element, String tagName) {
> 2120:         return getBlockTags(element,

For this or a later PR:

1. Even prior to this PR that `instanceof` seemed redundant. If we don't want to change the `getBlockTags(Element, Predicate)` return type to BlockTagTree, I suggest to downcast unconditionally:

        t -> ((BlockTagTree) t).getTagName().equals(tagName));

2. After this PR, the only client of the BaseTaglet.accepts method would be SimpleTaglet, which we should move that method to.

-------------

Changes requested by prappo (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14793#pullrequestreview-1525864677
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1261213211
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1261085256
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1261068881
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1261081510
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1261229264
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1261225513
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1261227232
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1261207886
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1261212560
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1261202272
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1261064410
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1261002952