JDK-8257925 enable more support for nested inline tags #2369

jonathan-gibbons · 2021-02-03T04:28:21Z

Please review an update to improve the support, where appropriate, for nested inline tags.

It has always been the case that certain inline tags allow text and HTML to appear within them. With tags like {@return} and {@summary} it becomes desirable to also generally allow nested inline tags to appear in those places as well. The work for this was started with the support for {@return} JDK-8075778, but applying the work more generally was out of scope at the time. This change completes the work that was started then.

The work can be grouped into 4 parts, in 3 commits.

Commit 1

Update DocCommentParser to syntactically allow nested inline tags in situations that permit text and HTML
Update the downstream code to semantically limit nestg where it does not make sense, and provide new tests to verify the behavior.

A family of new tests are added, each testing the ability to put an inline tag within another of the same kind, with and without doclint being enabled. In addition, test cases are added placing a simple instance {@value} in an enclosing tag: this is a useful case just because the expansion is plain text and therefore valid in all situations. Additional tests and test cases can be added as needed.

This commit left the {@index} tag generating "bad" code when it was nested. The error was "reference to an undeclared ID". The (temporary) solution was to disable the automatic link checking for this specific subtest.

Commit 2

HtmlDocletWriter and TagletWriterImpl pass around a pair of booleans isFirstSentence and inSummary to help determine the output to be generated. Conceptually, a third value is added to that group: a set containing the set of nested tag kinds, so that it is possible to determine the enclosing tags for a tag. But, rather than add a third parameter to be passed around, the 3 are grouped into a new class TagletWriterImpl.Context which encapsulates the two booleans and the new set. The new class is added in a way to minimize code churn. No tests are affected by this change: all continue to pass.

Commit 3

The new Context#inTags field is used to help improve the behavior of nested {@index} tags even when used incorrectly, with warnings disabled. As a result, the temporary change in the first commit to disable automatic link checking in one of the test cases is reverted.

The introduction of the new Context class is arguably more general than we need at this time, but it clears up some erratic and inconsistent use of the isFirstSentence and inSummary booleans. The new class also provides a better framework for any complex new inline tags we may add in future. We might want to change the Set<DocTree.Kind> to some other collection at some point, if needs be (a stack, for example.) We might also want to move more information into the Context, such as the related Element that is otherwise ubiquitously passed around.

The overall cleanup also revealed some latent bugs in the code, that were hidden in some of the tests. Most notable was that there were still some cases were < and > were not being correctly escaped as < and > leading to output in some tests of the form List<String> ! This triggered a minor cleanup/rewrite of the beginning of HtmlDocletWriter.seeTagsToContent which was previously a bit too liberal with the use of new RawHtml! The other minor cleanup was more consistent handling of whitespace at the end of the first sentence, as will be seen in a couple of places in one of the tests that was updated.

Progress

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

Issue

JDK-8257925: Enable more support for nested inline tags

Reviewers

Pavel Rappo (@pavelrappo - Reviewer) ⚠️ Review applies to 96bfeb5
Hannes Wallnöfer (@hns - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2369/head:pull/2369
$ git checkout pull/2369

…Index} tag

bridgekeeper · 2021-02-03T04:29:03Z

👋 Welcome back jjg! 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 · 2021-02-03T04:30:18Z

@jonathan-gibbons The following labels will be automatically applied to this pull request:

compiler
javadoc

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.

mlbridge · 2021-02-03T04:41:03Z

Webrevs

pavelrappo · 2021-02-03T12:28:06Z

Although this patch generally looks reasonable, I'm surprised to see some of the behavior it introduces. For example, consider composition of @link tags from the included tests:

/** First sentence. {@link #m1() ABC {@link #m2() DEF} GHI} */

Before the patch that doc comment resulted in the following HTML:

