RFR: 8285488: Improve DocFinder [v4]

Fri Oct 28 19:11:49 UTC 2022

On Fri, 28 Oct 2022 16:46:24 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 with a new target base due to a merge or a rebase. The pull request now contains 49 commits:
> 
>  - refactor: improve error handling
>  - refactor: clarify, reuse, simplify, clean up
>  - refactor: pass Utils & BaseConfiguration to taglet
>    
>    This simplifies lots of methods. Later this could be done for other
>    taglets too.
>  - refactor: better code comments
>  - refactor: add more relevant excerpts from JLS
>  - fix: introduce more control to search
>    
>    This is done for the sake of `@throws`. Two convenience methods are
>    added to assist migration from Optional with minimal change to
>    DocFinder call sites.
>    
>    This solves 8295800: When searching documentation for an exception,
>    don't jump over methods that don't mention that exception.
>  - refactor: clean up ThrowsTaglet
>  - Merge branch 'master' into HEAD
>  - 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.
>  - Merge branch 'master' into 8285488
>  - ... and 39 more: https://git.openjdk.org/jdk/compare/628820f4...c2db1ae6

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

> 120: doclet.inheritDocWithinInappropriateTag=@inheritDoc cannot be used within this tag
> 121: doclet.inheritDocNoDoc=overridden methods do not document exception type {0}
> 122: doclet.throwsInheritDocUnsupported=@inheritDoc for exception-type type parameters not declared by a method is unsupported; \

Minor grammar quibble: _not supported_ reads better than _unsupported_.

Even better would be to reorder the message:

@inheritDoc is not supported for exception-type type parameters that are not declared by a method; document such exception types directly

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

> 227:         Utils utils = writer.configuration().utils;
> 228:         Content result = writer.getOutputInstance();
> 229:         var r = utils.docFinder().search((ExecutableElement) holder, m -> Result.fromOptional(extract(utils, m, position, kind == ParamKind.TYPE_PARAMETER)))

long line

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

> 153:      */
> 154: 
> 155:     public ThrowsTaglet(BaseConfiguration configuration) {

I note the provision of this new argument as "unusual", but do not see anything against it at this time.

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

> 194:                 messages.warning(path, "doclet.throwsInheritDocUnsupported");
> 195:             } else if (f instanceof Failure.Undocumented e) {
> 196:                 messages.warning(ch.getDocTreePath(e.tag()), "doclet.inheritDocNoDoc", diagnosticDescriptionOf(e.exceptionElement));

I'm thinking that 4 of these warnings, except maybe the last one, should be upgraded to errors.

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

> 197:             } else {
> 198:                 // TODO: instead of if-else, use pattern matching for switch for both
> 199:                 //  readability and exhaustiveness when it's available

You beat me to it; after reading the preceding lines, I was going to make exactly that suggestion!

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

> 207:             configuration.getMessages().warning(holder, "doclet.noInheritedDoc", signature);
> 208:         }
> 209:         return writer.getOutputInstance(); // TODO: consider invalid rather than empty output

very much so.

Maybe consider constructing content in each of the arms of the if-else chain, and then (if possible) get at the method to create invalid output.   Look for `HtmlDocletWriter.invalidTagOutput`. Maybe that method should be moved to `TagletWriter[Impl]`.

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

> 448:     }
> 449: 
> 450:     private static sealed class Failure extends Exception {

instead of the `transient` keyword and `serialVersionUIDs`, consider just `@SuppressWarnings("serial") on the overall `Failure` class.  The class is internal and will never be serialized!

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

> 552:         } else {
> 553:             // type parameters are matched by position; the basis for position matching
> 554:             // is JLS sections 8.4.2 and 8.4.4

Is this a place to mention that only type parameters declared for methods are supported?

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

> 576:             // exceptions (i.e. Failure.ExceptionTypeNotFound | Failure.NotExceptionType),
> 577:             // so we instantiate it with a general Failure. We then refine the specific
> 578:             // exception being thrown, from the general exception we caught.

Really? Are you sure?  This is exactly why multi-catch was added.   Am I missing something here?

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

> 750:     }
> 751: 
> 752:     private static String detailedDescriptionOf(Element e) {

can't be static if you're going to solve i18n

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

> 780:         }
> 781:         var enclosingElementDescription = detailedDescriptionOf(e.getEnclosingElement());
> 782:         return enclosingElementDescription + " " + thisElementDescription;

A named package is enclosed by a module, although it may be the unnamed module for >= JDK 9.
A well-formed named module never has an unnamed package.

Consider return empty string from the switch and dealing with that in the `return` statement.

That being said, who uses this info where: you're including the raw element kind in the result, so is this mostly for debugging?

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

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