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-8272374: doclint should report missing "body" comments #5106
JDK-8272374: doclint should report missing "body" comments #5106
Conversation
👋 Welcome back jjg! A progress list of the required criteria for merging this PR into |
@jonathan-gibbons The following labels will be automatically applied to this pull request:
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. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this by using it to generate the JavaFX docs. We have 62 new warnings for methods with empty descriptions that we otherwise would not have easily found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One thing I wonder about is why you only look for @deprecated
in the first block tag. I guess it is just a convention to add this tag at the first position?
DocTree firstTag = tree.getBlockTags().get(0); | ||
// Don't report an empty description if the comment begins with @deprecated, | ||
// because javadoc will use the content of that tag in summary tables. | ||
if (firstTag.getKind() != DocTree.Kind.DEPRECATED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we only check the first block tag here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Various reasons,
- It seems the convention is to simply prefix an existing comment with
@deprecated
or to replace the existing body description with@deprecated reason-why-deprecated
- This is only for the case when there is no body description; it seems hard to imagine that someone would start a comment with
@see
@param
@return
etc and then have the@deprecated
tag.
That being said, it would be easy enough to change the code to check for any instance of @deprecated
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the downstream code does not impose any ordering restrictions on the tags, it is probably with doing the same here.
// Don't report an empty description if the comment begins with @deprecated, | ||
// because javadoc will use the content of that tag in summary tables. | ||
if (firstTag.getKind() != DocTree.Kind.DEPRECATED) { | ||
env.messages.report(MISSING, Kind.WARNING, tree, "dc.empty.description"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to not use reportMissing
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't have the right signature. reportMissing
reports a missing comment on an element; here, we're reporting a missing description within a DocTree
. There's a similar occurrence at line 1214.
@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:
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 24 new commits pushed to the
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 |
/integrate |
Going to push as commit ae45592.
Your commit was automatically rebased without conflicts. |
@jonathan-gibbons Pushed as commit ae45592. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please review a relatively simple update to have doclnt check for empty "descriptions" -- the body of a doc comment, before the block tags.
It is already the case that doclint checks for missing/empty descriptions in block tags, like @param, @return, etc.
There are three cases to consider:
No diagnostic is reported if the description is missing but the first tag is
@deprecated
, because in this case, javadoc will use the body of the@deprecated
tag for the summary. SeeCharacter.UnicodeBlock#SURROGATES_AREA
and the corresponding entry in the summary table to see an example of this situation.Diagnostics are reported if the declaration is not an overriding method and does not begin with
@deprecated
. This occurs in a number of places in thejava.desktop
module, often where the doc comment is of the form/** @return _description_ */
. To suppress those warnings for now, the-missing
option is added toDOCLINT_OPTIONS
for thejava.desktop
module. To see the effects of this anti-pattern, look at the empty descriptions forjavax.swing.text.html.parser.AttributeList
Many of the doclint tests needed to be updated, because of their over-simplistic minimal comments. It was not possible, in general, to avoid updating the source code while preserving line numbers, so in many cases, the golden
*.out
files had to be updated as well.A new test is added, focussing on the different forms of empty/missing descriptions, as described above.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5106/head:pull/5106
$ git checkout pull/5106
Update a local copy of the PR:
$ git checkout pull/5106
$ git pull https://git.openjdk.java.net/jdk pull/5106/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5106
View PR using the GUI difftool:
$ git pr show -t 5106
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5106.diff