From 992cea7d951d1bc1beb368e6bda383ad424a8b75 Mon Sep 17 00:00:00 2001 From: Erik Welch Date: Fri, 31 Jul 2026 13:43:32 -0500 Subject: [PATCH 1/4] Add test_automethods.py: guard the generated expression surface Adding a method to Matrix/Vector/Scalar without updating the automethods name sets left the expression classes silently missing it, and no test failed. The new introspection test closes that: - Forward: every public attribute (and value-forwarding dunder) of Scalar/Vector/Matrix/TransposedMatrix is either auto-computed on the expression classes or listed in an explicit OPT_OUT table with a reason (52 entries: mutators, constructors, native metadata, storage flags). - Reverse: every generated name still exists on the concrete type. - Hygiene: OPT_OUT entries must be live and not redundantly covered, so the table cannot rot. Coverage is derived at runtime from what the generator emitted (getter __module__ is graphblas.core.automethods), not from a second copy of the name sets, so reorganizing the sets does not break the test. Failure messages name the attribute and point to the sets and scripts/autogenerate.py. Teeth: adding a fake public method to Vector makes the test fail with that message. --- graphblas/tests/test_automethods.py | 346 ++++++++++++++++++++++++++++ 1 file changed, 346 insertions(+) create mode 100644 graphblas/tests/test_automethods.py diff --git a/graphblas/tests/test_automethods.py b/graphblas/tests/test_automethods.py new file mode 100644 index 000000000..efb3ee763 --- /dev/null +++ b/graphblas/tests/test_automethods.py @@ -0,0 +1,346 @@ +"""Guard the auto-generated expression surface against silent drift. + +``graphblas/core/automethods.py`` is a generated-code module. Its name sets +(near line 347) drive ``scripts/autogenerate.py``, which copies auto-compute +properties onto the expression classes (``VectorExpression`` / +``VectorIndexExpr`` and the Scalar/Matrix equivalents) so that, for example, +``(A @ B).to_coo()`` works without a manual ``.new()`` first. + +The trap this module closes: add a public method to ``Matrix``/``Vector``/ +``Scalar``, forget to add its name to the sets and rerun the generator, and +nothing fails. The expression classes silently lack the method, but the concrete +types have it, so CI stays green. + +The tests here assert, for each concrete type and for ``TransposedMatrix``: + +1. Forward: every public method/property is EITHER reachable on the expression + classes via auto-compute OR listed in ``OPT_OUT`` with a reason. Mutating + methods, constructors, and cheap metadata deliberately do not auto-compute. +2. Reverse: every auto-generated name still exists on the concrete type (a + rename that leaves a stale set entry is caught at import already, but this + makes the failure legible). +3. Hygiene: every ``OPT_OUT`` entry is a live attribute that is not in fact + covered, so the table cannot rot into stale excuses. + +Coverage is derived at runtime from what the generator actually emitted onto the +expression classes (properties whose getter lives in the ``automethods`` module), +not from a second copy of the name sets. That keeps this test honest if the sets +are reorganized: only a real change in the generated surface moves coverage. + +Dunder scope: value-forwarding dunders such as ``__getitem__``, ``__contains__``, +and ``__matmul__`` are generated and thus checked. Arithmetic and comparison +operator sugar such as ``__add__`` and ``__lt__`` is handled by the infix +expression system (``infixmethods.py``), not the value-forwarding automethods +path, so it is excluded from the dunder sweep by ``_OPERATOR_SUGAR_DUNDERS``. +Python object machinery is excluded via a baseline class so new +interpreter-version dunders never make this test flaky. +""" + +from graphblas.core.matrix import ( + Matrix, + MatrixExpression, + MatrixIndexExpr, + TransposedMatrix, +) +from graphblas.core.scalar import Scalar, ScalarExpression, ScalarIndexExpr +from graphblas.core.vector import Vector, VectorExpression, VectorIndexExpr + +import pytest # isort: skip + +_AUTOMETHODS_MODULE = "graphblas.core.automethods" + +# --- reasons an attribute deliberately does not auto-compute ---------------- + +_MUTATES = ( + "in-place mutator; auto-computing would mutate a throwaway temporary, " + "not the caller's object" +) +_CONSTRUCTS = ( + "constructor (classmethod) that builds a new object from external data; " + "not an accessor on a computed result" +) +_MATERIALIZES = ( + "produces a new concrete object; an expression is materialized with .new(), " + "so an auto-compute property would be redundant" +) +_METADATA = ( + "cheap metadata known without materializing; exposed natively on expressions " + "via BaseExpression, so it must not force a compute" +) +_SCALAR_STORAGE = ( + "storage-backing flag (C scalar vs GrB_Scalar); ScalarExpression and " + "ScalarIndexExpr define it natively since they know how their result will " + "materialize, so it must not force a compute" +) +_RAISES_MATERIALIZE = ( + "raises to require explicit materialization of the lazy transposed view " + "(np.asarray / bool); mirrors Matrix-expression behavior" +) + +# Every public name of a concrete type that intentionally is NOT auto-computed. +# Adding a public method almost never belongs here; it belongs in the automethods +# name sets. This table is for the deliberate exceptions only. +OPT_OUT = { + "Scalar": { + "clear": _MUTATES, + "update": _MUTATES, + "dup": _MATERIALIZES, + "from_value": _CONSTRUCTS, + "dtype": _METADATA, + "ndim": _METADATA, + "shape": _METADATA, + "is_cscalar": _SCALAR_STORAGE, + "is_grbscalar": _SCALAR_STORAGE, + }, + "Vector": { + "build": _MUTATES, + "clear": _MUTATES, + "resize": _MUTATES, + "update": _MUTATES, + "dup": _MATERIALIZES, + "from_coo": _CONSTRUCTS, + "from_dense": _CONSTRUCTS, + "from_dict": _CONSTRUCTS, + "from_pairs": _CONSTRUCTS, + "from_scalar": _CONSTRUCTS, + "dtype": _METADATA, + "ndim": _METADATA, + "shape": _METADATA, + "size": _METADATA, + }, + "Matrix": { + "build": _MUTATES, + "clear": _MUTATES, + "resize": _MUTATES, + "setdiag": _MUTATES, + "update": _MUTATES, + "dup": _MATERIALIZES, + "from_coo": _CONSTRUCTS, + "from_csc": _CONSTRUCTS, + "from_csr": _CONSTRUCTS, + "from_dcsc": _CONSTRUCTS, + "from_dcsr": _CONSTRUCTS, + "from_dense": _CONSTRUCTS, + "from_dicts": _CONSTRUCTS, + "from_edgelist": _CONSTRUCTS, + "from_scalar": _CONSTRUCTS, + "dtype": _METADATA, + "ncols": _METADATA, + "ndim": _METADATA, + "nrows": _METADATA, + "shape": _METADATA, + }, + "TransposedMatrix": { + "dup": _MATERIALIZES, + "new": _MATERIALIZES, + "dtype": _METADATA, + "ncols": _METADATA, + "ndim": _METADATA, + "nrows": _METADATA, + "shape": _METADATA, + "__array__": _RAISES_MATERIALIZE, + "__bool__": _RAISES_MATERIALIZE, + }, +} + +# Concrete type + the expression classes the generator targets for it. +# TransposedMatrix has no expression class of its own; as a read-only Matrix view +# it shares Matrix's generated surface. +_REGISTRY = { + "Scalar": (Scalar, (ScalarExpression, ScalarIndexExpr)), + "Vector": (Vector, (VectorExpression, VectorIndexExpr)), + "Matrix": (Matrix, (MatrixExpression, MatrixIndexExpr)), + "TransposedMatrix": (TransposedMatrix, (MatrixExpression, MatrixIndexExpr)), +} + + +# --- dunder scope helpers --------------------------------------------------- + + +class _Baseline: + __slots__ = () + + +# Object/interpreter machinery dunders. Computed from a trivial class so that +# version-specific additions (e.g. __firstlineno__, __static_attributes__) are +# absorbed automatically rather than hard-coded. +_MACHINERY_DUNDERS = ( + set(dir(_Baseline)) + | set(vars(_Baseline)) + | { + "__del__", + "__weakref__", + "__dict__", + "__reduce__", + "__reduce_ex__", + "__getstate__", + "__setstate__", + "__networkx_backend__", + "__networkx_plugin__", + } +) + +# Arithmetic / comparison / item-mutation sugar. These build lazy infix +# expressions (or mutate in place) and are handled by infixmethods.py, not the +# value-forwarding automethods path, so they are out of scope for this sweep. +_OPERATOR_SUGAR_DUNDERS = { + "__add__", + "__radd__", + "__sub__", + "__rsub__", + "__mul__", + "__rmul__", + "__truediv__", + "__rtruediv__", + "__floordiv__", + "__rfloordiv__", + "__mod__", + "__rmod__", + "__pow__", + "__rpow__", + "__divmod__", + "__rdivmod__", + "__xor__", + "__rxor__", + "__neg__", + "__abs__", + "__invert__", + "__lt__", + "__le__", + "__gt__", + "__ge__", + "__setitem__", + "__delitem__", +} + + +# --- introspection ---------------------------------------------------------- + + +def _accessible(cls, name): + """True if ``cls.name`` resolves without raising. + + ``dir()`` lists names that are not usable (e.g. ``ss`` under the + suitesparse-vanilla backend raises on access); those are not part of the + public surface a caller can rely on. + """ + try: + getattr(cls, name) + except Exception: + # Any failure to access means the name is not a usable public attribute. + return False + return True + + +def _public_surface(cls): + return {n for n in dir(cls) if not n.startswith("_") and _accessible(cls, n)} + + +def _own_dunders(cls): + return {n for n in vars(cls) if n.startswith("__") and n.endswith("__")} + + +def _generated_coverage(*expr_classes): + """Names the generator emitted onto ``expr_classes``. + + A name counts as covered when the expression class exposes it as a property + whose getter is defined in the automethods module, or as a callable copied + from that module (the ``__iadd__``-style guards). This reads the actual + generated surface, so it tracks the name sets without duplicating them. + """ + names = set() + for expr_class in expr_classes: + for klass in expr_class.__mro__: + for name, value in vars(klass).items(): + func = value.fget if isinstance(value, property) else value + if callable(func) and getattr(func, "__module__", None) == _AUTOMETHODS_MODULE: + names.add(name) + return names + + +def _coverage(label): + _concrete, expr_classes = _REGISTRY[label] + return _generated_coverage(*expr_classes) + + +def _fix_hint(label, names): + _concrete, expr_classes = _REGISTRY[label] + expr_names = " / ".join(c.__name__ for c in dict.fromkeys(expr_classes)) + return ( + f"{label} has public attribute(s) not reachable on its expression " + f"classes ({expr_names}) and not listed in OPT_OUT[{label!r}]:\n" + f" {sorted(names)}\n\n" + "If these should auto-compute on expressions, add each name to the " + "matching set in graphblas/core/automethods.py (the sets near line 347) " + "and regenerate with:\n" + " python scripts/autogenerate.py\n\n" + "If they must NOT auto-compute (an in-place mutator, a constructor, or " + f"cheap metadata), add them to OPT_OUT[{label!r}] in this file with a " + "one-line reason." + ) + + +# --- tests ------------------------------------------------------------------ + + +@pytest.mark.parametrize("label", list(_REGISTRY)) +def test_public_methods_covered_or_opted_out(label): + concrete, _expr_classes = _REGISTRY[label] + coverage = _coverage(label) + opt_out = OPT_OUT[label] + uncovered = _public_surface(concrete) - coverage - set(opt_out) + assert not uncovered, _fix_hint(label, uncovered) + + +@pytest.mark.parametrize("label", list(_REGISTRY)) +def test_relevant_dunders_covered_or_opted_out(label): + concrete, _expr_classes = _REGISTRY[label] + coverage = _coverage(label) + opt_out = OPT_OUT[label] + candidates = ( + _own_dunders(concrete) + - _MACHINERY_DUNDERS + - _OPERATOR_SUGAR_DUNDERS + - coverage + - set(opt_out) + ) + assert not candidates, _fix_hint(label, candidates) + + +@pytest.mark.parametrize("label", ["Scalar", "Vector", "Matrix"]) +def test_no_stale_generated_names(label): + concrete, _expr_classes = _REGISTRY[label] + # _get_value is the auto-compute helper itself, not a mirror of a concrete + # attribute, so it is expected not to exist on the concrete type. + stale = {n for n in _coverage(label) if n != "_get_value" and not hasattr(concrete, n)} + assert not stale, ( + f"Generated name(s) on the {label} expression classes no longer exist " + f"on {label}: {sorted(stale)}. A concrete method was renamed or removed " + "without updating the sets in graphblas/core/automethods.py; update the " + "sets and rerun `python scripts/autogenerate.py`." + ) + + +@pytest.mark.parametrize("label", list(_REGISTRY)) +def test_opt_out_entries_are_live(label): + concrete, _expr_classes = _REGISTRY[label] + opt_out = OPT_OUT[label] + surface = _public_surface(concrete) | _own_dunders(concrete) + missing = {n for n in opt_out if n not in surface} + assert not missing, ( + f"OPT_OUT[{label!r}] lists name(s) that are not attributes of {label}: " + f"{sorted(missing)}. Remove the stale entries (the method was renamed or " + "removed)." + ) + coverage = _coverage(label) + redundant = {n for n in opt_out if n in coverage} + assert not redundant, ( + f"OPT_OUT[{label!r}] lists name(s) that ARE auto-computed and so need no " + f"opt-out: {sorted(redundant)}. Remove them from OPT_OUT." + ) + + +@pytest.mark.parametrize("label", list(_REGISTRY)) +def test_opt_out_reasons_present(label): + empty = {n for n, reason in OPT_OUT[label].items() if not reason or not reason.strip()} + assert not empty, f"OPT_OUT[{label!r}] entries need a non-empty reason: {sorted(empty)}" From 33c9a93d496f0708c09292967cd9ec31a0b72aa8 Mon Sep 17 00:00:00 2001 From: Erik Welch Date: Fri, 31 Jul 2026 13:43:32 -0500 Subject: [PATCH 2/4] Cover the infix expression classes in the automethods guard core/infixmethods.py copies the same generated automethods surface onto the infix expression classes (VectorInfixExpr and friends, minus the private _get_value), so a name missing only from the infix classes was a gap test_automethods.py could not see: coverage was the UNION across expression classes, so a name present on the plain expression class masked its absence elsewhere. Coverage is now the INTERSECTION across plain, index, and infix classes (identical sets today: Scalar 42/42/41, Vector 49/49/48, Matrix 58/58/57, differing only by _get_value). Teeth: hiding to_coo on MatrixInfixExpr alone makes the Matrix forward test fail, naming the attribute, all three expression classes, and scripts/autogenerate.py. --- graphblas/tests/test_automethods.py | 72 +++++++++++++++++++---------- 1 file changed, 47 insertions(+), 25 deletions(-) diff --git a/graphblas/tests/test_automethods.py b/graphblas/tests/test_automethods.py index efb3ee763..607af65a5 100644 --- a/graphblas/tests/test_automethods.py +++ b/graphblas/tests/test_automethods.py @@ -3,19 +3,24 @@ ``graphblas/core/automethods.py`` is a generated-code module. Its name sets (near line 347) drive ``scripts/autogenerate.py``, which copies auto-compute properties onto the expression classes (``VectorExpression`` / -``VectorIndexExpr`` and the Scalar/Matrix equivalents) so that, for example, -``(A @ B).to_coo()`` works without a manual ``.new()`` first. +``VectorIndexExpr`` / ``VectorInfixExpr`` and the Scalar/Matrix equivalents) so +that, for example, ``(A @ B).to_coo()`` works without a manual ``.new()`` first. +The same generator run emits the surface onto the infix classes too (the +infix.py branch of ``automethods._main()``), minus the private ``_get_value`` +helper, so the infix classes must carry the identical public surface. The trap this module closes: add a public method to ``Matrix``/``Vector``/ ``Scalar``, forget to add its name to the sets and rerun the generator, and nothing fails. The expression classes silently lack the method, but the concrete -types have it, so CI stays green. +types have it, so CI stays green. A name that lands on the plain expression +classes but is missing from the infix classes slips through the same way. The tests here assert, for each concrete type and for ``TransposedMatrix``: -1. Forward: every public method/property is EITHER reachable on the expression - classes via auto-compute OR listed in ``OPT_OUT`` with a reason. Mutating - methods, constructors, and cheap metadata deliberately do not auto-compute. +1. Forward: every public method/property is EITHER reachable on ALL of the + expression classes (plain, index, and infix) via auto-compute OR listed in + ``OPT_OUT`` with a reason. Mutating methods, constructors, and cheap metadata + deliberately do not auto-compute. 2. Reverse: every auto-generated name still exists on the concrete type (a rename that leaves a stale set entry is caught at import already, but this makes the failure legible). @@ -24,8 +29,10 @@ Coverage is derived at runtime from what the generator actually emitted onto the expression classes (properties whose getter lives in the ``automethods`` module), -not from a second copy of the name sets. That keeps this test honest if the sets -are reorganized: only a real change in the generated surface moves coverage. +intersected across every variant so a name reachable on some classes but absent +from another (e.g. only the infix class) counts as uncovered. It is not a second +copy of the name sets, which keeps this test honest if the sets are reorganized: +only a real change in the generated surface moves coverage. Dunder scope: value-forwarding dunders such as ``__getitem__``, ``__contains__``, and ``__matmul__`` are generated and thus checked. Arithmetic and comparison @@ -36,6 +43,7 @@ interpreter-version dunders never make this test flaky. """ +from graphblas.core.infix import MatrixInfixExpr, ScalarInfixExpr, VectorInfixExpr from graphblas.core.matrix import ( Matrix, MatrixExpression, @@ -143,14 +151,17 @@ }, } -# Concrete type + the expression classes the generator targets for it. +# Concrete type + the expression classes the generator targets for it: the plain +# expression, the index expression, and the infix expression. Coverage is the +# intersection across all three, so a name emitted onto some but not the infix +# class is treated as uncovered. # TransposedMatrix has no expression class of its own; as a read-only Matrix view # it shares Matrix's generated surface. _REGISTRY = { - "Scalar": (Scalar, (ScalarExpression, ScalarIndexExpr)), - "Vector": (Vector, (VectorExpression, VectorIndexExpr)), - "Matrix": (Matrix, (MatrixExpression, MatrixIndexExpr)), - "TransposedMatrix": (TransposedMatrix, (MatrixExpression, MatrixIndexExpr)), + "Scalar": (Scalar, (ScalarExpression, ScalarIndexExpr, ScalarInfixExpr)), + "Vector": (Vector, (VectorExpression, VectorIndexExpr, VectorInfixExpr)), + "Matrix": (Matrix, (MatrixExpression, MatrixIndexExpr, MatrixInfixExpr)), + "TransposedMatrix": (TransposedMatrix, (MatrixExpression, MatrixIndexExpr, MatrixInfixExpr)), } @@ -240,24 +251,35 @@ def _own_dunders(cls): return {n for n in vars(cls) if n.startswith("__") and n.endswith("__")} -def _generated_coverage(*expr_classes): - """Names the generator emitted onto ``expr_classes``. +def _generated_coverage_one(expr_class): + """Names the generator emitted onto a single ``expr_class``. - A name counts as covered when the expression class exposes it as a property - whose getter is defined in the automethods module, or as a callable copied - from that module (the ``__iadd__``-style guards). This reads the actual - generated surface, so it tracks the name sets without duplicating them. + A name counts as covered when the class exposes it as a property whose getter + is defined in the automethods module, or as a callable copied from that module + (the ``__iadd__``-style guards). This reads the actual generated surface, so it + tracks the name sets without duplicating them. """ names = set() - for expr_class in expr_classes: - for klass in expr_class.__mro__: - for name, value in vars(klass).items(): - func = value.fget if isinstance(value, property) else value - if callable(func) and getattr(func, "__module__", None) == _AUTOMETHODS_MODULE: - names.add(name) + for klass in expr_class.__mro__: + for name, value in vars(klass).items(): + func = value.fget if isinstance(value, property) else value + if callable(func) and getattr(func, "__module__", None) == _AUTOMETHODS_MODULE: + names.add(name) return names +def _generated_coverage(*expr_classes): + """Names the generator emitted onto EVERY class in ``expr_classes``. + + The intersection: a name is covered only when it is reachable on all variants + (plain expression, index expression, and infix expression). One emitted onto + some but not others counts as uncovered, which is how a name missing from just + the infix surface is caught. + """ + per_class = [_generated_coverage_one(cls) for cls in expr_classes] + return set.intersection(*per_class) if per_class else set() + + def _coverage(label): _concrete, expr_classes = _REGISTRY[label] return _generated_coverage(*expr_classes) From bff0d33a8e2aa87c4e168978d4f4ad7b57c1107d Mon Sep 17 00:00:00 2001 From: Erik Welch Date: Fri, 31 Jul 2026 13:43:32 -0500 Subject: [PATCH 3/4] Add --check mode to autogenerate and a drift-guard test test_automethods.py guards the NAMES of the generated expression surface but not the generated file content: hand-edit a block between the auto-generated code markers, or edit the name sets in automethods._main without rerunning the generator, and the on-disk blocks go stale while CI stays green. scripts/autogenerate.py --check now regenerates every generated file into a scratch tree and compares, naming the files that drifted. The generator mains gained internal base-dir and callblack parameters to support this. The two guards are easy to confuse, so the scope is worth stating. The generator emits from the literal name sets in automethods._main rather than by introspecting the classes, so adding a method to Matrix does not change its output and cannot surface here; test_automethods.py covers that side. This covers the other one, generated files that no longer match the generator claiming to produce them. The check compares parsed syntax, not bytes. The generators shell out to black when it is on PATH and skip it when it is not, so a byte comparison reports drift on automethods.py and infixmethods.py in every environment lacking black, and black is in no test extra. Guarding that with skipif(black is None) meant the test never ran in the pytest_normal CI jobs, which is exactly where it needs teeth. Comparing ASTs makes the result independent of whether black is installed, so the check runs everywhere, and layout is already enforced repo-wide by black in pre-commit and the lint job. The residual gap is a layout-only edit inside a generated block, which black --check catches and this does not. The script also puts its own repo root on sys.path before importing graphblas. For a script sys.path[0] is the script's own directory, so a bare import resolves to whatever is installed; that coincides with the checkout in an ordinary dev setup and diverges in a git worktree, where --check would validate a tree nobody asked about while reporting green. The check now prints the package it validated and the test asserts on that line rather than inferring correctness from an exit code. Scratch files go to a TemporaryDirectory outside the repo. Comparing parsed syntax removed the reason to keep them inside it (letting black discover the project pyproject.toml), and an interrupted run no longer leaves .autogen_check_* directories in the working tree. --- graphblas/core/automethods.py | 17 ++++- graphblas/core/infixmethods.py | 14 +++- graphblas/tests/test_autogenerate_check.py | 62 ++++++++++++++++ scripts/autogenerate.py | 86 ++++++++++++++++++++++ 4 files changed, 174 insertions(+), 5 deletions(-) create mode 100644 graphblas/tests/test_autogenerate_check.py diff --git a/graphblas/core/automethods.py b/graphblas/core/automethods.py index 0c86cfe2d..32f6c37ca 100644 --- a/graphblas/core/automethods.py +++ b/graphblas/core/automethods.py @@ -366,11 +366,22 @@ def __ixor__(self, other): # End auto-generated code -def _main(): +def _main(base_dir=None, callblack=True): + # base_dir lets `scripts/autogenerate.py --check` redirect reads and writes to a + # scratch copy of the tree; when None we regenerate the real files in place. + # callblack=False keeps that check hermetic: black is optional here, and letting it + # run would make the scratch output depend on whether it is installed. + import functools from pathlib import Path from .utils import _autogenerate_code + if not callblack: + _autogenerate_code = functools.partial(_autogenerate_code, callblack=False) + + if base_dir is None: + base_dir = Path(__file__).parent + common = { "_name_html", "_nvals", @@ -488,7 +499,7 @@ def _main(): f' raise TypeError(f"{name!r} not supported for {{type(self).__name__}}")\n\n' ) - _autogenerate_code(Path(__file__), "\n".join(lines)) + _autogenerate_code(base_dir / "automethods.py", "\n".join(lines)) # Copy to scalar.py and infix.py lines = [] @@ -509,7 +520,7 @@ def _main(): continue lines.append(f" {name} = automethods.{name}") - thisdir = Path(__file__).parent + thisdir = base_dir infix_exclude = {"_get_value"} def get_name(line): diff --git a/graphblas/core/infixmethods.py b/graphblas/core/infixmethods.py index 67248223a..3f7f70ccb 100644 --- a/graphblas/core/infixmethods.py +++ b/graphblas/core/infixmethods.py @@ -320,7 +320,11 @@ def __itruediv__(self, other): # End auto-generated code -def _main(): +def _main(base_dir=None, callblack=True): + # base_dir lets `scripts/autogenerate.py --check` redirect the write to a scratch + # copy of the tree; when None we regenerate infixmethods.py in place. + # callblack=False keeps that check hermetic: black is optional here, and letting it + # run would make the scratch output depend on whether it is installed. # Run via `python -m graphblas.core.infixmethods` comparisons = { "lt": "lt", @@ -426,11 +430,17 @@ def _main(): " setattr(VectorIndexExpr, name, val)\n" " setattr(MatrixIndexExpr, name, val)\n" ) + import functools from pathlib import Path from .utils import _autogenerate_code - _autogenerate_code(Path(__file__), "\n".join(lines)) + if not callblack: + _autogenerate_code = functools.partial(_autogenerate_code, callblack=False) + + if base_dir is None: + base_dir = Path(__file__).parent + _autogenerate_code(base_dir / "infixmethods.py", "\n".join(lines)) if __name__ == "__main__": diff --git a/graphblas/tests/test_autogenerate_check.py b/graphblas/tests/test_autogenerate_check.py new file mode 100644 index 000000000..a10beb561 --- /dev/null +++ b/graphblas/tests/test_autogenerate_check.py @@ -0,0 +1,62 @@ +"""Guard the auto-generated code blocks against undetected drift. + +``test_automethods.py`` guards the *names* on the generated expression surface. +Nothing there guards the generated *content*: hand-edit a block between the +``# Begin auto-generated code`` / ``# End auto-generated code`` markers, or edit +the name sets in ``automethods._main`` without rerunning the generator, and the +on-disk blocks go stale while CI stays green. + +``scripts/autogenerate.py --check`` closes that gap. It regenerates every +generated file into a scratch tree and compares each against the file on disk, +exiting nonzero (and naming the drifted files) on any mismatch. The comparison is +on parsed syntax, so the check needs no formatter and runs everywhere rather than +skipping in every job that lacks one. + +Scope, since the two guards are easy to confuse. The generator emits from the +literal name sets in ``automethods._main``, not by introspecting the classes, so +adding a method to Matrix/Vector/Scalar does not change its output and cannot +show up here. That case is ``test_automethods.py``'s: it compares the concrete +types against the generated surface and fails when a new public attribute is +neither auto-computed nor opted out. This module covers the other half, the +generated files no longer matching the generator that claims to produce them. +""" + +import subprocess +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +AUTOGEN_SCRIPT = REPO_ROOT / "scripts" / "autogenerate.py" +PACKAGE_LINE = "graphblas package: " + + +def test_autogenerated_code_is_in_sync(): + if not AUTOGEN_SCRIPT.exists(): # pragma: no cover (source checkout only) + pytest.skip("scripts/autogenerate.py not present (installed package, not a checkout)") + result = subprocess.run( + [sys.executable, str(AUTOGEN_SCRIPT), "--check"], + capture_output=True, + text=True, + check=False, + ) + report = f"stdout:\n{result.stdout}\nstderr:\n{result.stderr}" + checked = [ + line[len(PACKAGE_LINE) :].strip() + for line in result.stdout.splitlines() + if line.startswith(PACKAGE_LINE) + ] + + # Assert the positive post-condition before the exit code. A check that ran against a + # different `graphblas` reports success about a tree nobody asked about: sys.path[0] + # for a script is the script's own directory, so that is what happens whenever an + # editable install points elsewhere, as in a git worktree. + assert checked, f"--check never reported a package, so it failed before starting.\n{report}" + expected = str(REPO_ROOT / "graphblas") + assert checked == [expected], f"--check validated {checked}, not {expected}.\n{report}" + + assert result.returncode == 0, ( + "Auto-generated code is out of date. Run `python scripts/autogenerate.py` and " + f"commit the result.\n{report}" + ) diff --git a/scripts/autogenerate.py b/scripts/autogenerate.py index 5f1116674..95a50fe94 100755 --- a/scripts/autogenerate.py +++ b/scripts/autogenerate.py @@ -11,8 +11,37 @@ Modifying infix-methods is much less common, but should be run if you want to modify it. +Pass --check to verify the generated files on disk match a fresh regeneration without +modifying anything. This exits nonzero (and names the drifted files) when they differ, +which is what the drift test uses to catch a hand-edited or stale generated block. + """ +import sys +from pathlib import Path + +# For a script, sys.path[0] is the script's own directory rather than the caller's cwd, so +# a bare `import graphblas` resolves to whatever is installed. That coincides with this +# checkout in an ordinary dev setup and diverges in a worktree, where the generator would +# then rewrite, or validate, a tree nobody asked about while looking perfectly normal. +_REPO_ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(_REPO_ROOT)) + +# --check prefixes the package it validated with this, so the drift test can assert it +# checked the checkout it lives in rather than infer correctness from an exit code. +_PACKAGE_LINE = "graphblas package: " + +# Files whose auto-generated blocks the generators (re)write. During --check, reads and +# writes are redirected to scratch copies of these so the real tree is never touched. +_GENERATED_FILES = ( + "automethods.py", + "infixmethods.py", + "scalar.py", + "vector.py", + "matrix.py", + "infix.py", +) + def main(): from graphblas.core.automethods import _main as auto_main @@ -22,5 +51,62 @@ def main(): infix_main() +def _parsed(path): + """Source parsed to an AST dump: identical for two files that differ only in layout.""" + import ast + + return ast.dump(ast.parse(path.read_bytes())) + + +def check(): + """Regenerate into a scratch tree and report any drift. + + Returns 0 when every generated file matches a fresh regeneration, else 1. + + The comparison is on parsed syntax rather than bytes, which keeps the check + independent of `black`. The generators shell out to black when it is on PATH and skip + it when it is not, so a byte comparison reports drift on automethods.py and + infixmethods.py in every environment lacking it, and black is not a test dependency. + Layout is already enforced repo-wide by black in pre-commit and the lint job, so the + drift left for this check to catch is a generated block whose content is stale. + """ + import shutil + import tempfile + from pathlib import Path + + import graphblas + from graphblas.core import automethods, infixmethods + + # Report the tree actually validated. For a script sys.path[0] is the script's own + # directory, so without the sys.path fix above this silently checks whichever + # graphblas is installed; the drift test asserts on this line. + print(f"{_PACKAGE_LINE}{Path(graphblas.__file__).resolve().parent}") + + src_dir = Path(automethods.__file__).parent + with tempfile.TemporaryDirectory(prefix="autogen_check_") as tmp: + scratch = Path(tmp) + for name in _GENERATED_FILES: + shutil.copyfile(src_dir / name, scratch / name) + automethods._main(base_dir=scratch, callblack=False) + infixmethods._main(base_dir=scratch, callblack=False) + + drifted = [ + name for name in _GENERATED_FILES if _parsed(scratch / name) != _parsed(src_dir / name) + ] + + if drifted: + print("Auto-generated code is out of date; run `python scripts/autogenerate.py`.") + print("Drifted file(s):") + for name in drifted: + print(f" graphblas/core/{name}") + return 1 + print("Auto-generated code is up to date.") + return 0 + + if __name__ == "__main__": + import sys + + if "--check" in sys.argv[1:]: + sys.exit(check()) main() From 32893f730b13458ee83e28dd70571c076db8a22a Mon Sep 17 00:00:00 2001 From: Erik Welch Date: Tue, 4 Aug 2026 16:57:28 -0700 Subject: [PATCH 4/4] Treat __slotnames__ as machinery in the dunder drift guard --- graphblas/tests/test_automethods.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/graphblas/tests/test_automethods.py b/graphblas/tests/test_automethods.py index 607af65a5..8cb2c25be 100644 --- a/graphblas/tests/test_automethods.py +++ b/graphblas/tests/test_automethods.py @@ -186,6 +186,10 @@ class _Baseline: "__reduce_ex__", "__getstate__", "__setstate__", + # copyreg._slotnames() caches this on the class the first time an + # instance is pickled, so its presence depends on whether test_pickle + # ran first in the randomized test order. + "__slotnames__", "__networkx_backend__", "__networkx_plugin__", }