RFR: 7903613: Bad nested names are sometimes attached to structs [v10]

[Jorn Vernee]

On Wed, 10 Jan 2024 15:17:04 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> The `NameMangler` visitor is used to compute the Java name of a jextract declaration. This is implemented as a declaration visitor. Unfortunately, the logic that computes the Java name can be sensitive to the order in which declarations are visited (because this visitor features a "parent" declaration, whose contents affect as to whether a "nested" struct name is generated or not).
>> 
>> In reality, the logic of the name mangler needs to be able to disambiguate between structs that are either anonymous, or already declared somewhere else, and structs that are declared as part of a typedef, variable, function parameter/return declaration. In the former case, we either need no Java name (anonymous struct) or a toplevel Java name. In the latter we need a nested struct name (as the struct class will be nested inside some other class).
>> 
>> This PR introduces a new visitor which tags all struct/union/enum declarations which fall in the latter bucket. This is done with an algorithm which:
>> 
>> 1. visits all declarations in a toplevel header
>> 2. remembers which scoped declarations have been seen *directly* (e.g. as part of the visit)
>> 3. keeps track of which scoped declarations can be seen *indirectly* (e.g. because they are behind some declared type)
>> 4. subtracts the declarations in (2) from the declarations in (3), and visits the declarations in the remaining set
>> 5. keeps performing (2), (3), (4) until there's no declaration in (3)
>> 
>> All scoped declarations that appear exclusively as part of some declared type are augmented with the `NestedDecl` attribute, which is then read when calling `Utils::nestedDeclarationFor`. This ensures that all the jextract visitor only recurse on a scoped declaration attached to a type which is known not to have been seen anywhere else. As a result, the behavior of the name mangler is independent of the order in which declarations are seen.
>> 
>> It should be possible, in principle, to leverage this infrastructure to define a declaration visitor that automatically looks inside "nested declarations" (so that subsequent visitors don't really need to concern with following declared types).
>> 
>> I've tested this change with windows.h, which works as expected.
>
> Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 20 commits:
> 
>  - Merge branch 'panama' into nested_structs
>  - Add fix and test for unsupported types in nested declarations
>  - Fix whitespaces in test
>  - Minimize diffs
>  - Fix mangling
>    Add mangling test for nested decls
>  - Add more comments
>  - Fix function pointer typedef mangled names for nested struct in param/returns
>  - Better names for function parameter/return structs
>  - Deal with param/return nested decls
>  - Drop spurious changes in OutputFactory
>  - ... and 10 more: https://git.openjdk.org/jextract/compare/82801be6...4c912c74

This looks good. Great work on fixing such a tricky issue!

I just have one last concern. WRT:

> Note: libclang is not perfect here, as it often gives jextract duplicate cursors. That is, sometimes the same cursor can appear both at level N and at level N + 1 (this looks like a bug in libclang). The fact that jextract has builtin deduplication for declarations work out in our favor here.

Are we sure we're filtering out the right duplicates? e.g. if we filter out the N + 1 definition, doesn't the actual struct class we generate appear at the wrong level?

src/main/java/org/openjdk/jextract/impl/NameMangler.java line 227:

> 225:                 if (func.argumentTypes().get(i) instanceof Type.Declared declared && declared.tree() == nested) {
> 226:                     // it's a function argument
> 227:                     suffix = "$x" + i;

Just checking: Is there no way to look up the actual parameter name here?

src/main/java/org/openjdk/jextract/impl/TreeMaker.java line 460:

> 458:                 .filter(m -> m instanceof Scoped)
> 459:                 .map(s -> Type.declared((Scoped)s))
> 460:                 .toList();

Here we go from the Declaration to the type, but then in Utils.forEachNested we go back to the declaration. I think we can just attach the nested declarations with an attribute instead? (Maybe it should even be named `NestedDecls`. It seems like we're really interested in inline type _declarations_, not just any inline type, which might just be a reference to a type declared elsewhere?)

test/jtreg/generator/outOfOrder/out_of_order_struct.h line 30:

> 28: struct Bar {
> 29:     int x;
> 30: };

I think `Bar` is not strictly needed for this test case. (same for the typedef one).

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

PR Review: https://git.openjdk.org/jextract/pull/167#pullrequestreview-1787699915
PR Review Comment: https://git.openjdk.org/jextract/pull/167#discussion_r1448861925
PR Review Comment: https://git.openjdk.org/jextract/pull/167#discussion_r1448846109
PR Review Comment: https://git.openjdk.org/jextract/pull/167#discussion_r1448924206