[jdk17] RFR: JDK-8270866: NPE in DocTreePath.getTreePath()

[Hannes Wallnöfer]

On Wed, 21 Jul 2021 17:26:50 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

>> This change fixes a NPE in `JavadocLog` when the `DocTreePath` for an inherited doc comment can't be retrieved. 
>> 
>> The most important change is to fix the `overriddenElement` feature in `CommentHelper`. The purpose of this feature is to enable lookup of the `DocTreePath` or `Element` if the comment was inherited from an overridden member, but it was not implemented and used correctly. With this change, the feature is implemented in `CommentHelper.getDocTreePath(DocTree)` and that method is used whenever the `DocTreePath` is needed instead of duplicating the functionality elsewhere.
>> 
>> The `CommentHelper.setOverrideElement(Element)` method was previously used in four places:
>> 
>>  - In `InheritDocTaglet.java` when generating a piece of inherited documentation
>>  - In `ReturnTaglet` when generating inherited return value documentation
>>  - In `ParamTaglet` when generating inherited documentation for a parameter
>>  - In `MemberSummaryBuilder` when generating inherited summary documentation for an executable member
>>  
>> The 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 `InheritDocTaglet` we already did that, in `ReturnTaglet` and `ParamTaglet` I changed the first argument to the doc-generating method from `holder` to `inheritedDoc.holder`. (Note that in `ParamTaglet` the name of the parameter which can change in the overriding member is passed as separate argument.)
>> 
>> The remaining use of `CommentHelper.setOverrideElement(Element)` in `MemberSummaryBuilder` was a bit more difficult, since the invoked method `MemberSummaryWriter.addMemberSummary` generates not just the doc comment, but the whole summary line including the member signature. I tried adding the `CommentHelper` or `Element` owning 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 `CommentHelper` we register the overridden element on in `MemberSummaryBuilder.buildSummary` to 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 a `SoftReference` in `CommentHelperCache`. 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 `DiagnosticSource` and `DiagnosticPosition` instances in `JavadocLog`. These null checks have been there in the method's precursors in the `Messager` class and were dropped in JDK-8267126. As far as I can tell, all the reporting functionality in `JavadocLog` accepts 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.
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/JavadocLog.java line 523:
> 
>> 521:         if (path == null || path.getTreePath() == null) {
>> 522:             return null;
>> 523:         }
> 
> 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.

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.

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

PR: https://git.openjdk.java.net/jdk17/pull/264