RFR: JDK-8236949: javadoc -Xdoclint does not accumulate options correctly

[Jonathan Gibbons]

On 01/30/2020 07:38 AM, Pavel Rappo wrote:
>> On 30 Jan 2020, at 01:25, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
>>
>> Please review a moderately simple fix to a regression in the way that javadoc handles doclint options.
>>
>> The feature was introduced in JDK 8, and at that time, the doclet options were accumulated in a list.
>>
>> The regression was introduced when the standard doclet was rewritten in JDK 9, and perhaps because of the introduction of other new doclint options, the options were changed to accumulate in a map, with "last one wins" semantics for like-named options. This is the root cause of this issue.
> Right. Arguments of an option are keyed by the instance, of the XOption type,
> that handles this option. Like-named options are handled by the same instance of
> the XOption type. Hence, when two sets of like-named options' arguments are
> keyed, one of these sets is effectively overwritten.
>
> This didn't necessarily mean we needed to do away with the map, we could use
> `merge` or similar. But I agree with this change. We should keep it simple.

If we were to keep the map, it would most naturally be a Map<Option, 
List<String>>
but even that is suspect for losing the ordering between -Xmsgs 
and-Xmsgs:...
>
> Nit. Could you please remove these (now obsolete) imports?
>
>      import java.util.LinkedHashMap;
>      import java.util.Map;

Done, also jdk.javadoc.doclet.Doclet;
>> The fix is to revert to accumulating options in a list, as originally intended. In addition, the detection of whether doclint should be enabled or not is improved, compared to JDK 8, such that doclint is only enabled if either specific groups or all groups are enabled. Previously, it would only be disabled if just -Xdoclint:none was specified.
> Yes, current behavior seems be short-circuited by a stand-alone occurrence of
> "none" (i.e. "-Xdoclint:none"). The proposed behavior always parses arguments
> to completion, left to right. Is it expected behavior? Does javac do the same?

javac does not do the short-circuit and also parses left to right.

>
> On a related note. I seem to understand how this code works. Still, why not
> delegate options processing to DocLint itself? Is it because javadoc only
> accepts a *subset* of DocLint options (e.g. no access qualifiers), or there
> are other reasons?

It's true that javadoc only accepts a subset of options: the "no access 
qualifiers"
is to avoid conflicts with javadoc acces options, like -private, 
-protected etc.
Put another way, javac's doclint options do leverage access qualifiers, 
because
javac does not have the equivalent of javadoc's access options.

javadoc also wants to avoid using doclint if it is disabled. For javac, 
doclint is
opt-in, for javadoc it is opt-out such that it is not initialized if the 
user opts out.

The design of doclint is to use string-based options. If we were to 
change anything,
I would open up a new interface in doclint to allow options to be set 
directly via
API, but that is beyond the scope of this changeset. That also ties into the
separate architectural issue of "which module does doclint live in?"

>
>> The penance for not writing a better unit test in JDK 8 is to write a better regression test now.
> I think I dislike that the implementation package was added for the sake of
> reusing the com.sun.tools.doclint.Messages.Group enum, but it is not that big
> of a problem. It just makes the test "gray-box".

This was somewhat deliberate, so that the test could leverage 
EnumSet.allOf(Group.class)
but maybe that's not so important, since it is explicit in other ways 
what the kinds of
diagnostic are, and there is no check in this test that we have full 
coverage of the Group
enum. That's a technique I sometimes use in other tests.

>
>> One other test (added in JDK 9) was relying on the bug, and needed to be updated.
>>
>> In this changeset ...
>>
>> HtmlConfiguration.java ... the collection is changed to a list
>>
>> HtmlOptions ... the options are transformed into doclint options (as before) and accumulated in the list, but only if they pass the initial check. There is also some minor cleanup ... the option for -Xdocrootparent is moved out of the group of doclint options, and some minor warnings highlighted by the IDE were fixed.
> Thanks for incremental improvements and miscellaneous cleanup!
>
>    180    doclint.init(t, doclintOpts.toArray(new String[0]), false);
>
> What was the reason for changing this?

The IDE suggested it. Conventional wisdom these days is that it is 
(surprisingly)
more efficient to use an array size of 0.

>
>> WorkArounds ... the primary change here is better analysis of the doclint options to determine if doclint should be enabled. Comments are added, and the "TODO: fix this up correctly" is removed ;-)
>>
>> The new test uses a source file that will trigger different doclint diagnostics, depending on which are enabled. It then runs javadoc with different doclint options to verify that (just) the expected diagnostics are generated.
>>
>> -- Jon
>>
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8236949
>> Webrev: http://cr.openjdk.java.net/~jjg/8236949/webrev.00/
> It would be nice if the original reporter could confirm this changeset solves
> their issue (this email is CC'ed to them).
>
> Looks good.
>
> -Pavel
>
>
>