RFR: JDK-8290243: move seeTagToContent from HtmlDocletWriter to TagletWriterImpl [v2]

[Pavel Rappo]

On Mon, 18 Jul 2022 16:14:28 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

>> Please review a pure-cleanup change to move refactor code from `HtmlDocletWriter` to `TagletWriterImpl`.
>> 
>> The methods in question are `seeTagToContent`, and `linkToContent`, which is a partial clone of `seeTagToContent` introduced to handle the `link` markup tag in snippets.
>> 
>> Despite the name, `seeTagToContent` handles both `@see` and `{@link ...}` tags, both of which have a more natural home in `TagletWriterImpl`.
>> 
>> The change is performed in a series of steps/commits:
>> 
>> 1. The methods `seeTagToContent` and `linkToContent` are moved from `HtmlDocletWriter` to `TagletWriterImpl`, with minimal fixup, such as imports, and either qualifying access to members of `HtmlDocletWriter`, or removing qualifiers to members of `TagletWriterImpl`. And, spoiler alert: `linkToContent` eventually goes away altogether.
>> 
>> 2. An unrelated annoying typo in a test was found and fixed. The copyright date is left unchanged and the test is not marked with the bug number for the issue.
>> 
>> 3. `seeTagToContent`, now in `TagletWriterImpl` was split in two, into a "front end", to handle the differences between `@see` and `{@link...}` tags, and a "back end", to handle the common behavior. The API for the back end was informed by the needs of `linkToContent`. Note: there are later refinements to the API for the back end.
>> 
>> 4. The "front end" method, `seeTagToContent` is further split into two separate methods for `{@link ...}` tags and `@see` tags, eliminating the existing ugly `switch` statement. At this time, an "impossible" case was removed from `HtmlDocletWriter.commentTagsToContent`.  The method is for handling text and inline tags, so the `visitSee` method to handle an isolated `@see` tag can never be called in practice. The method is deleted. A future improvement would be to verify that there are no block tags in  the argument list passed to `commentTagsToContent`. Somewhat surprising, this is not a trivial check, and so not done here at this time. It would maybe help to have a new method `isBlockTag` on `DocTree` but that would be a public API changed nd out of scope for this work.
>> 
>> 5. Finally, the signature of the "back end" method, now called `seeLinkReferenceOutput`, is adjusted, and used in `snippetTagOutput` instead of using the original `linkToContent`, which was a partial clone of `seeTagToOutput`, and which can now be deleted.
>> 
>> At this point, it is worth noting that all tests pass without change (the typo fix is unrelated) and the generated API docs  before and after these changes compare the same (apart from the obvious minor differences in timestamps, etc).
>> That being said, there was notable difference in one line of code in `linkToContent` and the corresponding line in `seeTagToContent`.  It corresponds to the `TODO` in the original `HtmlDocletWriter` line 1201.
>> 
>> 
>> // TODO:
>> //                  TypeMirror refType = ch.getReferencedType(target);
>>                     TypeMirror refType = target.asType();
>> 
>> 
>> The change is related to not having a `DocTree target` available in the snippet world. and so the code uses the `TypeMirror` of the referenced element.  If that change is made in the new code, references to raw types, such as `List` get generated with their type parameters, such as `List<T>` ... that's good ... but if the reference in the `DocTree` provided type arguments, such as `List<String>`, then the arguments were lost and the type parameters were given instead ... that is, `List<T>` ... and that's bad. The solution may be to use a hybrid solution ... if the reference in the source contains type arguments, show them, otherwise if the reference is to a raw type, show the type parameters. However, any such solution will require new tests and will affect a number of existing tests, and so is better left to a separate update. The fix for now is just to make the use of the `DocTree` reference optional, so that it need not be supplied when `linkSeeReferenceOutput` is called from `
 snippetTagOutput`.
>
> Jonathan Gibbons has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits:
> 
>  - Merge with upstream/master; resolve conflict
>  - Replace `linkToContent` by using the new `linkSeeReferenceOutput` method
>  - Further split seeTagToContent into linkTagOutput and seeTagOutput
>  - Split seeTagToContent in two: a front end and a back end
>  - Fix typo in test
>  - Move seeTagToContent, linkToContent from HtmlDocletWriter to TagletWriterImpl

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

> 1351:                         content.add(label);
> 1352:                     } else {
> 1353:                         TagletWriterImpl t = getTagletWriterInstance(context.within(node));

Separate to this PR: when I see a cast to or a variable declaration of the `TagletWriterImpl` type, I cannot help but wonder why most of the methods in `TagletWriter` are protected. Do you have any idea how we ended up like that?

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

> 469:                                    String tagName, boolean isLinkPlain,
> 470:                                    Content label,
> 471:                                    BiConsumer<String, Object[]> reportWarning) {

This method is huge: it has 8 parameters and spans 120 lines. We should refactor it in a later PR. For now, consider the below suggestion that changes access level, addresses a few doc typos, and formats parameters one per line.

Suggestion:

    /**
     * Worker method to generate a link from the information in different kinds of tags,
     * such as {@code {@link ...}} tags, {@code @see ...} tags and the {@code link} markup tag
     * in a {@code {@snippet ...}} tag.
     *
     * @param holder        the element that has the documentation comment containing the information
     * @param refTree       the tree node containing the information, or {@code null} if not available
     * @param refSignature  the normalized signature of the target of the reference
     * @param ref           the target of the reference
     * @param tagName       the name of the tag in the source, to be used in diagnostics
     * @param isLinkPlain   {@code true} if the link should be presented in "plain" font,
     *                      or {@code false} for "code" font
     * @param label         the label for the link,
     *                      or an empty content to use a default label derived from the signature
     * @param reportWarning a function to report warnings about issues found in the reference
     *
     * @return the output containing the generated link, or content indicating an error
     */
    private Content linkSeeReferenceOutput(Element holder,
                                           DocTree refTree,
                                           String refSignature,
                                           Element ref,
                                           String tagName,
                                           boolean isLinkPlain,
                                           Content label,
                                           BiConsumer<String, Object[]> reportWarning) {

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

PR: https://git.openjdk.org/jdk/pull/9499