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

[Maurizio Cimadamore]

On Tue, 29 Jul 2025 21:36:32 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` ?
>> 
>> 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.

Thanks for the comments -- mapped declarations are indeed sparse as you point out, and this might simplify the "position matching" process. However, the unmapped declarations are not sparse. It is true that, after Attr, we will go through the list of unmapped decls and remove them as we map them. But, after parsing the list of unmapped declaration does seem a copy of an AST.

Say you kept _mapped_ declaration as they are now. Can the list of _unmapped_ declaration just be replaced by the compilation unit tree? Or, maybe, a "list of declaration trees" to analyze? It is unclear to me what added value the unmapped data structure has compared to just an AST node. The only thing it adds are the "contains" methods -- which could be implemented as static methods in TreeInfo perhaps?

Also, I don't get the subclassing between `Decl` and `MappedDecl`. They seem to serve quite different purposes -- the former is essentially a *flat* "todo list", whereas the latter is a proper tree-based representation, with parent/child edges. I'm not even sure any client can call methods on a `Decl` when in reality the object could be a `MappedDecl` ?

My concrete suggestion here would be to:

* either just remove `Decl` and use `JCTree`s instead (and put `contains` methods in `TreeInfo`, as they might be useful), or create a small record around `JCTree` (and define `contains` methods in that record).
* when we create a `FileInfo`, we already know, from the start, what the list of "todo" trees/decls are
* then we have mapped declarations; here you have a node that can have many children nodes, using some kind of "delta encoding" (e.g. a child is there only if its lint is different and interesting, otherwise we just use the parent). I think the current class you have (`MappedDecl`) + the builder/scanner you have works well here.

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

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