On Fri, 28 Oct 2022 16:46:24 GMT, Pavel Rappo <[email protected]> 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