First sentence. <a href="#m1()"><code>ABC {@link #m2() DEF} GHI</code></a>

After the patch that same doc comment results in HTML that looks like this:

First sentence. <a href="#m1()"><code>ABC <a href="#m2()"><code>DEF</code></a> GHI</code></a>

I wouldn't expect that latter behavior. Not only nested <a> are invalid HTML, but they are also rendered strangely in browsers. In this particular case the trailing GHI appears as normal text, not as a hyperlink.

jonathan-gibbons · 2021-02-04T22:50:53Z

I wouldn't expect that latter behavior. Not only nested <a> are invalid HTML, but they are also rendered strangely in browsers. In this particular case the trailing GHI appears as normal text, not as a hyperlink.

Thanks; it was on my list to check whether nested links were allowed. I'm pleased they're not, and this will be another situation where the inTags info is useful.

pavelrappo

Thanks for fixing the nested <a> issue. With your last commit (96bfeb5) the output from

/** First sentence. {@link #m1() ABC {@link #m2() DEF} GHI} */

looks like this

First sentence. <a href="#m1()"><code>ABC DEF GHI</code></a>

and if doclint is enabled, javadoc issues a warning

warning: nested tag: @link
/** First sentence. {@link #m1() ABC {@link #m2() DEF} GHI} */
                                     ^

I would recommend comparing JDK API documentation before and after, if only to bring any uncovered surprises to maintainers' attention early.

openjdk · 2021-02-12T13:50:41Z

@jonathan-gibbons this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout inline-tags
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

hns

That's a very nice improvement! I have added a few comments and suggestions, but nothing of major importance.

hns · 2021-02-12T13:31:47Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java

+                    }
+                    default -> {
+                        assert false;
+                        return HtmlTree.EMPTY;


What is the reason to assert false here and in other places instead of, say, always throw a RuntimeException?

This is the general discussion about the use of assert, and whether it is better to throw an exception in production code, or "carry on regardless". I wish Java had a should-not-happen statement or at least a ShoudNotHappen exception.

In this case, IllegalStateException is a reasonable alternative

hns · 2021-02-12T14:03:48Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java

+                        Content label = commentTagsToContent(node, element, node.getLabel(), context);
+                        if (label.isEmpty()) {
+                            label = new StringContent(node.getReference().getSignature());
+                        }


That's quite a bit of work for something probably very few people should try...

hns · 2021-02-12T14:14:42Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java

@@ -1576,7 +1634,7 @@ public Boolean visitLiteral(LiteralTree node, Content c) {
                @Override
                public Boolean visitSee(SeeTree node, Content c) {
                    // we need to pass the DocTreeImpl here, so ignore node
-                    result.add(seeTagToContent(element, tag));
+                    result.add(seeTagToContent(element, tag, context));


Is above comment still correct? Aren't tag and node the same object?

Yes, updated

hns · 2021-02-12T14:25:23Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/TagletWriterImpl.java

@@ -109,7 +164,24 @@ public TagletWriterImpl(HtmlDocletWriter htmlWriter, boolean isFirstSentence) {
    public TagletWriterImpl(HtmlDocletWriter htmlWriter, boolean isFirstSentence, boolean inSummary) {
        super(isFirstSentence);


To avoid almost identical constructors this one could be implemented as

this(htmlWriter, new Context(isFirstSentence, inSummary));

hns · 2021-02-12T15:01:17Z

test/langtools/jdk/javadoc/doclet/testNestedInlineTags/TestNestedLinkTag.java

+                "{@link",
+                "ABC <a href=\"#m2()\"");
+        checkOutput("p/C.html", true,
+                "ABC DEF GHI");


Since you are running this with and without doclint, should this or any test in this file check for doclint warnings?

openjdk · 2021-02-16T19:49:30Z

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

8257925: enable more support for nested inline tags

Reviewed-by: prappo, 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 42 new commits pushed to the master branch:

7e2c909: 8260485: Simplify and unify handler vectors in Posix signal code
c99eeb0: 8260858: Implementation specific property xsltcIsStandalone for XSLTC Serializer
5caf686: 8261644: NMT: Simplifications and cleanups
ed93bc9: 8196301: java/awt/print/PrinterJob/Margins.java times out
7e78c77: 8261905: Move implementation of OopStorage num_dead related functions
78cde64: 8261860: Crash caused by lambda proxy class loaded in Shutdown hook
c158413: 8261098: Add clhsdb "findsym" command
0c31d5b: 8261977: Fix comment for getPrefixed() in canonicalize_md.c
9cf4f90: 8261473: Shenandoah: Add breakpoint support
c4664e6: 8261940: Fix references to IOException in BigDecimal javadoc
... and 32 more: https://git.openjdk.java.net/jdk/compare/61a659f4bfe066250af871b9d84943bed526e71d...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.

hns

Looks good!

jonathan-gibbons · 2021-02-19T15:58:30Z

/integrate

openjdk · 2021-02-19T15:59:28Z

@jonathan-gibbons Since your change was applied there have been 48 commits pushed to the master branch:

433096a: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths
efbaede: 8262018: Wrong format in SAP copyright header of OsVersionTest
55463b0: 8261984: Shenandoah: Remove unused ShenandoahPushWorkerQueuesScope class
a180a38: 8260694: (fc) Clarify FileChannel.transferFrom to better describe "no bytes available" case
1b0c36b: 8261649: AArch64: Optimize LSE atomics in C++ code
61820b7: 8259984: IGV: Crash when drawing control flow before GCM
7e2c909: 8260485: Simplify and unify handler vectors in Posix signal code
c99eeb0: 8260858: Implementation specific property xsltcIsStandalone for XSLTC Serializer
5caf686: 8261644: NMT: Simplifications and cleanups
ed93bc9: 8196301: java/awt/print/PrinterJob/Margins.java times out
... and 38 more: https://git.openjdk.java.net/jdk/compare/61a659f4bfe066250af871b9d84943bed526e71d...master

Your commit was automatically rebased without conflicts.

Pushed as commit c4f17a3.

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

jonathan-gibbons added 4 commits February 1, 2021 12:22

JDK-8257925: enable more support for nested inline tags

4687b19

initial introduction of TagletWriterImpl.Context

b4e75d1

use TagletWriterImpl.Context#inTags to modify behavior on a nested {@…

1552c5e

…Index} tag

neaten alignment in test description

b30d618

openjdk bot added javadoc javadoc-dev@openjdk.org compiler compiler-dev@openjdk.org labels Feb 3, 2021

fix trailing whitespace

bd84b5d

openjdk bot added the rfr Pull request is ready for review label Feb 3, 2021

improve handling of nested links

96bfeb5

pavelrappo approved these changes Feb 12, 2021

View reviewed changes

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Feb 12, 2021

hns approved these changes Feb 12, 2021

View reviewed changes

Merge with upstream/master

a6f3eb3

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Feb 16, 2021

Address review comments

bbb98e6

hns approved these changes Feb 19, 2021

View reviewed changes

openjdk bot closed this Feb 19, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 19, 2021

jonathan-gibbons deleted the inline-tags branch February 19, 2021 15:59

JDK-8257925 enable more support for nested inline tags #2369

JDK-8257925 enable more support for nested inline tags #2369

jonathan-gibbons commented Feb 3, 2021 •

edited by openjdk bot

bridgekeeper bot commented Feb 3, 2021

openjdk bot commented Feb 3, 2021

mlbridge bot commented Feb 3, 2021 •

edited

pavelrappo commented Feb 3, 2021

jonathan-gibbons commented Feb 4, 2021

pavelrappo left a comment

openjdk bot commented Feb 12, 2021

hns left a comment

hns Feb 12, 2021

jonathan-gibbons Feb 18, 2021

jonathan-gibbons Feb 18, 2021

hns Feb 12, 2021

hns Feb 12, 2021

jonathan-gibbons Feb 18, 2021

hns Feb 12, 2021

jonathan-gibbons Feb 18, 2021

hns Feb 12, 2021

openjdk bot commented Feb 16, 2021 •

edited

hns left a comment

jonathan-gibbons commented Feb 19, 2021

openjdk bot commented Feb 19, 2021

		@@ -109,7 +164,24 @@ public TagletWriterImpl(HtmlDocletWriter htmlWriter, boolean isFirstSentence) {
		public TagletWriterImpl(HtmlDocletWriter htmlWriter, boolean isFirstSentence, boolean inSummary) {
		super(isFirstSentence);

JDK-8257925 enable more support for nested inline tags #2369

JDK-8257925 enable more support for nested inline tags #2369

Conversation

jonathan-gibbons commented Feb 3, 2021 • edited by openjdk bot

Commit 1

Commit 2

Commit 3

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Feb 3, 2021

openjdk bot commented Feb 3, 2021

mlbridge bot commented Feb 3, 2021 • edited

Webrevs

pavelrappo commented Feb 3, 2021

jonathan-gibbons commented Feb 4, 2021

pavelrappo left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 12, 2021

hns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Feb 16, 2021 • edited

hns left a comment

Choose a reason for hiding this comment

jonathan-gibbons commented Feb 19, 2021

openjdk bot commented Feb 19, 2021

jonathan-gibbons commented Feb 3, 2021 •

edited by openjdk bot

mlbridge bot commented Feb 3, 2021 •

edited

openjdk bot commented Feb 16, 2021 •

edited