8371309: Diagnostic.getEndPosition can throw an NPE with typical broken code

[lahodaj]

When starting the compiler via API, a DiagnosticListener may be provided, to which all reported errors and warnings are reported.

The reported diagnostics are represented by javax.tools.Diagnostic, which has (among others) method getEndPosition, which requires the end-pos-table to be set in the DiagnosticSource object for the given file (see AbstractLog.sourceMap). But the end-pos-table is only set after the parse is done:

jdk/src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavaCompiler.java

Line 702 in 167c952

log.setEndPosTable(filename, t.endPositions);

There are three problems with this:

• when a Diagnostic is reported by the parser, and the clients code calls Diagnostic.getEndPosition, the end-pos-table is not set yet, and the method crashes with an NPE
• the end-pos-table is never set for implicitly compiled (parsed) files:
  

  jdk/src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavaCompiler.java

  Line 840 in 167c952

  tree = parse(filename, filename.getCharContent(false));

• when a Diagnostic is reported for a tree by the parser, the tree on which the Diagnostic appear does not typically have the end position set yet - it is usually set only after the error/warning is reported

All those problems used to be hidden by this check:

jdk/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java

Line 649 in 6c48f4e

if (endPosTable == null) {

but that has been removed:
c793de9

I have tried to fix this properly, by deferring the errors from the parser to a later point, properly setting the end-pos-table, and handling implicitly parsed files more correctly:
https://github.com/openjdk/jdk/compare/master...lahodaj:jdk:JDK-8371309b?expand=1
but this change is a bit involved, and potentially a bit dangerous.

So, for now, I propose to reinstate the null check in TreeInfo.getEndPos, to restore the JDK 25 behavior, and attempt to do the better solution later, hopefully earlier in the release cycle.

---

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-8371309: Diagnostic.getEndPosition can throw an NPE with typical broken code (Bug - P3)

Reviewers

• Vicente Romero (@vicente-romero-oracle - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28316

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jlahoda! 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]

@lahodaj 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:

8371309: Diagnostic.getEndPosition can throw an NPE with typical broken code

Reviewed-by: vromero

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 148 new commits pushed to the master branch:

• ad3dfaf: 8360564: Implement JEP 524: PEM Encodings of Cryptographic Objects (Second Preview)
• cc05530: 8371732: [redo] Change java.time month/day field types to 'byte'
• 91b97a4: 8371922: Remove unused NonblockingQueue class
• ... and 145 more: https://git.openjdk.org/jdk/compare/f5d8bd0dd50bcd963b4062997aecb4e15249e30d...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

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

• compiler

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.

[mlbridge]

Webrevs

• 00: Full (ef1f68a7)

[archiecobbs]

It's my fault for provoking this issue with the commit you referenced - apologies, that change was overly optimistic. I agree the practical answer for now is to add back the null check.

It makes me wonder if the specified behavior for Diagnostic.getEndPosition() was previously being adhered to. It says:

offset from beginning of file; NOPOS if and only if getPosition() returns NOPOS

It seems like more realistic (and accurate) specification would be:

offset from beginning of file; NOPOS if the ending position is not available

That's a separate question from this PR of course.

[vicente-romero-oracle]

looks good