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

[Jonathan Gibbons]

On Tue, 18 May 2021 13:44:47 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   minor cleanup
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/TagletManager.java line 264:
> 
>> 262:                         "jdk.javadoc.internal.doclets.formats.html");
>> 263:                 pkgs.forEach(p -> thisModule.addOpens(p, tagletLoaderUnnamedModule));
>> 264:             }
> 
> 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.

> src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Messager.java line 474:
> 
>> 472:      */
>> 473:     private DiagnosticPosition getDiagnosticPosition(DocTreePath path) {
>> 474:         ToolEnvironment toolEnv = getToolEnv();
> 
> toolEnv is unused.

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

> src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/ToolEnvironment.java line 147:
> 
>> 145:         enter = JavadocEnter.instance(context);
>> 146:         names = Names.instance(context);
>> 147:         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.

> test/langtools/jdk/javadoc/doclet/testJavaFX/TestJavaFX.java line 422:
> 
>> 420:                 "--javafx",
>> 421:                 "--disable-javafx-strict-checks",
>> 422:                 "--no-platform-links",
> 
> It just happens so that yesterday I [asked](https://github.com/openjdk/jdk/pull/4051#pullrequestreview-660862882) 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.

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

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