RFR: 8348611: Eliminate DeferredLintHandler and emit warnings after attribution [v3]

[Archie Cobbs]

On Tue, 29 Jul 2025 09:17:51 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>>> Not sure I'm understanding you... the remaining short-circuit checks are using "local" `Lint` instances, not the root `Lint` instance; for example `checkPotentiallyAmbiguousOverloads()`...
>> 
>> At the start of `checkPotentiallyAmbiguousOverloads()`, we check:
>> 
>> 
>> if (!lint.isEnabled(LintCategory.OVERLOADS))
>>             return;
>> 
>> 
>> Now, I do not see any other code setting this lint. There are a couple of visitors elsewhere that do, but these operations seems rather local. To my eyes, this is check applied to the root lint. Then there's a more subtle check (that I missed) below:
>> 
>> 
>> // Allow site's own declared methods (only) to apply @SuppressWarnings("overloads")
>>         methodGroups.forEach(list -> list.removeIf(
>>             m -> m.owner == site.tsym && !lint.augment(m).isEnabled(LintCategory.OVERLOADS)));
>> 
>> 
>> This "augments" the lint with one of the symbols to check, and removes the symbol if it is annotated with `@SuppressWarnings("overloads")`.
>> 
>> This code seems convoluted, in the sense that you don't really need to augment anything here? The toplevel check looks for enablement, whereas this loop looks for suppression. To implement the latter all you need is to check is a certain category appears suppressed in the given symbol. Note though: if we have a `@SuppressWarnings` on the enclosing class I don't think that will be considered. I will need to test a bit more.
>
>> Move the short-circuited warnings to the `WARN` phase
> 
> This is probably the best, as I suspect that we only shortcircuit in a handful of cases.
> 
> Another approach would be to try an augment lint earlier -- e.g. during `memberEnter`. After all, by that point we have seen the class, and we have seen a method/field symbol, so in principle we might know what suppressions these symbols have. The only thing that would be left after that is stuff inside method bodies (mostly local var declaration, and anon/local inner classes -- but I believe both go through `memberEnter` again, so we should be covered). This would ensure that the mappings are setup as early as possible -- which would allow shortcircuiting logic in `Check` to occur more naturally.
> 
> That said, doing this might have some bootstrapping issues -- @lahodaj any ideas?

> > Not sure I'm understanding you... the remaining short-circuit checks are using "local" `Lint` instances, not the root `Lint` instance; for example `checkPotentiallyAmbiguousOverloads()`...
> 
> At the start of `checkPotentiallyAmbiguousOverloads()`, we check... Now, I do not see any other code setting this lint.

That method is invoked from `Attr.attribClassBody()`, which is invoked from `Attr.attribClass()`, which invokes `chk.setLint()`, etc. So it does appear to be using a "local" `Lint` instance, right?

But I was missing your larger point - even if we replaced this local `Lint` with `rootLint`, the short circuit check would still be correct, just less precise. In other words, once a Lint category is disabled, it's never possible for it be re-enabled by a nested declaration. So if code is short-circuiting via `if (!lint.isEnabled(FOO)) skip...` then switching from a local lint to `rootLint` is a conservative (safe) change (side note: this wouldn't work for `DEFAULT_ENABLED` warnings, but the few warnings that have that flag are not involved here).

> This code seems convoluted, in the sense that you don't really need to augment anything here? The toplevel check looks for enablement, whereas this loop looks for suppression. To implement the latter all you need is to check is a certain category appears suppressed in the given symbol. Note though: if we have a @SuppressWarnings on the enclosing class I don't think that will be considered. I will need to test a bit more.

This check (overloads) as well as the `"this-escape"` check do "tricky" stuff because they are both examples in which whether the warning is emitted depends on non-local suppression. For some warnings, this is just inherent in what the warning means, invalidating the assumption that a `@SuppressWarnings` annotation always fully "contains" the warnings it might apply to.

For example, a `"this-escape"` warning is suppressed for a _field_ if every primary _constructor_ has `@SuppressWarnings("this-escape")`. This is of course because fields are initiailized when `super()` happens, so control flow jumps around.

In the `"overloads"` case, there is logic to allow "either/or" of the methods to suppress the warning: in other words, the warning is referring to two different methods at the same time, so that brings the question, Which method's `@SuppressWarnings` annotation should apply? The code applies _both_ at the same time in a conservative fashion, meaning either method (or both) may suppress the warning.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24584#discussion_r2239953767