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

JDK-8267219: Javadoc method summary breaks when {@inheritDoc} from an empty parent #4066

Closed
wants to merge 2 commits into from

Conversation

@liach
Copy link
Contributor

@liach liach commented May 17, 2021

This change fixes when a method body has only inline tags that produce no output, the method summary will get eaten.

This change allows {@inheritDoc} from empty parents to go through the code path used by -nocomment and properly generate tables.

All jtreg:test/langtools/jdk/javadoc/doclet tests pass.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4066/head:pull/4066
$ git checkout pull/4066

Update a local copy of the PR:
$ git checkout pull/4066
$ git pull https://git.openjdk.java.net/jdk pull/4066/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4066

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4066.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 17, 2021

👋 Welcome back liach! 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.

Loading

@openjdk openjdk bot added the rfr label May 17, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 17, 2021

@liach The following label will be automatically applied to this pull request:

  • javadoc

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

Loading

@openjdk openjdk bot added the javadoc label May 17, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 17, 2021

Loading

* questions.
*/

public class Second {
Copy link
Member

@hns hns May 17, 2021

Choose a reason for hiding this comment

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

Shouldn't this class extend First?

Loading

Copy link
Contributor Author

@liach liach May 17, 2021

Choose a reason for hiding this comment

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

Right. Turns out this bug can be replicated with just one file as well:

public class Second {
  /**
   * {@inheritDoc}
   */
  public void act() {}
  
