On Thu, 5 Dec 2024 08:55:19 GMT, Maurizio Cimadamore <mcimadam...@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
