On Wed, 27 Mar 2024 17:19:35 GMT, Pavel Rappo <pra...@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
