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

[Archie Cobbs]

On Tue, 3 Dec 2024 23:29:30 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>>> In terms of user experience, what would be the difference between using `log(LintWarning)` and `log(Warning)` ? I believe there is a need for having a method that "just logs" because sometimes (see my previous message) the is-enabled check occurs somewhere else, far from where the diagnostic is logged.
>> 
>> The methods `log(LintWarning)` and `log(Warning)` would behave the same. The reason I want both methods to exist is simply because I want `LintWarning` to not be a subclass of `Warning`.
>> 
>> Here's why I think that's important:
>> * We are using the same English word "warning" to describe the following two things:
>>   * Generic compiler warnings that have nothing to do with lint. Example: `Warnings.InvalidUtf8InClassfile`
>>   * Compiler warnings that have an associated lint category. Example: `Warnings.MissingDeprecatedAnnotation`
>> * But really two things are distinct: they do not overlap, and every compiler "warning" is one or the other (and never both). _Practically speaking_, "generic warnings" and "lint warnings" are just as distinct from each other as are `Error`, `Note`, and `Fragment`. Therefore, we can make them peer classes with no inheritance relationship.
>> * Why is this helpful? Because the stronger type checking prevents possible mistakes like this:
>> 
>> // somewhere in the code
>> LintWarning lw = ...;
>> 
>> // somewhere else in the code
>> Warning w = lw;
>> log.warning(pos, w);
>> 
>> The result of the above mistake would be a lint warning emitted that:
>> * Does not have the "[foo]" prefix; and
>> * Possibly escapes being suppressed by `-Xlint:-foo` as it should.
>> 
>> Put another way, I don't see any reason to make `LintWarning` a subclass of `Warning` - so why do it??
>
> Not sure I buy your arguments :-) 
> While the mechanics of lint warning is a bit different, they are _still_ warnings (e.g. the compiler still says `warning: <text>`, and it is still something that counts towards `Werror`. Making them completely separate seems like trying to make them more different than they really are.
> 
> And, if we have both `log(Warning)` and `log(LintWarning)` whether they are the same type or not becomes a mot question, because you can still use them in place of another. And, if you have subclassing and have only a single `log(Warning)` method which adds the category or not depending on the _runtime_ type of the warning, then the problem you describe above cannot occur (I think).
> 
> So, while I see how one can argue this several ways, the advantage for going down the path you describe seems less clear to me. In terms of object-oriented hierarchy, saying that a lint warning *is a* warning with a category added on top seems to me like a good modelling of the situation we have.

OK I think you're right and it will all work out - using the runtime type ensures the prefix always appears, and the other thing I was worried about, which is someone accidentally logging a lint warning without checking for enablement because they thought it was a generic warning, should be prevented now because the in the code you will see `LintWarnings.FooBar` instead of `Warnings.FooBar` so hopefully it will be very obvious.

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

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