RFR: JDK-8273244: Improve diagnostic output related to ErroneousTree [v2]

[Pavel Rappo]

On Tue, 21 Sep 2021 22:20:07 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

>> 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.
>
> Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision:
> 
>   revert unrelated debug edit

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.

src/jdk.compiler/share/classes/com/sun/tools/javac/api/JavacTrees.java line 269:

> 267:                     DCTree dcTree = (DCTree) tree;
> 268:                     if (dcComment.getSourcePosition(dcTree.getEndPosition()) == 0) {
> 269:                         System.err.println("WARNING: comment:" + dcComment.comment + ": " + dcTree.getKind() + " end:" + dcTree.getEndPosition() + " result: " + dcComment.getSourcePosition(dcTree.getEndPosition()));

This looks like a leftover debugging aid.

src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java line 971:

> 969:     /**
> 970:      * Creates an {@code ErroneousTree} node, for a range of text starting at a given position,
> 971:      * ending at the last non-white character before the current position,

In this file and in JDK overall, such characters as predominantly called "non-whitespace".

src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java line 985:

> 983:     /**
> 984:      * Creates an {@code ErroneousTree} node, for a range of text starting at a given position,
> 985:      * ending at the last non-white character before the current position,

ditto

src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java line 986:

> 984:      * Creates an {@code ErroneousTree} node, for a range of text starting at a given position,
> 985:      * ending at the last non-white character before the current position,
> 986:      * and with a given preferred position

Add full stop.

src/jdk.compiler/share/classes/com/sun/tools/javac/parser/ReferenceParser.java line 124:

> 122:             moduleName = switch (slash) {
> 123:                 case -1 -> null;
> 124:                 case 0 -> throw new ParseException(0, "dc.ref.syntax.error");

There's a slight change in error output. Although it is likely insignificant, I felt I should note it. For example, compare the error output for `{@link //java.lang.Object}`.

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 74:

> 72:  * </ul>
> 73:  *
> 74:  * All are values relative to the beginning of the

Transposition error? "All values are relative..."

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 88:

> 86:     /**
> 87:      * The position of the first character that is unique to this node.
> 88:      * It is normally set by the methods in {@link DocTreeMaker}.

Since this change also includes cleanup, we might as well fix "the the" and "non-white space" at DocTreeMaker.java:699.

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 161:

> 159:         }
> 160: 
> 161:         switch (getKind()) {

This switch might become even better after "Pattern Matching for switch" has been integrated (currently in preview, JEP 406).

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 265:

> 263:      * {@return a diagnostic position based on the positions in a comment}
> 264:      *
> 265:      * The positions are lazily converted to file-based positions as needed.

"lazily" somewhat duplicates "as needed", no?

test/langtools/tools/javac/doctree/SerialTest.java line 26:

> 24: /*
> 25:  * @test
> 26:  * @bug 7021614 8273244 8273244

Duplicating bug IDs.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5510