RFR: 8266666: Implementation for snippets [v7]

Fri Aug 20 18:53:33 UTC 2021

On Fri, 6 Aug 2021 12:24:06 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> This PR implements JEP 413 "Code Snippets in Java API Documentation", which hasn't been yet proposed to target JDK 18. The PR starts as a squashed merge of the https://github.com/openjdk/jdk-sandbox/tree/jdk.javadoc/snippets branch.
>
> Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Pass through FIXMEs and TODOs
>   
>   Downgrades FIXMEs that do not mark *feature issues* to TODOs, or removes those FIXMEs completely. For example, unlike Style hierarchy, Action hierarchy won't benefit from becoming sealed. So the respective FIXME is removed.

Generally impressive piece of work.

src/jdk.compiler/share/classes/com/sun/source/doctree/DocTreeVisitor.java line 291:

> 289: 
> 290:     /**
> 291:      * Visits a SnippetTree node.

There's been a recent update, and these class names should now be wrapped in `{@code }`

src/jdk.compiler/share/classes/com/sun/source/doctree/SnippetTree.java line 32:

> 30: /**
> 31:  * A tree node for an {@code @snippet} inline tag, as specified by
> 32:  * <a href="http://openjdk.java.net/jeps/413">JEP 413</a>.

The reference to a JEP is unusual.  The public spec for a snippet should be in the tag spec.

src/jdk.compiler/share/classes/com/sun/source/util/DocTreeScanner.java line 513:

> 511: 
> 512:     /**
> 513:      * {@inheritDoc} This implementation scans the children in left to right order.

(minor)
I would suggest/recommend a line-break after the `{@inheritDoc}`
Likewise for similar occurrences elsewhere.

src/jdk.compiler/share/classes/com/sun/source/util/DocTreeScanner.java line 516:

> 514:      *
> 515:      * @param node  {@inheritDoc}
> 516:      * @param p  {@inheritDoc}

Do you need the `{@inheritDoc}` ... isn't that the default behavior if no tag is specified?

Likewise for similar occurrences elsewhere.

src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java line 923:

> 921:          * probably be considered on a case-by-case basis, for an attribute
> 922:          * value of the {@snippet} tag we do not recurse.
> 923:          */

This discussion does not belong here.

We should specify simple defensible rules (in the spec) and implement them accordingly.
If there is nothing more obvious, I suggest the rules for (Unicode) identifiers.

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

> 1247: //                    messages.warning(
> 1248: //                            ch.getDocTreePath(see), "doclet.see.class_or_package_not_found",
> 1249: //                            tagName, seeText);

FIXME

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

> 397:                         classes.add(n.name());
> 398:                     } else if (s instanceof Style.Link l) {
> 399:                         assert !linkEncountered; // FIXME: do not assert; pick the first link

FIXME

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

> 404:                             // diagnostic output
> 405:                         }
> 406:                     } else if (s instanceof Style.Markup) {

Is the empty `if` expected?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/HtmlStyle.java line 87:

> 85:     snippet,
> 86:     //</editor-fold>
> 87: 

Suggest removing the editor-fold comments.  It'll be listed as not-yet-categorized by virtue of being in the initial list.

If the code generates standard styles, they should be listed here, so that they get documented in the sometime-forthcoming stylesheet spec.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/doclets.properties line 360:

> 358: 
> 359: doclet.tag.attribute.repeated=\
> 360:  repeated attribute: "{0}"

check the resource file for other quoted values; I think you'll find single quotes are the prevailing standard.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java line 39:

> 37: import jdk.javadoc.internal.doclets.toolkit.taglets.snippet.Parser;
> 38: import jdk.javadoc.internal.doclets.toolkit.taglets.snippet.StyledText;
> 39: import jdk.javadoc.internal.doclets.toolkit.util.Utils;

The langtools/javac/javadoc convention is to put standard JavaSE imports (`java.*`, `javax.*` ) first.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java line 96:

> 94:             // recently encountered of two, the iteration order might differ
> 95:             // from the source order
> 96:             error(writer, holder, a, "doclet.tag.attribute.repeated", a.getName().toString());

Can we use a `LinkedHashMap` or similar to preserve encounter order?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java line 178:

> 176:         }
> 177: 
> 178:         // FIXME cache parsed external snippet (WeakHashMap)

