8287379: Using @inheritDoc in an inapplicable context shouldn't crash javadoc by pavelrappo · Pull Request #54 · openjdk/jdk19

pavelrappo · 2022-06-21T16:12:44Z

This rights the wrongs of JDK-8008768. For more information, see the respective 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
Change requires a CSR request to be approved

Issues

JDK-8287379: Using @inheritdoc in an inapplicable context shouldn't crash javadoc
JDK-8288841: Using @inheritdoc in an inapplicable context shouldn't crash javadoc (CSR)

Reviewers

Jonathan Gibbons (@jonathan-gibbons - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 54

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk19/pull/54.diff

We can do this safely because the `trees` parameter is never null in any of the 3 call sites of the checkTags method.

The `element` cannot be null for various reasons: 1. If `element` were null, `getCommentHelper` would NPE (because it calls `Utils.getDocCommentInfo` which NPEs on a null element). 2. The Overview and Html files correspond to the OverviewElement and DocFileElement pseudo-elements, which are passed to checkTags when respective files are processed. Existing tests and ad-hoc experiments support that.

@inheritdoc

Aside from @inheritdoc, which is context-dependent, we now have a bimodal tag such as {@return}/@return.

overview.html and doc-files/**/*.html files cannot have a main description or be an empty comment. At the very least, the check for being "an empty comment" for such files cannot be performed by checking if the files contain any block tags. Block tags are applicable to a program element, which those files are not.

The type of a declaration (module, class or interface, constructor, method, etc.) for which a tag is applicable, is orthogonal to the type of the tag (inline, block, bimodal). The code up the stack knows which type of tags it has collected. If those tags are of type other than expected, it's a programming error.

This undoes undocumented changes introduced by 8008768.

@inheritdoc

1. Removes @inheritdoc from these type of declarations: * class and interface * constructor 2. Removes empty declarations. 3. Updates @APinote, @implSpec and @implNote definitions to match those of JDK API. 4. Fixes a few typos.

pavelrappo · 2022-06-21T16:12:52Z

/csr

bridgekeeper · 2022-06-21T16:19:56Z

👋 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 · 2022-06-21T16:22:34Z

@pavelrappo this pull request will not be integrated until the CSR request JDK-8288841 for issue JDK-8287379 has been approved.

openjdk · 2022-06-21T16:24:50Z

@pavelrappo 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.

mlbridge · 2022-06-21T16:27:41Z

Webrevs

jonathan-gibbons

It worries me that we are removing support for a feature that had explicit positive tests.

jonathan-gibbons · 2022-06-27T17:37:22Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java


+    // Checks if the passed tree path does NOT correspond to an entity, such as
+    // the overview file and doc-files/**/*.html files.
+    private boolean notPseudoElement(TreePath p) {


The use of a predicate named not... seems confusing and leads to various double negatives; it would be easier to read if this was isPseudoElement even if that means an extra ! at the call site.

We have public Stream.noneMatch(), Files.notExists(), Objects.nonNull(), and many more non-public methods that answer a negative question; but I'll change this one if you think that it will improve readability.

It worries me that we are removing support for a feature that had explicit positive tests.

We discussed this offline at length. The tests are not specifically for the feature, but instead, they are tests for a different feature (relative links) in a variety of situations, including the suspect case of {@inheritDoc}.

It might be worth investigating whether we can write tests for relative links when {@inheritDoc} is used in a legal position, such as on a method declaration.

We have public Stream.noneMatch(), Files.notExists(), Objects.nonNull(), and many more non-public methods that answer a negative question; but I'll change this one if you think that it will improve readability.

Would prefer to see it inverted, if you don't mind. There are good reasons for the negative form in the 3 cases you list, and none of those reasons apply here. Files.notExists is a fun one because it is not !Files.exists

jonathan-gibbons · 2022-06-27T17:48:13Z

test/langtools/jdk/javadoc/doclet/testRelativeLinks/TestRelativeLinks.java

-            """
-                <a href="relative-class-link.html">relative class link</a>""",
-            """
-                <a href="#class-fragment">fragment class link</a>""",
-            """
-                <a id="class-fragment">Class fragment</a>""",


Why are these lines (and similar, below) being deleted?

Because they come from {@inheritDoc} on a class' main description. Such a use case is undocumented, unsupported, and is now being prohibited by this PR.

jonathan-gibbons · 2022-06-27T17:49:18Z

test/langtools/jdk/javadoc/doclet/testRelativeLinks/pkg/C.java

-/**
- * Here are two relative links in a class:
- * <a href="relative-class-link.html">relative class link</a>,
- * <a href="#class-fragment">fragment class link</a>.
- *
- * <a id="class-fragment">Class fragment</a>.
- */


Why are these being deleted?

jonathan-gibbons · 2022-06-27T22:04:42Z

This is a general response to the edits in testRelativeLinks.

There are various circumstances in which relative links need to be fixed up, all related to taking content containing links from one place and using it in another. These include:

the first sentence of a class or interface comment, showing up in a package summary,
the first sentence of a package comment showing up in a module summary,
info being inherited from one file into another by means of {@inherotDoc},
etc ... info showing up on "other pages", like serialized-form, constant-values.

While I do not expect a full review of all these use cases, the intent of the use of {@inheritDoc} in testRelativeLinks was good and well-meaning, even if the specific instances are now seen in hindsight as bad. Bottom line: we should not simply remove the bad cases: we should replace them with good cases, meaning, test relative links with {@inheritDoc} used in method descriptions and/or tags.

This addresses Jon's fair concerns on me aggressively removing some test cases, that could still work.

pavelrappo · 2022-07-07T20:11:02Z

This is a general response to the edits in testRelativeLinks.

There are various circumstances in which relative links need to be fixed up, all related to taking content containing links from one place and using it in another. These include:

the first sentence of a class or interface comment, showing up in a package summary,

the first sentence of a package comment showing up in a module summary,

info being inherited from one file into another by means of {@inherotDoc},

etc ... info showing up on "other pages", like serialized-form, constant-values.

While I do not expect a full review of all these use cases, the intent of the use of {@inheritDoc} in testRelativeLinks was good and well-meaning, even if the specific instances are now seen in hindsight as bad. Bottom line: we should not simply remove the bad cases: we should replace them with good cases, meaning, test relative links with {@inheritDoc} used in method descriptions and/or tags.

I accept that I nixed some tests overly aggressively. I carefully returned them back in 7d540c4. Note that since {@inheritDoc} is no longer allowed on a class or interface declaration, I cannot return all of the tests I deleted.

If fixing up relative links needs to be tested for a top-level declaration, then they have to be tested some other way, outside of the context of this PR; do you agree?

jonathan-gibbons · 2022-07-11T15:01:42Z

This is a general response to the edits in testRelativeLinks.
There are various circumstances in which relative links need to be fixed up, all related to taking content containing links from one place and using it in another. These include:

the first sentence of a class or interface comment, showing up in a package summary,

the first sentence of a package comment showing up in a module summary,

info being inherited from one file into another by means of {@inherotDoc},

etc ... info showing up on "other pages", like serialized-form, constant-values.

While I do not expect a full review of all these use cases, the intent of the use of {@inheritDoc} in testRelativeLinks was good and well-meaning, even if the specific instances are now seen in hindsight as bad. Bottom line: we should not simply remove the bad cases: we should replace them with good cases, meaning, test relative links with {@inheritDoc} used in method descriptions and/or tags.

I accept that I nixed some tests overly aggressively. I carefully returned them back in 7d540c4. Note that since {@inheritDoc} is no longer allowed on a class or interface declaration, I cannot return all of the tests I deleted.

If fixing up relative links needs to be tested for a top-level declaration, then they have to be tested some other way, outside of the context of this PR; do you agree?

That's not the right question. The question is, how can we best replace the functionality of those tests. It's the relative link that's important, not the location. Since the comment containing the relative link can no longer be placed on a top level declaration, they should be moved to a declaration where they are legal, such as on a method.

pavelrappo · 2022-07-11T15:17:06Z

That's not the right question. The question is, how can we best replace the functionality of those tests. It's the relative link that's important, not the location. Since the comment containing the relative link can no longer be placed on a top level declaration, they should be moved to a declaration where they are legal, such as on a method.

Hm... While I understand what you are saying, I also think that we already have tests that exercise relative links on methods. Those tests are located in that same directory, no? Here, test/langtools/jdk/javadoc/doclet/testRelativeLinks/pkg/C.java:

public class C {

    /**
     * Here is a relative link in a field:\u0130
     * <a href="relative-field-link.html">relative field link</a>.
     */
    public C field = null;

    /**
     * Here are two relative links in a method:
     * <a href="relative-method-link.html">relative method link</a>,
     * <a href="#method-fragment">fragment method link</a>.
     */
    public C method() { return null;}

    /**
     * Here is a relative link in a method:
     * <a
     * href="relative-multi-line-link.html">relative-multi-line-link</a>.
     *
     * <a id="method-fragment">Method fragment</a>.
     */
    public C multipleLineTest() { return null;}

jonathan-gibbons · 2022-07-11T15:30:02Z

That's not the right question. The question is, how can we best replace the functionality of those tests.

I looked at the complete test in more detail. The test is specifically about {@inheritDoc} in user tags, and the concern was about losing coverage. But I see that we already have enough test cases for still-legal positions, so it is OK to remove the tests for now-illegal positions.

jonathan-gibbons · 2022-07-11T15:35:12Z

test/langtools/jdk/javadoc/doclet/InheritDocForUserTags/DocTest.java

@@ -64,16 +57,6 @@ public void test() {
        checkOutput(Output.STDERR, false, "at com.sun");


This is a general comment for a followup discussion and future work.

Should we have a standard default-on check in JavadocTester for "no stack traces"? That is, the equivalent of this line here. We could model such a check on checkLinks or checkAccessibility such that a hypothetical checkNoCrashes could be disabled in the (rare?) cases that they might be expected. (For example, checking the tool/doclet behavior when a crash does occur.)

This is a general comment for a followup discussion and future work.

While the bug and this test are about "no crashes", we should ensure that we have positive tests elsewhere for the actual output generated, assuming that no crash occurred.

We have such a check already, but it's used specifically for snippets. If you want, we could "hoist" that check from SnippetTester into JavadocTester. However, I would prefer doing that in a separate PR.

public class SnippetTester extends JavadocTester { ... /* * When checking for errors, it is important not to confuse one error with * another. This method checks that there are no crashes (which are also * errors) by checking for stack traces. We never expect crashes. */ protected void checkNoCrashes() { checking("check crashes"); Matcher matcher = Pattern.compile("\\s*at.*\$.*\\.java:\\d+\$") .matcher(getOutput(Output.STDERR)); if (!matcher.find()) { passed(""); } else { failed("Looks like a stacktrace: " + matcher.group()); } } ...

openjdk · 2022-07-11T15:39:34Z

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

8287379: Using @inheritDoc in an inapplicable context shouldn't crash javadoc

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

fed3af8: 8287809: Revisit implementation of memory session
cb6e9cb: 8290004: [PPC64] JfrGetCallTrace: assert(_pc != nullptr) failed: must have PC
0494291: 8289692: JFR: Thread checkpoint no longer enforce mutual exclusion post Loom integration
25f4b04: 8289894: A NullPointerException thrown from guard expression
b542bcb: 8289729: G1: Incorrect verification logic in G1ConcurrentMark::clear_next_bitmap
c86c51c: 8282071: Update java.xml module-info
9981c85: 8290033: ProblemList serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java on windows-x64 in -Xcomp mode
c142fbb: 8289697: buffer overflow in MTLVertexCache.m: MTLVertexCache_AddGlyphQuad
eeaf0bb: 8289872: wrong wording in @param doc for HashMap.newHashMap et. al.
460d879: 8289601: SegmentAllocator::allocateUtf8String(String str) should be clarified for strings containing \0
... and 5 more: https://git.openjdk.org/jdk19/compare/3212dc9c6f3538e1d0bd1809efd5f33ad8b47701...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.

pavelrappo · 2022-07-11T15:41:59Z

/integrate

openjdk · 2022-07-11T15:43:34Z

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

fed3af8: 8287809: Revisit implementation of memory session
cb6e9cb: 8290004: [PPC64] JfrGetCallTrace: assert(_pc != nullptr) failed: must have PC
0494291: 8289692: JFR: Thread checkpoint no longer enforce mutual exclusion post Loom integration
25f4b04: 8289894: A NullPointerException thrown from guard expression
b542bcb: 8289729: G1: Incorrect verification logic in G1ConcurrentMark::clear_next_bitmap
c86c51c: 8282071: Update java.xml module-info
9981c85: 8290033: ProblemList serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java on windows-x64 in -Xcomp mode
c142fbb: 8289697: buffer overflow in MTLVertexCache.m: MTLVertexCache_AddGlyphQuad
eeaf0bb: 8289872: wrong wording in @param doc for HashMap.newHashMap et. al.
460d879: 8289601: SegmentAllocator::allocateUtf8String(String str) should be clarified for strings containing \0
... and 5 more: https://git.openjdk.org/jdk19/compare/3212dc9c6f3538e1d0bd1809efd5f33ad8b47701...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-07-11T15:43:46Z

@pavelrappo Pushed as commit 62fbc3f.

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

pavelrappo added 8 commits June 21, 2022 16:58

Remove null-check

438a7db

We can do this safely because the `trees` parameter is never null in any of the 3 call sites of the checkTags method.

Expand checkTags to include non-inline tag

1dea10d

Aside from @inheritdoc, which is context-dependent, we now have a bimodal tag such as {@return}/@return.

Disallow @inheritdoc in type declarations

31d38ca

This undoes undocumented changes introduced by 8008768.

Add tests

7e13ca6

Update DocTest

2a81009

1. Removes @inheritdoc from these type of declarations: * class and interface * constructor 2. Removes empty declarations. 3. Updates @APinote, @implSpec and @implNote definitions to match those of JDK API. 4. Fixes a few typos.

openjdk bot changed the title ~~8287379~~ 8287379: Using @inheritDoc in an inapplicable context crashes javadoc Jun 21, 2022

openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Jun 21, 2022

openjdk bot added the javadoc javadoc-dev@openjdk.org label Jun 21, 2022

openjdk bot removed the csr Pull request needs approved CSR before integration label Jun 21, 2022

pavelrappo changed the title ~~8287379: Using @inheritDoc in an inapplicable context crashes javadoc~~ 8287379: Using @inheritDoc in an inapplicable context shouldn't crash javadoc Jun 23, 2022

jonathan-gibbons reviewed Jun 27, 2022

View reviewed changes

pavelrappo added 2 commits June 27, 2022 20:56

Merge remote-tracking branch 'jdk19/master' into 8287379

e3308c1

feedback: make method positive

3d88d48

pavelrappo added 3 commits July 4, 2022 20:19

Merge remote-tracking branch 'jdk19/master' into 8287379

b6e2df8

Merge remote-tracking branch 'jdk19/master' into 8287379

f8b4bbb

[feedback] revert some tests

7d540c4

This addresses Jon's fair concerns on me aggressively removing some test cases, that could still work.

jonathan-gibbons reviewed Jul 11, 2022

View reviewed changes

jonathan-gibbons approved these changes Jul 11, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 11, 2022

openjdk bot added the integrated Pull request has been integrated label Jul 11, 2022

openjdk bot closed this Jul 11, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 11, 2022

		@@ -64,16 +57,6 @@ public void test() {
		checkOutput(Output.STDERR, false, "at com.sun");

Conversation

pavelrappo commented Jun 21, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

pavelrappo commented Jun 21, 2022

Uh oh!

bridgekeeper bot commented Jun 21, 2022

Uh oh!

openjdk bot commented Jun 21, 2022

Uh oh!

openjdk bot commented Jun 21, 2022

Uh oh!

mlbridge bot commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jonathan-gibbons left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons commented Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pavelrappo commented Jul 7, 2022

Uh oh!

jonathan-gibbons commented Jul 11, 2022

Uh oh!

pavelrappo commented Jul 11, 2022

Uh oh!

jonathan-gibbons commented Jul 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jul 11, 2022

Uh oh!

pavelrappo commented Jul 11, 2022

Uh oh!

openjdk bot commented Jul 11, 2022

Uh oh!

openjdk bot commented Jul 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

pavelrappo commented Jun 21, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 21, 2022 •

edited

Loading

jonathan-gibbons Jun 27, 2022 •

edited

Loading

jonathan-gibbons commented Jun 27, 2022 •

edited

Loading

jonathan-gibbons Jul 11, 2022 •

edited

Loading