diff --git a/graphblas/core/base.py b/graphblas/core/base.py index b6cddc124..920ca98ac 100644 --- a/graphblas/core/base.py +++ b/graphblas/core/base.py @@ -171,7 +171,7 @@ def _expect_op(self, op, values, *, within, **kwargs): AmbiguousAssignOrExtract._expect_type = _expect_type -def _check_mask(mask, output=None): +def _check_mask(mask, output=None, strict_kind=False): if not isinstance(mask, Mask): # Convert bool objects to value masks if output_type(mask).__name__ in {"Vector", "Matrix"}: @@ -183,17 +183,48 @@ def _check_mask(mask, output=None): mask = mask.V # auto-compute (will raise if disabled) else: raise TypeError(f"Invalid mask: {type(mask)}") - if output is not None and output.ndim == 1 and mask.parent.ndim != 1: - raise TypeError(f"Mask object must be type Vector; got {type(mask.parent)}") + if output is not None: + if output.ndim == 1 and mask.parent.ndim != 1: + raise TypeError(f"Mask object must be type Vector; got {type(mask.parent)}") + # A full-tensor op (ewise, mxm, apply, extract, ...) into a Matrix needs + # a Matrix mask. Assignment is exempt (`strict_kind` stays False for it): + # a Vector mask on a Matrix row/column assign is valid and is validated + # separately in Matrix.__setitem__. Without this, a Vector mask on a + # full-Matrix op leaked a raw cffi "struct GB_Matrix_opaque" error. + if strict_kind and output.ndim == 2 and mask.parent.ndim != 2: + raise TypeError(f"Mask object must be type Matrix; got {type(mask.parent)}") return mask +# Curated hints for common attribute-access mistakes on Vector/Matrix/Scalar. +# Only consulted from __getattr__, which fires solely on a genuine attribute +# miss, so the normal (slotted) attribute hot path is untouched. +_INSTANCE_ATTR_HINTS = { + # `.new()` resolves expressions (e.g. `A.mxm(B).new()`); a concrete + # object is copied with `.dup()`. + "new": ( + "`.new()` resolves an expression (e.g. `A.mxm(B).new()`); a concrete " + "object has no `.new()`. Use `.dup()` to copy this object." + ), + # transpose is the `.T` property, not a method. + "transpose": "transpose is the `.T` property, e.g. `A.T` (not a method call).", +} + + class BaseType: # pylint: disable=assigning-non-slot __slots__ = "gb_obj", "dtype", "name", "__weakref__" # Flag for operations which depend on scalar vs vector/matrix _is_scalar = False + def __getattr__(self, name): + # Fires only on a genuine attribute miss (slots/methods resolve first), + # so this is free on the hot path. Adds hints for common mistakes. + base = f"{type(self).__name__!r} object has no attribute {name!r}" + if (hint := _INSTANCE_ATTR_HINTS.get(name)) is not None: + raise AttributeError(f"{base}; {hint}") + raise AttributeError(base) + def __call__( self, *optional_mask_accum_replace, @@ -465,7 +496,11 @@ def _update(self, expr, mask=None, accum=None, replace=False, input_mask=None, * complement = False structure = False else: - mask = _check_mask(mask, self) + # Assignment (`method_name == "__setitem__"`) may target a Matrix + # row/column with a Vector mask, so only enforce the strict + # mask-kind match for full-tensor operations. + strict_kind = expr.method_name != "__setitem__" + mask = _check_mask(mask, self, strict_kind=strict_kind) complement = mask.complement structure = mask.structure @@ -616,7 +651,9 @@ def _new(self, dtype, mask, name, is_cscalar=None, **opts): elif mask is None: output.update(self, **opts) else: - mask = _check_mask(mask, output) + # `.new()` always builds a full output matching this expression, so + # the mask kind must match the output dimensions. + mask = _check_mask(mask, output, strict_kind=True) output(mask=mask, **opts).update(self) return output diff --git a/graphblas/core/dtypes.py b/graphblas/core/dtypes.py index 8eb09725b..086ce7abf 100644 --- a/graphblas/core/dtypes.py +++ b/graphblas/core/dtypes.py @@ -549,6 +549,27 @@ def lookup_dtype(key, value=None): raise ValueError(f"Unknown dtype: {key} of type {type(key)}") +def _raise_dtype_or_arraylike(cls_name, dtype, exc): + """Turn a failed constructor dtype lookup into a helpful error. + + Call from ``Vector``/``Matrix`` after ``lookup_dtype(dtype)`` has already + raised ``exc``. The constructors take a dtype as the first argument, so a + common mistake is to pass the data instead (``Vector([1, 2, 3])``). When + ``dtype`` is array-like data rather than a dtype spec, point at the ``from_*`` + constructors; otherwise re-raise the original ``Unknown dtype`` error. Valid + list/tuple dtype specs (structured and subarray dtypes) never reach here + because ``lookup_dtype`` accepts them. + """ + if isinstance(dtype, (list, tuple, np.ndarray)): + raise TypeError( + f"{cls_name}() expects a dtype as the first argument, not " + f"{type(dtype).__name__} data. To build a {cls_name} from existing " + f"values, use a constructor such as {cls_name}.from_coo(...) or " + f"{cls_name}.from_dense(...)." + ) from None + raise exc + + def unify(type1, type2, *, is_left_scalar=False, is_right_scalar=False): """Returns a type that can hold both type1 and type2. diff --git a/graphblas/core/matrix.py b/graphblas/core/matrix.py index 21aea7212..a46423806 100644 --- a/graphblas/core/matrix.py +++ b/graphblas/core/matrix.py @@ -16,6 +16,7 @@ from . import _supports_udfs, automethods, ffi, lib, utils from .base import BaseExpression, BaseType, _check_mask, _is_recording, call from .descriptor import lookup as descriptor_lookup +from .dtypes import _raise_dtype_or_arraylike from .expr import _ALL_INDICES, AmbiguousAssignOrExtract, IndexerResolver, InfixExprBase, Updater from .mask import Mask, StructuralMask, ValueMask from .operator import ( @@ -196,7 +197,10 @@ class Matrix(BaseType): def __new__(cls, dtype=FP64, nrows=0, ncols=0, *, name=None): self = object.__new__(cls) - self.dtype = lookup_dtype(dtype) + try: + self.dtype = lookup_dtype(dtype) + except (ValueError, TypeError) as exc: + _raise_dtype_or_arraylike("Matrix", dtype, exc) nrows = _as_scalar(nrows, _INDEX, is_cscalar=True) ncols = _as_scalar(ncols, _INDEX, is_cscalar=True) self.name = f"M_{next(Matrix._name_counter)}" if name is None else name diff --git a/graphblas/core/vector.py b/graphblas/core/vector.py index 16647478b..7db694258 100644 --- a/graphblas/core/vector.py +++ b/graphblas/core/vector.py @@ -8,6 +8,7 @@ from . import _supports_udfs, automethods, ffi, lib, utils from .base import BaseExpression, BaseType, _check_mask, _is_recording, call from .descriptor import lookup as descriptor_lookup +from .dtypes import _raise_dtype_or_arraylike from .expr import _ALL_INDICES, AmbiguousAssignOrExtract, IndexerResolver, InfixExprBase, Updater from .mask import Mask, StructuralMask, ValueMask from .operator import ( @@ -158,7 +159,10 @@ class Vector(BaseType): def __new__(cls, dtype=FP64, size=0, *, name=None): self = object.__new__(cls) - self.dtype = lookup_dtype(dtype) + try: + self.dtype = lookup_dtype(dtype) + except (ValueError, TypeError) as exc: + _raise_dtype_or_arraylike("Vector", dtype, exc) size = _as_scalar(size, _INDEX, is_cscalar=True) self.name = f"v_{next(Vector._name_counter)}" if name is None else name self.gb_obj = ffi_new("GrB_Vector*") diff --git a/graphblas/tests/test_matrix.py b/graphblas/tests/test_matrix.py index 59babaa17..5d23d853b 100644 --- a/graphblas/tests/test_matrix.py +++ b/graphblas/tests/test_matrix.py @@ -2954,6 +2954,7 @@ def test_expr_is_like_matrix(A): "__call__", "__del__", "__delitem__", + "__getattr__", "__lshift__", "__setitem__", "_assign_element", @@ -3020,6 +3021,7 @@ def test_index_expr_is_like_matrix(A): expected = { "__del__", "__delitem__", + "__getattr__", "__setitem__", "_assign_element", "_delete_element", @@ -4570,3 +4572,97 @@ def test_setdiag(): A.setdiag(30, mask=v.S) expected[0, 0] = 30 assert A.isequal(expected) + + +def test_constructor_rejects_arraylike_first_arg(): + # The first positional arg is the dtype; passing data instead used to raise a + # confusing "Unknown dtype" ValueError. It should now raise TypeError pointing + # at the from_* constructors. + with pytest.raises(TypeError, match="Matrix.*dtype.*from_coo.*from_dense"): + Matrix([[1, 2], [3, 4]]) + with pytest.raises(TypeError, match="Matrix.*expects a dtype"): + Matrix(np.zeros((2, 2))) + # Valid dtype-first signatures still work, including list/tuple dtype specs + assert Matrix(int, 2, 2).dtype == dtypes.INT64 + assert Matrix("INT64", nrows=2, ncols=2).dtype == dtypes.INT64 + assert Matrix([("x", "i8"), ("y", "f8")], nrows=2, ncols=2).dtype._is_udt + assert Matrix((np.int32, (2, 2)), nrows=2, ncols=2).dtype._is_udt + # A non-array-like bad dtype keeps the original ValueError + with pytest.raises(ValueError, match="Unknown dtype"): + Matrix("not_a_dtype", nrows=2, ncols=2) + + +def test_wrong_kind_mask_on_matrix_raises(): + # A Vector mask on a Matrix output used to leak a raw cffi/C-signature + # error ("initializer for ctype 'struct GB_Matrix_opaque'"). It should + # now raise a clear TypeError, mirroring the Vector-output guard. + A = Matrix(int, 3, 3) + A[0, 0] = 1 + v = Vector(int, 3) + v[0] = 1 + # update path (full-matrix op with a Vector mask) + C = Matrix(int, 3, 3) + with pytest.raises(TypeError, match="Mask object must be type Matrix"): + C(mask=v.S) << A.ewise_mult(A) + # extract path + with pytest.raises(TypeError, match="Mask object must be type Matrix"): + A[:, :].new(mask=v.S) + # Valid Matrix mask on Matrix output still works + C = Matrix(int, 3, 3) + C(mask=A.S) << A.ewise_mult(A) + assert C.nvals == A.nvals + # Valid Vector input_mask broadcast on a Matrix extract still works + m = Vector(bool, 3) + m[0] = True + m[2] = True + assert A[0, [0, 1, 2]].new(input_mask=m.S) is not None + + +def test_new_constructor_misuse_hint(): + # `.new()` resolves expressions; it is not a constructor or an instance + # method on concrete objects. Both misuses should hint the right API. + A = Matrix(int, 3, 3) + v = Vector(int, 3) + s = Scalar.from_value(5) + # Class-level access stays a plain AttributeError: hinting it would need + # a metaclass, and both metaclass choices break something (see + # test_abc_mixin_subclass). + for cls in (Matrix, Vector, Scalar): + with pytest.raises(AttributeError, match="has no attribute 'new'"): + cls.new + # Instance-level misuse: A.new() -> hint .dup() + for obj in (A, v, s): + with pytest.raises(AttributeError, match=r"has no attribute 'new'.*\.dup\(\)"): + obj.new + # A genuinely-missing attribute keeps the plain AttributeError (no hint) + with pytest.raises(AttributeError, match="has no attribute 'frobnicate'"): + A.frobnicate + assert not hasattr(A, "new") + assert not hasattr(Matrix, "new") + + +def test_abc_mixin_subclass(): + # BaseType must stay metaclass-free: a plain `type` hint metaclass broke + # abc-based mixins with "metaclass conflict", and an ABCMeta-derived one + # leaks metaclass attributes into dir(Matrix), which the + # expression-surface guards report as drift. + import collections.abc + + class SizedMatrix(Matrix, collections.abc.Sized): + def __len__(self): + return 1 + + assert issubclass(SizedMatrix, collections.abc.Sized) + + +def test_transpose_hint(): + # A.transpose() should point at the .T property rather than fail with a + # bare "no attribute" message; transpose is not a method here. + A = Matrix(int, 3, 3) + v = Vector(int, 3) + with pytest.raises(AttributeError, match=r"has no attribute 'transpose'.*\.T"): + A.transpose + with pytest.raises(AttributeError, match=r"has no attribute 'transpose'.*\.T"): + v.transpose + # .T still works + assert A.T.shape == (3, 3) diff --git a/graphblas/tests/test_scalar.py b/graphblas/tests/test_scalar.py index 9d8a5b340..ac45a37f1 100644 --- a/graphblas/tests/test_scalar.py +++ b/graphblas/tests/test_scalar.py @@ -429,6 +429,7 @@ def test_expr_is_like_scalar(s): expected = { "__call__", "__del__", + "__getattr__", "__imatmul__", "__lshift__", "_carg", @@ -473,6 +474,7 @@ def test_index_expr_is_like_scalar(s): # Should we make any of these raise informative errors? expected = { "__del__", + "__getattr__", "__imatmul__", "_carg", "_deserialize", diff --git a/graphblas/tests/test_vector.py b/graphblas/tests/test_vector.py index 52068546d..d95ef386e 100644 --- a/graphblas/tests/test_vector.py +++ b/graphblas/tests/test_vector.py @@ -1682,6 +1682,7 @@ def test_expr_is_like_vector(v): "__call__", "__del__", "__delitem__", + "__getattr__", "__lshift__", "__setitem__", "_assign_element", @@ -1732,6 +1733,7 @@ def test_index_expr_is_like_vector(v): expected = { "__del__", "__delitem__", + "__getattr__", "__setitem__", "_assign_element", "_delete_element", @@ -2712,3 +2714,23 @@ def test_subarray_dtypes(): assert full1.isequal(full2, check_dtype=True) full2 = Vector.ss.import_bitmap(values=a, bitmap=[True, True, True]) assert full1.isequal(full2, check_dtype=True) + + +def test_constructor_rejects_arraylike_first_arg(): + # The first positional arg is the dtype; passing data instead used to raise a + # confusing "Unknown dtype" ValueError. It should now raise TypeError pointing + # at the from_* constructors. + with pytest.raises(TypeError, match="Vector.*dtype.*from_coo.*from_dense"): + Vector([1, 2, 3]) + with pytest.raises(TypeError, match="Vector.*expects a dtype"): + Vector((1, 2, 3)) + with pytest.raises(TypeError, match="Vector.*expects a dtype"): + Vector(np.array([1, 2, 3])) + # Valid dtype-first signatures still work, including list/tuple dtype specs + assert Vector(int, size=3).dtype == dtypes.INT64 + assert Vector("INT64", size=3).dtype == dtypes.INT64 + assert Vector(np.dtype("int64"), size=3).dtype == dtypes.INT64 + assert Vector([("x", "i8"), ("y", "f8")], size=3).dtype._is_udt + # A non-array-like bad dtype keeps the original ValueError + with pytest.raises(ValueError, match="Unknown dtype"): + Vector("not_a_dtype", size=3)