diff --git a/graphblas/core/automethods.py b/graphblas/core/automethods.py index 01b238634..f64c3a61f 100644 --- a/graphblas/core/automethods.py +++ b/graphblas/core/automethods.py @@ -374,11 +374,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", @@ -496,7 +507,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 = [] @@ -517,7 +528,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 dfe539468..d3e541f81 100644 --- a/graphblas/core/infixmethods.py +++ b/graphblas/core/infixmethods.py @@ -329,7 +329,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", @@ -435,11 +439,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..d30d968d7 --- /dev/null +++ b/graphblas/tests/test_autogenerate_check.py @@ -0,0 +1,101 @@ +"""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 ast +import re +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}" + ) + + +def test_generators_rewrite_only_the_copies_in_base_dir(tmp_path, monkeypatch): + # What --check relies on: given base_dir, each generator reads and rewrites the + # copies there and leaves the package alone, and callblack=False never runs black. + from graphblas.core import automethods, infixmethods + + def black(*args, **kwargs): # pragma: no cover (only on failure) + raise AssertionError(f"black was run: {args}") + + monkeypatch.setattr(subprocess, "check_call", black) + src_dir = Path(automethods.__file__).parent + names = ["automethods.py", "infixmethods.py", "scalar.py", "vector.py", "matrix.py", "infix.py"] + raw = {name: (src_dir / name).read_bytes() for name in names} + originals = {name: (src_dir / name).read_text() for name in names} + for name, text in originals.items(): + # Make every generated block in the copy stale + stale = re.sub(r"(# Begin auto-generated code.*\n)( *)", r"\1\2_STALE_ = 1\n\2", text) + assert "_STALE_" in stale, name + (tmp_path / name).write_text(stale) + + try: + automethods._main(base_dir=tmp_path, callblack=False) + infixmethods._main(base_dir=tmp_path, callblack=False) + rewritten = [name for name in names if (src_dir / name).read_bytes() != raw[name]] + finally: + # A generator that ignored base_dir rewrote the package's own file; put it back + for name in names: + if (src_dir / name).read_bytes() != raw[name]: # pragma: no cover (only on failure) + (src_dir / name).write_bytes(raw[name]) + assert not rewritten + + for name, text in originals.items(): + regenerated = (tmp_path / name).read_text() + assert "_STALE_" not in regenerated, name + # Without black the layout differs, so compare parsed syntax as --check does + assert ast.dump(ast.parse(regenerated)) == ast.dump(ast.parse(text)), name diff --git a/graphblas/tests/test_automethods.py b/graphblas/tests/test_automethods.py new file mode 100644 index 000000000..491d623a2 --- /dev/null +++ b/graphblas/tests/test_automethods.py @@ -0,0 +1,375 @@ +"""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`` / ``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. 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 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). +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), +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 +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.infix import MatrixInfixExpr, ScalarInfixExpr, VectorInfixExpr +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: 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, ScalarInfixExpr)), + "Vector": (Vector, (VectorExpression, VectorIndexExpr, VectorInfixExpr)), + "Matrix": (Matrix, (MatrixExpression, MatrixIndexExpr, MatrixInfixExpr)), + "TransposedMatrix": (TransposedMatrix, (MatrixExpression, MatrixIndexExpr, MatrixInfixExpr)), +} + + +# --- 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__", + # 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__", + # A class constant numpy reads to make its operators defer to ours, not a + # value of the result; the expression classes define it themselves. + "__array_priority__", + } +) + +# 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_one(expr_class): + """Names the generator emitted onto a single ``expr_class``. + + 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 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) + + +def _fix_hint(label, names): # pragma: no cover (only on failure) + _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)}" 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()