Re: RFR: 8332039: Cannot invoke "com.sun.source.util.DocTreePath.getTreePath()" because "path" is null
On Thu, 23 May 2024 10:39:52 GMT, Hannes Wallnöfer wrote: > Please review a patch to fix a NPE thrown when a `@since` tag inherited by a > nested class contains a nested inline tag. The solution is to make > `CommentHelper.getDocTreePath(DocTree)` able to handle block tags inherited > by nested classes. I filed a follow-up issue to refactor the code for inherited and derived doc comments: https://bugs.openjdk.org/browse/JDK-8333557 - PR Comment: https://git.openjdk.org/jdk/pull/19363#issuecomment-2148040277
Re: RFR: 8332039: Cannot invoke "com.sun.source.util.DocTreePath.getTreePath()" because "path" is null
On Fri, 31 May 2024 20:58:31 GMT, Jonathan Gibbons wrote: > > To clarify, I'm not too concerned with how we call an action whereby a > > nested class gets its `@since` tag from the enclosing class; but yes, > > "inheritance" might not be ideal. What I'm concerned with is the fact that > > the logic originally provided in > > [`getDefaultBlockTags`](https://github.com/openjdk/jdk/blob/6ee8407758c92d32e18642b0758d2d5c71ad09f5/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/taglets/SimpleTaglet.java#L141-L147) > > is now duplicated in `getInheritedDocTreePath`. > > Like I said, while it's bad, the new duplication merely replicates the one > > we already have between `InheritableTaglet.inherit` and > > `getInheritedDocTreePath`. > > If we can't fix this in the next few days, we should file a cleanup issue to > resolve this duplication. I don't quite see the problem with this, or how to improve it substantially (beyond finding better terminology). We have different ways of obtaining doc comment fragments from related elements. The major and well-established one is along the lines of Java type inheritance, and a minor/new one along enclosing/enclosed classes. Since `CommentHelper` provides a means to reverse lookup `DocTree` -> `DocTreePath`, it necessarily needs to know about all of these mechanisms. The code for the enclosing/enclosed classes is actually trivial, so I don't see how this could be simplified further. The code for finding doc comments along type inheritance is more complex, but it already delegates to the `DocFinder` class, which is also used for the primary lookup for inherited doc comments. I certainly don't think the existing/proposed code is perfect, but I don't see huge potential for improvement. Of course I'm open to learn and better understand the issues, and how to solve them. - PR Comment: https://git.openjdk.org/jdk/pull/19363#issuecomment-2145104410
Re: RFR: 8332039: Cannot invoke "com.sun.source.util.DocTreePath.getTreePath()" because "path" is null
On Fri, 31 May 2024 09:20:27 GMT, Pavel Rappo wrote: >> I understand the solution and see how it logically parallels the existing >> link between `getDocTreePath` and inheritable taglets. That said, I dislike >> the solution, but also cannot propose a better one at this time. The logic >> is repeated and spread out. > >> @pavelrappo raises an interesting point. nested classes do not "inherit" >> from their enclosing class, and so putting the fix in a method dealing with >> inheritance seems wrong. >> >> Either the fix should be moved or the method renamed. > > To clarify, I'm not too concerned with how we call an action whereby a nested > class gets its `@since` tag from the enclosing class; but yes, "inheritance" > might not be ideal. What I'm concerned with is the fact that the logic > originally provided in [`getDefaultBlockTags`][getDefaultBlockTags] is now > duplicated in `getInheritedDocTreePath`. > > Like I said, while it's bad, the new duplication merely replicates the one we > already have between `InheritableTaglet.inherit` and > `getInheritedDocTreePath`. > > [getDefaultBlockTags]: > https://github.com/openjdk/jdk/blob/6ee8407758c92d32e18642b0758d2d5c71ad09f5/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/taglets/SimpleTaglet.java#L141-L147 > > @pavelrappo raises an interesting point. nested classes do not "inherit" > > from their enclosing class, and so putting the fix in a method dealing with > > inheritance seems wrong. > > Either the fix should be moved or the method renamed. > > To clarify, I'm not too concerned with how we call an action whereby a nested > class gets its `@since` tag from the enclosing class; but yes, "inheritance" > might not be ideal. What I'm concerned with is the fact that the logic > originally provided in > [`getDefaultBlockTags`](https://github.com/openjdk/jdk/blob/6ee8407758c92d32e18642b0758d2d5c71ad09f5/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/taglets/SimpleTaglet.java#L141-L147) > is now duplicated in `getInheritedDocTreePath`. > > Like I said, while it's bad, the new duplication merely replicates the one we > already have between `InheritableTaglet.inherit` and > `getInheritedDocTreePath`. If we can't fix this in the next few days, we should file a cleanup issue to resolve this duplication. - PR Comment: https://git.openjdk.org/jdk/pull/19363#issuecomment-2142969145
Re: RFR: 8332039: Cannot invoke "com.sun.source.util.DocTreePath.getTreePath()" because "path" is null
On Thu, 30 May 2024 18:43:28 GMT, Pavel Rappo wrote: >> Please review a patch to fix a NPE thrown when a `@since` tag inherited by a >> nested class contains a nested inline tag. The solution is to make >> `CommentHelper.getDocTreePath(DocTree)` able to handle block tags inherited >> by nested classes. > > I understand the solution and see how it logically parallels the existing > link between `getDocTreePath` and inheritable taglets. That said, I dislike > the solution, but also cannot propose a better one at this time. The logic is > repeated and spread out. > @pavelrappo raises an interesting point. nested classes do not "inherit" from > their enclosing class, and so putting the fix in a method dealing with > inheritance seems wrong. > > Either the fix should be moved or the method renamed. To clarify, I'm not too concerned with how we call an action whereby a nested class gets its `@since` tag from the enclosing class; but yes, "inheritance" might not be ideal. What I'm concerned with is the fact that the logic originally provided in [`getDefaultBlockTags`][getDefaultBlockTags] is now duplicated in `getInheritedDocTreePath`. Like I said, while it's bad, the new duplication merely replicates the one we already have between `InheritableTaglet.inherit` and `getInheritedDocTreePath`. [getDefaultBlockTags]: https://github.com/openjdk/jdk/blob/6ee8407758c92d32e18642b0758d2d5c71ad09f5/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/taglets/SimpleTaglet.java#L141-L147 - PR Comment: https://git.openjdk.org/jdk/pull/19363#issuecomment-2141579378
Re: RFR: 8332039: Cannot invoke "com.sun.source.util.DocTreePath.getTreePath()" because "path" is null
On Thu, 30 May 2024 18:43:28 GMT, Pavel Rappo wrote: > I understand the solution and see how it logically parallels the existing > link between `getDocTreePath` and inheritable taglets. That said, I dislike > the solution, but also cannot propose a better one at this time. The logic is > repeated and spread out. @pavelrappo raises an interesting point. nested classes do not "inherit" from their enclosing class, and so putting the fix in a method dealing with inheritance seems wrong. Either the fix should be moved or the method renamed. - PR Comment: https://git.openjdk.org/jdk/pull/19363#issuecomment-2140698136
Re: RFR: 8332039: Cannot invoke "com.sun.source.util.DocTreePath.getTreePath()" because "path" is null
On Thu, 23 May 2024 10:39:52 GMT, Hannes Wallnöfer wrote: > Please review a patch to fix a NPE thrown when a `@since` tag inherited by a > nested class contains a nested inline tag. The solution is to make > `CommentHelper.getDocTreePath(DocTree)` able to handle block tags inherited > by nested classes. I understand the solution and see how it logically parallels the existing link between `getDocTreePath` and inheritable taglets. That said, I dislike the solution, but also cannot propose a better one at this time. The logic is repeated and spread out. - PR Comment: https://git.openjdk.org/jdk/pull/19363#issuecomment-2140644458
Re: RFR: 8332039: Cannot invoke "com.sun.source.util.DocTreePath.getTreePath()" because "path" is null
On Thu, 23 May 2024 10:39:52 GMT, Hannes Wallnöfer wrote:
> Please review a patch to fix a NPE thrown when a `@since` tag inherited by a
> nested class contains a nested inline tag. The solution is to make
> `CommentHelper.getDocTreePath(DocTree)` able to handle block tags inherited
> by nested classes.
Marked as reviewed by jjg (Reviewer).
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/CommentHelper.java
line 517:
> 515: private DocTreePath getInheritedDocTreePath(DocTree dtree) {
> 516: Utils utils = configuration.utils;
> 517: if (element instanceof ExecutableElement ee) {
Technically, it is an official anti-pattern to use `instanceof` on Language
Model classes. Yes, it works OK for the JDK implementation of
`javax.lang.model`, but it is not necessarily true for all other
implementations (with a known instance where it is not the case.)
The recommended solution is to either use visitors or test or switch on the
`ElementKind`.
That all being said, I accept this is in JDK code ... and now that we have
pattern-switch available ...
-
PR Review: https://git.openjdk.org/jdk/pull/19363#pullrequestreview-2089110276
PR Review Comment: https://git.openjdk.org/jdk/pull/19363#discussion_r1621264502
Re: RFR: 8332039: Cannot invoke "com.sun.source.util.DocTreePath.getTreePath()" because "path" is null
On Thu, 30 May 2024 18:33:25 GMT, Jonathan Gibbons wrote:
>> Please review a patch to fix a NPE thrown when a `@since` tag inherited by a
>> nested class contains a nested inline tag. The solution is to make
>> `CommentHelper.getDocTreePath(DocTree)` able to handle block tags inherited
>> by nested classes.
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/CommentHelper.java
> line 517:
>
>> 515: private DocTreePath getInheritedDocTreePath(DocTree dtree) {
>> 516: Utils utils = configuration.utils;
>> 517: if (element instanceof ExecutableElement ee) {
>
> Technically, it is an official anti-pattern to use `instanceof` on Language
> Model classes. Yes, it works OK for the JDK implementation of
> `javax.lang.model`, but it is not necessarily true for all other
> implementations (with a known instance where it is not the case.)
>
> The recommended solution is to either use visitors or test or switch on the
> `ElementKind`.
>
> That all being said, I accept this is in JDK code ... and now that we have
> pattern-switch available ...
On the one hand, I'd like to suggest using pattern-switch -- on the other hand,
I note that makes back porting harder.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19363#discussion_r1621270634
