From 724c4b4cfc4bea114b57eaad4f2d3449a4531360 Mon Sep 17 00:00:00 2001 From: Caroline Malin-Mayor Date: Tue, 9 Jun 2026 11:07:29 -0400 Subject: [PATCH 1/4] Add c++ name mangling to avoid keyword collision --- src/spatial_graph/_graph/graph_base.py | 16 ++++++++++++ src/spatial_graph/_graph/wrapper_template.pyx | 8 +++--- tests/test_attributes.py | 26 +++++++++++++++++++ 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/src/spatial_graph/_graph/graph_base.py b/src/spatial_graph/_graph/graph_base.py index 9dd44c6..6c81782 100644 --- a/src/spatial_graph/_graph/graph_base.py +++ b/src/spatial_graph/_graph/graph_base.py @@ -33,6 +33,21 @@ 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 alias ``int8``/``int16``/``int32``/ + ``int64`` to 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. + """ + return "_attr_" + name + + def _build_wrapper( node_dtype: str, node_attr_dtypes: Mapping[str, str] | None = None, @@ -60,6 +75,7 @@ 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 return str(wrapper_template) diff --git a/src/spatial_graph/_graph/wrapper_template.pyx b/src/spatial_graph/_graph/wrapper_template.pyx index e833e13..cb5d0e8 100644 --- a/src/spatial_graph/_graph/wrapper_template.pyx +++ b/src/spatial_graph/_graph/wrapper_template.pyx @@ -103,7 +103,7 @@ 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] @@ -111,7 +111,7 @@ cdef extern from *: %end for } %else - ${name}(_$name)%slurp + ${cpp_member(name)}(_$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 }; """ @@ -135,7 +135,7 @@ cdef extern from *: ) 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..bae8a1f 100644 --- a/tests/test_attributes.py +++ b/tests/test_attributes.py @@ -95,3 +95,29 @@ 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. In particular MSVC (with +# Microsoft extensions) treats ``int8``/``int16``/``int32``/``int64`` as aliases +# for the ``__intN`` keywords, so a member named ``int16`` expands to +# ``int16_t __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([0.0, 0.0, 0.0]), **{attr_name: 5}) + graph.add_node(2, position=np.array([1.0, 1.0, 1.0]), **{attr_name: 7}) + graph.add_edge([1, 2], **{attr_name: 9}) + + # round-trip through the Python-facing API, which keeps the user's name + assert getattr(graph.node_attrs[1], attr_name) == 5 + assert getattr(graph.node_attrs[2], attr_name) == 7 + assert getattr(graph.edge_attrs[(1, 2)], attr_name) == 9 From 13cb3588dc1a17fe54939836eb86cbbbd2babc03 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 9 Jun 2026 15:15:51 +0000 Subject: [PATCH 2/4] style(pre-commit.ci): auto fixes [...] --- tests/test_attributes.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_attributes.py b/tests/test_attributes.py index bae8a1f..c25caf8 100644 --- a/tests/test_attributes.py +++ b/tests/test_attributes.py @@ -103,7 +103,9 @@ def test_attr_dtypes(dtype): # Microsoft extensions) treats ``int8``/``int16``/``int32``/``int64`` as aliases # for the ``__intN`` keywords, so a member named ``int16`` expands to # ``int16_t __int16;`` and fails to compile. -@pytest.mark.parametrize("attr_name", ["int8", "int16", "int32", "int64", "new", "double"]) +@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, From e447908f80e721c41f8d56211a2294d8dadbda4b Mon Sep 17 00:00:00 2001 From: Caroline Malin-Mayor Date: Tue, 9 Jun 2026 11:38:33 -0400 Subject: [PATCH 3/4] also mangle arguments to constructor to avoid more _int8 name collision --- src/spatial_graph/_graph/graph_base.py | 23 +++++++++++++++---- src/spatial_graph/_graph/wrapper_template.pyx | 8 +++---- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/spatial_graph/_graph/graph_base.py b/src/spatial_graph/_graph/graph_base.py index 6c81782..30c2a3e 100644 --- a/src/spatial_graph/_graph/graph_base.py +++ b/src/spatial_graph/_graph/graph_base.py @@ -39,15 +39,27 @@ def _cpp_member_name(name: str) -> str: 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 alias ``int8``/``int16``/``int32``/ - ``int64`` to 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. + ``__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, @@ -76,6 +88,7 @@ def _build_wrapper( } 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 cb5d0e8..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 ) : @@ -106,12 +106,12 @@ cdef extern from *: ${cpp_member(name)}{ %set $isep="" %for i in range($dtype.size) - ${isep}_${name}[$i] + ${isep}${cpp_arg(name)}[$i] %set $isep=", " %end for } %else - ${cpp_member(name)}(_$name)%slurp + ${cpp_member(name)}(${cpp_arg(name)})%slurp %end if %set $sep=", " %end for @@ -129,7 +129,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 ) except + From 9b7a43154fd5740cbe339befd096fecaa346cb22 Mon Sep 17 00:00:00 2001 From: Caroline Malin-Mayor Date: Tue, 9 Jun 2026 11:43:49 -0400 Subject: [PATCH 4/4] Test all public API access of attrs --- tests/test_attributes.py | 41 +++++++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/tests/test_attributes.py b/tests/test_attributes.py index c25caf8..89f7664 100644 --- a/tests/test_attributes.py +++ b/tests/test_attributes.py @@ -99,10 +99,11 @@ def test_attr_dtypes(dtype): # 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. In particular MSVC (with -# Microsoft extensions) treats ``int8``/``int16``/``int32``/``int64`` as aliases -# for the ``__intN`` keywords, so a member named ``int16`` expands to -# ``int16_t __int16;`` and fails to compile. +# 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"] ) @@ -115,11 +116,37 @@ def test_attr_name_collides_with_cpp_keyword(attr_name): position_attr="position", ) - graph.add_node(1, position=np.array([0.0, 0.0, 0.0]), **{attr_name: 5}) - graph.add_node(2, position=np.array([1.0, 1.0, 1.0]), **{attr_name: 7}) + 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}) - # round-trip through the Python-facing API, which keeps the user's name + # 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