Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8075778: Add javadoc tag to avoid duplication of return information in simple situations. #1355

Closed
wants to merge 15 commits into from

Conversation

@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Nov 20, 2020

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.

Progress

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

Issue

  • JDK-8075778: Add javadoc tag to avoid duplication of return information in simple situations.

Reviewers

Download

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

…on in simple situations.
@jonathan-gibbons
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons commented Nov 20, 2020

/csr

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 20, 2020

👋 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 openjdk bot added the csr label Nov 20, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 20, 2020

@jonathan-gibbons has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@jonathan-gibbons please create a CSR request and add link to it in JDK-8075778. This pull request cannot be integrated until the CSR request is approved.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 20, 2020

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

@openjdk openjdk bot added the rfr label Nov 20, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 20, 2020

@openjdk openjdk bot removed the csr label Nov 24, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 25, 2020

Mailing list message from Joe Darcy on javadoc-dev:

FYI, I had a good experience taking a trial run of this patch to update
the java.compiler APIs to use the new feature. I didn't find any issues;
a specdiff comparing with and without use of the new tag didn't have any
unexpected diffs. (There were cases where small wording differences
existed and were regularized in the patch.)

After this goes back, looking forward to pushing a fix for JDK-8256917:
Use combo @returns tag in java.compiler javadoc.

Cheers,

-Joe

On 11/20/2020 4:34 PM, Jonathan Gibbons wrote:

@RogerRiggs
Copy link
Contributor

@RogerRiggs RogerRiggs commented Nov 27, 2020

/**
  * {@return the result} Optional additional text.
  */

The java source looks a bit odd/unusual because the "first sentence" does not appear to end with a period.
Though it seems like a convenience to include the '.' in the expansion, the source might be clearer if it did not, as in:

/**
  * {@return the result}. Optional additional text.
  */ 

Similarly, prepending the "Returns", forces the verb, which is conventional but always the most appropriate word.
Changing the expansion to be only the contents of @ return would allow more flexible use.
It would put more responsibility on the developer to form the first sentence. (With "Returns"... and the "." outside the tag.

@jonathan-gibbons
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons commented Nov 27, 2020

@RogerRiggs I think that having to write Returns {@return the result}. would look more weird than the current proposal to omit the .. Note also that the proposed form matches the existing {@summary ...} tag which can be used to explicit define the "first sentence" that appears in summary tables.

@pavelrappo
Copy link
Member

@pavelrappo pavelrappo commented Dec 1, 2020

@RogerRiggs ,

  1. I agree on that the lack of . after } looks unnatural:
/**
 * {@return the result} Optional additional text.
 */
