RFR: 8285488: Improve DocFinder [v3]

Mon Oct 24 21:29:57 UTC 2022

On Wed, 19 Oct 2022 11:15:15 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> Please have a look at this work-in-progress PR. The reason this is a (normal) PR rather than a more suitable draft PR is to mainly trigger a discussion on the mailing list on whether the suggested approach to solve multiple intertwined issues of exception documentation inheritance is a correct one.
>> 
>> In a nutshell, it turns out that the combination of `{@throws}` and `{@inheritDoc}` is quite complex. It's more complex than a combination of `{@inheritDoc}` with any other tag or `{@inheritDoc}` on its own. To account for that complexity, handling of `{@inheritDoc}` in `{@throws}` is lifted to `ThrowsTaglet`.
>> 
>> The said complexity stems from two facts:
>> 
>> 1. Processing of `{@inheritDoc}` is context free and is done by replacing `{@inheritDoc}` with the tags it expands to. That model does not account for `@throws` where `{@inheritDoc}` sometimes expands to multiple `@throws` tags, which correspond to _separate_ entries in the "Throws:" section of a method detail. Read: we change the exception section, not a description of one of its entries.
>> 
>> 2. Corresponding exception types in the hierarchy (i.e. matching `{@inheritDoc}` with exception documentation it must inherit) is not always clear-cut. This becomes especially apparent when type variables are involved.
>> 
>> History
>> -------
>> 
>> The work started as an attempt to fix JDK-8285488, hence the issue number for the PR. However, along its course the PR solved other issues, which will be soon added to the PR:
>> 
>> * 8287796: Stop auto-inheriting documentation for subclasses of exceptions whose documentation is inherited
>> * 8291869: Match exceptions using types of `javax.lang.model`, not strings
>> * 8295277: Expand `{@inheritDoc}` in `@throws` fully
>> * 8288045: Clean up ParamTaglet
>> * 8288046: Clean up ThrowsTaglet
>> 
>> As of today
>> -----------
>> 
>> * All tests (both existing and newly introduced) pass.
>> * JDK API Documentation is the same, except for two files. In the first file, `docs/api/java.management.rmi/javax/management/remote/rmi/RMIConnectionImpl_Stub.html`, the order of exceptions has changed, which is insignificant. As for the second file, `docs/api/java.management/javax/management/MBeanServer.html`, the new warning is generated and erroneous input added to the corresponding page. The issue has to be addressed on the component side and is tracked by JDK-8295151.
>
> Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix: test failed due to filesystem handling issues
>   
>   Filed 8295543 to track that filesystem issue and fixed the test to make
>   sure the package cannot be confused with the type parameter, whose
>   name is not pertinent to the test anyway.

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

> 833:         linkInfo.excludeTypeBounds = true;
> 834:         var link = htmlWriter.getLink(linkInfo);
> 835:         var concat = new ContentBuilder(HtmlTree.CODE(link));

hmmm, do you actually need the `CODE` here?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/standard.properties line 110:

> 108: doclet.see.nested_link=Tag {0}: nested link
> 109: doclet.throws.reference_not_found=cannot find exception type by name
> 110: doclet.throws.reference_bad_type=type found is not of exception type: {0}

suggest: `type is not an exception type` or even just `not an exception class`

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

> 119: doclet.UnknownTagLowercase={0} is an unknown tag -- same as a known tag except for case.
> 120: doclet.inheritDocWithinInappropriateTag=@inheritDoc cannot be used within this tag
> 121: doclet.inheritDocNoDoc=overridden methods do not document exception type {0}

we should check with Dan/Alex for the right terminology here; my best understanding is that we now use `exception class`.  Bottom line, we should follow JLS.

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

> 113:                     + utils.flatSignature(method, writer.getCurrentPageElement());
> 114:             messages.warning(method, "doclet.noInheritedDoc", signature);
> 115:         }

Lines 99-115: would it help to refactor this to use an `instanceof` pattern?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/InheritableTaglet.java line 32:

> 30: 
> 31: import javax.lang.model.element.Element;
> 32: import java.util.List;

Elsewhere, we list Java (`java.*`, `javax.*`) imports first.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/InheritableTaglet.java line 40:

> 38: public interface InheritableTaglet extends Taglet {
> 39: 
> 40:     Output inherit(Element owner, DocTree tag, boolean isFirstSentence, BaseConfiguration configuration);

It would be nice to have a javadoc comment describing this new method.

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

> 80:         Integer position = stringIntegerMap.get(ch.getParameterName(param));
> 81:         if (position == null) {
> 82:             return new Output(null, null, List.of(), true); // remodel, because it's an error

I'm not sure I understand the comment: is it some form of TODO?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ReturnTaglet.java line 95:

> 93:         }
> 94: 
> 95:         // TODO check for more than one @return

TODO

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

> 73: public class ThrowsTaglet extends BaseTaglet implements InheritableTaglet {
> 74: 
> 75:     /*

Nice comment!

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

> 122:      * thrown exceptions and, hence, document them.
> 123:      */
> 124: 

Would it make sense to add a 3rd section about characteristics of `@throws` tags to be taken into account, citing issues like repeatability, subtypes, and inheritance?

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

> 232:     //   one cannot practically use a list of tags cherry-picked from different doc comments)
> 233: 
> 234:     // This adds entries late; who starts an entry also ends it

Going forward (not here and now) we might want to generalize this.   For example, given that `{@inheritDoc}` applies to just the body, and not all block tags, we might want to allow something like `@see {@inheritDoc}` to inherit all `@see` tags from its supertype, suggesting that the one-to-many relationship might need to be more general. (And no, I don't have any suggestion for selective `@see` inheritance.)

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

> 363:         var subtypeTestInapplicable = t.getKind() == TypeKind.EXECUTABLE
> 364:                 || t.getKind() == TypeKind.PACKAGE
> 365:                 || t.getKind() == TypeKind.MODULE;

Just asking ... would this be more concise as an enhanced switch expression?

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

> 462:         //  exception jumping over methods that didn't
> 463:         //  document that exception to those methods
> 464:         //  that did. A method cannot "undocument"

agreed on "should probably not"

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

> 514:      * The section is being built sequentially from top to bottom.
> 515:      *
> 516:      * Adapts one off methods of writer to continuous building.

"one off" should probably be hyphenated

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

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