RFR: JDK-8265042: javadoc HTML files not generated for types nested in records

[Hannes Wallnöfer]

On Fri, 30 Apr 2021 16:39:14 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

> This addresses an oversight in the support for records.
> 
> The fix could have been as simple as a one-word addition to a list of element kinds to check for nested classes. However, instead of that, the fix is more of a cleanup of a group of methods in `Utils` to fix this issue and to reduce the likelihood of anything similar happening again, when the next new kind of class is added.
> 
> The cleanup uses new code idioms using lambdas, predicates, and generic methods to replace varargs and temporary sets and copying lists.  The naming is somewhat cleaned up as well, but it was a self-imposed restriction that the cleanup is limited to the `Utils` class, and does not leak into clients of the class.
> 
> There is a core `getItems` method which is somewhat multi-purpose, supporting recursive and non-recursive use, and examining either just documented members or all members, including members that are not directly documented. (The latter occurs mostly in serialization.)   It is possible there is a future refactoring to further improve this method, perhaps to break it into different methods.  With that in mind, the signature of some methods was narrowed from accepting any `Element` to a specific kind of element: generally, one of `PackageElement` or `TypeElement`.
> 
> A new test is provided that checks that all the expected files are generated for various kinds of nested classes and interfaces in all kinds of classes and interfaces. The test is designed to fail if a new kind of class or interface is added in future without a corresponding update to the test.

I like both the predicate/lambda based methods and the new test a lot! I found some bits that should be cleaned up before integrating.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 2149:

> 2147:         for (TypeElement c : classes) {
> 2148:             list.addAll(getItems0(c, all, filter, clazz));
> 2149:             recursiveGetItems(list, c, all, filter, clazz);

It seems you are adding these items twice, directly in the first line of the loop and then again in the recursive invocation. (I think this was carried over from the prior implementation.)

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 2159:

> 2157:      * @param select the predicate for the selected members
> 2158:      * @param clazz  the class of the filtered members
> 2159:      * @param <T>    the class of the filtered members

Parameter documentation in this and other new methods above is a bit messy/wrong. For example, the `boolean all` is not documented anywhere, and `<T>` is documented as a parameter.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 2166:

> 2164:         return e.getEnclosedElements().stream()
> 2165:                 .filter(e_ -> select.test(e_) && (all || shouldDocument(e_)))
> 2166:                 .map(ee -> clazz.cast(ee))

Could be written as method reference.

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

Marked as reviewed by hannesw (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3821