On Tue, 16 Jan 2024 02:17:10 GMT, Vladimir Petko <vpe...@openjdk.org> wrote:
> '--ignore-source-errors' allows generating Javadoc for the packages that
> contain compilation errors.
>
> jdk.javadoc.internal.doclets.toolkit.util.ClassTree generates a type
> hierarchy for javadoc that may include error types such as
>
> class Foo extends Bar {
> }
> ```
> where Bar is undefined.
>
> The user still wants to generate documentation for Foo and have Bar as a text
> label.
>
> For the unknown class Bar it is impossible to detect the enclosing class/file
> and javadoc crashes with exception.
>
> This PR returns Kind.OTHER for the error types, avoiding the javadoc crash.
Thanks for taking this on.
The work does show up more problems than it solves (!!) such as the use of
`JavaFileObject.Kind.OTHER`, and the inconsistent output generated by the test.
But both of those are out of scope for this work, which is primarily about
avoiding a crash caused by an unexpected exception.
There's a typo (`exeception`) in the title that I cannot fix for you, and there
are some suggestions to improve the comments and class names in the test. Do
those and you'll be good to go.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/ToolEnvironment.java
line 186:
> 184: public Kind getFileKind(TypeElement te) {
> 185: if (te.asType().getKind() == TypeKind.ERROR)
> 186: return Kind.OTHER;
On a medium deep-dive, I looked at whether this was the "best" solution, or
whether it was "second best" and that we should fix the origin of the
`ClassCastException` in `Symbol.outermostclass()`. Bottom line: while we could
fix it for this case, in general, there is no way to guarantee that there is a
reasonable result of type `ClassSymbol` for every reasonable symbol for which
this method is called. Arguably, `.outermostclass` could do more checking
and/or throw some exception and/or return `null`, but that is not the javac
style, and we would still have to deal with the issue at call sites, like this
one.
It's also debatable whether the correct return value is `OTHER` or `null`.
`OTHER` has typically meant _some kind of file with an unknown extension_ and
not _no file_. The spec is unclear on this point, and clarifying the spec is
out of scope in this PR, and would require a CSR. So, for now, `OTHER` is OK.
test/langtools/jdk/javadoc/doclet/testClassTree/TestClassTree.java line 58:
> 56: // Given badpkg package containing class ChildClass with an
> undefined
> 57: // base class, implementing undefined interface and a
> defined
> 58: // interface
While comments are good, the comments in this method are a little strange, in
that to understand them, you have to read them as a disjoint narrative composed
of all the comments in the narrative. Read individually, the comments stand as
"orphaned" phrases that do not make sense on their own. And, as such, they
only serve to confuse the reader!
If you don't want to edit the comments too much, and because this is "just"
test code, you could maybe begin and/or end comments with an ellipsis (`...`)
to indicate that the comment is either a continuation of what has gone before
and/or is continued later.
test/langtools/jdk/javadoc/doclet/testClassTree/TestClassTree.java line 65:
> 63: public class ChildClass extends ParentClass
> 64: implements AnInterface, Iterable {
> 65:
You can also reduce the need for an explanatory comment by using more
"interesting" names. For example, to indicate that `ParentClass` is undefined,
you could call it `UndefinedClass`. Likewise, `AnInterface` could be
`UndefinedInterface`. Using names like these would propagate into the
generated HTML to help the reader (and reader of the test code).
test/langtools/jdk/javadoc/doclet/testClassTree/TestClassTree.java line 91:
> 89: <span class="extends-implements">extends ParentClass
> 90: implements java.lang.Iterable</span></div>
> 91: """);
While this code may check what is generated, the output seems inconsistent. In
particular, `ParentClass` is mentioned, but `AnInterface` is not.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17435#pullrequestreview-1846273363
PR Review Comment: https://git.openjdk.org/jdk/pull/17435#discussion_r1468072034
PR Review Comment: https://git.openjdk.org/jdk/pull/17435#discussion_r1467985523
PR Review Comment: https://git.openjdk.org/jdk/pull/17435#discussion_r1467990658
PR Review Comment: https://git.openjdk.org/jdk/pull/17435#discussion_r1467977923