FIXME

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java line 260:

> 258:      * There's a separate issue of mapping discrepancies back to their
> 259:      * originating source in the doc comment and the external file. Maybe there
> 260:      * is a value in it, or may be there isn't. In any case, accurate mapping

"maybe"

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java line 274:

> 272:     private StyledText parse(String content) throws ParseException {
> 273:         // FIXME: need to be able to process more fine-grained, i.e. around a particular region...
> 274:         // or, which is even better, cache the styled text

FIXME

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java line 287:

> 285: 
> 286:     // FIXME: figure out how to do that correctly
> 287:     // FIXME: consider returning null from this method so it can be used as oneliner

FIXME

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java line 69:

> 67: // FIXME: How to treat Form Feed? (i.e. is it vertical or horizontal whitespace?)
> 68: // FIXME: what to do with lines not covered by any markup? (i.e. in between markup)
> 69: // FIXME: all parsing errors must be localized.

FIXME

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java line 134:

> 132:             this.eolMarker = eolMarker;
> 133:             // capture the rightmost eolMarker (e.g. "//")
> 134:             // The bellow Pattern.compile should never throw PatternSyntaxException

"below" ?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java line 177:

> 175:                 }
> 176:                 thisLineTags.addAll(parsedTags);
> 177:                 for (var tagIterator = thisLineTags.iterator(); tagIterator.hasNext(); ) {

oooh, not wrong; just unexpected mix of styles ;-)

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java line 186:

> 184:                 }
> 185:                 if (parsedTags.isEmpty()) { // (2)
> 186:                     // TODO: log this with NOTICE;

does this need to be addressed?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java line 207:

> 205: 
> 206:             append(text, Set.of(), line);
> 207:             // FIXME: mark up trailing whitespace!

FIXME

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/CommentHelper.java line 376:

> 374: 
> 375:     public TypeElement getReferencedClass(Element e) {
> 376:         Utils utils = configuration.utils;

Whinge. This new method does not really belong here, also implying that the body of the old method does not belong here (in the class) either.  Generally, queries on elements belong in `Utils`, not `CommentHelper`.

But, OK for now, I guess.

--

As a one-off, it was OK to let this slide. Given there are repeated similar occurrences later on, can you file an RFE to move the `Element` / non-`DocTree` methods to `Utils`.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/CommentHelper.java line 406:

> 404: 
> 405:     public Element getReferencedMember(Element e) {
> 406:         Utils utils = configuration.utils;

Same whinge as above.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/CommentHelper.java line 431:

> 429:     }
> 430: 
> 431:     public PackageElement getReferencedPackage(Element e) {

ditto

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/CommentHelper.java line 444:

> 442:     }
> 443: 
> 444:     public ModuleElement getReferencedModule(Element e) {

ditto

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetTag.java line 27:

> 25:  * @test
> 26:  * @bug 8201533
> 27:  * @library /tools/lib ../../lib

need @summary

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetTag.java line 81:

> 79: 
> 80:     public static void main(String... args) throws Exception {
> 81:         new TestSnippetTag().runTests(m -> new Object[]{Paths.get(m.getName())});

This is just a suggestion for future convenience.
For big tests like this, for ease of debugging, it can be useful to be able to specify the method(s) to be executed.   Maybe there should be a variants of `runTests` that takes a `String[]` or `List<String>`

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetTag.java line 1508:

> 1506:                 )
> 1507: // Uncomment when parser has improved (this would allow to write meta snippets,
> 1508: // i.e. snippets showing how to write snippets.

Just a question: what are the limitations in the parser that are getting in the way here?

test/langtools/tools/javac/doctree/SnippetTest.java line 26:

> 24: /*
> 25:  * @test
> 26:  * @bug 8201533

Need `@summary`

-------------

PR: https://git.openjdk.java.net/jdk/pull/4795