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

[Pavel Rappo]

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

Nit. Could you please remove these (now obsolete) imports?

    import java.util.LinkedHashMap;
    import java.util.Map;

> 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?

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?

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

> 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?

> 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