RFR: 8325088: Overloads that differ in type parameters may be lost [v4]

[Jonathan Gibbons]

On Thu, 4 Apr 2024 12:28:23 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> Creating a link to a constructor or a method or comparing constructors or methods __does not__ factor in type parameters. When constructors or methods are overloaded and differ only in type parameters -- a situation which is absent in JDK API, but present elsewhere -- that causes significant defects, such as:
>> 
>>   - missing entries in summary tables, lists and indexes,
>>   - duplicating links in the table of contents.
>> 
>> This PR fixes those defects, and the fix is two-fold. Firstly, we update comparators to consider type parameters. That takes care of missing constructors and methods. Secondly, we update id (anchor) and link generation to always use the "erased" notation. That takes care of duplicating links.
>> 
>> What's the "erased" notation? Suppose we have the following method:
>> 
>>     <T extends String> T m(T arg)
>> 
>> The current notation refers to it as `m(T)`. That works fine until there's no other method, such as
>> 
>>     <T> T m(T arg)
>> 
>> In which case, the current notation will produce a collision: `m(T)`. By contrast, the erased notation for those two methods is `m(java.lang.String)` and `m(java.lang.Object)` respectively. No collision.
>> 
>> While longer, I believe that the erased notation is collision-proof. Why? Because [JLS 8.4.2][] says that "it is a compile-time error to declare two methods with override-equivalent signatures in a class". Which means that for any two constructors or methods the erasure of their signatures must differ, or else it won't compile.
>> 
>> The change is pretty straightforward, except for some test fallout that required attention.
>> 
>> [JLS 8.4.2]: https://docs.oracle.com/javase/specs/jls/se22/html/jls-8.html#jls-8.4.2
>
> Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Respond to feedback

Some questions ...

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ConstructorWriter.java line 200:

> 198:         content.add(heading);
> 199:         return HtmlTree.SECTION(HtmlStyle.detail, content)
> 200:                 .setId(memberAnchor);

It's a bit disappointing that more of this isn't in `HtmlIds`.
It feels like it perpetuates the original ugly code.

Would it make sense for `htmlIds` to return a record/pair containing both the `memberId` and `erasureId` for `ExecutableElement` ?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlIds.java line 567:

> 565:         var methods = vmt.getVisibleMembers(VisibleMemberTable.Kind.METHODS);
> 566:         // for whatever reason annotation methods are not of Kind.METHODS
> 567:         var otherMethods = vmt.getVisibleMembers(VisibleMemberTable.Kind.ANNOTATION_TYPE_MEMBER);

I'm surprised you need to worry about annotation type members here -- annotation types cannot have type arguments, and so the "simple" id should always be sufficient.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/MethodWriter.java line 211:

> 209:         if (erasureAnchor != null
> 210:                 && !erasureAnchor.name().equals(memberAnchor.name())) {
> 211:             heading.setId(erasureAnchor);

See earlier comment about returning these together.

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

PR Review: https://git.openjdk.org/jdk/pull/18519#pullrequestreview-1980858688
PR Review Comment: https://git.openjdk.org/jdk/pull/18519#discussion_r1552261527
PR Review Comment: https://git.openjdk.org/jdk/pull/18519#discussion_r1552242033
PR Review Comment: https://git.openjdk.org/jdk/pull/18519#discussion_r1552262881