From 60e0dd17ec5039daffab015c5846f3b7e33ae78e Mon Sep 17 00:00:00 2001 From: Esteban Zimanyi Date: Thu, 3 Sep 2026 13:16:14 +0200 Subject: [PATCH] Carry, per function, the guard under which its answer is absent A C return type says what a value LOOKS like and never whether there is one. `bool tbool_value_at_timestamptz(..., bool *value)` answers a value and a flag; `Temporal *temporal_at_timestamptz(...)` answers a pointer that may be NULL; and neither signature separates "no answer here" from "the answer is false". The catalog states nullability for PARAMETERS -- `shape.nullable`, from the Doxygen `@param ... may be NULL` -- and states nothing about the RESULT, so each binding invents a convention for absence: a nullable in MEOS.NET, a `None` in PyMEOS-CFFI, a zero value and a dereference in the two that carry neither. The PostgreSQL wrapper already states it. Each wrapper guards `PG_RETURN_NULL()` with the condition under which the answer is absent, and that guard is a token: `if (! result)` for a null pointer, `if (! found)` for an out-parameter that reports absence itself, `if (result == DBL_MAX)` for the distance sentinel, `if (result < 0)` for a three-valued predicate. This reads that guard and attaches it as `shape.nullableResult`, over the `@csqlfn` chain `parser.sqlfn` already builds, so no new join and no new tag. MEASURED against MobilityDB 4d3de5fc0e: 1038 wrappers carry an absence guard, in the proportions the wrappers themselves state -- 275 `if (! result)`, 67 the distance sentinel, 61 the three-valued form, 11 `if (! found)`. A THIRD OF THE TREE DELEGATES, AND READING ONLY THE WRAPPER INVERTS THOSE. `Temporal_at_timestamptz` is one line, `return Temporal_restrict_timestamptz(fcinfo, REST_AT);`, and the PG_RETURN_NULL sits in that shared helper: 961 of 2894 wrapper bodies take that shape. Following the helper the body names takes the count from 529 to 1038 and, more to the point, turns the whole restriction family from "always answers" into what it is. The walk is bounded and cycle-safe. `PG_ARGISNULL` guards are excluded: those propagate a null ARGUMENT, which `shape.nullable` already carries, and counting them would mark a function absence-capable because its caller may pass nothing. tests/test_nullresult.py covers the guard forms, the delegation hop, a delegation cycle, the argument-null exclusion, and that a wrapper without a guard and a function without a wrapper are left without the field rather than given a false one. The suite goes 307 to 314 and the floor with it. --- .github/workflows/pytest.yml | 2 +- parser/nullresult.py | 140 +++++++++++++++++++++++++++++++ run.py | 8 ++ tests/test_nullresult.py | 155 +++++++++++++++++++++++++++++++++++ 4 files changed, 304 insertions(+), 1 deletion(-) create mode 100644 parser/nullresult.py create mode 100644 tests/test_nullresult.py diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml index 85f96e8..0af7100 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 307 + run: tools/check-test-outcome.py "$RUNNER_TEMP/pytest.log" --min-tests 314 # 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/nullresult.py b/parser/nullresult.py new file mode 100644 index 0000000..da7d10c --- /dev/null +++ b/parser/nullresult.py @@ -0,0 +1,140 @@ +"""Whether a MEOS function's answer can be absent, read from the PG wrapper. + +A C return type says what a value LOOKS like; it never says whether there is +one. ``bool tbool_value_at_timestamptz(..., bool *value)`` returns a value and +a flag, ``Temporal *temporal_at_timestamptz(...)`` returns a pointer that may be +NULL, and nothing in either signature distinguishes "no answer here" from "the +answer is false". Every binding needs that distinction, and without it each one +invents a convention: a nullable in one, an exception in another, a zero value +and a crash in a third. + +The PostgreSQL wrapper already states it. Each wrapper guards +``PG_RETURN_NULL()`` with the condition under which the answer is absent, and +that guard is a TOKEN rather than prose: + + if (! result) a null pointer -- no value was produced + if (! found) an out-parameter reporting absence itself + if (result == DBL_MAX) the distance sentinel + if (result < 0) a three-valued predicate answering unknown + if (count == 0) an empty array + +So the fact is derived, never inferred from a name or a group: the wrapper is +the SQL contract, and this reads it. The chain is the one ``parser.sqlfn`` +already builds -- a MEOS function names its wrapper through ``@csqlfn``, and +``mdbC`` carries that name -- so nothing new is joined here. + +``PG_ARGISNULL`` guards are EXCLUDED. Those propagate a null ARGUMENT, which is +a statement about the input (already carried by ``shape.nullable``) rather than +about whether an answer exists. Counting them would report a function as +absence-capable because its caller may pass nothing. + +Adds ``shape.nullableResult`` -- the guard text -- to every function whose +wrapper carries one. Its ABSENCE is as meaningful as its presence: a wrapper +with no ``PG_RETURN_NULL`` always produces a value. +""" +from __future__ import annotations + +import re +from pathlib import Path + +from parser.typescope import read_bodies + +_RETURN_NULL = re.compile(r"\bPG_RETURN_NULL\s*\(\s*\)") +_IF_OPEN = re.compile(r"\bif\s*\(") +# An argument-null guard says nothing about whether an answer exists. +_ARGISNULL = re.compile(r"\bPG_ARGISNULL\b") +# How far above a PG_RETURN_NULL its guard may sit. The guards in the tree are +# on the line before or, when the condition wraps, two or three lines above; a +# wider window would start attributing an unrelated earlier `if`. +_LOOKBACK = 4 +# A wrapper that states no guard of its own hands `fcinfo` to a shared helper +# that does. The helper is named in the body, so the hop is read rather than +# guessed. +_DELEGATES = re.compile(r"^\s*return\s+(\w+)\s*\(\s*fcinfo\b", re.M) +# Chains are one or two deep in the tree; the cap keeps a malformed source from +# walking forever, and the visited set already stops a cycle. +_MAX_HOPS = 4 + + +def _guard_for(lines: list[str], at: int) -> str | None: + """The `if` condition guarding the PG_RETURN_NULL on line ``at``.""" + for back in range(0, _LOOKBACK + 1): + i = at - back + if i < 0: + break + if _IF_OPEN.search(lines[i]): + guard = " ".join(part.strip() for part in lines[i:at + 1]) + guard = guard[_IF_OPEN.search(guard).start():] + guard = guard.split("PG_RETURN_NULL")[0].strip() + return re.sub(r"\s+", " ", guard) or None + return None + + +def _own_guard(body: str) -> str | None: + """The absence guard written in this body, ignoring argument-null checks.""" + lines = body.split("\n") + for n, line in enumerate(lines): + if not _RETURN_NULL.search(line): + continue + guard = _guard_for(lines, n) + if guard is None or _ARGISNULL.search(guard): + continue + return guard + return None + + +def _guard_through(name: str, bodies: dict[str, str]) -> str | None: + """The guard for ``name``, following the helper it hands ``fcinfo`` to. + + A THIRD of the wrappers state no guard of their own because they delegate: + ``Temporal_at_timestamptz`` is one line, ``return + Temporal_restrict_timestamptz(fcinfo, REST_AT);``, and the PG_RETURN_NULL + sits in that shared helper. Reading only the wrapper reports those as + always producing a value -- which is exactly backwards for the restriction + family, the one whose answer is absent most often. + + The chase is bounded and cycle-safe: a body naming itself, or a ring of + helpers, ends the walk rather than looping. + """ + seen: set[str] = set() + while name and name not in seen and len(seen) <= _MAX_HOPS: + seen.add(name) + body = bodies.get(name) + if body is None: + return None + guard = _own_guard(body) + if guard is not None: + return guard + m = _DELEGATES.search(body) + name = m.group(1) if m else None + return None + + +def extract_null_results(mdb_src: str | Path) -> dict[str, str]: + """Return ``{wrapper: guard}`` for every PG wrapper that can answer NULL.""" + bodies, _ = read_bodies(Path(mdb_src).parent) + out: dict[str, str] = {} + for name in bodies: + guard = _guard_through(name, bodies) + if guard: + out[name] = guard + return out + + +def attach_null_result(idl, mdb_src): + """Record, per function, the guard under which its SQL answer is absent. + + Returns ``(idl, n)`` with ``n`` the number of functions given the field. + """ + guards = extract_null_results(mdb_src) + n = 0 + for f in idl.get("functions", []): + wrapper = f.get("mdbC") + if not wrapper: + continue + guard = guards.get(wrapper) + if not guard: + continue + f.setdefault("shape", {})["nullableResult"] = guard + n += 1 + return idl, n diff --git a/run.py b/run.py index 17bc85e..67f29c8 100644 --- a/run.py +++ b/run.py @@ -11,6 +11,7 @@ from parser.header_types import reconcile from parser.shapeinfer import infer_shapes from parser.nullable import merge_nullable +from parser.nullresult import attach_null_result from parser.outparam import extract_param_names, merge_outparams from parser.boundargs import merge_boundargs from parser.enrich import enrich_idl @@ -205,6 +206,13 @@ def main(): # FUNCTION the wrapper backs rather than the aggregate above it. idl, naggfn = attach_sqlaggfn_map(idl, MEOS_SRC, MDB_SRC) print(f" Attached {naggfn} @sqlaggfn SQL aggregate names", file=sys.stderr) + # Whether the answer can be ABSENT, which no C return type states: the PG + # wrapper guards PG_RETURN_NULL with the condition, and that guard is the + # SQL contract. It rides the same @csqlfn chain, so it runs after sqlfn + # has set `mdbC`. Without it a binding has to invent a convention for + # absence, and each one invents a different one. + idl, nnullres = attach_null_result(idl, MDB_SRC) + print(f" Attached {nnullres} nullable-result guards", file=sys.stderr) # Guard: a copy-paste @csqlfn in meos/src can point an ever/always function at # the opposite-prefix wrapper (eintersects_* tagged #Aintersects_*), flipping its # SQL name and breaking the binding overload dispatch. The parser is faithful, so diff --git a/tests/test_nullresult.py b/tests/test_nullresult.py new file mode 100644 index 0000000..3aee4b5 --- /dev/null +++ b/tests/test_nullresult.py @@ -0,0 +1,155 @@ +"""Unit tests for parser/nullresult.py. + +Runs without libclang or pytest: python3 tests/test_nullresult.py + +Covers the three behaviours the field depends on: (1) the guard is read from the +PG wrapper that states it, (2) a wrapper that delegates `fcinfo` to a shared +helper inherits the helper's guard -- a third of the tree takes that shape, and +reading only the wrapper reports the whole restriction family as always +answering -- and (3) a `PG_ARGISNULL` guard is NOT an absence guard, since it +propagates a null argument rather than saying whether an answer exists. +""" + +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from parser.nullresult import attach_null_result, extract_null_results + + +def _tree(text, rel="src/temporal/temporal.c"): + """A mobilitydb-shaped checkout holding one wrapper source.""" + root = tempfile.mkdtemp() + p = Path(root) / "mobilitydb" / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(text) + return str(Path(root) / "mobilitydb" / "src") + + +class GuardTests(unittest.TestCase): + def test_reads_the_guard_the_wrapper_states(self): + src = _tree(""" +Datum +Temporal_at_value(PG_FUNCTION_ARGS) +{ + Temporal *result = temporal_at_value(temp, value); + if (! result) + PG_RETURN_NULL(); + PG_RETURN_TEMPORAL_P(result); +} +""") + self.assertEqual({"Temporal_at_value": "if (! result)"}, + extract_null_results(src)) + + def test_a_sentinel_guard_is_carried_verbatim(self): + src = _tree(""" +Datum +NAD_tpoint_geo(PG_FUNCTION_ARGS) +{ + double result = nad_tpoint_geo(temp, gs); + if (result == DBL_MAX) + PG_RETURN_NULL(); + PG_RETURN_FLOAT8(result); +} +""") + self.assertEqual({"NAD_tpoint_geo": "if (result == DBL_MAX)"}, + extract_null_results(src)) + + def test_a_wrapper_that_always_answers_carries_no_guard(self): + src = _tree(""" +Datum +Tbool_out(PG_FUNCTION_ARGS) +{ + char *result = tbool_out(temp); + PG_RETURN_CSTRING(result); +} +""") + self.assertEqual({}, extract_null_results(src)) + + +class DelegationTests(unittest.TestCase): + def test_a_delegating_wrapper_inherits_its_helper_guard(self): + src = _tree(""" +static Datum +Temporal_restrict_timestamptz(FunctionCallInfo fcinfo, bool atfunc) +{ + Temporal *result = temporal_restrict_timestamptz(temp, t, atfunc); + if (! result) + PG_RETURN_NULL(); + PG_RETURN_TEMPORAL_P(result); +} + +Datum +Temporal_at_timestamptz(PG_FUNCTION_ARGS) +{ + return Temporal_restrict_timestamptz(fcinfo, REST_AT); +} +""") + found = extract_null_results(src) + self.assertEqual("if (! result)", found.get("Temporal_at_timestamptz")) + + def test_a_delegation_cycle_terminates(self): + # A ring names no guard and must not hang or recurse without end. + src = _tree(""" +Datum +A_wrapper(PG_FUNCTION_ARGS) +{ + return B_wrapper(fcinfo); +} + +Datum +B_wrapper(PG_FUNCTION_ARGS) +{ + return A_wrapper(fcinfo); +} +""") + self.assertEqual({}, extract_null_results(src)) + + +class ArgIsNullTests(unittest.TestCase): + def test_an_argument_null_guard_is_not_an_absence_guard(self): + # PG_ARGISNULL says the CALLER passed nothing, which is already carried + # by shape.nullable and says nothing about whether an answer exists. + src = _tree(""" +Datum +Temporal_tcount_transfn(PG_FUNCTION_ARGS) +{ + if (PG_ARGISNULL(0)) + PG_RETURN_NULL(); + PG_RETURN_POINTER(result); +} +""") + self.assertEqual({}, extract_null_results(src)) + + +class AttachTests(unittest.TestCase): + def test_the_field_rides_the_existing_wrapper_link(self): + src = _tree(""" +Datum +Temporal_at_value(PG_FUNCTION_ARGS) +{ + if (! result) + PG_RETURN_NULL(); + PG_RETURN_TEMPORAL_P(result); +} +""") + idl = {"functions": [ + {"name": "temporal_at_value", "mdbC": "Temporal_at_value"}, + {"name": "temporal_out", "mdbC": "Temporal_out"}, + {"name": "unwrapped"}, + ]} + idl, n = attach_null_result(idl, src) + self.assertEqual(1, n) + self.assertEqual("if (! result)", + idl["functions"][0]["shape"]["nullableResult"]) + # A wrapper with no guard, and a function with no wrapper at all, are + # left without the field rather than given a false one. + self.assertNotIn("shape", idl["functions"][1]) + self.assertNotIn("shape", idl["functions"][2]) + + +if __name__ == "__main__": + unittest.main()