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

[Jonathan Gibbons]

On Wed, 27 Mar 2024 17:19:35 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

Generally, great work!

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

> 171:      */
> 172:     HtmlId forMember(TypeElement typeElement, ExecutableElement member) {
> 173:         return HtmlId.of(forErasure(typeElement, member).replaceAll("\\s", ""));

I suggest adding something like an `@apiNote` to explain why the `typeElement` may not be the same as the enclosing element, if the enclosing element is not documented.   The hint is there in your use of the word `documented` -- and while that's true, it's also very subtle.

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

> 213:         var parameterTypes = ((ExecutableType) utils.typeUtils
> 214:                 .erasure(utils.asInstantiatedMethodType(site, executable)))
> 215:                 .getParameterTypes();

Cool expression!

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

> 214:                 .erasure(utils.asInstantiatedMethodType(site, executable)))
> 215:                 .getParameterTypes();
> 216:         var stv = new SimpleTypeVisitor9<String, Void>() {

It's conventional to use the latest visitor, which in this case is `SimpleTypeVisitor14`, but that being said, there seems to be no obvious difference between 9 and 14.

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

> 217:             @Override
> 218:             public String visitArray(ArrayType t, Void p) {
> 219:                 return visit(t.getComponentType()) + utils.getDimension(t);

Obviously, `utils.getDimension()` harkens back to the old (JDK 8-era) doclet, but even the current implementation of that method could use some TLC. But that's not your problem here.

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

> 231:         };
> 232:         return executable.getSimpleName() + parameterTypes.stream()
> 233:                 .map(stv::visit)

Why do you need this (`stv::visit`) -- does the normal `.toString` for an array type not do the right thing?

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

> 440:     public HtmlId forPreviewSection(Element el) {
> 441:         return HtmlId.of("preview-" + switch (el.getKind()) {
> 442:             case CONSTRUCTOR, METHOD -> forMember((TypeElement) el.getEnclosingElement(), (ExecutableElement) el).name(); // fixme?

what needs fixing?

test/langtools/jdk/javadoc/doclet/testErasure/TestErasure.java line 70:

> 68:                     public abstract <T extends X> T m(T arg);
> 69:                     public abstract <T extends Y> T m(T arg);
> 70:                 }

Nice example

test/langtools/jdk/javadoc/doclet/testErasure/TestErasure.java line 81:

> 79:         checkExit(Exit.OK);
> 80:         // constructors
> 81:         checkOutput("Foo.html", true, """

This is not wrong, and so does not need to be changed, but situations like this are why `checkOrder` was added -- so that you can "spot check" the significant parts of the summary table (such as the links to the detail entries), without all the surrounding table-related stuff.

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

PR Comment: https://git.openjdk.org/jdk/pull/18519#issuecomment-2023718905
PR Review Comment: https://git.openjdk.org/jdk/pull/18519#discussion_r1541583038
PR Review Comment: https://git.openjdk.org/jdk/pull/18519#discussion_r1541810596
PR Review Comment: https://git.openjdk.org/jdk/pull/18519#discussion_r1541759579
PR Review Comment: https://git.openjdk.org/jdk/pull/18519#discussion_r1541826582
PR Review Comment: https://git.openjdk.org/jdk/pull/18519#discussion_r1541830699
PR Review Comment: https://git.openjdk.org/jdk/pull/18519#discussion_r1541828519
PR Review Comment: https://git.openjdk.org/jdk/pull/18519#discussion_r1541573333
PR Review Comment: https://git.openjdk.org/jdk/pull/18519#discussion_r1541838818