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

[Archie Cobbs]

On Tue, 29 Jul 2025 18:45:16 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

>> What I'm trying to say here is: having things in a side-map adds costs. Because, regardless of whether warnings are enabled or not, we're still going to add things to the `LintMapper` side-map. At which point it might be "cheaper" (memory-wise) to just add a field to the tree, and not having to "replicate" a tree-like structure inside `LintMapper` ?
>
>> What I'm trying to say here is: having things in a side-map adds costs. Because, regardless of whether warnings are enabled or not, we're still going to add things to the `LintMapper` side-map. At which point it might be "cheaper" (memory-wise) to just add a field to the tree, and not having to "replicate" a tree-like structure inside `LintMapper` ?
> 
> Remember that the tree of nodes that `LintMapper` builds is sparse - nodes are only created for (a) top-level declarations and (b) nested declarations for which the effective `Lint` is different from the parent, i.e., nested declarations that actually have a material `@SuppressWarnings` annotation.
> 
>> I still find the design a bit odd here. We create an "empty" root node here, but then in the constructor we set up all its children. Maybe we should create the full "node" from the original `JCCompilationUnit` tree (in a single shot) ? Also, I can't quite figure out if we end up adding both a `Decl` and a `MappedDecl` as children of the same parent decl?
> 
> This is a side-effect of the fact that we need to always create `Decl` nodes for top-level declarations, but for nested declarations we only create them when/if needed, and when we do they are already `MappedDecl` nodes.
> 
> So the top-level declaration nodes just under root are "special". They are the only nodes that are ever `Decl` (not-yet-mapped) nodes before transitioning to `MappedDecl` nodes.
> 
>> Perhaps, if the only difference between mapped and non-mapped decl is that the former have a Lint object, just having a single decl class with a mutable Lint field that is only set once might work?
> 
> Given the above, let me know what you think is most appropriate. There are basically three types of nodes: root, non-mapped (top-level only), and mapped. The second type converts from non-mapped to mapped when the declaration is attributed.
> 
>> Meta-comment -- I also realize how a lot of this could probably be avoided if we could just stash the Lint directly in the corresponding tree? (e.g. if we could do that, then we'd just need something to "find" the most specific tree given a position and see if it has a Lint set. We'd probably still need a map from source files to compilation unit trees)
> 
> The difficulty in the "finding" is the problem with that idea. Also you'd have to recurse through every node in the AST tree, instead of just the `@SuppressWarnings`-annotated declarations. Right now the "finding" is fast because the tree is sparse.

By the way, I have to agree with you that the code in `LintMapper` is still more confusing than necessary.

I've done another pass to clean it up; please see if 147cb2dc30a looks better. Thanks.

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

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