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

[Maurizio Cimadamore]

On Tue, 3 Dec 2024 13:10:52 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>>> 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?
>
>> See the first suggestion in this comment... would that work?
> 
> I like that idea. So, one concrete proposal might be:
> 
> 1. have LintWarning as a subclass of Warning
> 2. have CompilerProperties reflect the lint categories, by having different nested classes where LintWarnings factories and fields would appear (one nested class per lint category)
> 3. don't change `AbstractLog` - that will still offer a `log` method that takes a `Warning` that can be used for both regular warning and lint warnings (in case the lint warning is not really suppressible)
> 4. Add a method in `lint` to "logIfEnabled". This method accepts a `LintWarning`, not a warning. So we get some static type checking.
> 
> (That said, if lint warnings are to be reported using (4), what would be the added benefit of (2) ? )

There's also another issue with `logIfEnabled`. While that seems to be the majority of cases, there is a non-trivial amount of uses where the is-enabled check occurs at some outer level, at which point the inner code just warns w/o a check (see all the serial warnings inside `Check` -- but there's several other cases too). So, not sure how much that infrastructure would be worth adding.

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

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