RFR: 8344079: Minor fixes and cleanups to compiler lint-related code [v2]

[Archie Cobbs]

On Mon, 2 Dec 2024 15:57:13 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Filed:
>> 
>> https://bugs.openjdk.org/browse/JDK-8345263
>> 
>> I will take a look.
>
> Initial stab:
> https://github.com/openjdk/jdk/compare/master...mcimadamore:jdk:lint_warning_strong_checks?expand=1
> 
> The idea is to mark in compiler.properties which keys are associated to which lint category, and track the association in the generated `CompilerProperties` file. Then we can just drop the additional `LintCategory` parameter from all the call sites.

I like that a lot - very nice!

I have one comment in two parts...

(1) Even though the following current idiom is verbose and contains redundancy:

if (lint.isEnabled(LintCategory.DEP_ANN))
    log.warn(LintCategory.DEP_ANN, pos, Warnings.MissingDeprecatedAnnotation));

that redundancy has a nice side effect which is that it makes it easy to spot a mismatch between the lint category being tested and the lint category of the subsequent warning, where as your patch makes it easier to make a mistake like this:

if (lint.isEnabled(LintCategory.DEPRECATION))
    log.warn(pos, Warnings.MissingDeprecatedAnnotation));

Such a mismatch would result in a bug causing a very frustrating (dis)appearance of some warning, potentially failing builds, etc.

(2) The connection of a warning to a lint category, which is based on a properties file comment line, could easily get out of sync with the code without notice causing similar problems to (1). E.g., someone could put the wrong category in the comment (or change it later in the file but not the code), or accidentally forget to add the comment line, etc.

Both (1) and (2) could be addressed easily I think - for example, what if we baked the lint category more firmly into the property name, and used that to make it more visible in the code?

Example:

compiler.warn[dep-ann].missing.deprecated.annotation=\
    deprecated item is not annotated with @Deprecated


if (lint.isEnabled(LintCategory.DEP_ANN))
    log.warn(pos, Warnings.LintDepAnn.MissingDeprecatedAnnotation));


That way it's more visually obvious that the warning is associated to the `DEP_ANN` lint category in both the code and the properties file, and so they hopefully would have a harder time getting out of sync.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22056#discussion_r1866311399