Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8223358: Incorrect HTML structure in annotation pages #5746

Closed
wants to merge 6 commits into from

Conversation

hns
Copy link
Member

@hns hns commented Sep 29, 2021

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 VisibleMemberTable 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 VisibleMemberTable 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.


Progress

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

Issue

  • JDK-8223358: Incorrect HTML structure in annotation pages

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5746

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 29, 2021

👋 Welcome back hannesw! 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
Copy link

@openjdk openjdk bot commented Sep 29, 2021

@hns 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.

@openjdk openjdk bot added the javadoc label Sep 29, 2021
@hns hns marked this pull request as ready for review Sep 29, 2021
@openjdk openjdk bot added the rfr label Sep 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 29, 2021

Webrevs

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 27, 2021

@hns This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Nov 8, 2021

(Minor: can you edit the initial long comment to fix spelling/typo: you use VisualMemberTable in some places instead of VisibleMemberTable. )

@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Nov 8, 2021

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.

Generally, I think this is a bad idea and we should not do it. We are stuck with the existing naming in the reflection interfaces (java.lang.reflect.* and javax.lang.model.*) where the Type terminology persists and will remain. I think we have to accept that the "classes and interfaces" terminology is for user-facing text, and that we will have to live with the old-style terminology in code, just to keep the code consistent.

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

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.

private final Kind kind;

/**
* Construct a new AnnotationTypeMemberWriterImpl for any kind of member.
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* Construct a new AnnotationTypeMemberWriterImpl for any kind of member.
*
* @param writer The writer for the class that the member belong to.
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: belongs

case REQUIRED -> memberSummaryTree.add(selectComment(
MarkerComments.START_OF_ANNOTATION_TYPE_REQUIRED_MEMBER_SUMMARY,
MarkerComments.START_OF_ANNOTATION_INTERFACE_REQUIRED_MEMBER_SUMMARY));
case ANY -> throw new RuntimeException("unsupported member kind");
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider using UnsupportedOperationException.

switch (kind) {
case REQUIRED -> HtmlIds.ANNOTATION_TYPE_REQUIRED_ELEMENT_SUMMARY;
case OPTIONAL -> HtmlIds.ANNOTATION_TYPE_OPTIONAL_ELEMENT_SUMMARY;
case ANY -> throw new RuntimeException("unsupported member kind");
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto consider UOE

*
* <p><b>This is NOT part of any supported API.
* If you write code that depends on this, you do so at your own risk.
* This code and its internal interfaces are subject to change or
* deletion without notice.</b>
*/
public class AnnotationTypeRequiredMemberWriterImpl extends AbstractMemberWriter
implements AnnotationTypeRequiredMemberWriter, MemberSummaryWriter {
public class AnnotationTypeMemberWriterImpl extends AbstractMemberWriter
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

switch (kind) {
case OPTIONAL -> memberSummaryTree.add(selectComment(
MarkerComments.START_OF_ANNOTATION_TYPE_OPTIONAL_MEMBER_SUMMARY,
MarkerComments.START_OF_ANNOTATION_INTERFACE_OPTIONAL_MEMBER_SUMMARY));
case REQUIRED -> memberSummaryTree.add(selectComment(
MarkerComments.START_OF_ANNOTATION_TYPE_REQUIRED_MEMBER_SUMMARY,
MarkerComments.START_OF_ANNOTATION_INTERFACE_REQUIRED_MEMBER_SUMMARY));
case ANY -> throw new RuntimeException("unsupported member kind");
}
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@hns hns Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I don't see how to do it, but these methods would have to return quite a lot of constants from all over the place. I don't think putting these into fields would make things much nicer.

@@ -256,7 +256,7 @@ protected Content getSummaryLink(Element e) {
case ENUM_CONSTANT -> new EnumConstantWriterImpl(this);
case RECORD_COMPONENT ->
throw new AssertionError("Record components are not supported by SummaryListWriter!");
default -> new AnnotationTypeOptionalMemberWriterImpl(this, null);
default -> new AnnotationTypeMemberWriterImpl(this);
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.

Copy link
Member Author

@hns hns Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one surprised me: Annotation elements do not have an ElementKind constant of their own, they use ElementKind.METHOD. This means the code above was never executed. It is also the reason annotation elements are linked as methods, including the empty parentheses. I'm not sure whether this is an oversight, a bug, or there is some intention behind it.

As for the code above I think it can be replaced with throwing a UnsupportedOperationException.

@@ -115,4 +115,12 @@
* @param annotationDocTree the content tree to which the tags will be added
*/
void addTags(Element member, Content annotationDocTree);

/**
* Add the the default value documentation if the member has one.
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the the

@openjdk
Copy link

@openjdk openjdk bot commented Nov 8, 2021

@hns 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:

8223358: Incorrect HTML structure in annotation pages

Reviewed-by: jjg

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 540 new commits pushed to the master branch:

  • dde959d: 8276808: java/nio/channels/Channels/TransferTo.java timed out
  • daf77eb: 8276337: Use override specifier in HeapDumper
  • 93692ea: 8274395: Use enhanced-for instead of plain 'for' in jdk.internal.jvmstat
  • e35abe3: 8256208: Javadoc's generated overview does not show classes of unnamed package
  • f65db88: 8276841: Add support for Visual Studio 2022
  • c27afb3: 8276863: Remove test/jdk/sun/security/ec/ECDSAJavaVerify.java
  • e198594: 8250678: ModuleDescriptor.Version parsing treats empty segments inconsistently
  • 4bd5bfd: 8276731: Metaspace chunks are uncommitted twice
  • 5c7f77c: 8276850: Remove outdated comment in HeapRegionManager::par_iterate
  • 945f408: 8276098: Do precise BOT updates in G1 evacuation phase
  • ... and 530 more: https://git.openjdk.java.net/jdk/compare/6a573b888d4d3322b9165562f85e1b7b781a5ff1...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.

@openjdk openjdk bot added the ready label Nov 8, 2021
@hns
Copy link
Member Author

@hns hns commented Nov 9, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Nov 9, 2021

Going to push as commit 055de6f.
Since your change was applied there have been 541 commits pushed to the master branch:

  • a60e912: 8198626: java/awt/KeyboardFocusmanager/TypeAhead/TestDialogTypeAhead.html fails on mac
  • dde959d: 8276808: java/nio/channels/Channels/TransferTo.java timed out
  • daf77eb: 8276337: Use override specifier in HeapDumper
  • 93692ea: 8274395: Use enhanced-for instead of plain 'for' in jdk.internal.jvmstat
  • e35abe3: 8256208: Javadoc's generated overview does not show classes of unnamed package
  • f65db88: 8276841: Add support for Visual Studio 2022
  • c27afb3: 8276863: Remove test/jdk/sun/security/ec/ECDSAJavaVerify.java
  • e198594: 8250678: ModuleDescriptor.Version parsing treats empty segments inconsistently
  • 4bd5bfd: 8276731: Metaspace chunks are uncommitted twice
  • 5c7f77c: 8276850: Remove outdated comment in HeapRegionManager::par_iterate
  • ... and 531 more: https://git.openjdk.java.net/jdk/compare/6a573b888d4d3322b9165562f85e1b7b781a5ff1...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 9, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 9, 2021

@hns Pushed as commit 055de6f.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated javadoc
2 participants