RFR: Simplify jextract backend pipeline

[Maurizio Cimadamore]

On Thu, 30 Nov 2023 21:26:47 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Now that we have mutable attributes, we can make the jextract pipeline a little simpler, by making sure that each step in the jextract pipeline doesn't destroy the information contained in the jextract model and, additionally, by making sure that each step can only add attributes to the IR (which are acted upon later on).
>> 
>> To do this, I had to do some foundational changes to TreeMaker, to make sure that all declaration nodes were being correctly de-duplicated: since declarations are referenced back by types, if a declaration is re-created multiple times, the types pointing to that declaration end up being out of sync, and that's not desirable (because that could lead to a loss of some of the attributes).
>> 
>> I also removed the `Transformer` interface: altering the model is not something the jextract pipeline should attempt to do. This meant that existing transformers have been reimplemented as side-effecting visitors, as follows:
>> 
>> * `EnumConstantLifter` adds an attribute to constants which belong to an enum (but no longer drops the enum on the ground);
>> * `IncludeFilter` marks several declaration with a `Skip` attribute, to signal `outputFactory` not to generate any code for them;
>> * `DuplicateFilter` also uses the `Skip` attribute to drop redundant declarations.
>> 
>> Additionally, the `NameMangler` has been simplified quite a bit, to the point that now it is just a simple visitor which ends up attaching attributes to declarations (`JavaName`, `JavaParameterNames`, `JavaFunctionalInterfaceName`). These attributes are later inspected (from `OutputFactory`). Since now the attribute is added directly to the relevant declaration, there is no need to have complex keying logic inside `NameMangler`.
>
> src/main/java/org/openjdk/jextract/impl/IncludeFilter.java line 94:
> 
>> 92:         if (parent == null && !includeHelper.isIncluded(tree)) {
>> 93:             //skip
>> 94:             tree.addAttribute(new Skip());
> 
> Maybe we could use a singleton instance?

Yeah...

> src/main/java/org/openjdk/jextract/impl/NameMangler.java line 163:
> 
>> 161:     public Void visitScoped(Declaration.Scoped scoped, Declaration parent) {
>> 162:         String name = scoped.name();
>> 163:         if (name.isEmpty() && parent != null) {
> 
> Can we see a `null` parent here now? The old code doesn't have the null check.

Yes, we can now see that. In the old code, the mangler ran after the enum lifting, so there were only toplevel constants. But now we don't drop enums on the floor anymore, so we can call viistConstant with the parent being the toplevel scoped (null).

> src/main/java/org/openjdk/jextract/impl/TreeMaker.java line 124:
> 
>> 122:             declarationCache.put(pos, decl);
>> 123:         }
>> 124:         return decl;
> 
> IIRC we had a cache here before, but removed it because it was faster to re-create trees. That might have inadvertently also created latent issues with inconsistent IR views of the same decl?
> 
> Any way, we need the cache to 'canonicalize' IR nodes. Good to know.

Yes, we used to have a cache and removed it - I don't think we thought too much about what it meant in terms of losing indentity of the objects involved, since we never relied on that anyway. But I remember this being a problem when we took our first stab at the name mangling patch, which was confirmed with an experiment I did few days ago: w/o de-duplication, it is possible to set an attribute on a struct, but to still have types in the IR that point to the old, un-attributed struct, which is bad.

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

PR Review Comment: https://git.openjdk.org/jextract/pull/148#discussion_r1411936447
PR Review Comment: https://git.openjdk.org/jextract/pull/148#discussion_r1411937354
PR Review Comment: https://git.openjdk.org/jextract/pull/148#discussion_r1411940014