JDK-8273244: Improve diagnostic output related to ErroneousTree

[jonathan-gibbons]

Please review a medium size update/overhaul to the way that positions and diagnostic positions are handled in the DocTree/DCTree world.

Previously ...

Previously, most DCTree nodes only had an explicit "position" (pos). Start and end positions were provided by DocSourcePositions, but these were not directly available in tree nodes, because of the lack of access to the DocSourcePositions class. (In contrast, JCTree nodes do have position info, via static methods in TreeInfo.) The one notable exception to these guidelines was DCErroneous which (by analogy with JCTree) directly implemented JCDiagnostic.DiagnosticPosition but that was an arguably bad implementation because the positions were relative to the start of the comment, and not the start of the file. This did not show up in code and tests, because diagnostics related to DocTree nodes used DCTree.pos which returned a SimpleDiagnosticPosition referencing just the start position -- at least in part because more specific information was not easily available.

Now ...

All DCTree nodes have 4 positions, 3 publicly available.

• the position of the first unique character, pos
• the starting position of the range of characters, getStartPosition()
• the "preferred" position in the range, used to position the caret in diagnostic messages, getPreferredPosition()
• the end position of the range of characters, getEndPosition().
  These are all relative to the beginning of the comment text, and are converted to file positions as needed.

Code to implement the end position is moved from JavacTrees to DCTree. It's still a switch on kind, but could reasonably be converted to using overriding methods.

DCErroneous no longer implements JCDiagnostic.DiagnosticPosition. the constructor methods to create a diagnostic are removed, in favor of passing in a correctly-formed diagnostic.

DCTree has a new improved pos(DCDocComment) method which now uses the new start/pref/end position methods.

DocCommentParser.ParseException and the erroneous method now take an optional "pos" parameter to allow the position of an error to be more accurately specified.

Testing ...

Up until the point at which DCTree.pos was modified, all tests passed without change. When DCTree.pos() was modified, a few (3) doclint tests starting failing, demonstrating a latent reliance of the old form of DCTree.pos(). These tests are updated.

Rather than write a single new test, the existing DocCommentTester class is updated to include a new Checker which, generally, checks the "left to right" nature of all positions in a doc comment tree and its subtrees. This leverages all the existing good and bad test cases in the tools/javac/doctree directory, which therefore all get tagged with the bug number for this issue.

Behavior ...

Apart from fixing generally bad behavior, there is one other tiny behavioral change. For an empty DocCommentTree the ending position is now the same at the starting position, and not NOPOS. This was triggered by the new code in DocCommentTester, but which affected one jshell test, which started getting StringIndexOutOfBounds exception. This is minimally fixed for now, but the code in question would arguably be better to use the new comment-based positions, rather than the file-based positions. But that seems out of scope for this work. Also worth investigating is the method JavacTrees.getDocCommentTree(FileObject fileObject) which uses a fixed offset of 0 (JavacTrees, line 1052) when creating doc comments for HTML files.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8273244: Improve diagnostic output related to ErroneousTree

Reviewers

• Pavel Rappo (@pavelrappo - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5510/head:pull/5510
$ git checkout pull/5510

Update a local copy of the PR:
$ git checkout pull/5510
$ git pull https://git.openjdk.java.net/jdk pull/5510/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5510

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5510.diff

[bridgekeeper]

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

@jonathan-gibbons The following labels will be automatically applied to this pull request:

• compiler
• javadoc

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

[mlbridge]

Webrevs

• 05: Full - Incremental (31547632)
• 04: Full - Incremental (a532b2f3)
• 03: Full - Incremental (14c29bdc)
• 02: Full - Incremental (cf6d3b8e)
• 01: Full - Incremental (4ddc0aaf)
• 00: Full (4a485168)

[pavelrappo]

As this PR contains a lot clean-up changes and improvements, it will take me extra time to review it. Here's the first batch of comments.

[pavelrappo]

I'd feel more confident if Hannes looked at changes related to ReferenceParser.

[pavelrappo]

Apart from fixing generally bad behavior, there is one other tiny behavioral change. For an empty DocCommentTree the ending position is now the same at the starting position, and not NOPOS.

I suppose we don't need to reflect this change anywhere in the API (CSR), do we?

[jonathan-gibbons]

Apart from fixing generally bad behavior, there is one other tiny behavioral change. For an empty DocCommentTree the ending position is now the same at the starting position, and not NOPOS.

I suppose we don't need to reflect this change anywhere in the API (CSR), do we?

I don't believe the specification is so fine-grained.

[jonathan-gibbons]

It turns out there is remarkable similarity between the new CoverageTest (2021) and the much older SimpleDocTreeVisitorTest (2012) written with the API was first introduced. The new test is effectively an unintentional modern rewrite that supersedes the old test. We might want to merge them at some point, or simply delete the old one.

[pavelrappo]

Looks ok.

[openjdk]

@jonathan-gibbons 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:

8273244: Improve diagnostic output related to ErroneousTree

Reviewed-by: prappo

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

• 7eb0372: 8274606: Fix jaxp/javax/xml/jaxp/unittest/transform/SurrogateTest.java test
• 47bfc8a: 8274563: jfr/event/oldobject/TestClassLoaderLeak.java fails when GC cycles are not happening
• 0828273: 8274521: jdk/jfr/event/gc/detailed/TestGCLockerEvent.java fails when other GC is selected
• 6726c59: 8274634: Use String.equals instead of String.compareTo in java.desktop
• 3281102: 8268084: [macos] Disabled JMenuItem arrow is not disabled
• 7957994: 8273695: Safepoint deadlock on VMOperation_lock
• 9ca6bf0: 8274505: Too weak variable type leads to unnecessary cast in java.desktop
• 0786d8b: 8268435: (ch) ChannelInputStream could override readAllBytes
• bb4500d: 8274465: Fix javax/swing/text/ParagraphView/6364882/bug6364882.java failures
• 05d3860: 8274605: Fix predicate guarantees on returned values in (Doc)SourcePositions
• ... and 243 more: https://git.openjdk.java.net/jdk/compare/fc0f8542c387e7f25992cc7eaa2bb45aeace3c39...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.

[jonathan-gibbons]

/integrate

[openjdk]

Going to push as commit 0ca094b.
Since your change was applied there have been 257 commits pushed to the master branch:

• 6f727d8: 8274666: rename HtmlStyle.descfrmTypeLabel to be less cryptic
• 139a833: 8268869: java in source-file mode suggests javac-only Xlint flags
• f63c4a8: 8274471: Verification of OCSP Response signed with RSASSA-PSS fails
• f2404d6: 8274658: ISO 4217 Amendment 170 Update
• 7eb0372: 8274606: Fix jaxp/javax/xml/jaxp/unittest/transform/SurrogateTest.java test
• 47bfc8a: 8274563: jfr/event/oldobject/TestClassLoaderLeak.java fails when GC cycles are not happening
• 0828273: 8274521: jdk/jfr/event/gc/detailed/TestGCLockerEvent.java fails when other GC is selected
• 6726c59: 8274634: Use String.equals instead of String.compareTo in java.desktop
• 3281102: 8268084: [macos] Disabled JMenuItem arrow is not disabled
• 7957994: 8273695: Safepoint deadlock on VMOperation_lock
• ... and 247 more: https://git.openjdk.java.net/jdk/compare/fc0f8542c387e7f25992cc7eaa2bb45aeace3c39...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jonathan-gibbons Pushed as commit 0ca094b.

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