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

jonathan-gibbons · 2021-04-30T16:39:14Z

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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

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

Reviewers

Hannes Wallnöfer (@hns - Reviewer) ⚠️ Review applies to 23940d0

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3821/head:pull/3821
$ git checkout pull/3821

Update a local copy of the PR:
$ git checkout pull/3821
$ git pull https://git.openjdk.java.net/jdk pull/3821/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3821

View PR using the GUI difftool:
$ git pr show -t 3821

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3821.diff

…ords

bridgekeeper · 2021-04-30T16:39:51Z

👋 Welcome back jjg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-04-30T16:41:31Z

@jonathan-gibbons The following label will be automatically applied to this pull request:

javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-04-30T16:45:20Z

Webrevs

01: Full - Incremental (39014cab)
00: Full (23940d05)

hns

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.

hns · 2021-05-06T13:34:17Z

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

+     * @param e      the element, such as a package element or type element
+     * @param select the predicate for the selected members
+     * @param clazz  the class of the filtered members
+     * @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.

Will fix the docs for all, which was a late-addition. I had previously tried to get away with using just the select filter. <T> is supposed to be documented when it is a type parameter in the signature of a generic method.

hns · 2021-05-06T13:47:13Z

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

+        List<TypeElement> classes = getItems0(e, all, this::isTypeElement, TypeElement.class);
+        for (TypeElement c : classes) {
+            list.addAll(getItems0(c, all, filter, clazz));
+            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.)

hns · 2021-05-06T13:50:13Z

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

+    private <T extends Element> List<T> getItems0(Element e, boolean all, Predicate<Element> select, Class<T> clazz) {
+        return e.getEnclosedElements().stream()
+                .filter(e_ -> select.test(e_) && (all || shouldDocument(e_)))
+                .map(ee -> clazz.cast(ee))


Could be written as method reference.

openjdk · 2021-05-06T14:51:27Z

@jonathan-gibbons This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: hannesw

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 113 new commits pushed to the master branch:

c97f56c: 8266172: -Wstringop-overflow happens in vmError.cpp
43ad24f: 8265465: jcmd VM.cds should keep already dumped archive when exception happens
66191ff: 8266193: BasicJMapTest does not include testHistoParallel methods
36e5ad6: 8263236: runtime/os/TestTracePageSizes.java fails on old kernels
0ca86da: 8266002: vmTestbase/nsk/jvmti/ClassPrepare/classprep001 should skip events for unexpected classes
52f1db6: 8262002: java/lang/instrument/VerifyLocalVariableTableOnRetransformTest.sh failed with "TestCaseScaffoldException: DummyClassWithLVT did not match .class file"
04f7112: 8266293: Key protection using PBEWithMD5AndDES fails with "java.security.InvalidAlgorithmParameterException: Salt must be 8 bytes long"
a90b33a: 8266573: Make sure blackholes are tagged for all JVMCI paths
2dcbedf: 8266044: Nested class summary should show kind of class or interface
e840597: 8266460: java.io tests fail on null stream with upgraded jtreg/TestNG
... and 103 more: https://git.openjdk.java.net/jdk/compare/294347b59be684ee398b442a1c64824eb03311dc...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

jonathan-gibbons · 2021-05-07T14:49:33Z

/integrate

openjdk · 2021-05-07T14:51:23Z

@jonathan-gibbons Since your change was applied there have been 127 commits pushed to the master branch:

946b0fe: 8266645: javac should not check for sealed supertypes in intersection types
74fecc0: 8266503: [UL] Make Decorations safely copy-able and reduce their size
86b8dc9: 8265426: Update java.security to use instanceof pattern variable
3fcdc50: 8266646: Add more diagnostic to java/lang/System/LoggerFinder/modules
9a19a0c: 8264760: JVM crashes when two threads encounter the same resolution error
14f0afe: 8214237: Join parallel phases post evacuation
2798b0d: 8266349: Pass down requested page size to reserve_memory_special
e0c8688: 8262992: Improve @see output
d2b5350: 8263507: Improve structure of package summary pages
a65021e: 8266618: Remove broken -XX:-OptoRemoveUseless
... and 117 more: https://git.openjdk.java.net/jdk/compare/294347b59be684ee398b442a1c64824eb03311dc...master

Your commit was automatically rebased without conflicts.

Pushed as commit 947d69d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8265042: javadoc HTML files not generated for types nested in rec…

23940d0

…ords

openjdk bot added the rfr Pull request is ready for review label Apr 30, 2021

openjdk bot added the javadoc javadoc-dev@openjdk.org label Apr 30, 2021

hns approved these changes May 6, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 6, 2021

address review comments

39014ca

openjdk bot closed this May 7, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 7, 2021

jonathan-gibbons deleted the nesting-records branch May 7, 2021 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

jonathan-gibbons commented Apr 30, 2021 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Apr 30, 2021

Uh oh!

openjdk bot commented Apr 30, 2021

Uh oh!

mlbridge bot commented Apr 30, 2021 •

edited

Loading

Uh oh!

hns left a comment

Uh oh!

hns May 6, 2021

Uh oh!

jonathan-gibbons May 7, 2021 •

edited

Loading

Uh oh!

hns May 6, 2021

Uh oh!

jonathan-gibbons May 7, 2021

Uh oh!

hns May 6, 2021

Uh oh!

jonathan-gibbons May 7, 2021

Uh oh!

openjdk bot commented May 6, 2021 •

edited

Loading

Uh oh!

jonathan-gibbons commented May 7, 2021

Uh oh!

openjdk bot commented May 7, 2021

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

jonathan-gibbons commented Apr 30, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Apr 30, 2021

Uh oh!

openjdk bot commented Apr 30, 2021

Uh oh!

mlbridge bot commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

hns left a comment

Choose a reason for hiding this comment

Uh oh!

hns May 6, 2021

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hns May 6, 2021

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons May 7, 2021

Choose a reason for hiding this comment

Uh oh!

hns May 6, 2021

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons May 7, 2021

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathan-gibbons commented May 7, 2021

Uh oh!

openjdk bot commented May 7, 2021

Uh oh!

Uh oh!

jonathan-gibbons commented Apr 30, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 30, 2021 •

edited

Loading

jonathan-gibbons May 7, 2021 •

edited

Loading

openjdk bot commented May 6, 2021 •

edited

Loading