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

[Maurizio Cimadamore]

On Tue, 3 Dec 2024 16:41:43 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

>>> 3. Change `AbstractLog` to add methods taking a `LintWarning`; these will automatically add the "[foo]" prefix to the warning text that is emitted
>> 
>> 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.
>
>> 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.

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

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