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

[Maurizio Cimadamore]

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

>>> One thing that might help would be to maybe use the name `rootLint` for all lint variables we use to shortcircuit -- to make it clear that these checks are not really... 100% correct.
>> 
>> Not sure I'm understanding you... the remaining short-circuit checks are using "local" `Lint` instances, not the root `Lint` instance; for example `checkPotentiallyAmbiguousOverloads()`...
>> 
>>> Another possibility would be to have a centralized check in LintMapper which, if the tree has been attributed does a check similar to what `ThisEscapeAnalyzer` does, otherwise we just look at the root lint (as a stop gap).
>> 
>> Continuing the above comment, wouldn't that cause `checkPotentiallyAmbiguousOverloads()` to start ignoring any enclosing `@SuppressWarnings("overrides")` annotations?
>> 
>>> At least all clients that need shortcircuiting would use same logic, and the impl could do the best it can to support that (and maybe in the future we can see if we can improve on that front).
>> 
>> Other ideas:
>> * Add a method that basically says "Call me back when my `Lint` is ready". Echoes of `DeferredLintHandler` :)
>> * Get rid of as much short-circuiting as we possible (i.e., where the performance impact is negligible, we can simply not short-circuit)
>> * Move the short-circuited warnings to the `WARN` phase
>
>> 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?

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

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