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

[Maurizio Cimadamore]

On Mon, 2 Dec 2024 17:39:53 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

>> 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.

Changing the key is possible. I even prototyped that - but ran out of steam when I realized that all compiler negative tests required some fixing. So I'm a bit wary of going down that path.

IMHO, the issue with stuff like:


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


Is that compiler code should even bother to do this! Ideally, you might want to have a single routine:


log.warnIfEnabled(DEP_ANN, pos, Warnings.MissingDeprecatedAnnotation)


This could test whether `DEP_ANN` is enabled _and_ also check that the provided warning key has the correct category set

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

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