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

[Jonathan Gibbons]

On Fri, 5 Apr 2024 15:03:36 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 seven additional commits since the last revision:
> 
>  - Update copyright years
>    
>    Note: any commit hashes below might be outdated due to subsequent
>    history rewriting (e.g. git rebase).
>    
>     + update src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlIndexBuilder.java due to 0f33a6477e0
>  - Clean up
>  - Use a list of alternative ids
>  - Cache forMember
>  - Clarify some comments
>  - Do not handle @interface methods specially
>  - Nix forMember used by AnnotationTypeMemberWriter

Approved, with two optional suggestions.  Both could be considered style suggestions.

## 1
(Minor) While I like the new multi-valued return for `forMember(ExecutableElement executable)` I'm slightly surprised at the use of `List` rather than, say, a `record` that gives semantic meaning to the alternatives.  

## 2
Bigger, maybe for later. This could be thought of as more of a suggestion for a space _and_ time performance improvement, should such an improvement become necessary.

Most classes and interfaces do not have type parameters, not just annotation type interfaces. A quick grep survey of JDK indicates 242 classes and interfaces have type parameters out of a total of 3735 classes and interface. I accept that does not take into account classes and interfaces methods that have type parameters.  Anyway, since most classes and interfaces are "simple", the suggestion would be to have a two-level cache.instead of the simple one-level cache in `HtmlIds`.  

The current one-level cache is a simple `Map<ExecutableElement, List<HtmlId>> ids` meaning that there will an entry for every executable element that is encountered. Many of those entries are somewhat unnecessary, since any clash can only occur in classes or methods with type parameters, right?   So, a two-level cache would have an initial level that records whether a type element needs a subsidiary cache of ids for its executable elements. Once it has been determined that a type element has no type parameters or enclosed elements with type parameters, it should be enough to take the simple code path to determine the ids in the simple old-fashioned way. Conversely, if it is determined that the type element has type parameters or enclosed elements with type parameters, then the code can go the xtra mile to compute the second-level cache info.

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

Marked as reviewed by jjg (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18519#pullrequestreview-1983805843