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

Pavel Rappo prappo at openjdk.java.net
Mon Sep 27 15:58:10 UTC 2021


On Wed, 22 Sep 2021 16:11: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:
> 
>   address preliminary review comments

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

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

I can see that you changed this switch while moving it from JavacTrees.

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

> 172:             case IDENTIFIER -> {
> 173:                 DCIdentifier ident = (DCIdentifier) this;
> 174:                 return ident.pos + ident.name.length();

While in JavacTrees, this logic contained some special handling for `names.error`. What was that about?

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

> 175:             }
> 176: 
> 177:             case AUTHOR, DEPRECATED, HIDDEN, PARAM, RETURN, SEE, SERIAL, SERIAL_DATA, SERIAL_FIELD, SINCE, THROWS, UNKNOWN_BLOCK_TAG, VERSION -> {

You added HIDDEN and merged PARAM into this case of block tags. Should we also add PROVIDES and USES? If so, then we should add respective tests.

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

> 190:             case ENTITY -> {
> 191:                 DCEntity endEl = (DCEntity) this;
> 192:                 return endEl.pos + endEl.name.length() + 2;

A similar question about `names.error`.

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

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


More information about the compiler-dev mailing list