8292892: Javadoc index descriptions are not deterministic #10070
Conversation
|
|
Webrevs
|
|
This is an interesting bug. It's the first one I've encountered that is impossible to miss with reproducible builds but might have been impossible to find without them. It also reveals a surprising feature of the Java language: inaccessible members can be made public by an intermediate subclass. That feature first caused problems for Javadoc in 2002 with bug JDK-4780441. The fix for that bug made it so that "inherited members from package private classes are now documented as though they were declared in the inheriting class." The fix was released in Java SE 5 ("Tiger"), adding the members to the pages of their inheriting classes. The members were added to the index in Java SE 15. Below is a description of the problem, as I understand it, and my proposed solution. The problemThe Javadoc code refers to the type in which a member is found as the containing type element and stores it in its own For example, there are four index items of i1.getLabel() "LOCCRC"
i1.getContainingTypeElement() "java.util.jar.JarEntry"
i1.getElement().getEnclosingElement() "java.util.zip.ZipConstants"
i2.getLabel() "LOCCRC"
i2.getContainingTypeElement() "java.util.jar.JarFile"
i2.getElement().getEnclosingElement() "java.util.zip.ZipConstants"
i3.getLabel() "LOCCRC"
i3.getContainingTypeElement() "java.util.jar.JarInputStream"
i3.getElement().getEnclosingElement() "java.util.zip.ZipConstants"
i4.getLabel() "LOCCRC"
i4.getContainingTypeElement() "java.util.jar.JarOutputStream"
i4.getElement().getEnclosingElement() "java.util.zip.ZipConstants"Index items are stored in a set, and the set's comparator function uses the index label and the enclosing element to determine whether an item is already in the set. The four items above appear the same to the comparator, so only the first one makes it into the set. The first one is just the one first found in the package directory when they are loaded in directory order (unsorted). So the random order in which the source files are stored by the file system in their directory determines which one of the items gets listed in the index. There's a related problem in the member search index. The class name of the containing type element is appended to the package name of the enclosing element. That works for most members, but for these specially-inherited members, it creates entries that don't exist. For example, the search index might list The solutionThe solution is to add a step to the comparator: when two index items are the same after comparing their labels and enclosing elements, make an additional comparison of their containing type elements. For the related problem in the member search index, the solution is to get the package name from the containing type element instead of from the enclosing element. The 423 new index entries are shown as insertions in the As far as I can tell, all of the new entries are inherited from just the following members: package java.awt;
abstract class AttributeValue {
public int hashCode() {...}
public String toString() {...}
}
package java.lang;
abstract sealed class AbstractStringBuilder implements Appendable, CharSequence
permits StringBuilder, StringBuffer {
public IntStream chars() {...}
public IntStream codePoints() {...}
}
package java.lang.foreign;
public sealed interface MemoryLayout permits AbstractLayout, SequenceLayout,
GroupLayout, PaddingLayout, ValueLayout {
long bitAlignment();
long bitSize();
long byteSize();
boolean isPadding();
Optional<String> name();
}
package java.time.chrono;
public interface ChronoLocalDate extends Temporal, TemporalAdjuster,
Comparable<ChronoLocalDate> {
String toString();
long until(Temporal endExclusive, TemporalUnit unit);
}
package java.util.zip;
interface ZipConstants {
static final int CENATT = 36;
...
static final int LOCVER = 4;
}
package jdk.incubator.vector;
abstract class AbstractVector<E> extends Vector<E> {
public final <F> Vector<F> castShape(VectorSpecies<F> toSpecies, int part) {...}
public final <F> Vector<F> check(Class<F> elementType) {...}
public final <F> Vector<F> check(VectorSpecies<F> species) {...}
public abstract <F> Vector<F> convertShape(Conversion<E,F> conv,
VectorSpecies<F> rsp, int part);
public final <F> Vector<F> convert(Conversion<E,F> conv, int part) {...}
public final VectorMask<E> maskAll(boolean bit) {...}
public DoubleVector reinterpretAsDoubles() {...}
public FloatVector reinterpretAsFloats() {...}
public IntVector reinterpretAsInts() {...}
public LongVector reinterpretAsLongs() {...}
public ShortVector reinterpretAsShorts() {...}
public final VectorSpecies<E> species() {...}
} |
|
I've checked the code, the tests and the resulting JDK API Documentation. All look good. That said, I leave it to Jon to make the final determination, as he's an expert in this area and knows a lot more about the problem of intermediate types than I do. Separately, thanks for using the JLS terminology (package access) for what is colloquially known as package-private or package-protected. |
There was a problem hiding this comment.
Excellent analysis and write up.
(Just) for the record, I will note there is potentially another case that could be tested, where the element with package access is not exposed until a couple of subtypes down, as compared to being exposed in a direct subtype. That would stress the use of containingType in the code. But the general policy of using and documenting these somewhat hidden elements is all a bit ugly anyway, and I don't know that this extra case appears in JDK.
| */ | ||
| public static void main(String... args) throws Exception { | ||
| TestIndexInherited tester = new TestIndexInherited(); | ||
| tester.runTests(m -> new Object[]{Path.of(m.getName())}); |
There was a problem hiding this comment.
These days, you can simplify this down to just tester.runTests();. The implementation of the method will check the signature of the test methods and provide the appropriate path automatically.
| @@ -0,0 +1,41 @@ | |||
| /* | |||
There was a problem hiding this comment.
It's not wrong to use separate files, as you have for ClassA, ClassB and ClassC but generally the prevailing style these days is to generate small test files (like these) from text blocks in the main test file. This makes it easier to see the sources all together, and avoids the overhead of the legal block. That being said, The amount of text after the legal block puts these in the gray area where either style is OK.
There was a problem hiding this comment.
I found it useful to have the actual files so that I could prove to myself that this quirk in the Java language is real. I even added a main method to pkg2.ClassC so that I could run it and see that ClassC really does have runtime access to methodA with package access in pkg1.ClassA.
I also found it useful to have the class files so that I could generate the Javadoc API documentation and look at it myself outside of the test case.
There was a problem hiding this comment.
Yeah, there's a difference between what is best for an initial sandbox to play to investigate a feature, and the best form for a test.
But I guess this comes down to a 50/50 style issue in this case, so OK.
| @Test | ||
| public void testSuperclass(Path base) { | ||
| String dir = base.resolve("out").toString(); | ||
| javadoc("-d", dir, "-sourcepath", testSrc, "-private", "pkg1", "pkg2"); |
There was a problem hiding this comment.
At first glance, the use of the -private option does not seem to match the method name testSuperclass or its matching description (tests entries in the superclass).
There are two characteristics here that seem to be getting mixed up:
- testing subclasses and superclasses
- testing with default access (document public and protected) and with "more" access, such as using
-packageor-privateoptions.
Ideally, the tests should test subclasses and superclasses with default access, and then repeat, testing subclasses and superclasses with "more" access, such as with -private.
... the point being, that we should not see the replicative behavior when documenting with -private, should we? The elements with package access should be documented in their defining class, right?
There was a problem hiding this comment.
Right. I renamed most of the members and methods of the test class and rewrote its comments, which were far too terse. I also added a check that the inherited members do not show up in the index when they're already listed for the declaring superclass. I think it's much better now, but let me know what you think.
|
“The sins of the father are to be laid upon the children.” I see this arises from While the approach in this PR is probably correct and for the best, it is depressing to see the exact same constants being defined multiple times in the documentation. |
| /** | ||
| * The default constructor. | ||
| */ | ||
| public ClassB() { | ||
| } |
There was a problem hiding this comment.
One nit: here and elsewhere in this PR an explicit no-arg (parameterless, nullary, etc.) constructor is called default, which, technically speaking, it isn't. If a constructor is default, then it is implicit. Once you make it explicit, it is no longer default. A default constructor is provided by the compiler, when there are no other constructors defined, see: JLS 8.8.9. Default Constructor.
There was a problem hiding this comment.
Fixed. Thank you, Pavel. I wondered that exact question while writing the comment, but I didn't bother to look it up. Thanks for taking the time to let me know. I changed the comments to "sole constructor."
Thank you, Jonathan. And thanks for the quick review!
I noticed several cases in the JDK where the problem occurs (see list above):
I thought of writing test cases for each of these, but then just settled on the last item for a simple test case that detects the bug and verifies the fix. Let me know if you think I should add some of the other cases. |
If it's any consolation, nobody noticed their absence in the past 20 years, so perhaps nobody will notice their presence. |
jgneff
requested review from
pavelrappo and
jonathan-gibbons
and removed request for
pavelrappo and
jonathan-gibbons
|
@jgneff This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 277 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jonathan-gibbons) but any other Committer may sponsor as well.
|
|
Thanks, Jonathan. A minor protocol question (my first JDK commit since it moved to GitHub): Is a pull request author supposed to press the "Re-request review" icons on GitHub when changes from a prior review are complete? I cycled through those buttons a couple of times because each time I clicked the button for one reviewer, it seemed to disable it for the other. In general, do reviewers wait for that request? |
|
/integrate |
|
/sponsor |
|
Going to push as commit 844a95b.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@pavelrappo @jgneff Pushed as commit 844a95b. |
Please review these changes to the Javadoc index builders. This pull request adds 423 missing entries to the index of the JDK API Specification, corrects their package names in the member search index, and makes their descriptions deterministic. I'll follow up with more information in a separate comment.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10070/head:pull/10070$ git checkout pull/10070Update a local copy of the PR:
$ git checkout pull/10070$ git pull https://git.openjdk.org/jdk pull/10070/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10070View PR using the GUI difftool:
$ git pr show -t 10070Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10070.diff