RFR: JDK-8309566: Migrate away from TagletWriter and TagletWriterImpl

[Pavel Rappo]

On Fri, 7 Jul 2023 00:14:43 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 tests are modified, and the JD...

This is good work, Jon. We discussed it with you offline and agreed that it's an incremental step towards simpler jdk.javadoc.

Please update the copyright years and if you change something before pushing, re-run tier1. Thanks!

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/BaseTaglet.java line 63:

> 61:     protected TagletWriter tagletWriter;
> 62: 
> 63:     public BaseTaglet(BaseConfiguration config, DocTree.Kind tagKind, boolean inline, Set<Location> sites) {

Good to see that the taglets now uniformly have useful state (BaseConfiguration). That reminded me of this [prior discussion of ours](https://github.com/openjdk/jdk/pull/10746/commits/1f24f1854d3aa93a77f002a2f2cf561959a98e2f#r1008348685).

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/BaseTaglet.java line 109:

> 107: 
> 108:     /**
> 109:      * Returns whether this taglet accepts a {@code DocTree} node.

Thanks for removing that redundancy!

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/LinkTaglet.java line 38:

> 36: import jdk.javadoc.internal.doclets.toolkit.Content;
> 37: 
> 38: public abstract class LinkTaglet extends BaseTaglet {

`{@link}` has finally got its own taglet. However, I note that `{@code}` has lost its taglet.

In the markup sense, `{@link}` is to `{@code}` as `{@linkplain}` is to `{@literal}`. If you agree with this, then I think we should leave `{@code}`, rather than `{@literal}`.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/TagletManager.java line 190:

> 188:         this.utils = configuration.utils;
> 189:         this.tagletPath = options.tagletPath();
> 190: //        initStandardTaglets();

A leftover?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/TagletWriter.java line 107:

> 105:      * The context in which to generate the output for a series of {@code DocTree} nodes.
> 106:      */
> 107:     public final Context context;

I dislike it when a variable is accessible both directly and through the getter. Nonencapsulated fields like that is harder to debug and maintain. That said, I realise that it's somewhat of a pattern in jdk.javadoc. So, no need to change it at this time; I just make an observation.

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

Marked as reviewed by prappo (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14793#pullrequestreview-1522204109
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1258342101
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1258343051
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1258351572
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1258372499
PR Review Comment: https://git.openjdk.org/jdk/pull/14793#discussion_r1258375179