Read a structure from its definition, not from a declaration before it

A structure the headers declare before they define it is read from its
definition: its record keeps the place where the parse first meets the
structure, and its fields, offsets and file are those of the definition.
A structure the headers only declare, an opaque one such as NumericData,
keeps its declaration and states no field.

Why. A binding laying a structure out reads its fields and offsets from
the catalog. meos.h declares MeosArray and SkipList, which
meos_internal.h defines; read from the first record the parse meets, the
declaration, they carry no field, and neither would varlena once
pg_basetypes.h declares struct varlena ahead of the splice defining it,
which MobilityDB #2893 does.

Measured. Over MobilityDB d5e3e9946e, three structures gain their
fields and nothing else changes: MeosArray five, read in meos_internal.h
for meos.h, SkipList fourteen, read in meos_internal.h for meos.h, and
PCSCHEMA eleven, read in pc_api.h for meos_pointcloud.h. Over MobilityDB
#2893 (dd0426728b) varlena keeps its two fields, and that catalog differs
from master's only in where its structures and typedefs sit: NumericData
is found in pg_basetypes.h instead of pg_numeric.h, so it no longer reads
as vendored, and the structures come in another order.

Witness. tests/test_struct_layout.py reads MeosArray, SkipList and
varlena with their fields from the catalog, and parses a header
declaring one structure before a second header defines it, beside one
only declared: the first carries its fields and the second none. The
suite floor goes from 341 to 343.

Author: Esteban Zimanyi

.github/workflows/pytest.yml

@@ -96,7 +96,7 @@ jobs:
       # carries, or a change to them is not exercised until after it merges.
       # Consumers use the action; this repository owns the rules.
       - name: Refuse a skip, and a suite that shrank
-        run: tools/check-test-outcome.py "$RUNNER_TEMP/pytest.log" --min-tests 341
+        run: tools/check-test-outcome.py "$RUNNER_TEMP/pytest.log" --min-tests 343
 
       # The rules earn their place by refusing a log that carries what they
       # name. Both fixtures are written here rather than tracked, and the

parser/parser.py

@@ -170,7 +170,7 @@ def parse_meos(entry: Path, include_dir: Path,
                     struct["name"] = typedef_name
                 else:
                     continue
-            structs.append(struct)
+            structs.append((struct, node.is_definition()))
 
         elif node.kind == clang.cindex.CursorKind.ENUM_DECL and node.spelling:
             enums.append(extract_enum(node))
@@ -186,8 +186,24 @@ def _dedup(items: list) -> list:
                 result.append(item)
         return result
 
+    def _dedup_structs(items: list) -> list:
+        """One record per structure, as #_dedup keeps one per name, read from its
+        definition when the unit holds one: a forward declaration (`struct varlena;`)
+        met before the definition has no fields, and a structure only ever declared
+        (opaque, `struct NumericData;`) keeps its declaration. The structure keeps the
+        place where the parse first meets it."""
+        order, chosen = [], {}
+        for struct, is_def in items:
+            name = struct["name"]
+            if name not in chosen:
+                order.append(name)
+                chosen[name] = (struct, is_def)
+            elif is_def and not chosen[name][1]:
+                chosen[name] = (struct, is_def)
+        return [chosen[name][0] for name in order]
+
     functions = _dedup(functions)
-    structs   = _dedup(structs)
+    structs   = _dedup_structs(structs)
     enums     = _dedup(enums)
     macros    = _dedup(macros)
 

tests/test_struct_layout.py

@@ -79,6 +79,33 @@ def test_core_structs_have_real_offsets(self):
                     fld["offset_bits"], 0,
                     f"{s['name']}.{fld['name']} has unresolved offset")
 
+    def test_a_structure_declared_first_is_read_from_its_definition(self):
+        # `meos.h` declares MeosArray and SkipList before `meos_internal.h` defines
+        # them, so their layout is read where they are defined
+        for name, field in (("MeosArray", "count"), ("SkipList", "capacity"),
+                            ("varlena", "vl_len_")):
+            self.assertIn(field, self._fields(name), name)
+
+
+class ForwardDeclarationTests(unittest.TestCase):
+    """#_dedup_structs of parser/parser.py over headers that declare a structure before
+    defining it, and one they only declare, parsed as #StructLayoutTests's catalog is."""
+
+    def test_the_definition_wins_and_an_opaque_structure_stays(self):
+        import os
+        import tempfile
+        if not os.environ.get("MDB_SRC_ROOT"):
+            self.skipTest("MDB_SRC_ROOT not set; the parse reads MobilityDB's families")
+        from parser.parser import parse_all_headers
+        with tempfile.TemporaryDirectory() as d:
+            root = Path(d)
+            (root / "a.h").write_text("struct S;\nstruct O;\n"
+                                      "extern int f(struct S *s, struct O *o);\n")
+            (root / "b.h").write_text("struct S { int x; double y; };\n")
+            idl = parse_all_headers(root)
+        structs = {s["name"]: [f["name"] for f in s["fields"]] for s in idl["structs"]}
+        self.assertEqual(structs, {"S": ["x", "y"], "O": []})
+
 
 if __name__ == "__main__":
     unittest.main()