On Wed, 4 Dec 2024 17:11:26 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!
src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java line 398:
> 396: */
> 397: public void logIfEnabled(DiagnosticPosition pos, LintWarning
> warning) {
> 398: if (isEnabled(warning.getLintCategory())) {
I'm not 100% sure about this method. On the one hand it makes clients simpler -
but it does require `Lint` keeping track of a `log`. An alternative could be to
add a method in `AbstractLog`, e.g.:
warningIfEnabled(DiagnosticPosition pos, Lint lint, LintWarning key)
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4472:
> 4470: // a warning. Make allowance for the class of an array type
> e.g. Object[].class)
> 4471: if (!sym.owner.isAnonymous()) {
> 4472: chk.lint.logIfEnabled(tree,
> LintWarnings.StaticNotQualifiedByType(sym.kind.kindName(), sym.owner));
This is an odd one - the rest of `Attr` uses `env.info.lint` but if I do that
here I get some NPE in some unrelated JVM test. I wonder how robust using two
different lints is... but for now I'll keep compatibility with what we have.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 4664:
> 4662: if ((rd.module.flags() & Flags.AUTOMATIC_MODULE) != 0) {
> 4663: deferredLintHandler.report(_ -> {
> 4664: if (rd.isTransitive() &&
> lint.isEnabled(LintCategory.REQUIRES_TRANSITIVE_AUTOMATIC)) {
This one is a bit odd - the warning can be reported under two categories, so
the code makes sure we don't report twice. This is deliberate I've checked with
@lahodaj
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22553#discussion_r1870145545
PR Review Comment: https://git.openjdk.org/jdk/pull/22553#discussion_r1870139558
PR Review Comment: https://git.openjdk.org/jdk/pull/22553#discussion_r1870141200
