JDK-8267126: javadoc should show "line and caret" for diagnostics. #4074

jonathan-gibbons · 2021-05-17T20:04:49Z

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:.

Progress

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

Issue

JDK-8267126: javadoc should show "line and caret" for diagnostics.

Reviewers

Pavel Rappo (@pavelrappo - Reviewer) ⚠️ Review applies to 55e6be3

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4074

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

Using diff file

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

bridgekeeper · 2021-05-17T20:06:20Z

👋 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 · 2021-05-17T20:09:17Z

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

javadoc

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 · 2021-05-17T20:13:24Z

Webrevs

pavelrappo

This is not a review, but preliminary comments to help me get started with the review.

pavelrappo · 2021-05-18T13:44:47Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/TagletManager.java

+            if (configuration.workArounds.accessInternalAPI()) {
+                Module thisModule = getClass().getModule();
+                Module tagletLoaderUnnamedModule = tagClassLoader.getUnnamedModule();
+                List<String> pkgs = List.of(
+                        "jdk.javadoc.doclet",
+                        "jdk.javadoc.internal.doclets.toolkit",
+                        "jdk.javadoc.internal.doclets.formats.html");
+                pkgs.forEach(p -> thisModule.addOpens(p, tagletLoaderUnnamedModule));
+            }


What's that and the corresponding WorkArounds.accessInternalAPI about?

It's Module System Magic. :-) Don't look too hard at the man behind the curtains.

The general problem is allowing external code to access module internals if they really want it and ask nicely. For external code that is provided on the class path, there are command-line options like --add-exports and --add-opens. These options allow a user to override access to module-internal classes for classes in the application class loader, which (colloquially) is the unnamed module for classes on the classpath.

The problem is that those command-line options do not grant access from module-internal classes to classes in "other" unnamed modules ... yes, there are many unnamed modules: essentially one per class loader. So, it is up to the creator of any additional class loader to decide whether to grant privileges to the classes in that class loader.

For javac, there are (regrettably) annotation processors and plugins that want to access javac internals, and those classes are (you guessed) loaded into a new class loader based on the -processorpath option. And so the decision was made to provide an undocumented option to allow such code the same access that they had before the module system was introduced. That javac option is -XDaccessInternalAPI. Note that -XD is a semi-magic option that effectively allows any name=value to be set in the internal options map, thus bypassing the normal mechanism for documentated options. And, side-note, javadoc provides the same -XD option for the same reasons. If you search the javac code for the string "accessInternalAPI" you will find two usages, one for plugins and one for anno-processors, and both result in a low-level utility method being called, ModuleHelper.addExports, which gives various access rights to any classes in that class loader.

Side-story: In JDK 9, ModuleHelper was much more complicated in its implementation, because it had to work entirely through core-reflection, because of the "bootstrap compiler" issue. Nowadays, it's much simpler.

Back to this PR. You've probably managed to guess most of the story at this point. There's functionality missing from taglets, but I want to write a test taglet that gets at the missing information. So, I've copied the javac backdoor mechanism, and opened up access to some of the internal javadoc packages, provided that a suitable command-line option is given. Further out, we should provide some of the API that is currently missing, so that the test code no longer needs access to doclet internals.

As for WorkArounds, that class is the only one we allow in the standard doclet to access "tool stuff" via backdoors. In this case, WorkArounds needs to access the compiler options, to see if XDaccessInternalAPI has been set.

Note that both the javac and javadoc mechanism are in line with JPMS policy that you have to ask for the enhanced access by using command-line options.

pavelrappo · 2021-05-18T13:59:17Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java

