From 8046e5811eaf3c489f4851a9b48a8ae32a7a9e7b Mon Sep 17 00:00:00 2001 From: Esteban Zimanyi Date: Thu, 1 Oct 2026 10:51:32 +0200 Subject: [PATCH] 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. --- .github/workflows/pytest.yml | 2 +- parser/parser.py | 20 ++++++++++++++++++-- tests/test_struct_layout.py | 27 +++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml index ca91be7..5aed1c7 100644 --- a/.github/workflows/pytest.yml +++ b/.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 diff --git a/parser/parser.py b/parser/parser.py index e1eaa73..04c3924 100644 --- a/parser/parser.py +++ b/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) diff --git a/tests/test_struct_layout.py b/tests/test_struct_layout.py index eef2fa2..fdf901a 100644 --- a/tests/test_struct_layout.py +++ b/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()