8285488: Improve DocFinder #10746
8285488: Improve DocFinder #10746
Conversation
javax.lang.model.element.Element.provides rationale for that: Elements should be compared using the equals(Object) method. There is no guarantee that any particular element will always be represented by the same object.
**WARNING**: This commit changes the JDK API Documentation. All changes are of the same kind. One such change can be seen in java.io.ObjectInputStream.readUTF. That method now documents previously undocumented EOFException. The reason why EOFException was not documented previously is that the old code incorrectly started documentation search for IOException and its subclasses (for subclasses see 4947455) from ObjectInputStream.readUTF itself, rather than from the methods it overrides. Because ObjectInputStream.readUTF documents IOException, the search succeeded immediately, never finding EOFException which is documented in java.io.DataInput.readUTF. This commit changes the code so that the search skips ObjectInputStream.readUTF when looking for exception documentation for exceptions listed in the `throws` clause and their subexceptions.
**WARNING**: This commit changes the JDK API Documentation.
All changes are of the same kind: some methods document more exceptions
without any of the related doc comments having been changed.
For example, along with DateTimeException, ChronoLocalDate.plus(long,
TemporalUnit) now documents UnsupportedTemporalTypeException.
The reason why UnsupportedTemporalTypeException was not documented
previously (although it should've been because of JDK-4947455) is that
ThrowsTaglet.inherit was incorrectly obtaining
javax.lang.model.element.Element corresponding to the exception in
the `@throws` tag. ThrowsTaglet.inherit was looking for a ThrowsTree
node in a wrong DocCommentTree node; what was done like this:
var ch = utils.getCommentHelper(input.element);
target = ch.getException(tag);
should've been done like this:
target = utils.getCommentHelper(input.docTreeInfo.element()).getException(tag);
The new code obtains the target correctly.
The new behavior sometimes results in doubling of exception
documentation. This happens when a subexception is inherited both
explicitly and implicitly when a superexception is inherited.
For example, java.nio.channels.SocketChannel#bind now documents
ClosedChannelException twice.
The first time because:
@throws ClosedChannelException {@inheritdoc}
And the second time because:
@throws IOException {@inheritdoc}
Such doubling is an intermediate artifact of this commit and will
disappear in subsequent commits.
**CAVEATS**: This is an intermediate commit, whose primary purpose is
to finally stop using old DocFinder.search and friends so that they
could be deleted. As such, the commit cuts a few corners.
In particular, SeeTaglet throws an exception on an attempt to use:
@see {@inheritdoc}
That and other issue will be cleaned up and refactored in subsequent
commits.
**NOTES:**
* It would be nice to have this method instead of a newly introduced
`inherit` so that all the specifics of inheritance processing would
be hidden in individual taglets:
void inherit(ExecutableElement owner,
DocTree tag,
TagletWriter writer)
But I don't see how we could do this, because the `inherit` is called
while parsing an individual tag, rather than a group of similar tags.
So we cannot use such a method to implement, say, a one-to-many type
of inheritance. (See the existing ThrowsTaglet.flatten workaround).
* SimpleTaglet first sentence handling seems misguided. It needs to be
sorted out.
- Method signatures no longer require BaseConfiguration - DocFinder no longer depends on Utils or BaseConfiguration - Methods are still grouped under a separate type, DocFinder, rather than flattened into Utils
Stream.peek is designed for debugging and can be optimized away. Flipping the flag in flatMap seems wrong too; so use iterator.
There's no reason to have a stream-based implementation when a simpler
iterator-based implementation will do. Also, this commit fixes a bug
where if called with
includeMethod && throwExceptionIfNoOverriddenMethods
on a method that didn't override anything, the search would not throw
an exception.
Comparing exceptions by their string names is incorrect. This commit makes ThrowsTaglet "javax.lang.model typed" rather than "stringly typed". Changing the comparison mechanism broke 2 tests, which were subsequently fixed by removing problematic @throws/@exception tag. In neither test exceptions were pertinent: * testHtmlDefinitionListTag used `@exception HeadlessException` to test HTML structure of `<dl>` tags generated by javadoc (see 6786690). For whatever reason, the exception was unknown. It's possible that it was supposed to be java.awt.HeadlessException, but it was neither imported nor FQNed in the tag. Since a particular type of exception does not matter in that test, this PR FQNed that exception. * TestStdDoclet used `@throws DoesNotExist` as one of the warnings to contribute to the total warning count. Interestingly enough, the unknown exception in `@throws` was never warned about, unlike unknown references in 2 `@see` tags. This PR substitutes that exception with a separate test. Note that if "-Xdoclint:none" is removed, then that @throws tag triggers a doclint error: error: reference not found To test and support the comparison mechanism change, many new tests were introduced. They clearly show why being "stringly typed" is wrong for ThrowsTaglet.
Doclint does this already:
error: reference not found
* @throws X {@inheritdoc}
^
Not sure how to coordinate doclint with the other diagnostic system
so that they work in a mutually exclusive fashion and not double
report.
Generally speaking, the order was and still is the encounter order. It's the resolution of 8287796 that changed a few places so that you can see the difference between BFS and DFS. There was a commit with a message that describes that. Let me see if I can quickly find it. Yes, here it is: 2475c1f |
|
/issue remove JDK-8295800 |
|
@pavelrappo |
|
I removed 8295800 ("When searching documentation for an exception, don't jump over methods that don't mention that exception") from this PR because fixing it here would be too disruptive. It should better be fixed in the context of 8285368 ("Overhaul doc-comment inheritance"), which proposes changes to the documentation search algorithm. |
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
Outdated
Show resolved
Hide resolved
It was never a goal of this PR to fix InheritableTaglet.
It will be re-introduced in 8285368.
It will be interesting to see the list of affected places, when we have away to address this issue. |
|
@pavelrappo This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. 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 13 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. ➡️ To integrate this PR with the above commit message to the |
Also removes three TODOs. The first TODO was about "binary name". The notion of "binary name" seems to be inapplicable to packages and modules (see JLS 13.1. The Form of a Binary) and as such is a less than ideal here.
|
/integrate |
|
Going to push as commit 499406c.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@pavelrappo Pushed as commit 499406c. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please have a look at this work-in-progress PR. The reason this is a (normal) PR rather than a more suitable draft PR is to mainly trigger a discussion on the mailing list on whether the suggested approach to solve multiple intertwined issues of exception documentation inheritance is a correct one.
In a nutshell, it turns out that the combination of
{@throws}and{@inheritDoc}is quite complex. It's more complex than a combination of{@inheritDoc}with any other tag or{@inheritDoc}on its own. To account for that complexity, handling of{@inheritDoc}in{@throws}is lifted toThrowsTaglet.The said complexity stems from two facts:
Processing of
{@inheritDoc}is context free and is done by replacing{@inheritDoc}with the tags it expands to. That model does not account for@throwswhere{@inheritDoc}sometimes expands to multiple@throwstags, which correspond to separate entries in the "Throws:" section of a method detail. Read: we change the exception section, not a description of one of its entries.Corresponding exception types in the hierarchy (i.e. matching
{@inheritDoc}with exception documentation it must inherit) is not always clear-cut. This becomes especially apparent when type variables are involved.History
The work started as an attempt to fix JDK-8285488, hence the issue number for the PR. However, along its course the PR solved other issues, which will be soon added to the PR:
javax.lang.model, not strings{@inheritDoc}in@throwsfullyAs of today
docs/api/java.management.rmi/javax/management/remote/rmi/RMIConnectionImpl_Stub.html, the order of exceptions has changed, which is insignificant. As for the second file,docs/api/java.management/javax/management/MBeanServer.html, the new warning is generated and erroneous input added to the corresponding page. The issue has to be addressed on the component side and is tracked by JDK-8295151.Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10746/head:pull/10746$ git checkout pull/10746Update a local copy of the PR:
$ git checkout pull/10746$ git pull https://git.openjdk.org/jdk pull/10746/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10746View PR using the GUI difftool:
$ git pr show -t 10746Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10746.diff