RFR: JDK-8267126: javadoc should show "line and caret" for diagnostics. [v2]

[Pavel Rappo]

On Mon, 17 May 2021 20:40:18 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

>> Please review a change to overhaul the javadoc support for diagnostics to better leverage the support available in javac. This includes the ability for all javadoc diagnostics to show a "source line and caret" to indicate the position of a reported issue. As a side-effect, it normalizes the formatting of javadoc messages, to be consistent with javac messages.
>> 
>> The primary changes are in the javadoc `Messager` class, and are primarily focussed "downward" on the internal use of the javac `Log.report` method, which is the nexus for reporting methods. There is additional cleanup that could be done in `Messager` in the API it provides to clients, but (for the most part) that is not done in this work.
>> 
>> Additional changes are done to facilitate writing a test for this work, and reflect the current shortcomings of the existing `Doclet` API. Most notably:
>> * changes in `Utils` to allow a user-defined taglet to override a built-in taglet
>> * changes in `TagletManager` and `Workarounds` to allow a user-defined taglet to access internal API, to workaround API that would be useful to provide on `StandardDoclet`
>> 
>> There are a few minor specific cleanup changes in code style and/or improved comments.
>> 
>> There is one primary new test, `TestDiagsLineCaret.java` which exercises different kinds of diagnostics at different positions in a file, to verify that the source line and a caret are produced as appropriate.
>> 
>> There are additional test changes triggered by the slight change in the format of error messages.  Most notably, prefixes like `error -` and `warning -` become `error:` and `warning:`.
>
> Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision:
> 
>   minor cleanup

I will continue reviewing this change. Meanwhile, please update copyright years and address other nits below.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/WorkArounds.java line 543:

> 541:      * Returns whether or not to permit dynamically loaded components to access
> 542:      * part of the javadoc internal API. The flag is the same (hidden) compiler
> 543:      * option that allows javac plugins and annotation processors to javac internal

Do you use "allow x to y" here in the sense of "letting x into y"? If so, consider using "allow x into y" instead of "allow x to y". Otherwise it might sound like there's a verb missing: allow to (access? enter?) javac internal API. My non-native English speaker's two cents.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java line 67:

> 65:  * of diagnostic messages and to avoid code duplication.
> 66:  *
> 67:  * The class is a subtype of javac's Log, and is primarily a transducer between

Would "adapter" be more suitable than "transducer"?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java line 92:

> 90:  *     <li>{@code Diagnostic.Kind} -- maps to {@code DiagnosticType} and {@code Set<DiagnosticFlag>}
> 91:  *     <li>{@code Element} -- maps to {@code DiagnosticSource and {@code DiagnosticPosition}
> 92:  *     <li>{@code DocTreePath} -- maps to {@code DiagnosticSource and {@code DiagnosticPosition}

Close `@code` tags.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java line 96:

> 94:  *
> 95:  * The javac layer deals primarily in pre-localized (key, args) pairs,
> 96:  * while the javadoc layer, especially the {@code Reporter} interface, deals in localized strings.

If it is not a typo, consider changing "deal in" to something simpler.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java line 125:

> 123:     private ToolEnvironment toolEnv;
> 124: 
> 125:     /** The utility class to access the positions of items in doc comments., */

Remove that trailing comma.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java line 332:

> 330: 
> 331:     /**
> 332:      * Print a "notice" message.

Add "s" to "print" for consistency with doc comment of other methods in this class.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java line 448:

> 446:      * such as  "Generating class ...".  Therefore, for now, we detect and report those
> 447:      * messages directly. (A better solution would be to expose access to the output
> 448:      * and error streams via {@code DocletEnvironment}.

Sentence: either remove "(" or add ")".

test/langtools/jdk/javadoc/doclet/testDiagsLineCaret/MyTaglet.java line 127:

> 125:             @Override
> 126:             public Void visitEntity(EntityTree node, Diagnostic.Kind k) {
> 127:                 if (node.getName().contentEquals("#x1f955")) {

Is this... a carrot (��)?

test/langtools/jdk/javadoc/doclet/testMissingComment/TestMissingComment.java line 115:

> 113:         javadoc("-d", base.resolve("api").toString(),
> 114:                 "-Xdoclint:missing",
> 115:                 "--no-platform-links",

More of those... sigh.

test/langtools/jdk/javadoc/tool/MaxWarns.java line 78:

> 76:         StringWriter sw = new StringWriter();
> 77:         PrintWriter pw = new PrintWriter(sw);
> 78:         String[] args = { "-Xdoclint:none", "--no-platform-links", "-d", "api", f.getPath() };

Argh...

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

Changes requested by prappo (Reviewer).

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