JDK-8270866: NPE in DocTreePath.getTreePath() #264
Conversation
|
|
Webrevs
|
| if (path == null || path.getTreePath() == null) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Please justify this change and the other similar changes in this file.
It was a deliberate change to drop these checks; I don't see why they're being undone. These checks have the effect of covering up bugs. Just because the old code was sloppy doesn't mean this code should be as well.
There was a problem hiding this comment.
I know the removal was a deliberate change. The decision to propose to reintroduce those checks was based on the following considerations:
- reporting/formatting code tolerates missing source/position
- NPE crashes on the other hand are quite serious
- this code is called from a lot of places
- we removed these checks quite recently, and we're very close to release JDK 17
That said, I'm not fully convinced we need those checks either. If you feel we can do without it I'm fine with that.
There was a problem hiding this comment.
I had a bit of a surprise when I removed the null checks in JavadocLog as I saw the same NPE when building JDK API docs (the tests were obviously fine). Turns out the problem was the code I removed from CommentHelper that was labeled as "Case B":
Although the purpose of this code was supposedly to handle the case of a comment inherited via @inheritDoc (which I confirmed, and which is why I concluded the code was no longer needed because InheritDocTaglet already uses the "correct" member), there is another case where this code was needed, which is when an overriding member has a doc comment to only add a block tag but still inherits the main description from the overridden member (without {@inheritDoc} tag).
I added commit afc0605 that adds code to CommentHelper.getPath that is equivalent to the old "Case B" code. It also adds a test for this case and removes the null checks in JavadocLog.
…t but relevant part is still inherited
There was a problem hiding this comment.
This looks a better (but more aggressive) solution than I might have done, which would have been to pass around a record containing the CommentHelper with the list of DocTrees. The "cost" is passing around a different holder element which is a bigger deal.
The comment in the description about the use of SoftReference should be followed up on, later.
I'd prefer the checks to stay out of JavadocLog because long term I think they cover up bugs when nulls are incorrectly being passed around. If anything, I'd consider replacing the checks with either assert or Objects.requireNonNull. But I'm OK to leave the checks in 17 if you think there is significant benefit to do so, and because we don't have the time to track down why nulls might be being passed down.
I recommend building JDK docs before/after this change, to verify no changes in the generated output. Bonus points for comparing the output of jtreg tests before/after this change, to verify that only expected differences are found/
| @@ -38,7 +38,6 @@ | |||
| import jdk.javadoc.internal.doclets.formats.html.markup.HtmlStyle; | |||
| import jdk.javadoc.internal.doclets.formats.html.markup.TagName; | |||
| import jdk.javadoc.internal.doclets.formats.html.markup.HtmlTree; | |||
| import jdk.javadoc.internal.doclets.formats.html.markup.RawHtml; | |||
|
@hns This change now passes all automated pre-integration checks. 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.
|
| public void addSummaryLinkComment(Element member, Content contentTree) { | ||
| List<? extends DocTree> tags = utils.getFirstSentenceTrees(member); | ||
| addSummaryLinkComment(mw, member, tags, contentTree); | ||
| addSummaryLinkComment(member, tags, contentTree); |
There was a problem hiding this comment.
I find this (a bit) surprising, since it is (potentially) useful to have access to the enclosing writer, but that being said, if we ever really need access to the enclosing AbstractMemberWriter it is probably better passed in to the constructor.
|
Thanks for the review, Jon! I did a recursive diff on the generated docs before and after this change, there are no differences. I'm leaving the |
|
/integrate |
|
Going to push as commit fbe28e4.
Your commit was automatically rebased without conflicts. |
openjdk
bot
added
integrated
This change fixes a NPE in
JavadocLogwhen theDocTreePathfor an inherited doc comment can't be retrieved.The most important change is to fix the
overriddenElementfeature inCommentHelper. The purpose of this feature is to enable lookup of theDocTreePathorElementif the comment was inherited from an overridden member, but it was not implemented and used correctly. With this change, the feature is implemented inCommentHelper.getDocTreePath(DocTree)and that method is used whenever theDocTreePathis needed instead of duplicating the functionality elsewhere.The
CommentHelper.setOverrideElement(Element)method was previously used in four places:InheritDocTaglet.javawhen generating a piece of inherited documentationReturnTagletwhen generating inherited return value documentationParamTagletwhen generating inherited documentation for a parameterMemberSummaryBuilderwhen generating inherited summary documentation for an executable memberThe first three usages have been removed with this change because they were not necessary. We can simply use the overridden member owning the comment instead of the overriding one to generate the output. In
InheritDocTagletwe already did that, inReturnTagletandParamTagletI changed the first argument to the doc-generating method fromholdertoinheritedDoc.holder. (Note that inParamTagletthe name of the parameter which can change in the overriding member is passed as separate argument.)The remaining use of
CommentHelper.setOverrideElement(Element)inMemberSummaryBuilderwas a bit more difficult, since the invoked methodMemberSummaryWriter.addMemberSummarygenerates not just the doc comment, but the whole summary line including the member signature. I tried adding theCommentHelperorElementowning the comment as an additional parameter, but there is pretty long line of methods that must carry the extra parameter around (as can be seen in the stack trace in the JBS issue). In the end I decided to keep the current mechanism to register the overridden holder element with the comment helper.One thing I am unsure about is whether it is possible for the
CommentHelperwe register the overridden element on inMemberSummaryBuilder.buildSummaryto be garbage collected under extreme memory pressure before it is retrieved and used again down the call graph. The local reference we use to register the comment holder is no longer reachable at that time and it is referenced by aSoftReferenceinCommentHelperCache. This is probably more of a theoretical issue, and it has existed before, but I thought I should mention it.Although it should no longer be required, I added back the null checks in the methods to retrieve the
DiagnosticSourceandDiagnosticPositioninstances inJavadocLog. These null checks have been there in the method's precursors in theMessagerclass and were dropped in JDK-8267126. As far as I can tell, all the reporting functionality inJavadocLogaccepts null values for source and position, so this seemed like the right thing to do.Finally, as a byproduct of my attempt of adding a new parameter I dropped some unused parameters in various writer classes. Since this is just cleanup I can undo these changes to keep it as small as possible.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17 pull/264/head:pull/264$ git checkout pull/264Update a local copy of the PR:
$ git checkout pull/264$ git pull https://git.openjdk.java.net/jdk17 pull/264/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 264View PR using the GUI difftool:
$ git pr show -t 264Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk17/pull/264.diff