int method() { 
  1. I disagree on allowing a flexible expansion. This enhancement aims to support a very particular case of redundancy in doc comments. I believe that the proportion of redundant doc comments that use some other verb (instead of "Return(s)") is smaller than it needs to be to support the increase in tag's complexity.
@RogerRiggs
Copy link
Contributor

@RogerRiggs RogerRiggs commented Dec 2, 2020

There is lots of other duplication/repetition in most javadoc. I'd rather see some kind of text macro that would allow a single definition of a string that can be repeated. The source would be a bit less readable, but it would be lower maintenance when the same phrase or sentence is repeated to make the javadoc more locally complete and easier to read in isolation. Now many times do you have to say "throws NullPointerException when the reference is null" or similar assertion.

@pavelrappo
Copy link
Member

@pavelrappo pavelrappo commented Dec 2, 2020

@RogerRiggs,

Here are some more thoughts on flexible expansion (i.e. not forcing a particular verb). To understand the nature and estimate the prevalence of cases where "Return(s)" was inappropriate as the first word of an internally repetitive doc comment, I conducted the following experiment in JDK.

I searched for doc comments whose first sentence was at least 90% similar to the contents of @ return tag. My program ignored differences in markup and compared contents as strings using normalized Levenshtein similarity; I got 3432 results. Then I modified the program to ignore those doc comments whose first sentence case-insensitively started with "return"; I got 194 results.

While more analysis might be required, I can preliminary see that we are talking about 5% of the cases. Which is not much, if you ask me. Here are a few findings made by the modified program:


/**
 * Cleaner for use within system modules.
 ...
 * @return a Cleaner for use within system modules
 */
public static Cleaner cleaner() {

/**
 * Get the localization resource bundle
 ...
 * @return the localization resource bundle
 */
public ResourceBundle getResourceBundle() {

/**
 * Obtains a line that is available for use and that matches the description
 * in the specified {@code Line.Info} object.
 ...
 * @return a line that is available for use and that matches the description
 *         in the specified {@code Line.Info} object
 ...
 */
Line getLine(Line.Info info) throws LineUnavailableException;

/**
 * The description of this filter. For example: "JPG and GIF Images."
 *
 * @return the description of this filter
 */
public String getDescription() {

/**
 * Fetch the object representing the layout state of
 * of the child at the given index.
 ...
 * @return the object representing the layout state of
 * of the child at the given index
 */
protected ChildState getChildState(int index) {

/**
 * Creates a new instance of the {@code DatatypeFactory} {@linkplain
 * #DATATYPEFACTORY_IMPLEMENTATION_CLASS builtin system-default
 * implementation}.
 *
 * @return A new instance of the {@code DatatypeFactory} builtin
 *         system-default implementation.
 ...
 */
public static DatatypeFactory newDefaultInstance() {

/**
 * True if this iterator has a reversed axis.
 *
 * @return <code>true</code> if this iterator is a reversed axis.
 */
public boolean isReverse() {

/**
 * Check if the annotation contains an array of annotation as a value. This
 * check is to verify if a repeatable type annotation is present or not.
 ...
 * @return true if the annotation contains an array of annotation as a value.
 */
private boolean isAnnotationArray(Map<? extends ExecutableElement, ? extends AnnotationValue> pairs) {

...

The most frequent first word in those 194 results was "Get(s)".

@jddarcy
Copy link
Member

@jddarcy jddarcy commented Dec 3, 2020

/**
  * {@return the result} Optional additional text.
  */

The java source looks a bit odd/unusual because the "first sentence" does not appear to end with a period.
Though it seems like a convenience to include the '.' in the expansion, the source might be clearer if it did not, as in:

/**
  * {@return the result}. Optional additional text.
  */ 

Similarly, prepending the "Returns", forces the verb, which is conventional but always the most appropriate word.
Changing the expansion to be only the contents of @ return would allow more flexible use.
It would put more responsibility on the developer to form the first sentence. (With "Returns"... and the "." outside the tag.

I've done a trial use of this feature in the java.compiler API. While initially I found the lack of a period after "{@return ...}" odd, I quickly adapted to this usage.

@jddarcy
Copy link
Member

@jddarcy jddarcy commented Dec 3, 2020

There is lots of other duplication/repetition in most javadoc. I'd rather see some kind of text macro that would allow a single definition of a string that can be repeated. The source would be a bit less readable, but it would be lower maintenance when the same phrase or sentence is repeated to make the javadoc more locally complete and easier to read in isolation. Now many times do you have to say "throws NullPointerException when the reference is null" or similar assertion.

IMO this is a case to avoid the perfect being the enemy of the good. There are many structural cases of repeated or nearly repeated return information in the first sentence and @return tag. Therefore, I think it is reasonable for don't-repeat-yourself purposes to have dedicated support for this usage pattern.

Separately, I agree it would be helpful to have a more general facility to allow structured placement of repeated text blocks.

@jonathan-gibbons
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons commented Dec 3, 2020

Copy link
Member

@pavelrappo pavelrappo left a comment

  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.
* or {@code {@return} } tag.
*
* @implSpec This implementation throws {@code UnsupportedOperationException} if
* {@code isInline} is {@code true}, and calls {@link #newReturnTree(List)} otherwise.
*
* @param description the description of the return value of a method
* @return a {@code ReturnTree} object
* @throws UnsupportedOperationException if inline {@code {@return} } tags are
Comment on lines 270 to 277

This comment has been minimized.

@pavelrappo

pavelrappo Dec 8, 2020
Member

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

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 8, 2020
Author Contributor

fixed

case INLINE:
if (tp.allowsBlock()) {
return tp.parse(p, TagParser.Kind.BLOCK);
} else {
return erroneous("dc.bad.inline.tag", p);

This comment has been minimized.

@pavelrappo

pavelrappo Dec 8, 2020
Member

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

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 8, 2020
Author Contributor

fixed

/**
* Returns whether this instance is an inline tag.
*
* @return {@code true} if this instance is an inline tag

This comment has been minimized.

@pavelrappo

pavelrappo Dec 8, 2020
Member

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.

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 8, 2020
Author Contributor

hmm, OK, seems a bit redundant but will add and {@code false} otherwise

} else { // handle block tags (for example, @see) in inline content
DCTree text = inlineText(WhitespaceRetentionPolicy.REMOVE_ALL); // skip content
nextChar();
return m.at(p).newUnknownInlineTagTree(name, List.of(text)).setEndPos(bp);
Comment on lines +330 to +333

This comment has been minimized.

@pavelrappo

pavelrappo Dec 8, 2020
Member

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.

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 8, 2020
Author Contributor

The code is correct, and even the comment: which refers to "block tags in inline content".
The case being handled is one like this:

    /**
     * This is a bad comment. {@see Object} or {@since 7}.
     */
}

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

This comment has been minimized.

@pavelrappo

pavelrappo Dec 8, 2020
Member

Consider updating the version to 15. Later, we'd hopefully be able to use the @spec tag here.

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 8, 2020
Author Contributor

done

Yes, this could eventually be a use of the proposed @spec tag with a relative URL

@Override
public String getTagName() {
Comment on lines 689 to 690

This comment has been minimized.

@pavelrappo

pavelrappo Dec 8, 2020
Member

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

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 8, 2020
Author Contributor

Oops, yes; fixed; I guess I'm mildly surprised the checker didn't catch it, but that's a different issue

@@ -250,13 +263,34 @@ public Object visitLiteral(LiteralTree node, Object p) {
return scan(node.getBody(), p);
}

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

This comment has been minimized.

@pavelrappo

pavelrappo Dec 8, 2020
Member

Fix the typos in "bimodfal tage".

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 8, 2020
Author Contributor

Oops, also fixed an instance of null to {@code null}

@@ -57,6 +57,7 @@ dc.no.alt.attr.for.image = no "alt" attribute for image
dc.no.summary.or.caption.for.table=no summary or caption for table
dc.param.name.not.found = @param name not found
dc.ref.not.found = reference not found
dc.return.not.first = '{@return'} not at beginning of description

This comment has been minimized.

@pavelrappo

pavelrappo Dec 8, 2020
Member

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 }.

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 8, 2020
Author Contributor

I'll investigate and follow up. It looks like general confusion whether to escape the curly braces, or to quote the enclosed text in the generated message.

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 8, 2020
Author Contributor

It looks like the quotes are never needed. I'll remove all of them.

/**
 * This is a comment.
 * This is a bad {@return fred}
 * This is a bad {@value}
 * This is a bad <code>{@code code}</code>.
 */
public class C { }
play/doclint-msgs/src/C.java:3: warning: {@return} not at beginning of description
 * This is a bad {@return fred}
                 ^
play/doclint-msgs/src/C.java:3: error: invalid use of @return
 * This is a bad {@return fred}
                 ^
play/doclint-msgs/src/C.java:4: error: {@value} not allowed here
 * This is a bad {@value}
                 ^
play/doclint-msgs/src/C.java:5: warning: {@code} within <code>
 * This is a bad <code>{@code code}</code>.
                       ^
2 errors
2 warnings
@@ -137,7 +137,7 @@ public void testReflow() {
"1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234\n" +
"1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234\n" +
"1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234\n" +
"1234 1234 1234 1234 1234 1234 1234 1234 1234\n";
"1234 1234 1234 1234 1234 1234 1234 1234 1234 \n";

This comment has been minimized.

@pavelrappo

pavelrappo Dec 8, 2020
Member

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

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 8, 2020
Author Contributor

It was needed to fix a test failure

This comment has been minimized.

@lahodaj

lahodaj Dec 8, 2020
Contributor

FWIW, I believe the reason is that JavadocFormatter.scan (around line 580 in the new code) is checking for inline tags using the instanceof operator, and adds a space for inline tags. We could add a test to prevent that (i.e. not add the space , but not sure if there is a real problem with generating the space.

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

This comment has been minimized.

@pavelrappo

pavelrappo Dec 8, 2020
Member

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?

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 8, 2020
Author Contributor

I think the code is better as is. It would be wrong to have a firstSentence containing multiple items, beginning with {@return} and then to only return the first one in the following line of code. Arguably, a better form may be to use isEmpty() and then subsequently assert (somehow) that the size is exactly 1, but using assertions like that is a different discussion (as you know!)

This comment has been minimized.

@pavelrappo

pavelrappo Dec 8, 2020
Member

I guess I was thinking about a corner case like {@summary {@return ...} ...} which this patch and the complementary specification consider illegal anyway.

@lahodaj
lahodaj approved these changes Dec 8, 2020
Copy link
Contributor

@lahodaj lahodaj left a comment

The javac/jshell changes look sensible to me.

@@ -137,7 +137,7 @@ public void testReflow() {
"1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234\n" +
"1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234\n" +
"1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234 1234\n" +
"1234 1234 1234 1234 1234 1234 1234 1234 1234\n";
"1234 1234 1234 1234 1234 1234 1234 1234 1234 \n";

This comment has been minimized.

@lahodaj

lahodaj Dec 8, 2020
Contributor

FWIW, I believe the reason is that JavadocFormatter.scan (around line 580 in the new code) is checking for inline tags using the instanceof operator, and adds a space for inline tags. We could add a test to prevent that (i.e. not add the space , but not sure if there is a real problem with generating the space.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 8, 2020

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

8075778: Add javadoc tag to avoid duplication of return information in simple situations.

Reviewed-by: prappo, jlahoda

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

  • 48d8650: 8257845: Integrate JEP 390
  • ed4c4ee: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default
  • c47ab5f: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event
  • 291ba97: 8251267: CDS tests should use CDSTestUtils.getOutputDir instead of System.getProperty("user.dir")

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

@openjdk openjdk bot added the ready label Dec 8, 2020
Copy link
Member

@pavelrappo pavelrappo left a comment

Thanks for considering my feedback.

@jonathan-gibbons
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons commented Dec 8, 2020

/integrate

@openjdk openjdk bot closed this Dec 8, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 8, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 8, 2020

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

  • 48d8650: 8257845: Integrate JEP 390
  • ed4c4ee: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default
  • c47ab5f: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event
  • 291ba97: 8251267: CDS tests should use CDSTestUtils.getOutputDir instead of System.getProperty("user.dir")

Your commit was automatically rebased without conflicts.

Pushed as commit b29f9cd.

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

@jonathan-gibbons jonathan-gibbons deleted the jonathan-gibbons:new-return branch Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants