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

[Archie Cobbs]

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

>>> `log.warnIfEnabled(DEP_ANN, pos, Warnings.MissingDeprecatedAnnotation)`
>> 
>> Or, even better:
>> 
>> `log.warnIfEnabled(pos, Warnings.MissingDeprecatedAnnotation)`
>> 
>> (e.g. just use the warning key option for the `isEnabled` check)
>
> Another observation: it would be better if we had two distinct set of diagnostics; warnings and lint warnings. If one couldn't be passed in place of the other that will force clients to use the correct `log` method. Which might make it easier to check that all the places issuing a lint warning also check as to whether the warning is enabled.
> 
> (that said, we can't check for enable-ment in all cases - e.g. for lint warnings in the tokenizer, so there will always be some ad-hocness somewhere, I think. I think we win if we can find something that works in most cases, but we can't make everything statically type-checkable, not without significant cost and effort).

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

I think we could still minimize the chance for mismatches without changing the keys, e.g. by using combining my idea of baking the lint category into the warning name with your idea of having two distinct set of diagnostics.

For example:

# lint: dep-ann
compiler.warn.missing.deprecated.annotation=\
    deprecated item is not annotated with @Deprecated

then in the code we have this - which is both type safe and visually confirms the lint category:

log.warn(pos, LintWarnings.DepAnn.MissingDeprecatedAnnotation);


> Ideally, you might want to have a single routine...

Agreed.. but (minor detail) of course that requires a `lint` object, e.g.:

lint.warnIfEnabled(log, pos, LintWarnings.DepAnn.MissingDeprecatedAnnotation);

> we can't check for enable-ment in all cases...

Actually that's not the problem - I think it's always possible to check for enablement. Put another way, all warnings that are generated in a place where it's not possible to check for enablement are not lint warnings. Put yet another way, there is no lint warning in category `foo` that is generated when `-Xlint:-foo` is given.

The limitations that do occur are:
* In some places in the code, we have to check enablement "manually" by inspecting `Options` directly because no `lint` object is available. So you can't use `Lint.warnIfEnabled()` but so what, it's just a convenience.
* In some places in the code, there is no way to apply `@SuppressWarnings` (e.g., `output-file-clash`). No problem, you just always use the root `Lint` singleton (or `Options` directly).

> but we can't make everything statically type-checkable, not without significant cost and effort...

See the first suggestion in this comment... would that work?

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

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