RFR: 8285488: Improve DocFinder [v4]

Fri Oct 28 21:04:44 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

test/langtools/jdk/javadoc/doclet/testTagInheritance/TestTagInheritance.java line 67:

> 65:                     "Test " + i + " fails");
> 66:         }
> 67: 

Not sure I understand the constants 38, 100, 101 here.  

Seeing the contents of the files that follow, we can easily change to use constants, but perhaps you can add comments/explanation to help the reader.

test/langtools/jdk/javadoc/doclet/testThrowsInheritance/pkg/Abstract.java line 32:

> 30:     abstract void method() throws NullPointerException;
> 31: 
> 32:     // NOTE: Not sure why this test suggests that IndexOutOfBoundsException

I'm guessing there is nothing in the history of this file to explain it.

Given the presumably long-standing nature of the code, I'd be inclined to change `<em>should not for bug-compatibility</em>` to just `not`. and then simplify your comment below accordingly.

This file must be associated with a test class, containing `@test` and theoretically a `@bug` number. Is there any info in the JBS issue for that bug?

test/langtools/jdk/javadoc/doclet/testThrowsInheritanceMatching/TestExceptionTypeMatching.java line 47:

> 45:  *    the same element
> 46:  * 2. Provoke javadoc into treating differently named but semantically
> 47:  *    same elements as different elements

ooooooh, clever!

test/langtools/jdk/javadoc/doclet/testThrowsInheritanceMatching/TestExceptionTypeMatching.java line 58:

> 56:     private final ToolBox tb = new ToolBox();
> 57: 
> 58:     /*

Different test case, maybe not for here and now.
If you have and throw two exceptions with the same simple name (presumably using at least one fully-qualified name), what do the links look like. Do you just get two different links with the same simple name?  This is arguably not a test case that is specific to exceptions, but a test for generating links in any context where the simple names might be ambiguous.

test/langtools/jdk/javadoc/doclet/testThrowsInheritanceMatching/TestExceptionTypeMatching.java line 61:

> 59:      * In Child, MyException is c.MyException, whereas in Parent, MyException
> 60:      * is p.MyException. Those are different exceptions which happen two
> 61:      * share a simple name.

1. spelling typo:  `two` -> `to`
2. suggest change `a simple name` to `the same simple name`

test/langtools/jdk/javadoc/doclet/testThrowsInheritanceMatching/TestExceptionTypeMatching.java line 72:

> 70: 
> 71:                 public class Child extends Parent {
> 72: 

I know this is the default IDE style, but FWIW, IMO, the blank line after the `{` is annoying and unnecessary.

test/langtools/jdk/javadoc/doclet/testThrowsInheritanceMatching/TestExceptionTypeMatching.java line 99:

> 97:         checkOutput(Output.OUT, true, """
> 98:                 Child.java:7: warning: overridden methods do not document exception type c.MyException \
> 99:                 (MODULE <unnamed module> PACKAGE c CLASS MyException)

In an ideal world, the element kinds should be i18n.  As an interim measure you could convert them to lower case to get equivalent keywords, which do not get i18n-ed

test/langtools/jdk/javadoc/doclet/testThrowsInheritanceMatching/TestExceptionTypeMatching.java line 148:

> 146:      * the other, are matched by position, not by name.
> 147:      *
> 148:      * Here the match is cris-cross:

typo: `cris` -> `criss`

test/langtools/jdk/javadoc/doclet/testThrowsInheritanceMatching/TestExceptionTypeMatching.java line 225:

> 223:         //    /** @throws T {@inheritDoc} */
> 224:         //        ^
> 225:         javadoc("-d", base.resolve("out").toString(), "-sourcepath", src.toString(), "x", "-Xdoclint:none");

As a matter of style, I dislike putting options _after_ package or file args

test/langtools/jdk/javadoc/doclet/testThrowsInheritanceMatching/TestExceptionTypeMatching.java line 238:

> 236:     // detected by JDK-8291869, which is tested by tests in this test
> 237:     // suite.
> 238:     // TODO: consider moving this test to a more suitable test suit.

"suit" -> "suite"

test/langtools/jdk/javadoc/doclet/testThrowsInheritanceMatching/TestExceptionTypeMatching.java line 277:

> 275:     // detected by JDK-8291869, which is tested by tests in this test
> 276:     // suite.
> 277:     // TODO: consider moving this test to a more suitable test suit.

ditto "suit"

test/langtools/jdk/javadoc/doclet/testThrowsInheritanceMatching/TestExceptionTypeMatching.java line 323:

> 321:                 public class R extends RuntimeException { }
> 322:                 """);
> 323:         javadoc("-d", base.resolve("out").toString(), src.resolve("Parent.java").toString(),

does it help to use `tb.findJavaFiles(src)` ?

test/langtools/jdk/javadoc/doclet/testThrowsInheritanceMatching/TestExceptionTypeMatching.java line 362:

> 360:      *
> 361:      * A permissible but equally unconventional alternative would be to
> 362:      * keep the package lower-case but give the class a lower-case name p.

shades of "Project Amber" on the horizon ...

test/langtools/jdk/javadoc/doclet/testThrowsInheritanceMatching/TestExceptionTypeMatching.java line 397:

> 395:                 }
> 396:                 """);
> 397:         setAutomaticCheckLinks(false); // otherwise the link checker reports that P.MyException is defined twice

What is the error here? is it a valid error or a false positive?

test/langtools/jdk/javadoc/doclet/testThrowsInheritanceMatching/TestExceptionTypeMatching.java line 426:

> 424:      * a method. For example, consider that for B to be a subtype of A,
> 425:      * it is not necessary for A and B to have the same number or
> 426:      * types of type parameters.

it's clear and well-defined; just not simple!
`javac` can clearly do it, which means that in theory, it could be exposed in the Language Model API.

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

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