+     * @return the diagnostic position
+     */
+    private DiagnosticPosition getDiagnosticPosition(DocTreePath path) {
+        ToolEnvironment toolEnv = getToolEnv();


toolEnv is unused.

Thanks, I'll double check and delete if unused.

Confirmed; deleted. It preceded the use of utility method getSourcePositions.

I'll update this in the next commit.

pavelrappo · 2021-05-18T14:03:49Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/ToolEnvironment.java

@@ -144,7 +142,6 @@ protected ToolEnvironment(Context context) {
        finder = JavadocClassFinder.instance(context);
        enter = JavadocEnter.instance(context);
        names = Names.instance(context);
-        externalizableSym = syms.enterClass(syms.java_base, names.fromString("java.io.Externalizable"));


What was externalizableSym about and why have you deleted it?

TL;DR: the value was unused. It's gratuitous cleanup ;-)

The value was the Symbol (javac subtype of Element) for the class java.io.Externalizable. I presume at some point it was required for handling serializable/externalizable classes, by checking if it was a supertype of a class being documented.

pavelrappo · 2021-05-18T14:09:51Z

test/langtools/jdk/javadoc/doclet/testJavaFX/TestJavaFX.java

@@ -374,7 +374,7 @@ public void test4() {
        checkExit(Exit.OK);

        // make sure the doclet indeed emits the warning
-        checkOutput(Output.OUT, true, "C.java:0: warning - invalid usage of tag <");
+        checkOutput(Output.OUT, true, "C.java:31: warning: invalid usage of tag <");


This is interesting: while the line coordinate of an error has changed in this test, those of TestSearch.java L676-L679 from this same PR have not. Mind you, I haven't investigated it yet.

It's a different error message, coming from a different place in the code, presumably with different contextual info.

In the other case, it was correct both before and after. In this case, it was wrong before and now correct. So that's good.

Tip: to find where errors come from, use (I think) -XD-doe. As I wrote earlier in a different PR, -XD is the general mechanism to set any option. -doe is a hidden javac option, short for "dump on error", and triggers a track trace when any diagnostic is reported. It's handled down in Log. Look for uses of Log.dumpOnError, especially roundabout line 771.

pavelrappo · 2021-05-18T14:24:19Z

test/langtools/jdk/javadoc/doclet/testJavaFX/TestJavaFX.java

@@ -419,6 +419,7 @@ public final BooleanProperty somethingProperty() {
        javadoc("-d", "out5",
                "--javafx",
                "--disable-javafx-strict-checks",
+                "--no-platform-links",


It just happens so that yesterday I asked Hannes about proliferation of --no-platform-links in tests and now we have 3 more of those in this PR alone.

I was getting more messages reporting that the classes could not be found. It was throwing off warning counts in golden output. My unproven suspicion is that we may not have been accurately counting warnings before this change.

pavelrappo

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

pavelrappo · 2021-05-21T17:39:43Z

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

+    /**
+     * Returns whether or not to permit dynamically loaded components to access
+     * part of the javadoc internal API. The flag is the same (hidden) compiler
+     * 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.

Yes, there was a missing verb. Fixed.

(Copyrights: only TagletManager needed updating; done.)

pavelrappo · 2021-05-21T17:53:06Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java

+ * The class leverages the javac support for reporting diagnostics, for stylistic consistency
+ * of diagnostic messages and to avoid code duplication.
+ *
+ * The class is a subtype of javac's Log, and is primarily a transducer between


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

Both are valid, but agreed that "adapter" is a more common term.

pavelrappo · 2021-05-21T18:08:26Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java

    }

    /**
-     * Print a message.
-     * Part of DocErrorReporter.
+     * Print a "notice" message.


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

Definitely.

pavelrappo · 2021-05-21T18:16:51Z

test/langtools/jdk/javadoc/doclet/testDiagsLineCaret/MyTaglet.java

+
+            @Override
+            public Void visitEntity(EntityTree node, Diagnostic.Kind k) {
+                if (node.getName().contentEquals("#x1f955")) {


Is this... a carrot (🥕)?

https://www.fileformat.info/info/unicode/char/search.htm :-)

pavelrappo · 2021-05-21T18:17:51Z

test/langtools/jdk/javadoc/doclet/testMissingComment/TestMissingComment.java

@@ -112,6 +112,7 @@ private void test(Path base, String code, String expect) throws Exception {

        javadoc("-d", base.resolve("api").toString(),
                "-Xdoclint:missing",
+                "--no-platform-links",


More of those... sigh.

A different solution would be to disable platform links by default (in JavadocTester) and only enables for specific tests. But that would be a minor paradigm shift of JavadocTester providing default options.

pavelrappo · 2021-05-21T18:20:02Z

test/langtools/jdk/javadoc/tool/MaxWarns.java

@@ -75,7 +75,7 @@ File genSrc(int count) throws IOException {
    String javadoc(File f) {
        StringWriter sw = new StringWriter();
        PrintWriter pw = new PrintWriter(sw);
-        String[] args = { "-Xdoclint:none", "-d", "api", f.getPath() };
+        String[] args = { "-Xdoclint:none", "--no-platform-links", "-d", "api", f.getPath() };


pavelrappo · 2021-05-21T18:34:57Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java

+ *     <li>{@code Element} -- maps to {@code DiagnosticSource and {@code DiagnosticPosition}
+ *     <li>{@code DocTreePath} -- maps to {@code DiagnosticSource and {@code DiagnosticPosition}


Close @code tags.

mildly worrying that "make docs" did not give errors ... but I guess the error was masked by the subsequent } of the subsequent {@code ...}

mildly worrying that "make docs" did not give errors

This should be currently detectable by DocLint: since </ul> is consumed by @code, the ul element is left unclosed.

pavelrappo · 2021-05-21T18:36:13Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java

+ * The javac layer deals primarily in pre-localized (key, args) pairs,
+ * 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.

I'll try. Maybe something based on "operates on" or "uses" or something.

Rephrased the sentence and added more details as well.

pavelrappo · 2021-05-21T18:36:42Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java

+    /** The tool environment, providing access to the tool's utility classes and tables. */
+    private ToolEnvironment toolEnv;
+
+    /** The utility class to access the positions of items in doc comments., */


Remove that trailing comma.

pavelrappo · 2021-05-21T18:37:45Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java

+     * messages directly. (A better solution would be to expose access to the output
+     * and error streams via {@code DocletEnvironment}.


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

Add ")" and change DocletEnvironment to Reporter for now, and to delete the ~~sentence~~ paragraph when we add streams to Reporter.

openjdk · 2021-05-24T21:53:47Z

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

8267126: javadoc should show "line and caret" for diagnostics.

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

461a3fe: 8261478: InstanceKlass::set_classpath_index does not match comments
de27da7: 8267431: Rename InstanceKlass::has_old_class_version to can_be_verified_at_dumptime
c519ba2: 8267614: Outline VarHandleGuards exact behavior checks
f690959: 8267446: Taskqueue code fails with assert(bottom_relaxed() == age_top_relaxed()) failed: not empty

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 · 2021-05-25T01:31:29Z

/integrate

openjdk · 2021-05-25T01:33:48Z

@jonathan-gibbons Since your change was applied there have been 4 commits pushed to the master branch:

461a3fe: 8261478: InstanceKlass::set_classpath_index does not match comments
de27da7: 8267431: Rename InstanceKlass::has_old_class_version to can_be_verified_at_dumptime
c519ba2: 8267614: Outline VarHandleGuards exact behavior checks
f690959: 8267446: Taskqueue code fails with assert(bottom_relaxed() == age_top_relaxed()) failed: not empty

Your commit was automatically rebased without conflicts.

Pushed as commit b4d4884.

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

JDK-8267126: javadoc should show "line and caret" for diagnostics.

78ba02f

openjdk bot added the rfr Pull request is ready for review label May 17, 2021

openjdk bot added the javadoc javadoc-dev@openjdk.org label May 17, 2021

minor cleanup

c57d93a

pavelrappo reviewed May 18, 2021

View reviewed changes

pavelrappo suggested changes May 21, 2021

View reviewed changes

jonathan-gibbons added 2 commits May 24, 2021 12:19

address review feedback

85d910c

fix typos

55e6be3

pavelrappo approved these changes May 24, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 24, 2021

Merge with upstream/master

38756a9

openjdk bot closed this May 25, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 25, 2021

jonathan-gibbons deleted the jdk-8267126-line-caret branch May 25, 2021 01:34

		* <li>{@code Element} -- maps to {@code DiagnosticSource and {@code DiagnosticPosition}
		* <li>{@code DocTreePath} -- maps to {@code DiagnosticSource and {@code DiagnosticPosition}

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

		* messages directly. (A better solution would be to expose access to the output
		* and error streams via {@code DocletEnvironment}.

JDK-8267126: javadoc should show "line and caret" for diagnostics. #4074

JDK-8267126: javadoc should show "line and caret" for diagnostics. #4074

Conversation

jonathan-gibbons commented May 17, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 17, 2021

openjdk bot commented May 17, 2021

mlbridge bot commented May 17, 2021 • edited

Webrevs

pavelrappo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-gibbons May 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelrappo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-gibbons May 24, 2021 • edited

Choose a reason for hiding this comment

openjdk bot commented May 24, 2021 • edited

jonathan-gibbons commented May 25, 2021

openjdk bot commented May 25, 2021

jonathan-gibbons commented May 17, 2021 •

edited by openjdk bot

mlbridge bot commented May 17, 2021 •

edited

jonathan-gibbons May 19, 2021 •

edited

jonathan-gibbons May 24, 2021 •

edited

openjdk bot commented May 24, 2021 •

edited