On Wed, 19 Oct 2022 11:15:15 GMT, Pavel Rappo <pra...@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
