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()