RFR: JDK-8075778: Add javadoc tag to avoid duplication of return information in simple situations. [v6]

Tue Dec 8 17:59:24 UTC 2020

On Mon, 7 Dec 2020 18:37:37 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

>> This change extends the functionality of the `@return` tag so that it can also be used as an inline tag in the first sentence of a description.
>> 
>> The goal is to be able to simplify the following common pattern:
>> 
>>     /**
>>      * Returns the result. Optional additional text.
>>      * @return the result
>>      */
>>     int method() { 
>> 
>> by 
>> 
>>     /**
>>      * {@return the result} Optional additional text.
>>      */
>>     int method() { 
>> 
>> Note:
>> 
>> * The inline tag may only be used at the beginning of the description. A warning will be given if it is used elsewhere.
>> * The expansion of the inline tag is `Returns " _content_ `.`  where _content_ is the content of the tag. 
>> * If there is no block `@return` tag, the standard doclet will look for an inline tag at the beginning of the description
>> * The inline tag can be inherited into overriding methods as if it was provided as a block tag.
>
> Jonathan Gibbons has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'upstream/master' into new-return
>  - Update JShell to handle inline `{@return}`
>  - Merge remote-tracking branch 'upstream/master' into new-return
>  - fix test
>  - Update for new `@return` tag
>  - Merge remote-tracking branch 'upstream/master' into new-return
>  - Update DocCommentParser to permit nested inline tags in specified cases: @return
>  - Add default impl for new method
>  - Fix test failure
>  - Fix trailing whitespace in test
>  - ... and 2 more: https://git.openjdk.java.net/jdk/compare/8343a9ff...a25dadca

1. Although the change generally looks good, there are some issues that should be fixed.
2. I have NOT reviewed the `jdk.internal.shellsupport.doc` portion of the change in detail; please ask someone else to do that.
3. When I run this change through our CI, the `tools/javac/diags/CheckExamples.java` test fails consistently on all platforms. Try to merge with the latest `master` to rule out this PR as a cause.
4. Consider updating the copyright years when addressing this feedback.

src/jdk.compiler/share/classes/com/sun/source/util/DocTreeFactory.java line 277:

> 275:      * @param description the description of the return value of a method
> 276:      * @return a {@code ReturnTree} object
> 277:      * @throws UnsupportedOperationException if inline {@code {@return} } tags are

To be consistent with the rest of the file, I suggest using `{@code {@return }}` instead of `{@code {@return} }` (note the position of the whitespace).

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

> 273:                         return tp.parse(p, TagParser.Kind.BLOCK);
> 274:                     } else {
> 275:                             return erroneous("dc.bad.inline.tag", p);

This indentation fits the current `switch` but not the proposed `if` statement.

src/jdk.compiler/share/classes/com/sun/source/doctree/ReturnTree.java line 44:

> 42:      * Returns whether this instance is an inline tag.
> 43:      *
> 44:      * @return {@code true} if this instance is an inline tag

Although I'm not sure how `boolean` queries are generally specified in the `DocTree` API, consider explicitly specifying that this method returns `false` if called on a non-inline instance.

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

> 331:                     DCTree text = inlineText(WhitespaceRetentionPolicy.REMOVE_ALL); // skip content
> 332:                     nextChar();
> 333:                     return m.at(p).newUnknownInlineTagTree(name, List.of(text)).setEndPos(bp);

There's a discrepancy between the inline comment and code. While the comment says that the `else` clause handles a block tag, the returned tree corresponds to an (unknown) inline tag.

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

> 1130: 
> 1131:     /**
> 1132:      * @see <a href="https://docs.oracle.com/en/java/javase/14/docs/specs/javadoc/doc-comment-spec.html">JavaDoc Tags</a>

Consider updating the version to 15. Later, we'd hopefully be able to use the [`@spec` tag](https://bugs.openjdk.java.net/browse/JDK-8226279) here.

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

> 1124:             return parse(pos);
> 1125:         }
> 1126:         DCTree parse(int pos) throws ParseException {

Please add an empty line between those two to separate the methods.

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 690:

> 688: 
> 689:         @Override
> 690:         public String getTagName() {

Isn't this method missing `@DefinedBy(Api.COMPILER_TREE)`?

src/jdk.compiler/share/classes/jdk/internal/shellsupport/doc/JavadocFormatter.java line 268:

> 266:         /**
> 267:          * {@inheritDoc}
> 268:          * {@code @return} is a bimodfal tage and can be used as either a block tag or an inline

Fix the typos in "bimodfal tage".

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/resources/doclint.properties line 60:

> 58: dc.param.name.not.found = @param name not found
> 59: dc.ref.not.found = reference not found
> 60: dc.return.not.first = '{@return'} not at beginning of description

I can see an inconsistency here. While the templates for `@return` and `@code` put an apostrophe immediately before the closing `}`, the template for `@value` (further below in this file) puts an apostrophe immediately after the closing `}`.

test/langtools/jdk/internal/shellsupport/doc/JavadocFormatterTest.java line 140:

> 138:                    "1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234\n" +
> 139:                    "1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234\n" +
> 140:                    "1234 1234 1234 1234 1234 1234 1234 1234 1234 \n";

What's that about? I mean is it related to this work on `{@return}`?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ReturnTaglet.java line 76:

> 74:         } else {
> 75:             List<? extends DocTree> firstSentence = utils.getFirstSentenceTrees(input.element);
> 76:             if (firstSentence.size() == 1 && firstSentence.get(0).getKind() == DocTree.Kind.RETURN) {

There's one more place where `firstSentence.size() == 1` is checked. I wonder if it could be a simpler `!firstSentence.isEmpty()` check?

Are there any non-trivial cases which might bite us later, should we change that check?

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

Changes requested by prappo (Reviewer).

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