diff --git a/src/spatial_graph/_graph/graph_base.py b/src/spatial_graph/_graph/graph_base.py index 9dd44c6..30c2a3e 100644 --- a/src/spatial_graph/_graph/graph_base.py +++ b/src/spatial_graph/_graph/graph_base.py @@ -33,6 +33,33 @@ SRC_DIR = Path(__file__).parent +def _cpp_member_name(name: str) -> str: + """C++ struct member name for a user-provided node/edge attribute. + + User attribute names are used verbatim as the Python-facing API (property + names, ``get_*``/``set_*`` methods), but they cannot be used directly as + C++ identifiers: a name like ``int16`` collides with the MSVC built-in + ``__int16`` (Microsoft extensions treat ``int8``/``int16``/``int32``/ + ``int64`` *and* their single-underscore forms ``_int8``/... as synonyms for + the ``__intN`` keywords), and others may clash with C++ keywords (``class``, + ``new``, ...). We therefore mangle the C++ member with a fixed prefix and + alias it back to the user name on the Cython side, so the public API is + unchanged while the generated C++ always compiles. The prefix matters: a + bare ``_`` prefix would turn ``int8`` into ``_int8``, which is itself an + MSVC keyword, so the prefix must not be ``_`` followed by a digit. + """ + return "_attr_" + name + + +def _cpp_arg_name(name: str) -> str: + """C++ constructor parameter name for a user-provided attribute. + + Must be distinct from :func:`_cpp_member_name` (the constructor initializes + ``member(arg)``) and, like it, must avoid the MSVC ``_intN`` keyword forms. + """ + return "_arg_" + name + + def _build_wrapper( node_dtype: str, node_attr_dtypes: Mapping[str, str] | None = None, @@ -60,6 +87,8 @@ def _build_wrapper( name: DType(dtype) for name, dtype in edge_attr_dtypes.items() } wrapper_template.directed = directed + wrapper_template.cpp_member = _cpp_member_name + wrapper_template.cpp_arg = _cpp_arg_name return str(wrapper_template) diff --git a/src/spatial_graph/_graph/wrapper_template.pyx b/src/spatial_graph/_graph/wrapper_template.pyx index e833e13..fc663de 100644 --- a/src/spatial_graph/_graph/wrapper_template.pyx +++ b/src/spatial_graph/_graph/wrapper_template.pyx @@ -95,7 +95,7 @@ cdef extern from *: ${class_name}( %set $sep="" %for name, dtype in $dtypes.items() - $sep$dtype.to_c_decl("_" + $name) + $sep$dtype.to_c_decl($cpp_arg(name)) %set $sep=", " %end for ) : @@ -103,15 +103,15 @@ cdef extern from *: %for name, dtype in $dtypes.items() $sep %if $dtype.is_array - ${name}{ + ${cpp_member(name)}{ %set $isep="" %for i in range($dtype.size) - ${isep}_${name}[$i] + ${isep}${cpp_arg(name)}[$i] %set $isep=", " %end for } %else - ${name}(_$name)%slurp + ${cpp_member(name)}(${cpp_arg(name)})%slurp %end if %set $sep=", " %end for @@ -119,7 +119,7 @@ cdef extern from *: %end if %for name, dtype in $dtypes.items() - $dtype.to_c_decl($name); + $dtype.to_c_decl($cpp_member(name)); %end for }; """ @@ -129,13 +129,13 @@ cdef extern from *: ${class_name}( %set $sep="" %for name, dtype in $dtypes.items() - $sep$dtype.to_c_decl("_" + $name) + $sep$dtype.to_c_decl($cpp_arg(name)) %set $sep=", " %end for ) except + %for name, dtype in $dtypes.items() - $dtype.to_pyxtype() $name + $dtype.to_pyxtype() $name "$cpp_member(name)" %end for cdef class ${class_name}View: diff --git a/tests/test_attributes.py b/tests/test_attributes.py index 2fc954b..89f7664 100644 --- a/tests/test_attributes.py +++ b/tests/test_attributes.py @@ -95,3 +95,58 @@ def test_attr_dtypes(dtype): graph.add_node(1, position=np.array([0.0, 0.0, 0.0]), **{f"node_attr_{dtype}": 0}) graph.add_node(2, position=np.array([0.0, 0.0, 0.0]), **{f"node_attr_{dtype}": 1}) graph.add_edge([1, 2], **{f"edge_attr_{dtype}": 0}) + + +# Attribute names that collide with C++ keywords or MSVC built-in type +# specifiers (but are not Python keywords, so they are valid attribute names). +# These cannot appear verbatim as C++ struct members or constructor parameters. +# In particular MSVC (with Microsoft extensions) treats ``int8``/``int16``/ +# ``int32``/``int64`` *and* their single-underscore forms ``_int8``/... as +# aliases for the ``__intN`` keywords, so a member named ``int16`` (or a +# constructor parameter ``_int16``) expands to ``__int16`` and fails to compile. +@pytest.mark.parametrize( + "attr_name", ["int8", "int16", "int32", "int64", "new", "double"] +) +def test_attr_name_collides_with_cpp_keyword(attr_name): + graph = sg.SpatialGraph( + ndims=3, + node_dtype="uint64", + node_attr_dtypes={attr_name: "int16", "position": "double[3]"}, + edge_attr_dtypes={attr_name: "int16"}, + position_attr="position", + ) + + graph.add_node(1, position=np.array([1.0, 1.0, 1.0]), **{attr_name: 5}) + graph.add_node(2, position=np.array([2.0, 2.0, 2.0]), **{attr_name: 7}) + graph.add_edge([1, 2], **{attr_name: 9}) + + # The user-provided name stays the public attribute (the colliding name only + # affects the internal C++ identifier). Reading it back through every public + # access path must return the stored value -- a regression in the C++ member + # mangling / Cython cname-aliasing would silently corrupt these. + nodes = np.array([1, 2], dtype="uint64") + + # single node + assert getattr(graph.node_attrs[1], attr_name) == 5 + assert getattr(graph.node_attrs[2], attr_name) == 7 + # array of nodes + np.testing.assert_array_equal( + getattr(graph.node_attrs[nodes], attr_name), np.array([5, 7], dtype="int16") + ) + # all nodes (order is unspecified, so compare sorted) + np.testing.assert_array_equal( + np.sort(getattr(graph.node_attrs, attr_name)), np.array([5, 7], dtype="int16") + ) + # edge + assert getattr(graph.edge_attrs[(1, 2)], attr_name) == 9 + + # the position attribute is mangled the same way and must be unaffected + np.testing.assert_array_equal( + graph.node_attrs[1].position, np.array([1.0, 1.0, 1.0]) + ) + + # setting through the public attribute round-trips too + setattr(graph.node_attrs[1], attr_name, 11) + assert getattr(graph.node_attrs[1], attr_name) == 11 + setattr(graph.edge_attrs[(1, 2)], attr_name, 13) + assert getattr(graph.edge_attrs[(1, 2)], attr_name) == 13