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

[Maurizio Cimadamore]

On Wed, 30 Jul 2025 15:51:27 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

>> 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.
>
>> 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.
> 
> There number of unmapped declarations equals the number of top-level class, package, or module declarations. I'm describing this as "sparse" because it does not include any methods, variables or nested classes. But of course if you're considering only top-level declarations then it's not "sparse" with respect to just that set.
> 
>> 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.
> 
> It's only a copy of the AST to a depth of one... so really a list, not a tree... but regardless, I think you have a good point (see below)...
> 
>> 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?
> 
> To restate your question: What value does storing a list of `Decl`'s provide over just storing a list of the corresponding original `JCTree` objects? There are two reasons I did this:
> 1. If we just store the `JCTree`'s, then we also must retain a reference to every source file's `EndPosTable` (it's required by `MappedDeclBuilder`), which I was trying to avoid because, at least in some cases (depending on compiler configuration), those tables are (otherwise) discarded after parsing. So the new references to `EndPosTable` will increase compiler memory usage. But I may have been overreacting or misunderstanding that.
> 2. This is a minor detail, but the `JCTree` we see after parsing is not always the same `JCTree` we see after attribution (not sure why; it happens in test method `EdgeCases.testWrongPackageInExportedPackage()` for some reason). So even if we store the trees, we still have to compare them by position, not object equality, so in effect it's really the position we care about, not the tree...
> 
>> Does this mean we create a mapped decl for all toplevel decls in a file (even if their lint is not interesting) ? Why?
> 
> Yes, but! That special case is no longer required with the recent cleanups, so now we can fix this too :)
> 
>> My concrete suggestion here would be to...
> 
> Thanks, this makes things simpler and cleaner. Should all be done in 6a1289e9372.
> 
> Thanks again for your thoughtful review and comments!

The new code looks clearer to follow. What we really want is:
* first, capture only the "spans" of toplevel declarations
* then, after attr, organize all these spans into some kind of tree structure that optimizes the search

If you are worried about holding onto the end pos table too long (I don't have strong opinion on this), I'm ok with using some kind of "span" class to model... a span -- but that class should be completely disconnected from the `MappedDecl` class (which was really my main point).

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

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