On Wed, 25 May 2022 20:55:21 GMT, Jonathan Gibbons <[email protected]> wrote:
>> As described in the JBS issue, the observed problem is a side-effect of a
>> related but different issue, which is that record classes are not treated
>> the same was as enum classes when it comes to generating the class hierarchy
>> in `ClassTree`. Because record classes are not treated
>> specially/differently, they are treated as ordinary/plain classes, with the
>> side-effect that the page for `java.lang.Record` shows `Direct Known
>> Subclasses`.
>>
>> The underlying fix is therefore to clone the enum support in `ClassTree` and
>> related classes, to provide support for record classes. It is possible to do
>> an extreme minimal clone, but that just clones some of the messy evolution
>> already there. Instead, the code in `ClassTree` and its clients is
>> refactored and modernized.
>>
>> Previously there were four explicit pairs of member fields, containing data
>> for different groups (hierarchies) of classes, namely: plain classes, enum
>> classes, interfaces and annotation interfaces. These fields are most of the
>> code to support them are moved into some new abstractions to encapsulate
>> related functionality.
>>
>> 1. The main new abstraction in `ClassTree` is `Hierarchy` which provides the
>> support for the different hierarchies displayed in the generated pages.
>> 2. A new enum `HierarchyKind` identifies the four existing hierarchies
>> (listed above) and now a new one, for record classes. The hierarchies
>> correspond to the different kinds of declared type.
>> 3. A small new class `SubtypeMap` which is a multi-map for mapping a type
>> element to its subtypes. This is used in `Hierarchy` and to record the
>> classes that implement an interfaces.
>>
>> Generally, the naming should be clearer and more obvious. The most confusing
>> name in the old code was `enumSubtypes` which was weird because enum classes
>> don't have subtypes. It meant "subtypes of supertypes of enum classes".
>> This was a prime motivator to switch to the `hierarchy` terminology. The
>> variant spellings of `intfacs` have all been replaced by `interfaces`, and
>> `classtree` becomes `classTree`.
>>
>> *Testing*: a new test case has been added to the `TestRecordTypes.java`
>> test, which verifies the new record hierarchy is displayed on a a package
>> tree page. It is not simple to directly test the observed/reported
>> behavior, because it is specific to the JDK API documentation, and because
>> it is about the content of the `java.lang.Record` API page. However, manual
>> inspection and diff comparison between JDK API documentation generated
>> before and after this change reveals only expected differences. These
>> differences are on the `java.lang.R4cord` page (where the offending section
>> is no longer displayed) and on the pages related to the two existing records
>> in JDK ... which are now listed in `Record Class Hierarchy` sections in the
>> appropriate `package-tree.html` files.
>
> Jonathan Gibbons has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains five commits:
>
> - Merge remote-tracking branch 'upstream/master' into 8285939.record-subtypes
> - address review comments: add doc comments to new methods
> - merge with upstream master
> - fix copyright; update test description
> - JDK-8285939: javadoc java.lang.Record should not have "Direct Known
> Subclasses:" section
Looks good.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/ClassTree.java
line 78:
> 76: /**
> 77: * {@return the roots of the hierarchy}
> 78: * The roots are the classes or interfaces with no superclass or
> superinterfaces.
Use singular "root" for clarity:
Suggestion:
* A root is a class or an interface with no superclass or
superinterfaces.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/ClassTree.java
line 88:
> 86: * {@return a map containing the type elements in this hierarchy
> and their subtypes}
> 87: */
> 88: public Map<TypeElement, SortedSet<TypeElement>> subtypes() {
This method is unused; consider deleting it.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/ClassTree.java
line 103:
> 101: * {@return the set of all subtypes of the given type element,
> or an empty set if there are none}
> 102: *
> 103: * The set of all subtypes is the transitive closure of the
> {@linkplain #subtypes() immediate subtypes}
Did you mean to link to #subtypes(TypeElement)?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/ClassTree.java
line 128:
> 126: /**
> 127: * A map giving the subtypes for each of a collection of type
> elements.
> 128: * The subtypes may be subclasses or subinterfaces, depending on the
> context.
For a separate PR: we should consider dropping hyphens from "super-" and "sub-"
in words that relate to classes, interfaces, types, and packages.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/ClassTree.java
line 228:
> 226:
> 227: /**
> 228: * Generate the hierarchies for the given set of type elements.
Suggestion:
* Generate the hierarchies for the given type elements.
test/langtools/jdk/javadoc/doclet/testRecordTypes/TestRecordTypes.java line 610:
> 608:
> 609: @Test
> 610: public void testPackageTree(Path base) throws IOException {
Am I right saying that we cannot easily test the original bug symptom, which is
the presence of "Direct Known Subclasses:" on the
api/java.base/java/lang/Record.html page?
-------------
Marked as reviewed by prappo (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8523
