RFR: 8266666: Implementation for snippets [v23]

[Jonathan Gibbons]

On Thu, 9 Sep 2021 15:27:34 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:
> 
>   Remove trailing whitespace to satisfy jcheck

The primary actionable item is that all strings that are intended for use in end-user error messages need to be localizable.  i.e. see `Parser.ParseException`

Other comments are minor and can be deferred.

src/jdk.compiler/share/classes/com/sun/source/util/SimpleDocTreeVisitor.java line 460:

> 458:      * @return  the result of {@code defaultAction}
> 459:      * @since 18
> 460:      */

optional: reformat the latter part of the comment to align param descriptions ... i.e. use IntelliJ IDEA reformat code

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

> 1075:         return ch == ' ' || ch == '\t' /* || ch == '\f'*/;
> 1076:     }
> 1077: 

It would be good to resolve this issue sooner rather than later, but not right now.
I suggest we file a JBS issue to make a determination and update the doc comment world accordingly. Yes, it will be a minor change for DocCommentParser. The JBS issue will provide a good place to record the decision and its rationale.

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

> 1445:                         throw new ParseException("dc.no.content");
> 1446:                     } else {
> 1447:                         throw new ParseException("dc.unexpected.content");

Heads up, there may be a merge conflict with the upcoming work on `DCTree` diagnostic positions. `ParseException` will soon take an optional "pos" parameter to better specify the position at which the error was detected.

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

> 402:                         e = getLinkedElement(element, t);
> 403:                         if (e == null) {
> 404:                             // diagnostic output

Use TODO?

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

> 442:      * the provided signature; returns null if such element could not be found.
> 443:      *
> 444:      * This method is to be used when it's the target of the link that is

very minor ... this would read better without the contraction (replace "it's" with "it is")

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

> 453:         var fabricatedPath = new DocTreePath(rootPath, reference);
> 454:         return utils.docTrees.getElement(fabricatedPath);
> 455:     }

This feels like a plausible addition to the `DocTrees`API, where it ought to be possible to do it without creating the `ReferenceTree` and `DoctreePath` wrapper objects.

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

> 198:             var path = writer.configuration().utils.getCommentHelper(holder)
> 199:                 .getDocTreePath(snippetTag.getBody());
> 200:             // FIXME: there should be a method in Messages; that method should mirror Reporter's; use that method instead accessing Reporter.

downgrade to TODO?

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

> 268:      * diff view, but for now simply outputting both sides of a hybrid snippet
> 269:      * would do. A user could then use a diff tool of their choice to compare
> 270:      * those sides.

For discussion ... a partial solution that may be good enough is to identify the position (e.g. line+offset) of the first character that is different.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/MarkupParser.java line 87:

> 85:         if (!Character.isUnicodeIdentifierStart(ch)) {
> 86:             // TODO: internationalize!
> 87:             throw new ParseException("Bad character: '%s' (0x%s)".formatted(ch, Integer.toString(ch, 16)), bp);

Heads up: this should be updated when `ParseException` takes a position.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/ParseException.java line 50:

> 48:      * @param position the approximate position
> 49:      */
> 50:     public ParseException(String message, int position) {

I know it is common to use English-only strings in exceptions that are not intended to be seen in normal use by an end user, but here, you're using `ParseException` to contain content that is intended to appear in end-user error messages, triggered by errors in the content provided by an end-user.   As such, it is close to a firm requirement that these messages can be localized.

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

> 83:     // Incomplete actions waiting for their complementary @end
> 84:     private final Regions regions = new Regions();
> 85:     // List of tags; consumed from top to bottom

"top to bottom" is somewhere between ambiguous and confusing.  Is it file-centric or queue-as-stack -centric?

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

> 215:                 case "link" -> {
> 216:                     var target = attributes.get("target", Attribute.Valued.class)
> 217:                             .orElseThrow(() -> new ParseException("target is absent",

internationalize

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

> 232:                 case "replace" -> {
> 233:                     var replacement = attributes.get("replacement", Attribute.Valued.class)
> 234:                             .orElseThrow(() -> new ParseException("replacement is absent",

internationalize

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

> 253:                 case "start" -> {
> 254:                     var region = attributes.get("region", Attribute.Valued.class)
> 255:                             .orElseThrow(() -> new ParseException("Unnamed start",

internationalize

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

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