RFR: 8345263: Make sure that lint categories are used correctly when logging lint warnings

[Archie Cobbs]

On Wed, 4 Dec 2024 17:11:26 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> This PR tightens up the logic by which javac reports lint warnings. Currently, lint warnings are typically emitted using the following idiom:
> 
> 
> if (lint.isEnabled(LintCategory.DIVZERO) {
>     log.warning(LintCategory.DIVZERO, pos, Warnings.DIVZERO);
> }
> 
> There are some issues with this approach:
> 
> * logging a lint warning has to be preceded by the correct `isEnabled` check
> * the check and the `log::warning` call must share the same `LintCategory`
> * the selected warning key in the `Warnings` class must also make sense for the selected `LintCategory`
> 
> This PR addresses these issues, so that the above code is now written as follows:
> 
> 
> lint.logIfEnabled(pos, LintWarnings.DIVZERO);
> 
> 
> The new idiom builds on a number of small improvements:
> 
> * the lint category is now tracked directly in the `compiler.properties` file;
> * a new `LintWarning` class is added to `JCDiagnostic` to model a warning key that is also associated with a speicfic `LintCategory` enum constant;
> * the `CompilerProperties` class has a new group of compiler keys, nested in the new `LintWarnings` class. This class defines the `LintWarning` objects for all the warning keys in `compiler.properties` that have a lint category set
> * A new method `Lint::logIfEnabled(Position, LintWarning)` is added - which simplifies the logging of lint warnings in many common cases, by merging the `isEnabled` check together with the logging.
> 
> As bonus points, the signatures of some methods in `Check` and `MandatoryWarningHandler` have been tightened to accept not just a `Warning`, but a `LintWarning`. This makes it impossible, for instance, to use `Check::warnUnchecked` with a warning that is not a true lint warning.
> 
> Many thanks @archiecobbs for the useful discussions!

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 1327:

> 1325:         DiagnosticPosition prevLintPos = deferredLintHandler.setPos(pos);
> 1326:         try {
> 1327:             deferredLintHandler.report(_ -> {

Nit: we can remove the lambda braces now. Also in a few other places below.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 5137:

> 5135:                  log.warning(
> 5136:                          TreeInfo.diagnosticPositionFor(svuid, tree),
> 5137:                              Warnings.ImproperSVUID((Symbol)e));

Shouldn't this be changed to `LintWarnings.ImproperSVUID`? There are several other similar examples in this class.

src/jdk.compiler/share/classes/com/sun/tools/javac/file/BaseFileManager.java line 528:

> 526:         // Check whether we've already opened this file for output
> 527:         if (!outputFilesWritten.add(realPath))
> 528:             log.warning(LintWarnings.OutputFileClash(path)); // @@@: shouldn't we check for suppression?

Re: "shouldn't we check for suppression?": If `-Xlint:-output-file-clash` is in effect, then `outputFilesWritten` will be null and you'll never get here.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22553#discussion_r1870023374
PR Review Comment: https://git.openjdk.org/jdk/pull/22553#discussion_r1870030235
PR Review Comment: https://git.openjdk.org/jdk/pull/22553#discussion_r1870034373