8177100: APIs duplicated in JavaDoc

[nizarbenalla]

Please review this patch to fix a bug where a method can be documented multiple times
Consider these 4 classes

                    A       (interface)
                   / \
                  /   \
(abstract class)  C     B   ( interface)
                  \   /
                   \ /
                    D       (class)

Where A declares testA(), C implements it public final void testA(), B extends A but does not override it, D extends C and implements B

In the generated javadoc, testA() is documented twice.

After the patch, testA() is only documented once:

---

Progress

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

Issue

• JDK-8177100: APIs duplicated in JavaDoc (Bug - P4)

Reviewers

• Chen Liang (@liach - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25123

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back nbenalla! 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]

@nizarbenalla This change is no longer ready for integration - check the PR body for details.

[openjdk]

@nizarbenalla 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]

Webrevs

• 06: Full - Incremental (caeb20ad)
• 05: Full - Incremental (b53817df)
• 04: Full - Incremental (4c645c7b)
• 03: Full - Incremental (c7b62698)
• 02: Full - Incremental (8d300463)
• 01: Full - Incremental (41578ad2)
• 00: Full (4a55b006)

[liach]

Can't we change this to if (list != null && !list.isEmpty) return false;?

Comments can be to update the overall block comment to indicate that there is no contention - when a method overrides multiple superinterfaces' methods, including when it is final from superclasses, the superclass method always prevails due to method resolution rules in Java. All interface methods have low resolution priority.

[nizarbenalla]

I think this simplification works just fine.

[liach]

Looks good, thanks!

[hns]

I'm not sure about this. @nizarbenalla have you tested whether this changes the output for JDK docs?

[hns]

There's a problem with character encoding in this patch, please do not integrate:

 /Users/runner/work/jdk/jdk/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/VisibleMemberTable.java:681: error: unmappable character (0x80) for encoding ascii
        // Multiple-Inheritance: No Contention. In Java???s method resolution,

[liach]

/reviewers 2 committer

[openjdk]

@liach
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Committer).

[hns]

As I suspected, this removes methods from the "Methods declared in interface XY" sections. For example, in the java.lang.StringBuilder doc page section for methods declared in java.lang.CharSequence it removes all methods except isEmpty(). So this is not the right place to fix this. I think we should only eliminate these overridden methods when they are to be documented in the subclass (when the declaring class is undocumented).

[nizarbenalla]

Hannes suggested in offline discussion where the check can be instead of the old approach.
I have checked that there is no change in the current JDK doc build after applying this patch.

Thanks for catching this oversight during the review.

[nizarbenalla]

Please don't review this yet, I plan on pushing an other update.

[nizarbenalla]

I've updated the test as the name was not accurate, and simplified the code in VisibleMemberTable

[hns]

I have to apologize for my previous review. When I noticed that JDK documentation had changed, my knee-jerk reaction was to assume that the change was wrong. But it is indeed the current documentation that is wrong.

For example in StringBuilder, the methods overridden in the hidden AbstractStringBuilder class should not be shown as declared in CharSequence as they are documented as local methods in StringBuilder (even though they are implemented in a hidden superclass).

Similarly (but without the hidden superclass), the equals and hashCode methods in HashMap should not be documented as declared in interface Map, but only in AbstractMap where they override the default implementation.

So your first solution (in the simplified form) was actually correct after all.

[liach]

Looks identical to the initial version.

[hns]

Looks identical to the initial version.

@liach I realized the first version was right after all (see my comment above). We should not document a method as inherited from some interface when it is implemented in the local class or some superclass. So this gets rid of duplicate inherited methods, such as equals(Object) in HashMap appearing as inherited both from class AbstractMap and interface Map.

[hns]

Could you add a positive check to this test and testHashMapInheritance that the method is documented as expected (as local method in this test, and as inherited from the public abstract class in testHashMapInheritance), and that the method details has a "Specified by: ..." entry pointing to the interface method?