Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8294377: Prepare to stop auto-inheriting documentation for subclasses of exceptions whose documentation is inherited #10449

Closed
wants to merge 2 commits into from

Conversation

pavelrappo
Copy link
Member

@pavelrappo pavelrappo commented Sep 27, 2022

This adds exception documentation to JDK methods that would otherwise lose that documentation once JDK-8287796 is integrated. While adding this exception documentation now does not change 1 the JDK API Documentation, it will automatically counterbalance the effect that JDK-8287796 will have.

JDK-8287796 seeks to remove an old, undocumented, and esoteric javadoc feature that cannot be suppressed 2. The feature is so little known about that IDEs either do not implement it correctly or do not implement it at all, thus rendering documentation differently from how the javadoc tool renders it.

That feature was introduced in JDK-4947455 and manifests as follows. If a method inherits documentation for an exception, along with that documentation the method automatically inherits documentation for all exceptions that are subclasses of that former exception and are documented in an overridden method.

To illustrate that behavior, consider the following example. A method Subtype.m inherits documentation for SuperException, while the overridden method Supertype.m documents SuperException, SubExceptionA and SubExceptionB, where the latter two exceptions are subclasses of the former exception:

public class Subtype extends Supertype {

    @Override
    public void m() throws SuperException {
...

public class Supertype {

    /**
     * @throws SuperException general problem
     * @throws SubExceptionA  specific problem A
     * @throws SubExceptionB  specific problem B
     */
    public void m() throws SuperException {
...

public class SuperException extends Exception {
...

public class SubExceptionA extends SuperException {
...

public class SubExceptionB extends SuperException {
...

For the above example, the API Documentation for Subtype.m will contain documentation for all three exceptions; the page fragment for Subtype.m in "Method Details" will look like this:

public void m()
       throws SuperException

Overrides:
    m in class Supertype
Throws:
    SuperException - general problem
    SubExceptionA - specific problem A
    SubExceptionB - specific problem B 

It works for checked and unchecked exceptions, for methods in classes and interfaces.

If it's the first time you hear about that behavior and this change affects your area, it's a good opportunity to reflect on whether the exception documentation this change adds is really needed or you were simply unaware of the fact that it was automatically added before. If exception documentation is not needed, please comment on this PR. Otherwise, consider approving it.

Keep in mind that if some exception documentation is not needed, leaving it out from this PR might require a CSR.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8294377: Prepare to stop auto-inheriting documentation for subclasses of exceptions whose documentation is inherited

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10449/head:pull/10449
$ git checkout pull/10449

Update a local copy of the PR:
$ git checkout pull/10449
$ git pull https://git.openjdk.org/jdk pull/10449/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10449

View PR using the GUI difftool:
$ git pr show -t 10449

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10449.diff

Footnotes

  1. Aside from insignificant changes on class-use and index pages. There's one relatively significant change. This change is in jdk.jshell, where some methods disappeared from "Methods declared in ..." section and other irregularities. We are aware of this and have filed JDK-8291803 to fix the root cause.

  2. In reality, the feature can be suppressed, but it looks like a bug rather than intentional design. If we legitimize the feature and its suppression behavior, the model for exception documentation inheritance might become much more complicated.

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 27, 2022

👋 Welcome back prappo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 27, 2022
@openjdk
Copy link

openjdk bot commented Sep 27, 2022

@pavelrappo The following labels will be automatically applied to this pull request:

  • client
  • core-libs
  • javadoc
  • kulla
  • nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added javadoc javadoc-dev@openjdk.org nio nio-dev@openjdk.org client client-libs-dev@openjdk.org core-libs core-libs-dev@openjdk.org kulla kulla-dev@openjdk.org labels Sep 27, 2022
@mlbridge
Copy link

mlbridge bot commented Sep 27, 2022

Webrevs

@pavelrappo
Copy link
Member Author

/label remove javadoc

@openjdk openjdk bot removed the javadoc javadoc-dev@openjdk.org label Sep 27, 2022
@openjdk
Copy link

openjdk bot commented Sep 27, 2022

@pavelrappo
The javadoc label was successfully removed.

Copy link
Member

@dfuch dfuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to JNDI and NIO look reasonable to me - though I haven't verified whether anything was missing (or no longer needed). If you can assert that the generated docs are the same before and after the proposed change then that's enough for me.

Comment on lines +184 to +186
/**
* @throws AttributeModificationException {@inheritDoc}
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't {@inheritDoc} needed in this case to preserve the method & params description?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elements of a method declaration such as formal and type parameters as well as exceptions in the throws clause implicitly inherit descriptions, unless those elements are described in that method declaration. The same goes for the main description of a method: unless it exists in the method declaration, it is inherited.

To sum up, those descriptions you ask about are implicitly inherited. No additional javadoc markup is needed.

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updates to the channel classes look okay.

@pavelrappo
Copy link
Member Author

If you can assert that the generated docs are the same before and after the proposed change then that's enough for me.

Would it suffice if I show the result of the diff between JDK API Documentation built at the tip of this PR branch (docs) and that of the jdk/master commit (docs-old) from which this PR branches? If so, please have a look:

% diff -qnr build/macosx-aarch64/images/docs build/macosx-aarch64/images/docs-old                                                
Files build/macosx-aarch64/images/docs/api/index-files/index-9.html and build/macosx-aarch64/images/docs-old/api/index-files/index-9.html differ
Files build/macosx-aarch64/images/docs/api/java.base/java/lang/class-use/Exception.html and build/macosx-aarch64/images/docs-old/api/java.base/java/lang/class-use/Exception.html differ
Files build/macosx-aarch64/images/docs/api/java.base/java/lang/class-use/Override.html and build/macosx-aarch64/images/docs-old/api/java.base/java/lang/class-use/Override.html differ
Files build/macosx-aarch64/images/docs/api/java.base/java/lang/class-use/String.html and build/macosx-aarch64/images/docs-old/api/java.base/java/lang/class-use/String.html differ
Files build/macosx-aarch64/images/docs/api/java.base/java/lang/reflect/class-use/Method.html and build/macosx-aarch64/images/docs-old/api/java.base/java/lang/reflect/class-use/Method.html differ
Files build/macosx-aarch64/images/docs/api/jdk.jshell/jdk/jshell/execution/JdiDefaultExecutionControl.html and build/macosx-aarch64/images/docs-old/api/jdk.jshell/jdk/jshell/execution/JdiDefaultExecutionControl.html differ
Files build/macosx-aarch64/images/docs/api/jdk.jshell/jdk/jshell/execution/RemoteExecutionControl.html and build/macosx-aarch64/images/docs-old/api/jdk.jshell/jdk/jshell/execution/RemoteExecutionControl.html differ
Files build/macosx-aarch64/images/docs/api/member-search-index.js and build/macosx-aarch64/images/docs-old/api/member-search-index.js differ

As I said in the PR description, the differences are insignificant and confined to class-use and index structures. Well, except for jdk.jshell. But even there it might be okay for now.

@@ -200,6 +200,9 @@ public void readBytes(IIOByteBuffer buf, int len) throws IOException {
buf.setLength(len);
}

/**
* @throws EOFException {@inheritDoc}
*/
public boolean readBoolean() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the docs end up the same as you say that is fine by me since for the two ImageIO classes
that seems to be what we'd want.

But inheritDoc behaviour is still "surprising".
I've never been sure I understood it and now I'm just sure I don't.

  1. The two ImageIO methods don't have @OverRide or anything and yet javadoc infers
    they'd want to inherit the docs from the interface .. clever javadoc .. I guess I can see
    that some doc is better than none so this is OK

  2. When it does inherit I'd expected that it copies the EXACT and ENTIRE javadoc
    from the super-interface, which for your change to work can't be all its doing.
    You've added JUST
    /**

  • @throws SomeExceptionSubType blah-blah
    */

and yet javadoc somehow figures out this is to be ADDED to the super-type doc
for the method and not replace it .. ???

  1. What the old code was doing seems to me to be "natural" to the extent any of
    this does and the fix you are preparing would add to the surprising behaviours ..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the docs end up the same as you say that is fine by me since for the two ImageIO classes that seems to be what we'd want.

Since this change does not affect HTML pages for java.desktop, I take it as "approved".

But inheritDoc behaviour is still "surprising". I've never been sure I understood it and now I'm just sure I don't.

  1. The two ImageIO methods don't have @OverRide or anything and yet javadoc infers
    they'd want to inherit the docs from the interface .. clever javadoc .. I guess I can see
    that some doc is better than none so this is OK
  2. When it does inherit I'd expected that it copies the EXACT and ENTIRE javadoc
    from the super-interface, which for your change to work can't be all its doing.
    You've added JUST
    /**
  • @throws SomeExceptionSubType blah-blah
    */

and yet javadoc somehow figures out this is to be ADDED to the super-type doc for the method and not replace it .. ???

  1. What the old code was doing seems to me to be "natural" to the extent any of
    this does and the fix you are preparing would add to the surprising behaviours ..

Let me try to clarify exception documentation inheritance for you.

A method never inherits exception documentation unless that method explicitly requests to do so. A method that wants to inherit documentation for a particular exception has two options: use a doc comment or use a method declaration.

Let's see how those options work. Suppose, a method wants to inherit documentation for an exception X. To inherit documentation through a doc comment, the method adds the following exception tag (@throws or @exception) to that method's doc comment:

​    @throws X {@inheritDoc}

To inherit documentation through a method declaration, the method lists X in that method's throws clause:

​    throws ..., X, ...

If a method chooses neither of those options, then that method won't inherit exception documentation for X (assuming that exception documentation for X exists in the supermethod).

Here's an informal, simplified, but conceptually correct version of the algorithm that javadoc uses to document exceptions thrown by a method:

Step 1. For each exception tag, do the following. If an exception tag does not have {@inheritDoc} in its description, add an entry for the exception that this tag describes to the "Throws:" section. Otherwise, look for corresponding documentation in the supermethod, to which apply this step (Step 1) recursively.

Step 2. For each exception from the throws clause, do the following. If an exception has not been documented on the previous step, document it using corresponding documentation in the supermethod, to which apply this algorithm (both steps in order) recursively.

A few notes on the algorithm:

  • Exception tags are examined prior to the throws clause. This allows a method to override documentation that could be otherwise inherited from the supermethod: if the method provides exception tags for a particular exception, then documentation for that exception will be found on Step 1 and, hence, won't be looked for in the supermethod on Step 2.
  ​    @throws X <overriding-description>
  • If a method wants to add documentation for a particular exception, rather than override it, the method should both add documentation and inherit it using tags:
  ​    @throws X <additional-description>
  ​    @throws X {@inheritDoc}

The above model might explain a common misconception according to which methods inherit documentation for checked exceptions and do not inherit it for unchecked exceptions. In reality, javadoc treats all exceptions equally. It's just that if a method throws a checked exception, that exception (or its superclass) must be listed in that method's throws clause. Now, if such an exception is not documented by that method but documented by the supermethod, that exception documentation is inherited. That gives a false impression that the documentation is inherited because the exception is checked. In fact, the documentation would still be inherited if that exception, listed in the throws clause, were unchecked.

The above is how it has been working (barring bugs) since documentation comment inheritance appeared in its current form. Implicit inheritance (filling in missing text) appeared in JDK 1.3, explicit inheritance ({@inheritDoc}) appeared in JDK 1.4, auto-inheriting documentation for subclasses of exceptions whose documentation is inherited (JDK-4947455) appeared in JDK 5.

Back to this PR change in java.desktop. ImageInputStreamImpl.readBoolean inherits EOFException documentation not because that method doesn't have a doc comment of its own and, thus, inherits "the exact and entire" doc comment from its supermethod (ImageInputStream.readBoolean). It's because that when the algorithm reaches Step 2 and tries to find documentation for IOException (because it is listed in the throws clause), JDK-4947455 kicks in. And JDK-4947455 says that if a method inherits documentation for a particular exception, it should also inherit documentation for that exception's subclasses. So, javadoc goes ahead and inherits documentation for IOException, because it's a direct match, and for EOFException, because it's a subclass of IOException.

To inherit EOFException documentation after JDK-4947455 has been reverted and, thus, the subclassing 🪄 has gone, ImageInputStreamImpl.readBoolean has two options:

  • list EOFException in the throws clause
  • @throws EOFException {@inheritDoc}

Thanks to the current implementation of documentation inheritance, any of the above can be done before JDK-4947455 is reverted. For now doing so just adds a redundant but harmless inheritance route, which will become the only route in time. For this PR to be less disruptive and qualify as "noreg-doc" I chose to add an exception tag rather than amend the throws clause.

I hope that my reply clarifies the matter. Documentation inheritance can be surprising. We're trying hard to capture its model and simplify it where possible. We are constantly improving the Documentation Comment Specification for the Standard Doclet1 (spec), but since documentation inheritance is a popular topic which is often misunderstood, along with improving the spec in this area, we might also provide a less formal document dedicated to documentation inheritance.

Footnotes

  1. https://docs.oracle.com/en/java/javase/19/docs/specs/javadoc/doc-comment-spec.html

@jonathan-gibbons
Copy link
Contributor

jonathan-gibbons commented Oct 6, 2022

I've browsed all the changes and read the jshell files in more detail, since no once else has so far.

I understand the reason for the change, and generally approve of undoing the magic of the old feature in JDK-8287796. That being said, the proposed state is "not great", and feels like the interim stage of "one step back, to take two steps forward", even though I do not have a good sense at this time of what that future direction might be.

So, looks OK for now, but I hope we revisit this issue again sometime. Thanks, in general, for taking on this topic.

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm marking the change as approved, in terms of the cumulative efforts of those who have commented.

@openjdk
Copy link

openjdk bot commented Oct 6, 2022

@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:

8294377: Prepare to stop auto-inheriting documentation for subclasses of exceptions whose documentation is inherited

Reviewed-by: jjg

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 148 new commits pushed to the master branch:

  • f888aa9: 8293061: Combine CDSOptions and AppCDSOptions test utility classes
  • 73f0646: 8294839: Disable StressLongCountedLoop in compiler/loopopts/TestRemoveEmptyLoop.java
  • 2ceebf6: 8294456: Fix misleading-indentation warnings in core JDK libraries
  • ad7b7d4: 8294697: java/lang/Thread/virtual/ThreadAPI.testGetStackTrace2 failed with non-empty stack trace
  • e38ae8a: 8294759: Print actual lock/monitor ranking
  • 7012d4b: 8294837: unify Windows 2019 version check in os_windows and java_props_md
  • 8c15f77: 8270915: GIFImageReader disregards ignoreMetadata flag which causes memory exhaustion
  • 6029120: 8278086: [REDO] ImageIO.write() method will throw IndexOutOfBoundsException
  • 8f56115: 8294679: RISC-V: Misc crash dump improvements
  • e986a97: 8292330: Update JCov version to 3.0.13
  • ... and 138 more: https://git.openjdk.org/jdk/compare/91a23d775fbf482244ace5758f7b3084ea564460...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 6, 2022
@pavelrappo
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Oct 10, 2022

Going to push as commit eb90c4f.
Since your change was applied there have been 180 commits pushed to the master branch:

  • 4df4a1f: 8287832: jdk/jfr/event/runtime/TestActiveSettingEvent.java failed with "Expected two batches of Active Setting events"
  • 35d17a0: 8293864: Kitchensink24HStress.java fails with SIGSEGV in JfrCheckpointManager::lease
  • c5f462e: 8294956: GHA: qemu-debootstrap is deprecated, use the regular one
  • 269252a: 8295007: javax/swing/JRadioButton/4314194/bug4314194.java fails in mach5 for WIndowLookAndFeel
  • 6ed74ef: 8295005: compiler/loopopts/TestRemoveEmptyLoop.java fails with release VMs after JDK-8294839
  • 8a148bc: 8294848: Unnecessary SSLCipher dispose implementations
  • 8713dfa: 8294541: java/io/BufferedInputStream/TransferTo.java fails with OOME
  • 542cc60: 8294366: RISC-V: Partially mark out incompressible regions
  • 495c043: 7124710: interleaved RedefineClasses() and RetransformClasses() calls may have a problem
  • d39d8c8: 8170389: java.text.DigitList.getDouble() : Controversy between javadoc and code
  • ... and 170 more: https://git.openjdk.org/jdk/compare/91a23d775fbf482244ace5758f7b3084ea564460...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 10, 2022
@openjdk openjdk bot closed this Oct 10, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 10, 2022
@openjdk
Copy link

openjdk bot commented Oct 10, 2022

@pavelrappo Pushed as commit eb90c4f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated kulla kulla-dev@openjdk.org nio nio-dev@openjdk.org
5 participants