8242564: javadoc crashes:: class cast exception com.sun.tools.javac.code.Symtab$6 #17435

vpa1977 · 2024-01-16T02:17:10Z

'--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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8242564: javadoc crashes:: class cast exception com.sun.tools.javac.code.Symtab$6 (Bug - P3)

Reviewers

Jonathan Gibbons (@jonathan-gibbons - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17435/head:pull/17435
$ git checkout pull/17435

Update a local copy of the PR:
$ git checkout pull/17435
$ git pull https://git.openjdk.org/jdk.git pull/17435/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17435

View PR using the GUI difftool:
$ git pr show -t 17435

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17435.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-16T02:17:58Z

👋 Welcome back vpetko! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-01-16T02:18:45Z

@vpa1977 The following label will be automatically applied to this pull request:

javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

vpa1977 · 2024-01-16T02:31:13Z

test ran at commit 53d2985 fail due to the ClassCastException:

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/langtools/jdk/javadoc/doclet/testClassTree/TestClassTree.java
>>                                                       1     0     1     0 <<
==============================
TEST FAILURE

Test ran at the commit 9ebb1ac (tip of the branch(with fix included) pass:


==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/langtools/jdk/javadoc/doclet/testClassTree/TestClassTree.java
                                                         1     1     0     0   
==============================
TEST SUCCESS

Stopping javac server
Finished building target 'test' in configuration 'linux-x86_64-server-release'


$make TEST=:langtools_javadoc test

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/langtools:langtools_javadoc              500   500     0     0   
==============================
TEST SUCCESS

test/langtools/jdk/javadoc/doclet/testClassTree/TestClassTree.java

test/langtools/jdk/javadoc/doclet/testClassTree/badpkg/ChildClass.java

mlbridge · 2024-01-16T22:15:00Z

Webrevs

01: Full - Incremental (f30c7b5d)
00: Full (7b810b23)

vpa1977 · 2024-01-22T08:04:47Z

@jonathan-gibbons I apologise for the ping, would it be possible to take another look at this MR?

jonathan-gibbons

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.

jonathan-gibbons · 2024-01-26T18:11:03Z

test/langtools/jdk/javadoc/doclet/testClassTree/TestClassTree.java

+        checkOutput("badpkg/ChildClass.html", true,
+                """
+                    <div class="type-signature"><span class="modifiers">public class </span>\
+                    <span class="element-name type-name-label">ChildClass</span>
+                    <span class="extends-implements">extends ParentClass
+                    implements java.lang.Iterable</span></div>
+                    """);


While this code may check what is generated, the output seems inconsistent. In particular, ParentClass is mentioned, but AnInterface is not.

I've split it into a separate issue - https://bugs.openjdk.org/browse/JDK-8324791, would it be ok for me to look into addressing it?

test/langtools/jdk/javadoc/doclet/testClassTree/TestClassTree.java

jonathan-gibbons · 2024-01-26T19:35:42Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/ToolEnvironment.java

+        if (te.asType().getKind() == TypeKind.ERROR)
+            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.

vpa1977 · 2024-01-27T04:29:35Z

Thank you for reviewing PR!
I've updated as per comments, and if you find that test comments are not strictly necessary, i can remove those.

jonathan-gibbons

Thanks for doing the final round of changes.

openjdk · 2024-01-29T21:20:22Z

@vpa1977 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: jjg

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 165 new commits pushed to the master branch:

e999dfc: 8323503: x86: Shorter movptr(reg, imm) for 32-bit unsigned immediates
84deeb6: 8324667: fold Parse::seems_stable_comparison()
fb07bbe: 8324717: Remove HotSpotJVMCICompilerFactory
d1e6763: 8324733: [macos14] Problem list tests which fail due to macOS bug described in JDK-8322653
c1281e6: 8324678: Replace NULL with nullptr in HotSpot gtests
a6bdee4: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files
951b5f8: 8324723: GHA: Upgrade some actions to avoid deprecated Node 16
fe0eec7: 8324840: windows-x64-slowdebug does not build anymore after JDK-8317572
4df04f0: 8324769: Serial: Remove unused TenuredGeneration::unsafe_max_alloc_nogc
3066d49: 8317572: C2 SuperWord: refactor/improve TraceSuperWord, replace VectorizeDebugOption with TraceAutoVectorization
... and 155 more: https://git.openjdk.org/jdk/compare/b3634722655901b8d3e43dd1f8aa2b4487509a34...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jonathan-gibbons) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

vpa1977 · 2024-01-29T21:24:02Z

/integrate

openjdk · 2024-01-29T21:25:35Z

@vpa1977
Your change (at version f30c7b5) is now ready to be sponsored by a Committer.

jonathan-gibbons · 2024-01-29T21:27:56Z

/sponsor

openjdk · 2024-01-29T21:28:50Z

Going to push as commit 64c3642.
Since your change was applied there have been 165 commits pushed to the master branch:

e999dfc: 8323503: x86: Shorter movptr(reg, imm) for 32-bit unsigned immediates
84deeb6: 8324667: fold Parse::seems_stable_comparison()
fb07bbe: 8324717: Remove HotSpotJVMCICompilerFactory
d1e6763: 8324733: [macos14] Problem list tests which fail due to macOS bug described in JDK-8322653
c1281e6: 8324678: Replace NULL with nullptr in HotSpot gtests
a6bdee4: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files
951b5f8: 8324723: GHA: Upgrade some actions to avoid deprecated Node 16
fe0eec7: 8324840: windows-x64-slowdebug does not build anymore after JDK-8317572
4df04f0: 8324769: Serial: Remove unused TenuredGeneration::unsafe_max_alloc_nogc
3066d49: 8317572: C2 SuperWord: refactor/improve TraceSuperWord, replace VectorizeDebugOption with TraceAutoVectorization
... and 155 more: https://git.openjdk.org/jdk/compare/b3634722655901b8d3e43dd1f8aa2b4487509a34...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-01-29T21:28:57Z

@jonathan-gibbons @vpa1977 Pushed as commit 64c3642.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

vpa1977 added 2 commits January 16, 2024 15:01

Add ClassTree test with undefined symbols

53d2985

Return Kind.OTHER for undefined symbols

9ebb1ac

openjdk bot added the javadoc javadoc-dev@openjdk.org label Jan 16, 2024

jonathan-gibbons reviewed Jan 16, 2024

View reviewed changes

test/langtools/jdk/javadoc/doclet/testClassTree/TestClassTree.java Outdated Show resolved Hide resolved

jonathan-gibbons reviewed Jan 16, 2024

View reviewed changes

test/langtools/jdk/javadoc/doclet/testClassTree/badpkg/ChildClass.java Outdated Show resolved Hide resolved

vpa1977 added 3 commits January 17, 2024 08:40

add text block indentation

20d1666

Use toolbox to write the test file

7d92504

Move test comments

7b810b2

vpa1977 marked this pull request as ready for review January 16, 2024 22:11

vpa1977 requested a review from jonathan-gibbons January 16, 2024 22:11

openjdk bot added the rfr Pull request is ready for review label Jan 16, 2024

jonathan-gibbons reviewed Jan 26, 2024

View reviewed changes

vpa1977 changed the title ~~8242564: javadoc crashes:: class cast exeception com.sun.tools.javac.code.Symtab$6~~ 8242564: javadoc crashes:: class cast exception com.sun.tools.javac.code.Symtab$6 Jan 26, 2024

vpa1977 added 2 commits January 27, 2024 12:53

move test comments into method's javadoc comment

603da53

rename test classes as per review comments

f30c7b5

vpa1977 requested a review from jonathan-gibbons January 27, 2024 04:29

jonathan-gibbons approved these changes Jan 29, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 29, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Jan 29, 2024

openjdk bot added the integrated Pull request has been integrated label Jan 29, 2024

openjdk bot closed this Jan 29, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jan 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8242564: javadoc crashes:: class cast exception com.sun.tools.javac.code.Symtab$6 #17435

8242564: javadoc crashes:: class cast exception com.sun.tools.javac.code.Symtab$6 #17435

vpa1977 commented Jan 16, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 16, 2024

openjdk bot commented Jan 16, 2024

vpa1977 commented Jan 16, 2024

mlbridge bot commented Jan 16, 2024 •

edited

Loading

vpa1977 commented Jan 22, 2024

jonathan-gibbons left a comment

jonathan-gibbons Jan 26, 2024

vpa1977 Jan 26, 2024

jonathan-gibbons Jan 26, 2024

vpa1977 commented Jan 27, 2024

jonathan-gibbons left a comment

openjdk bot commented Jan 29, 2024

vpa1977 commented Jan 29, 2024

openjdk bot commented Jan 29, 2024

jonathan-gibbons commented Jan 29, 2024

openjdk bot commented Jan 29, 2024

openjdk bot commented Jan 29, 2024

		if (te.asType().getKind() == TypeKind.ERROR)
		return Kind.OTHER;

8242564: javadoc crashes:: class cast exception com.sun.tools.javac.code.Symtab$6 #17435

8242564: javadoc crashes:: class cast exception com.sun.tools.javac.code.Symtab$6 #17435

Conversation

vpa1977 commented Jan 16, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jan 16, 2024

openjdk bot commented Jan 16, 2024

vpa1977 commented Jan 16, 2024

mlbridge bot commented Jan 16, 2024 • edited Loading

Webrevs

vpa1977 commented Jan 22, 2024

jonathan-gibbons left a comment

Choose a reason for hiding this comment

jonathan-gibbons Jan 26, 2024

Choose a reason for hiding this comment

vpa1977 Jan 26, 2024

Choose a reason for hiding this comment

jonathan-gibbons Jan 26, 2024

Choose a reason for hiding this comment

vpa1977 commented Jan 27, 2024

jonathan-gibbons left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 29, 2024

vpa1977 commented Jan 29, 2024

openjdk bot commented Jan 29, 2024

jonathan-gibbons commented Jan 29, 2024

openjdk bot commented Jan 29, 2024

openjdk bot commented Jan 29, 2024

vpa1977 commented Jan 16, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 16, 2024 •

edited

Loading