On Thu, 4 Apr 2024 12:28:23 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 > > 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
