RFR: 8242564: javadoc crashes:: class cast exeception com.sun.tools.javac.code.Symtab$6

[Jonathan Gibbons]

On Tue, 16 Jan 2024 02:17:10 GMT, Vladimir Petko <vpetko at 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