  public void bite() {}
}

Somehow javadoc still replaces {@inheritDoc} tag with empty output (tested with 16), and this serious bug doesn't affect the validity of the test 😅 in an unexpected way. Pushing the fixed version of test classes.

Loading

@pavelrappo
Copy link
Member

@pavelrappo pavelrappo commented May 17, 2021

Right off the bat: I'd be interested to know why an empty content demands special treatment.

Loading

@liach
Copy link
Contributor Author

@liach liach commented May 18, 2021

Right off the bat: I'd be interested to know why an empty content demands special treatment.

if an empty content stays empty, it's handled like htmltree.empty in a special case (just like what happens to nocomment) and included in table output. if it's wrapped in a div class=block, the check will not be triggered as it's not empty but yet invalid content, so when the table converts to content that row is lost.

Loading

@pavelrappo
Copy link
Member

@pavelrappo pavelrappo commented May 18, 2021

if an empty content stays empty, it's handled like htmltree.empty in a special case (just like what happens to nocomment) and included in table output. if it's wrapped in a div class=block, the check will not be triggered as it's not empty but yet invalid content, so when the table converts to content that row is lost.

Your analysis together with the fact that this PR changes code that presumably had no such issue in JDK 15 leaves me with more questions. For example, (1) Why does the lower level that prints out HTML leave out pieces of structure? (2) Why does the higher level that creates this structure knows about and works around that behavior of the lower level?

Loading

@liach
Copy link
Contributor Author

@liach liach commented May 18, 2021

So I will share how I reached this as the solution than patching at somewhere else in the code path:

First, I directly modified

check to include the case when the contents generated by first-sentence inline tags are invalid (not just empty, so <div class="block"></div> would be included). However, this inserts extraneous no-break whitespaces to all-package index and fails one test.

Then, I patched at

to make it insert a no-break whitespace in order to prevent cell deletion. However, this breaks -nocomment, and I figured that reusing -nocomment's code path would be the best approach as a result, which is the current PR.


(1) Why does the lower level that prints out HTML leave out pieces of structure?

// Replace empty content with HtmlTree.EMPTY to make sure the cell isn't dropped
HtmlTree cell = HtmlTree.DIV(cellStyle, !c.isEmpty() ? c : HtmlTree.EMPTY);

It's done by this piece of code afai see.

(2) Why does the higher level that creates this structure knows about and works around that behavior of the lower level?

This would be more fascinating. In that method, there is a whitespace insertion, which is used for like methods without /** */ comments. So there are two ways to handle the empty comments, and I find out for resolving this issue, reusing the one used by -nocomment is easier and more compatible than reusing that used by regular empty methods.


So this check in Table introduced in JDK-8256580 (#1438) might have been the culprit.

If we seek a more comprehensive change, I personally would keep the change introduced in 8256580 and remove the original no-break whitespace insertion, so we consistently yield empty content for empty comments, and we can then properly handle these empty comments downstream.

What are your thoughts?

Another update: just checked 8256580, and the problem is exactly the same as what we see here.

Loading

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

As a matter of style, these days we avoid small files like First.java and Second.java and put the source instead in a text block in the main test file and write the file(s) on the fly, typically using ToolBox.writeJavaFiles. This makes it easier to read the test all at once, instead of switching between small files that are dominated by a legal header.

Loading

Copy link
Member

@hns hns left a comment

Thanks for looking into this and doing all the detective work, @liach!

Your fix is a viable workaround, but I think we must fix the problem in Table.java, which is caused by the maybe surprising behaviour of isEmpty() and isValid() in HtmlTree. At least I find it surprising that a <div> with attributes but no content is considered empty but not valid.

But regardless of this question, the only safe fix is to replace the !isEmpty() check in line 331 of Table.java with an isValid() check. It's a bit counter-intuitive that a div with an attribute and no content will be "lost" in the process, and maybe the implementation of HtmlTree.isValid() should be changed to consider that as valid. But the important thing is to make the Table class safe against similar problems in the future.

Loading

@liach
Copy link
Contributor Author

@liach liach commented May 18, 2021

Addressed both comments from @jonathan-gibbons and @hns.

Turns out the isValid check is way better, though it took me a bit of time to debug because the row supplied to the table was wrapped in a ContentBuilder, which did not have a proper implementation of isValid (so when it has an invalid but non-empty content, it reports itself as valid)

Loading

@pavelrappo
Copy link
Member

@pavelrappo pavelrappo commented May 18, 2021

Meta: I would appreciate it if next time you could update your PR normally rather than forcefully. Pushing forcefully makes it harder to follow the progression of a PR.

Loading

@liach
Copy link
Contributor Author

@liach liach commented May 18, 2021

Meta: I would appreciate it if next time you could update your PR normally rather than forcefully. Pushing forcefully makes it harder to follow the progression of a PR.

For this change, in fact, you can pretty much discard all previous change; the current commit is not related to the previous commit contents, as the way of approach and the test both got rewritten.

For squashing and pushing, I thought it was a requirement for JDK pull requests to be merged; maybe the openjdk bot prompted me so before since I cannot find the reference on http://openjdk.java.net/contribute/. Would you mind enlightening me on how to keep commit histories while adhering to the bot's requirements?

Loading

@hns
Copy link
Member

@hns hns commented May 18, 2021

Addressed both comments from @jonathan-gibbons and @hns.

Turns out the isValid check is way better, though it took me a bit of time to debug because the row supplied to the table was wrapped in a ContentBuilder, which did not have a proper implementation of isValid (so when it has an invalid but non-empty content, it reports itself as valid)

Thanks @liach. I think I agree with your ContentBuilder.isValid() implementation, but this problem shows that the area is a bit of a mine field. It's very easy to have some content dropped silently because some isValid() implementation decided it was ok to do so. I'm not sure we need to be protected from ourselves in this way. Unfortunately, we rely on this mechanism quite heavily to suppress unwanted output. I think I want to spend a bit more time looking into this, although I think yours is the proper fix.

Loading

@pavelrappo
Copy link
Member

@pavelrappo pavelrappo commented May 18, 2021

For this change, in fact, you can pretty much discard all previous change; the current commit is not related to the previous commit contents, as the way of approach and the test both got rewritten.

Yet, it is preferable to have preceding commits. GitHub and Webrev links refer to commits by their hashes. If you spare these commits, links will break. Broken links make it harder to follow the history.

For squashing and pushing, I thought it was a requirement for JDK pull requests to be merged; maybe the openjdk bot prompted me so before since I cannot find the reference on http://openjdk.java.net/contribute. Would you mind enlightening me on how to keep commit histories while adhering to the bot's requirements?

The page you refer to, http://openjdk.java.net/contribute/, is somewhat outdated. While it is being updated, may I refer you to https://wiki.openjdk.java.net/display/SKARA instead? In particular, please read https://wiki.openjdk.java.net/display/SKARA/FAQ.

As a PR author the only thing you need to be concerned with, from a VCS perspective, is that your PR has no conflicts with the master. If that's the case, all the rebasing and squashing will be done for you automatically. Your best course of action is to respond to feedback normally by adding commits to your PR branch, not by overwriting them.

Loading

hns
hns approved these changes May 19, 2021
Copy link
Member

@hns hns left a comment

Looks good!

I briefly thought we might want to make isValid() a bit more liberal in what it considers a valid HTML element, but after taking a closer look I think it's fine as it is.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 19, 2021

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

8267219: Javadoc method summary breaks when {@inheritDoc} from an empty parent

Reviewed-by: hannesw

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

  • 99fcc41: 8234532: Remove ThreadLocalAllocBuffer::_fast_refill_waste since it is never set
  • 237fee8: 8267339: Temporarily disable os.release_multi_mappings_vm on macOS x64
  • 64e2479: 8267407: ProblemList vmTestbase/vm/mlvm/anonloader/stress/oome/metaspace/Test.java on linux-aarch64
  • 9760dba: 8267321: Use switch expression for VarHandle$AccessMode lookup
  • fdd0352: 8267338: [JVMCI] revive JVMCI API removed by JDK-8243287
  • 0b49f5a: 8267257: Shenandoah: Always deduplicate strings when it is enabled during full gc
  • 12050f0: 8266651: Convert Table method parameters from String to Content
  • e749f75: 8267304: Bump global JTReg memory limit to 768m
  • e858dd6: 8267361: JavaTokenizer reads octal numbers mistakenly
  • 1b93b81: 8267133: jdk/jfr/event/gc/collection/TestG1ParallelPhases.java fails with Not expected phases: RestorePreservedMarks, RemoveSelfForwardingPtr: expected true, was false
  • ... and 42 more: https://git.openjdk.java.net/jdk/compare/39a454bb879fe316a69a4ec33ab287db2b5837db...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.

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, @hns, @pavelrappo) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Loading

@openjdk openjdk bot added the ready label May 19, 2021
for (Content content: contents) {
if (content.isValid())
return true;
Copy link
Member

@pavelrappo pavelrappo May 19, 2021

Choose a reason for hiding this comment

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

If this is correct, then it deserves a comment. The reason is that it looks counterintuitive: I would expect isValid to have the semantics of &&, not ||.

Loading

Copy link
Contributor Author

@liach liach May 19, 2021

Choose a reason for hiding this comment

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

I looked at the usages of isValid; all of them are guarding the calls to HtmlTree.add, in SerialFieldWriter, TagletWriter, and HtmlTree.add itself. Hence, if a content builder has both valid and invalid parts, I still believe it should be added to the html tree, where only the valid subparts can be accepted. I shall document the reasons in an implSpec section.

Loading

Copy link
Member

@hns hns May 19, 2021

Choose a reason for hiding this comment

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

I was initially unsure about this, too. But take a look at HtmlTree.add(Content) which handles ContentBuilder arguments in the following way:

    if (content instanceof ContentBuilder) {
        ((ContentBuilder) content).contents.forEach(this::add);
    }

Thus, if a ContentBuilder contains any valid Content objects, they will be added, while the invalid ones will be silently dropped.

Loading

Copy link
Member

@pavelrappo pavelrappo May 19, 2021

Choose a reason for hiding this comment

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

Something is wrong, but I cannot quite put my finger on it. I think this PR merely highlights a pre-existing issue that should be investigated separately.

I'm surprised by the concept of validity for the generated content. I'm also surprised by the fact that a part of the content can be silently omitted depending on whether or not that part is valid. This suggests that javadoc uses instances of HtmlTree as its data model which it shouldn't do.

Loading

Copy link
Contributor Author

@liach liach May 19, 2021

Choose a reason for hiding this comment

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

This special path for ContentBuilder was added in JDK-8026370, commit 99e02c2, which, surprisingly, is like the opposite of this missing empty tag issue; the isValid was not touched since its addition from long ago in JDK-6851834. ContentBuilder itself was introduced in JDK-8011642 or commit f961eaf.

It is not quite clear to me the original intentions of isValid from over one decade ago. One of the clue may be that when ContentBuilder was added, jonathan maybe did not have isValid handling in mind? And we are now in a somewhat weird shape.

Loading

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons May 19, 2021

Choose a reason for hiding this comment

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

My general recollection is that this dates all the way back to the introduction of HtmlTree. Prior to that, HTML was generated directly with PrintWriter.print calls (!) and that code gave rise to some notable errors; specifically, empty lists. A list was started, beginning with <ul> or <dl>, items were added, and the list was closed. Except that sometimes no items were added. Which was generally tolerated by browsers, but not by checkers like HTML Tidy when we started using that.

Roll the clock forward to the introduction of HtmlTree. For the most part, this was done at that time by a mostly direct translation of print calls into creating and adding HtmlTree nodes. The "empty list" problem was solved by checking ex post facto that nodes were valid before adding them into an enclosing container.

The conversion was a big enough job in itself that it was not reasonable to restructure the code at that time to decide whether we should even be creating the list in the first place. It would be better to check the collection of items to be added up front, and not attempt to create an invalid HTML list with no contents in the first place.

I've not (yet) followed this thread in detail, but IMO the isValid mechanism may be a wart of Technical Debt that it is time to remove. If nothing else, the general improvements to the API over the past few years should mean that we should always and only create valid nodes in the first place, and that we should never create or need to check for invalid nodes. That being said, such a change may be a Big Deal and should probably be handled separately.

Loading

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons May 19, 2021

Choose a reason for hiding this comment

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

Exercise for the reader: it may be an interesting exercise to temporarily instrument javadoc to see if we still create any invalid nodes in either make docs or in any test.

Loading

Copy link
Contributor Author

@liach liach May 19, 2021

Choose a reason for hiding this comment

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

Thanks jonathan! Since I have documented the behavior of isValid and all the tests pass, I would assume my changes as of now is ready for this issue.

For the further practice, I guess isValid() calls will be replaced by !isEmpty() then? But this revamp is out of the scope of this issue.

Loading

@liach
Copy link
Contributor Author

@liach liach commented May 19, 2021

Since the problems around isValid is somehow deeper and beyond the scope of 8267219, I will mark this ready for integrate. The discussion on isValid deserves its dedicated issue.

Feel free to point out other problems before anyone sponsors.

/integrate

Loading

@openjdk openjdk bot added the sponsor label May 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 19, 2021

@liach
Your change (at version 93c5090) is now ready to be sponsored by a Committer.

Loading

@pavelrappo
Copy link
Member

@pavelrappo pavelrappo commented May 20, 2021

FYI, this issue manifests itself in JDK 16 API Documentation: https://docs.oracle.com/en/java/javase/16/docs/api/java.desktop/javax/swing/JInternalFrame.html

Loading

@hns
Copy link
Member

@hns hns commented May 20, 2021

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 20, 2021

@hns @liach Since your change was applied there have been 66 commits pushed to the master branch:

  • aba2265: 8260267: ZGC: Reduce mark stack usage
  • f979523: 8267463: Problemlist runtime/os/TestTracePageSizes.java on linux-aarch64 to reduce noise
  • f07dcf4: 8264290: Create implementation for NSAccessibilityComponentGroup protocol peer
  • 31320c3: 8267262: com/sun/net/httpserver/Filter improve API documentation of static methods
  • 7dcb9fd: 8265684: implement Sealed Classes as a standard feature in Java, javadoc changes
  • 0fa9223: 8260517: implement Sealed Classes as a standard feature in Java
  • 31b98e1: 8265319: implement Sealed Classes as a standard feature in Java, javax.lang.model changes
  • 726785b: 8267155: runtime/os/TestTracePageSizes times out
  • 8e3549f: 8266332: Adler32 intrinsic for x86 64-bit platforms
  • b961f25: 8267191: Avoid repeated SystemDictionaryShared::should_be_excluded calls
  • ... and 56 more: https://git.openjdk.java.net/jdk/compare/39a454bb879fe316a69a4ec33ab287db2b5837db...master

Your commit was automatically rebased without conflicts.

Pushed as commit 459abd5.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants