RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]

Mon Jun 6 21:12:02 UTC 2022

On Sun, 5 Jun 2022 20:55:12 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only copies one instance of the specified exception.
>
> Pavel Rappo has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 33 commits:
> 
>  - Merge branch 'master' into 8287333
>    
>    This resolves a conflict in ParamTaglet.
>  - Clean up if-branch
>  - Remove upper-bounded wildcard
>    
>    This change simplifies code without any disadvantages:
>    
>      * Those `List<? extends XTree>` are read-only
>      * An argument of the `List<XTree>` type can still be passed to a `List<? extends XTree>` parameter
>  - Simplify inheritThrowsDocumentation
>  - Reuse more specific variable
>  - Merge branch 'master' into 8287333
>  - Incremental update
>    
>    - Renames local variables and method parameters
>    - Improves comments
>    - Removes debug leftovers
>  - Update top-level doc comment
>  - Trivially re-order assignments
>    
>    ...for re-use
>  - Reformat for clarity
>    
>    Now it's very clear that the "Throws:" section consists of three types of exceptions:
>    
>      1. documented
>      2. inherited
>      3. undocumented
>  - ... and 23 more: https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b

Finished reviewing May 25 commits.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java line 70:

> 68:      */
> 69:     private static Map<String, String> mapNameToPosition(Utils utils, List<? extends Element> params) {
> 70:         Map<String, String> result = new HashMap<>();

Is there a reason not to change this to `Map<String, Integer>`

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java line 94:

> 92:                     ? ee.getTypeParameters()
> 93:                     : ee.getParameters();
> 94:             String target = ch.getParameterName((ParamTree) input.docTreeInfo.docTree());

I'm guessing that it is not practical to have DocTreeInfo be generic in the DocTree field.

An alternative coding style here is to pass in an argument for the expected return type, as in

    String target = ch.getParameterName(input.docTreeInfo.docTree(ParamTree.class));

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java line 108:

> 106:         ExecutableElement md = (ExecutableElement) input.element;
> 107:         CommentHelper ch = utils.getCommentHelper(md);
> 108:         List<? extends ParamTree> tags = input.isTypeVariableParamTag

This sort of cleanup is nice, and welcome.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java line 147:

> 145:     /**
> 146:      * Returns a {@code Content} representation of a list of {@code ParamTree}.
> 147:      * Tries to inherit the param tags that are missing.

mild grumble to see information being removed, even it is obvious to you.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java line 149:

> 147:      * Tries to inherit the param tags that are missing.
> 148:      */
> 149:     private Content getTagletOutput(Element holder, ParamKind kind,

It's not wrong, but I don't see the reason for rearranging the parameter list

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java line 177:

> 175:         Content result = writer.getOutputInstance();
> 176:         Input input = new DocFinder.Input(writer.configuration().utils, holder, this,
> 177:                 Integer.toString(position), kind == ParamKind.TYPE_PARAMETER);

Inconsistent w.r.t line 76 ... `String.valueOf` vs `Integer.toString`

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java line 194:

> 192:      * Returns a {@code Content} representation of a list of {@code ParamTree}.
> 193:      *
> 194:      * Warns about {@code @param} tags that do not map to parameter elements

It does more than just warn ...

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java line 209:

> 207:                                      List<? extends Element> formalParameters,
> 208:                                      TagletWriter writer) {
> 209:         Map<String, ParamTree> tagOfPosition = new HashMap<>();

I still think that one of the type parameters should be `Integer`

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java line 281:

> 279:      * @param name            the name of the parameter.  We can't rely on
> 280:      *                        the name in the param tag because we might be
> 281:      *                        inheriting documentation.

This is useful info you are deleting :-(

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java line 56:

> 54: 
> 55: /**
> 56:  * A taglet that processes {@link ThrowsTree}, which represents tags like

rephrase without "like"

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java line 90:

> 88:                 output.tagList.add(tag);
> 89:             } else if (target != null && candidate != null &&
> 90:                     utils.isTypeElement(candidate) && utils.isTypeElement(target) && // FIXME: can they be anything else other than type elements?

what about `TypeParameterElement`?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java line 112:

> 110:         Content result = writer.getOutputInstance();
> 111:         Set<String> alreadyDocumented = new HashSet<>();
> 112:         result.add(throwsTagsOutput(tagsMap, writer, alreadyDocumented, typeSubstitutions, true));

I presume `throwsTagsOutput` does not include the header...?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java line 115:

> 113:         result.add(inheritThrowsDocumentation(holder, thrownTypes, alreadyDocumented, typeSubstitutions, writer));
> 114:         result.add(linkToUndocumentedDeclaredExceptions(thrownTypes, alreadyDocumented, writer));
> 115:         return result;

Content.add returns `this`, so you could simplify this further to

    return writer.getOutletInstance()
        .add(throwsTagOutput....)
        .add(inheritThrowsDocumentation...)
        .add(linktoUndocumentedExceptions...);

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java line 140:

> 138:                 TypeMirror t2 = i2.next();
> 139:                 if (!types.isSameType(t1, t2))
> 140:                     map.put(t1.toString(), t2);

lots of opportunities for `var` here!

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java line 219:

> 217:                             .map(t -> (ThrowsTree) t)
> 218:                             .toList();
> 219:                     ExecutableElement r = declaredExceptionTags.put(inheritedTags, (ExecutableElement) inheritedDoc.holder);

I do not understand the need for saving the result

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java line 237:

> 235:         Utils utils = writer.configuration().utils;
> 236:         Content result = writer.getOutputInstance();
> 237:         for (TypeMirror declaredExceptionType : declaredExceptionTypes) {

Two comments may have been too many, but zero is too few.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/CommentHelper.java line 552:

> 550:     public ReferenceTree getExceptionName(ThrowsTree tt) {
> 551:         return tt.getExceptionName();
> 552:     }

Is this method worth it?

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

PR: https://git.openjdk.java.net/jdk/pull/8886