From dc4ba5d5da00ef135ba8d25ff29c7f2f2fecf757 Mon Sep 17 00:00:00 2001 From: Erik Welch Date: Mon, 3 Aug 2026 14:52:07 -0500 Subject: [PATCH] Make config attribute assignment raise instead of silently no-op Measured before this change: `graphblas.config.autocompute = False` left `config["autocompute"]` at True and added a dead `autocompute` entry to `vars(config)`. donfig.Config defines no __setattr__ for options, so the write landed on the instance and the real option never moved. The mistake is easy to make and leaves no trace. graphblas.config is now an instance of a donfig.Config subclass whose __setattr__ rejects writes that are not donfig's own. For a known option the AttributeError names the canonical idioms (config.set(name=value), optionally as a `with` block, and config[name] to read). For an unknown name it says so and lists the known options, deliberately WITHOUT advising config.set(): donfig's set() accepts arbitrary keys, so pointing there would trade a loud error for a silently-created bogus option, the same class of trap this change exists to close. The allowlist of writable attributes is derived rather than hardcoded: the subclass snapshots vars(self) once donfig's __init__ returns, so a donfig release that adds or renames an internal field cannot break attribute access here. Today that snapshot is name, env_prefix, env, main_path, paths, defaults, deprecations, config, and config_lock. An instance built without running __init__ has no allowlist yet and stays permissive, so the guard cannot raise from a half-built object. What this does not change: reading options, config.set() as a call or as a context manager, config[name], get(), update(), refresh(), to_dict(), and writes to donfig's own attributes all behave as before. Item assignment (config[name] = value) was never supported by donfig and still raises TypeError. Under the pinned config (--backend suitesparse --blocking --no-mapnumpy), the suite goes from 1073 to 1075 passed by the two tests added here; 141 skipped is unchanged. --- graphblas/__init__.py | 40 +++++++++++++++++++++++++++++++- graphblas/tests/test_core.py | 45 ++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/graphblas/__init__.py b/graphblas/__init__.py index d6a2d6e72..0868a45c8 100644 --- a/graphblas/__init__.py +++ b/graphblas/__init__.py @@ -25,7 +25,45 @@ def get_config(): import donfig import yaml - config = donfig.Config("graphblas") + class Config(donfig.Config): + """donfig Config that rejects silent attribute writes. + + Options are set with ``config.set(name=value)`` (optionally as a + ``with config.set(name=value):`` block) and read with ``config[name]``. + Plain attribute assignment (``config.name = value``) would otherwise + create a dead instance attribute and leave the real option unchanged, + so we raise instead. + """ + + def __init__(self, *args, **kwargs): + # Writes are unrestricted until donfig's own __init__ finishes, and + # the attributes it set there become the allowlist. Deriving that + # list beats hardcoding donfig's internals: a donfig release that + # adds or renames a field can't then break attribute access here. + object.__setattr__(self, "_initializing", True) + super().__init__(*args, **kwargs) + object.__setattr__(self, "_donfig_attrs", frozenset(self.__dict__) - {"_initializing"}) + object.__setattr__(self, "_initializing", False) + + def __setattr__(self, key, value): + # An instance built without running __init__ has no allowlist yet, + # so stay permissive rather than raising a confusing AttributeError + # from the guard. + if self.__dict__.get("_initializing", True) or key in self._donfig_attrs: + object.__setattr__(self, key, value) + return + if key in self.config: + raise AttributeError( + f"Cannot set config option {key!r} by attribute assignment. " + f"Use `graphblas.config.set({key}=...)` to change it (optionally " + f"in a `with` block for a scoped change) and " + f"`graphblas.config[{key!r}]` to read it." + ) + raise AttributeError( + f"Unknown config option {key!r}; known options are {sorted(self.config)}." + ) + + config = Config("graphblas") path = Path(__file__).parent / "graphblas.yaml" with path.open() as f: defaults = yaml.safe_load(f) diff --git a/graphblas/tests/test_core.py b/graphblas/tests/test_core.py index a15da6f54..859c5c21c 100644 --- a/graphblas/tests/test_core.py +++ b/graphblas/tests/test_core.py @@ -94,3 +94,48 @@ def test_packages(): def test_index_max(): assert gb.MAX_SIZE == 2**60 # True for all current backends + + +def test_config_attribute_assignment_raises(): + # Setting an option by plain attribute assignment used to silently no-op: + # donfig has no __setattr__ for options, so it created a dead instance + # attribute and left the real option alone. It should raise and name the + # canonical API instead. + orig = gb.config["autocompute"] + try: + with pytest.raises(AttributeError, match=r"config\.set\(autocompute"): + gb.config.autocompute = not orig + assert gb.config["autocompute"] == orig # the failed write changed nothing + # An unknown name is a mistaken write too, not a new attribute + with pytest.raises(AttributeError, match="not_a_real_option"): + gb.config.not_a_real_option = 5 + assert "not_a_real_option" not in vars(gb.config) + # Canonical idioms still work: read by item, write by set() + gb.config.set(autocompute=not orig) + assert gb.config["autocompute"] == (not orig) + with gb.config.set(autocompute=orig): + assert gb.config["autocompute"] == orig + assert gb.config["autocompute"] == (not orig) # restored on exit + finally: + gb.config.set(autocompute=orig) + + +def test_config_donfig_attrs_are_derived(monkeypatch): + # The allowlist of writable attributes must come from what donfig's + # __init__ actually set, not a hardcoded list. Simulate a future donfig + # that grows a field: the derived allowlist absorbs it, where a + # hardcoded list would start rejecting donfig's own writes. + import donfig + + real_init = donfig.Config.__init__ + + def init_with_extra_field(self, *args, **kwargs): + real_init(self, *args, **kwargs) + self.hypothetical_future_field = 1 + + monkeypatch.setattr(donfig.Config, "__init__", init_with_extra_field) + probe = type(gb.config)("test_derived_allowlist") + assert "hypothetical_future_field" in probe._donfig_attrs + probe.hypothetical_future_field = 2 # writable, not rejected + with pytest.raises(AttributeError, match="autocompute"): + probe.autocompute = False