RFR: JDK-8223358: Incorrect HTML structure in annotation pages

Mon Nov 8 20:34:36 UTC 2021

On Wed, 29 Sep 2021 06:09:53 GMT, Hannes Wallnöfer <hannesw at openjdk.org> wrote:

> Please review a fix for a simple bug that somehow mushroomed into a major cleanup of the annotation member builder and writer code. I still think it is justified to do this as part of a bug fix since in my eyes it is the only reasonable way to fix the issue (lest we want to add more complex workarounds).
> 
> The problem is that currently we have two versions of each class used to generate annotation interface member documentation, one for required members and one for optional ones. However, only the summary tables are separated for these two kinds of members, while the details section uses one single list for both. This required coordination between the two builder classes to generate a single list without generating faulty (missing or duplicate) HTML. Obviously this workaround was flawed, since it avoided the duplicate headers but still generated duplicate lists and sections that should have been single elements. 
> 
> Even for the summary section, the dual class setup seemed like overkill, since the two summary lists differ only in label/text content, and the only thing the optional member writer could do extra was to generate the default value info.
> 
> So the core of this change unifies the dual annotation member builder and writer classes into single classes. For the writer, this simply involves adding a few switch expressions to retrieve the correct text values based on the value of a new nested enum class. The builder class is now simply instantiated once instead of twice to generate the member details list, and it does it for all annotation members.
> 
> Since the builder also uses the writer to generate the unified details list, the new enum has 3 values: `OPTIONAL`, `REQUIRED` and `ANY`. I'm not totally happy with this setup, but IMO it is still better than before and I have added a few comments to explain the reasons behind it.
> 
> To make retrieval of combined annotation members easier I added a new member kind to `VisualMemberTable` class called `ANNOTATION_TYPE_MEMBER`. This also allowed me to simplify the subnavigation code in `Navigation.java` which also contained some ugly workarounds for annotation interfaces. In the process I also reestablished the old order of annotation member subnavigation links to put the required members link first - this had been changed inadvertently in JDK 17. 
> 
> The change in return type to the `getVisibleMembers` methods in `VisualMemberTable` from `List<? extends Element>` to `List<Element>` is from when I did manual list merging with these return values. I left it in because I think it potentially makes other future uses of these methods easier. 
> 
> I did a recursive diff on the generated documentation before and after the fix. Obviously the reversed annotation member subnav links are changed in every annotation page. Apart from that, the only annotation interface that contains both required and optional members in the JDK (and therefore the only one that is affected and benefits from this fix) is `javax.annotation.processing.Generated` in the `java.compiler` module. 
> 
> As a sidenote: I also considered changing the nomenclature of the whole bunch of classes from the obsolete "annotation type" to "annotation interface", but I think that would have been an even bigger disruption in the code. If we want to do this I would prefer to do it as a separate task.

Good work. I agree that simplifying the classes is worthwhile. Previously, I've only looked at simplifying the writers, not the builders as well.

I'll ask the following questions, and then approve the review.

Should we go all the way and merge all annotation interface members into a single summary table?  Is there benefit (other than, "they've always been separate") to keeping them separate?   Should we merge them in the same way we're thinking to merge "Exceptions" and "Errors" into "Throwable"?  Is the distinction somewhat similar to the minor distinction between void methods and methods that return a result?

The counter-argument is that you've done a lot of work to maintain the separation, and I don't want to unnecessarily discard that. Merging would probably make us have to look at what the merged table would look like, and how (or whether) to indicate the difference between required and optional members. But perhaps that detail just belongs down in the `Details` section.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AnnotationTypeMemberWriterImpl.java line 53:

> 51:  *  deletion without notice.</b>
> 52:  */
> 53: public class AnnotationTypeMemberWriterImpl extends AbstractMemberWriter

Git-grumble .... the changes in this file are big enough that Git will likely not track the rename correct ... end-git-grumble.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AnnotationTypeMemberWriterImpl.java line 70:

> 68: 
> 69:     /**
> 70:      * Construct a new AnnotationTypeMemberWriterImpl for any kind of member.

I'll say it just the once, here, to avoid getting tedious. Consider using the 2nd person declarative form, "Constructs ...", in comments.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AnnotationTypeMemberWriterImpl.java line 72:

> 70:      * Construct a new AnnotationTypeMemberWriterImpl for any kind of member.
> 71:      *
> 72:      * @param writer The writer for the class that the member belong to.

typo: belongs

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AnnotationTypeMemberWriterImpl.java line 103:

> 101:                     MarkerComments.START_OF_ANNOTATION_TYPE_REQUIRED_MEMBER_SUMMARY,
> 102:                     MarkerComments.START_OF_ANNOTATION_INTERFACE_REQUIRED_MEMBER_SUMMARY));
> 103:             case ANY -> throw new RuntimeException("unsupported member kind");

consider using `UnsupportedOperationException`.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AnnotationTypeMemberWriterImpl.java line 104:

> 102:                     MarkerComments.START_OF_ANNOTATION_INTERFACE_REQUIRED_MEMBER_SUMMARY));
> 103:             case ANY -> throw new RuntimeException("unsupported member kind");
> 104:         }

There are a number of `switch (kind) ....` statements. Would it help to put methods on the `Kind` enum?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AnnotationTypeMemberWriterImpl.java line 121:

> 119:                     case REQUIRED -> HtmlIds.ANNOTATION_TYPE_REQUIRED_ELEMENT_SUMMARY;
> 120:                     case OPTIONAL -> HtmlIds.ANNOTATION_TYPE_OPTIONAL_ELEMENT_SUMMARY;
> 121:                     case ANY -> throw new RuntimeException("unsupported member kind");

Ditto consider UOE

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/SummaryListWriter.java line 259:

> 257:             case RECORD_COMPONENT ->
> 258:                 throw new AssertionError("Record components are not supported by SummaryListWriter!");
> 259:             default -> new AnnotationTypeMemberWriterImpl(this);

1. I see it was always this way, but maybe it would be better to have an explicit `case` label, and have `default` throw some sort of `not handled` exception, or else rely on javac doing a completeness check.
2. I'm surprised there isn't any parameter to the new constructor to indicate whether optional or required members are required.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/AnnotationTypeMemberWriter.java line 120:

> 118: 
> 119:     /**
> 120:      * Add the the default value documentation if the member has one.

`the the`

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

Marked as reviewed by jjg (Reviewer).

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