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

[Hannes Wallnöfer]

On Thu, 22 Jul 2021 11:23:13 GMT, Hannes Wallnöfer <hannesw 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.
>
> Hannes Wallnöfer has updated the pull request incrementally with one additional commit since the last revision:
> 
>   JDK-8270866: Add output check for tests

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 `JavadocLog` checks out as you suggest.

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

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