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

[Jan Lahoda]

On Thu, 5 Dec 2024 08:55:19 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!
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add missing lint categories in compiler.properties
>   Simplify some lambda expressions
>   Simplify Lint::logIfEnabled by accepting a log parameter

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

> 4257:      */
> 4258:     void checkForBadAuxiliaryClassAccess(DiagnosticPosition pos, Env<AttrContext> env, ClassSymbol c) {
> 4259:         if ((c.flags() & AUXILIARY) != 0 &&

Here, and on a few other places, the checks were ordered so that the potentially more expensive checks were done after the `isEnabled` check. Checking the exact places, I don't think delaying the check is terrible for the cases in this PR, but if we would like to be adding new lints, we probably need something that will avoid running the lint's code completely in a (semi-)automatic way. That could be part of a more generic 22088.

(Admittedly, on some places, the `isEnabled` check was last, after the more expensive checks. This is particularly the case for the `TRY` lint, where the checks might be considerable.)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22553#discussion_r1871495437