8327824: Type annotation placed on incorrect array nesting levels #18211

liach · 2024-03-12T03:33:38Z

Please review this patch that fixes placement of type annotations on array types in Javadoc output. This oversight seems to have existed since JDK 8 but was never noticed or reported.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8327824: Type annotation placed on incorrect array nesting levels (Bug - P4)

Reviewers

Pavel Rappo (@pavelrappo - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18211/head:pull/18211
$ git checkout pull/18211

Update a local copy of the PR:
$ git checkout pull/18211
$ git pull https://git.openjdk.org/jdk.git pull/18211/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18211

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18211.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-12T03:34:12Z

👋 Welcome back liach! 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 · 2024-03-12T03:34:37Z

@liach 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 · 2024-03-12T04:18:22Z

Webrevs

01: Full (7c37d67d)
00: Full (7468e8f2)

pavelrappo · 2024-03-12T11:58:34Z

I've read your discussion with @cushon in the comment section for the bug and I agree that type annotations on arrays are unintuitive. Welp, we should make sure they are documented correctly.

Do you know if there's a test that exercises this particularly surprising bit from Example 10.2-2. Array Variables and Array Types?

And perhaps surprisingly, the following field declarations have the same array type:
int @A [] f @B [];
int @B [] @A [] g;

If there isn't, we should add it.

pavelrappo

I looked into it; that wasn't easy 😅.

7 years ago a seemingly similar bug, https://bugs.openjdk.org/browse/JDK-8068737, was fixed in javac. It's only now that javadoc is catching up.

Your change seems to be correct, but it disrupts the visitor's pattern. On the other hand, maybe keeping the change confined to visitArray is for the better here.

Here's what's left to be done:

resolve merge conflicts due to recently integrated 8325433: Type annotations on primitive types are not linked #18179
add a couple more test cases; for example, the JLS one that I mentioned earlier and maybe extract something from JBS-8068737

Thanks for doing it.

…rray-anno-order

cushon · 2024-03-12T21:49:35Z

Thanks for this! The changes look good to me overall.

…when new members are added

liach · 2024-03-12T22:03:50Z

Now fixed merge conflict and added a test case for JLS 10.2-2, with the 2 styles of declarations present in the parameter and the return type respectively.

pavelrappo

This PR passes our CI and looks good; thanks.

pavelrappo · 2024-03-13T11:53:36Z

test/langtools/jdk/javadoc/doclet/testTypeAnnotations/typeannos/DeepArrays.java

+    String @ArrA [] @ArrB [] @ArrC [] @ArrD [] array4() { return null; }
+    @ArrA ArrParameterized<@ArrC String @ArrA [] @ArrB []> @ArrC [] @ArrD [] manyNested() { return null; }
+    void varargs(@ArrA String @ArrB [] @ArrC [] @ArrD ... arg) {}
+    int @ArrA [] mixedStyles(int @ArrB [] @ArrA [] arg) @ArrB [] { return null; } // JLS example 10.2-2


Took me a second or so to remind myself that this is a valid method declaration with the return type int[][]:

int[] mixedStyles(int[][] arg) []

openjdk · 2024-03-13T15:13:40Z

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

8327824: Type annotation placed on incorrect array nesting levels

Reviewed-by: prappo

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

0db6231: 8314508: Improve how relativized pointers are printed by frame::describe
0353245: 8325874: Improve checkbox-based interface in summary pages
4d64467: 8328079: JDK-8326583 broke ccache compilation
7e05a70: 8251330: Reorder CDS archived heap to speed up relocation
7d8561d: 8327109: Refactor data graph cloning used in create_new_if_for_predicate() into separate class
a4a5196: 8327872: Convert javax/swing/JToolTip/4644444/bug4644444.java applet test to main
da4dd7c: 8327989: java/net/httpclient/ManyRequest.java should not use "localhost" in URIs
49d8008: 8327452: G1: Improve scalability of Merge Log Buffers
0ae4fa7: 8327210: AIX: Delete obsolete parameter Use64KPagesThreshold
107cb53: 8327701: Remove the xlc toolchain
... and 5 more: https://git.openjdk.org/jdk/compare/d5b95a0ed38b10ed9f51d20255e06eb38fdd8b82...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@pavelrappo) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

liach · 2024-03-13T15:15:58Z

/integrate

openjdk · 2024-03-13T15:16:23Z

@liach
Your change (at version 7c37d67) is now ready to be sponsored by a Committer.

pavelrappo · 2024-03-13T16:44:47Z

/sponsor

openjdk · 2024-03-13T16:45:16Z

Going to push as commit 35b00e6.
Since your change was applied there have been 16 commits pushed to the master branch:

be344e4: 8327475: Add analysis code for JDK-8327169
0db6231: 8314508: Improve how relativized pointers are printed by frame::describe
0353245: 8325874: Improve checkbox-based interface in summary pages
4d64467: 8328079: JDK-8326583 broke ccache compilation
7e05a70: 8251330: Reorder CDS archived heap to speed up relocation
7d8561d: 8327109: Refactor data graph cloning used in create_new_if_for_predicate() into separate class
a4a5196: 8327872: Convert javax/swing/JToolTip/4644444/bug4644444.java applet test to main
da4dd7c: 8327989: java/net/httpclient/ManyRequest.java should not use "localhost" in URIs
49d8008: 8327452: G1: Improve scalability of Merge Log Buffers
0ae4fa7: 8327210: AIX: Delete obsolete parameter Use64KPagesThreshold
... and 6 more: https://git.openjdk.org/jdk/compare/d5b95a0ed38b10ed9f51d20255e06eb38fdd8b82...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-13T16:45:23Z

@pavelrappo @liach Pushed as commit 35b00e6.

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

liach added 3 commits March 11, 2024 19:34

8327824: Type annotation placed on incorrect array nesting levels

cf13ba0

Fix bugs, add tests, fix wrong anticipated test results

ca098b1

Make sure varargs work too

0075633

openjdk bot added the javadoc javadoc-dev@openjdk.org label Mar 12, 2024

Fix whitespace

7468e8f

openjdk bot added the rfr Pull request is ready for review label Mar 12, 2024

pavelrappo reviewed Mar 12, 2024

View reviewed changes

Merge branch 'master' of https://github.com/openjdk/jdk into fix/jd-a…

dbaca31

…rray-anno-order

liach added 2 commits March 12, 2024 16:50

Add test case for JLS example 10.2-2

aaf53e0

Check output from body instead of summary, summary changes css class …

7c37d67

…when new members are added

pavelrappo approved these changes Mar 13, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 13, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 13, 2024

openjdk bot added the integrated Pull request has been integrated label Mar 13, 2024

openjdk bot closed this Mar 13, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 13, 2024

liach deleted the fix/jd-array-anno-order branch July 11, 2024 21:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8327824: Type annotation placed on incorrect array nesting levels #18211

8327824: Type annotation placed on incorrect array nesting levels #18211

liach commented Mar 12, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 12, 2024

openjdk bot commented Mar 12, 2024

mlbridge bot commented Mar 12, 2024 •

edited

Loading

pavelrappo commented Mar 12, 2024

pavelrappo left a comment

cushon commented Mar 12, 2024

liach commented Mar 12, 2024

pavelrappo left a comment

pavelrappo Mar 13, 2024

openjdk bot commented Mar 13, 2024 •

edited

Loading

liach commented Mar 13, 2024

openjdk bot commented Mar 13, 2024

pavelrappo commented Mar 13, 2024

openjdk bot commented Mar 13, 2024

openjdk bot commented Mar 13, 2024

8327824: Type annotation placed on incorrect array nesting levels #18211

8327824: Type annotation placed on incorrect array nesting levels #18211

Conversation

liach commented Mar 12, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 12, 2024

openjdk bot commented Mar 12, 2024

mlbridge bot commented Mar 12, 2024 • edited Loading

Webrevs

pavelrappo commented Mar 12, 2024

pavelrappo left a comment

Choose a reason for hiding this comment

cushon commented Mar 12, 2024

liach commented Mar 12, 2024

pavelrappo left a comment

Choose a reason for hiding this comment

pavelrappo Mar 13, 2024

Choose a reason for hiding this comment

openjdk bot commented Mar 13, 2024 • edited Loading

liach commented Mar 13, 2024

openjdk bot commented Mar 13, 2024

pavelrappo commented Mar 13, 2024

openjdk bot commented Mar 13, 2024

openjdk bot commented Mar 13, 2024

liach commented Mar 12, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 12, 2024 •

edited

Loading

openjdk bot commented Mar 13, 2024 •

edited

Loading