On Wed, 3 Apr 2024 18:14:39 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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 11 additional > commits since the last revision: > > - Merge branch 'master' into 8325088 > - Remove registration phase > > Makes the code more robust and simple. > - Merge branch 'master' into 8325088 > - Update copyright years > > Note: any commit hashes below might be outdated due to subsequent > history rewriting (e.g. git rebase). > > - revert > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlIndexBuilder.java > as spurious > - revert > test/langtools/jdk/javadoc/doclet/testMemberInheritance/TestMemberInheritance.java > as spurious > - revert > test/langtools/jdk/javadoc/doclet/testMemberSummary/TestMemberSummary.java as > spurious > - revert > test/langtools/jdk/javadoc/doclet/testNewLanguageFeatures/TestNewLanguageFeatures.java > as spurious > - revert test/langtools/jdk/javadoc/doclet/testOrdering/TestOrdering.java > as spurious > - revert > test/langtools/jdk/javadoc/doclet/testOverriddenMethods/TestOverrideMethods.java > as spurious > - revert > test/langtools/jdk/javadoc/doclet/testPrivateClasses/TestPrivateClasses.java > as spurious > - revert test/langtools/jdk/javadoc/doclet/testSeeTag/TestSeeTag.java as > spurious > - revert > test/langtools/jdk/javadoc/doclet/testVisibleMembers/TestVisibleMembers.java > as spurious > - Use erased notation only when necessary > > Partially reverts 4f028269, which is the bulk of the previous solution, > then adds a centralised ID registry for executable elements. > > The centralised registry is an alternative solution, which is more > gentle and less disruptive to tests and composability (-link and > -linkoffline). > - 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 4f0282694fd > + update > test/langtools/jdk/javadoc/doclet/testMemberInheritance/TestMemberInheritance.java > due to 4f0282694fd > + update > test/langtools/jdk/javadoc/doclet/testMemberSummary/TestMemberSummary.java > due to 4f0282694fd > + update > test/langtools/jdk/javadoc/doclet/testNewLanguageFeatures/TestNewLanguageFeatures.java > due to 4f0282694fd > + update test/langtools/jdk/javadoc/doclet/testOrdering/TestOrdering.... src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ConstructorWriter.java line 195: > 193: HtmlId erasureAnchor = htmlIds.forErasure(constructor); > 194: if (erasureAnchor != null > 195: && !erasureAnchor.name().equals(memberAnchor.name())) { Is this redundant now that the erasure handling is encapsulated within forMember? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlIds.java line 562: > 560: && e.getKind() != ElementKind.METHOD) > 561: throw new > IllegalArgumentException(String.valueOf(e.getKind())); > 562: var vmt = configuration.getVisibleMemberTable((TypeElement) > e.getEnclosingElement()); Will the vmt differ if we specify `-private`? And say if we have 2 methods like: private <T extends Runnable> void method(T arg) {} public <T extends Number> void method(T arg) {} Will the Number public method be consistently erased whether or not `-private` is set? src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlIds.java line 571: > 569: var list = Stream.concat(Stream.concat(ctors.stream(), > methods.stream()), otherMethods.stream()) > 570: .map(e1 -> (ExecutableElement) e1) > 571: .toList(); We can maybe do something like: record ErasureCheck(ExecutableElement element, HtmlId erasure) {} // ... .map(e1 -> new ErasureCheck(e1, forErasure(e1))) .collect(Collectors.groupingBy(check -> check.erasure == null)); // 1. Map all elements that can _only_ be addressed by the simple id for (var m : groups.get(true)) {...} ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18519#discussion_r1550637811 PR Review Comment: https://git.openjdk.org/jdk/pull/18519#discussion_r1550635930 PR Review Comment: https://git.openjdk.org/jdk/pull/18519#discussion_r1550643002
