JDK-8252717: Integrate/merge legacy standard doclet diagnostics and doclint

[jonathan-gibbons]

Please review a moderate change to enable (most) doclet warnings even if doclint is enabled.

Since the introduction of doclint, there was some (small) overlap between the small set of warnings generated by the doclet and the new larger set of diagnostics that could be generated by doclint. The solution, until now, has been to disable doclet warnings when doclint is enabled. But, neither set contains the other, and the policy has inappropriately suppressed some warnings and inhibited writing new code to generate new warnings by the doclet that could only be done by the doclet, and not doclint.

One notable group of warnings that has been inappropriately suppressed is the warnings generated by using the -serial warn option.

The fundamental core of the change is to remove the conditional checks in the doclet Messages.java, which would suppress messages when doclint was enabled. A consequence is that some specific messages have to disabled if they are duplicate checks if doclint is enabled. (The messages cannot be removed altogether because doclint might not be enabled.) A test is added to verify that either one message or the other is generated, but never both.

As previously noted, an issue with the earlier policy is that warnings generated by using the -serial warn option were inappropriately suppressed, and this change fixes that ... revealing a number of latent warnings in the JDK API that need to be addressed. The short term fix here is to temporarily remove the use of the -serialwarn option. See JDK-8287749.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8252717: Integrate/merge legacy standard doclet diagnostics and doclint

Reviewers

• Erik Joelsson (@erikj79 - Reviewer) ⚠️ Review applies to 2f4bc7fb
• Pavel Rappo (@pavelrappo - Reviewer) ⚠️ Review applies to 2f4bc7fb

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9006

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

Using diff file

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

[bridgekeeper]

👋 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]

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

• build
• javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (55b5576f)
• 00: Full (2f4bc7fb)

[erikj79]

Build change looks ok.

[openjdk]

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

8252717: Integrate/merge legacy standard doclet diagnostics and doclint

Reviewed-by: erikj, 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 22 new commits pushed to the master branch:

• 28c112f: 8287685: [BACKOUT] JDK-8287384 Speed up jdk.test.lib.util.ForceGC
• 34bb0a5: 8287734: Shenandoah: Consolidate marking closures
• 33b9d77: 8287729: Loom: Check alternative implementation on x86_32
• a75299c: 8287686: Add assertion to ensure that disarm value offset < 128
• 407abf5: 8233477: [Win LAF]The tooltip doesn't display correctly in Win LAF
• 625821d: 8287661: Fix and improve BitMap::print_on(outputStream*)
• b544b8b: 8276789: Support C++ lambda in ResourceHashtable::iterate
• ba9ee8c: 8287748: Fix issues in java.lang.foreign package javadoc
• deb0653: 8287337: SnippetUtils should throw exceptions if snippets not found
• cbaeb00: 8287600: AA Ovals not rendered under metal
• ... and 12 more: https://git.openjdk.java.net/jdk/compare/3cfd38caf10c18f71c0fc8c9a09c0d1179373ce7...master

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.

[pavelrappo]

Since the introduction of doclint, there was some (small) overlap between the small set ... and the new larger set ... But, the sets do not overlap

Do or do they not overlap? :)

the policy has inappropriately suppressed some warnings and inhibited the generation of new warnings

What's the difference between "suppressed" and "inhibited" here?

Generally, how can we be sure that we haven't missed any double-reported cases?

[jonathan-gibbons]

Since the introduction of doclint, there was some (small) overlap between the small set ... and the new larger set ... But, the sets do not overlap

Do or do they not overlap? :)

Ooops. Wrong word. I was meaning that neither set contains the other.

the policy has inappropriately suppressed some warnings and inhibited the generation of new warnings

What's the difference between "suppressed" and "inhibited" here?

I was using "suppressed" with respect to existing doclet warnings, which have not been displayed if doclint was enabled, and was using "inhibited" with respect to writing code to generate new warnings, meaning that there was not much benefit to adding new warnings if they were typically going to be suppressed when doclint was enabled.

Generally, how can we be sure that we haven't missed any double-reported cases?

I did a visual inspection over the Checker class. Generally, Checker does more in the area of "missing" comments and HTML issues, with less focus on "reference" issues, while the doclet does not report missing comments and does not even attempt to detect HTML issues. The overlap is "syntax" issues and "reference" issues, which are covered here.

The test is set up to be a framework to accommodate new cases if any should arise.

[pavelrappo]

I did a visual inspection over the Checker class.

The test is set up to be a framework to accommodate new cases if any should arise.

This is good. It would be even better if we had a mechanism to reduce risk of diagnostic duplication in the future. I don't have any concrete proposal, but unique internal error codes shared between doclint and doclet come to mind.

[pavelrappo]

Looks good.

[jonathan-gibbons]

I did a visual inspection over the Checker class.

The test is set up to be a framework to accommodate new cases if any should arise.

This is good. It would be even better if we had a mechanism to reduce risk of diagnostic duplication in the future. I don't have any concrete proposal, but unique internal error codes shared between doclint and doclet come to mind.

I would also like to have a policy to align the user-facing content of the diagnostic message. This includes the severity (warning or error) as well as the position and message text, including substituted values.

[jonathan-gibbons]

/integrate

[openjdk]

Going to push as commit 59e9700.
Since your change was applied there have been 22 commits pushed to the master branch:

• 28c112f: 8287685: [BACKOUT] JDK-8287384 Speed up jdk.test.lib.util.ForceGC
• 34bb0a5: 8287734: Shenandoah: Consolidate marking closures
• 33b9d77: 8287729: Loom: Check alternative implementation on x86_32
• a75299c: 8287686: Add assertion to ensure that disarm value offset < 128
• 407abf5: 8233477: [Win LAF]The tooltip doesn't display correctly in Win LAF
• 625821d: 8287661: Fix and improve BitMap::print_on(outputStream*)
• b544b8b: 8276789: Support C++ lambda in ResourceHashtable::iterate
• ba9ee8c: 8287748: Fix issues in java.lang.foreign package javadoc
• deb0653: 8287337: SnippetUtils should throw exceptions if snippets not found
• cbaeb00: 8287600: AA Ovals not rendered under metal
• ... and 12 more: https://git.openjdk.java.net/jdk/compare/3cfd38caf10c18f71c0fc8c9a09c0d1179373ce7...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jonathan-gibbons Pushed as commit 59e9700.

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