X golang_str: More fixes for bstr to be accepted as name of an attribute

This time we hit that builtin getattr was rejecting it. Fix it via
patching _PyObject_LookupAttr, that builtin getattr uses, and by adding
tests for this functionality.

Reported by Jérome at nexedi/slapos!1575 (comment 206080)

golang/.gitignore

@@ -6,6 +6,7 @@
 /_fmt_test.cpp
 /_golang.cpp
 /_golang_test.cpp
+/_golang_str_test.cpp
 /_io.cpp
 /_os.cpp
 /_os_test.cpp

golang/_golang_str.pyx

@@ -36,7 +36,8 @@ from cpython cimport PyObject_CheckBuffer
 from cpython cimport Py_TPFLAGS_HAVE_GC, Py_TPFLAGS_HEAPTYPE, Py_TPFLAGS_READY, PyType_Ready
 from cpython cimport Py_TPFLAGS_VALID_VERSION_TAG
 from cpython cimport PyBytes_Format, PyUnicode_Format, PyObject_Str
-from cpython cimport PyObject_GetAttr, PyObject_SetAttr
+from cpython cimport PyObject_GetAttr, PyObject_SetAttr, PyObject_HasAttr
+from cpython cimport PyBytes_Check
 
 cdef extern from "Python.h":
     PyTypeObject PyBytes_Type
@@ -1997,34 +1998,64 @@ cdef  _patch_capi_object_str():
 
 # XXX place, comments, test
 # on py3 PyObject_GetAttr & co insist on name to be unicode
-# XXX _PyObject_LookupAttr
 # XXX _PyObject_GenericGetAttrWithDict
 # XXX _PyObject_GenericSetAttrWithDict
 # XXX type_getattro
 IF PY3:
+    cdef extern from "Python.h":
+        int _PyObject_LookupAttr(object obj, object attr, PyObject** pres) except -1
+
     ctypedef object obj_getattr_func(object, object)
     ctypedef int    obj_setattr_func(object, object, object) except -1
-
-    cdef obj_getattr_func* _pobject_GetAttr = PyObject_GetAttr
-    cdef obj_setattr_func* _pobject_SetAttr = PyObject_SetAttr
+    #               delattr is implemented via setattr(v=NULL)
+    ctypedef bint   obj_hasattr_func(object, object) # no except
+    ctypedef int    obj_lookupattr_func(object, object, PyObject**) except -1
+
+    cdef obj_getattr_func*      _pobject_GetAttr    = PyObject_GetAttr
+    cdef obj_setattr_func*      _pobject_SetAttr    = PyObject_SetAttr
+    cdef obj_hasattr_func*      _pobject_HasAttr    = PyObject_HasAttr
+    cdef obj_lookupattr_func*   _pobject_LookupAttr = _PyObject_LookupAttr
+
+    # isbstr returns whether obj is bstr instance or not.
+    # it avoids going to isinstance unless really needed because isinstance,
+    # internally, uses _PyObject_LookupAttr and we need to patch that function
+    # with using isbstr in the hook.
+    cdef bint isbstr(obj) except -1:
+        if not PyBytes_Check(obj):
+            return False
+        if Py_TYPE(obj) == <PyTypeObject*>pybstr:
+            return True
+        # it might be also a pybstr subclass
+        return isinstance(obj, pybstr)
 
     cdef object _object_xGetAttr(object obj, object name):
-#       fprintf(stderr, "xgetattr...\n")
-        if isinstance(name, pybstr):
+        if isbstr(name):
             name = pyustr(name)
         return _pobject_GetAttr(obj, name)
 
-    cdef int    _object_xSetAttr(object obj, object name, object v) except -1:
-#       fprintf(stderr, "xsetattr...\n")
-        if isinstance(name, pybstr):
+    cdef int    _object_xSetAttr(object obj, object name, object v) except -1:  # XXX v=NULL on del
+        if isbstr(name):
             name = pyustr(name)
         return _pobject_SetAttr(obj, name, v)
 
+    cdef bint   _object_xHasAttr(object obj, object name): # no except
+        if isbstr(name):
+            name = pyustr(name)
+        return _pobject_HasAttr(obj, name)
+
+
+    cdef int    _object_xLookupAttr(object obj, object name, PyObject** pres) except -1:
+        if isbstr(name):
+            name = pyustr(name)
+        return _pobject_LookupAttr(obj, name, pres)
+
 
 cdef _patch_capi_object_attr_bstr():
     IF PY3:
-        cpatch(<void**>&_pobject_GetAttr, <void*>_object_xGetAttr)
-        cpatch(<void**>&_pobject_SetAttr, <void*>_object_xSetAttr)
+        cpatch(<void**>&_pobject_GetAttr,       <void*>_object_xGetAttr)
+        cpatch(<void**>&_pobject_SetAttr,       <void*>_object_xSetAttr)
+        cpatch(<void**>&_pobject_HasAttr,       <void*>_object_xHasAttr)
+        cpatch(<void**>&_pobject_LookupAttr,    <void*>_object_xLookupAttr)
 
 
 # ---- misc ----
@@ -2397,7 +2428,7 @@ cdef _patch_str():
 
     _patch_capi_str_format()
     _patch_capi_object_str()
-    _patch_capi_object_attr_bstr()
+    _patch_capi_object_attr_bstr()  # XXX activate under plain py as well
     _patch_capi_unicode_decode_as_bstr()
     _patch_str_pickle()
     # ...

golang/_golang_str_test.pyx

+# -*- coding: utf-8 -*-
+# cython: language_level=2
+# distutils: language=c++
+#
+# Copyright (C) 2024  Nexedi SA and Contributors.
+#                     Kirill Smelkov <kirr@nexedi.com>
+#
+# This program is free software: you can Use, Study, Modify and Redistribute
+# it under the terms of the GNU General Public License version 3, or (at your
+# option) any later version, as published by the Free Software Foundation.
+#
+# You can also Link and Combine this program with other software covered by
+# the terms of any of the Free Software licenses or any of the Open Source
+# Initiative approved licenses and Convey the resulting work. Corresponding
+# source of such a combination shall include the source code for all other
+# software used.
+#
+# This program is distributed WITHOUT ANY WARRANTY; without even the implied
+# warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+#
+# See COPYING file for full licensing terms.
+# See https://www.nexedi.com/licensing for rationale and options.
+
+# helpers for golang_str_test.py that need C-level access.
+
+from cpython cimport PyObject_GetAttr, PyObject_SetAttr, PyObject_DelAttr, PyObject_HasAttr
+
+def CPyObject_GetAttr(obj, attr):           return PyObject_GetAttr(obj, attr)
+def CPyObject_SetAttr(obj, attr, v):               PyObject_SetAttr(obj, attr, v)
+def CPyObject_DelAttr(obj, attr):                  PyObject_DelAttr(obj, attr)
+def CPyObject_HasAttr(obj, attr):           return PyObject_HasAttr(obj, attr)
+
+
+IF PY3:
+    cdef extern from "Python.h":
+        int _PyObject_LookupAttr(object obj, object attr, PyObject** pres) except -1
+
+    def CPyObject_LookupAttr(obj, attr):
+        cdef PyObject* res
+        _PyObject_LookupAttr(obj, attr, &res)
+        if res == NULL:
+            raise AttributeError((obj, attr))
+        return <object>res
+
+# XXX +more capi func
+#def CPyObject_GenericGetAttr(obj, attr):    return PyObject_GenericGetAttr(obj, attr)
+#def CPyObject_GenericSetAttr(obj, attr, v): PyObject_GenericSetAttr(obj, attr, v)

golang/_golang_test.pyx

@@ -435,3 +435,8 @@ cdef void _bench_select_nogil__func1(chan[int] ch1, chan[int] ch2, chan[structZ]
             if not ok:
                 done.close()
                 return
+
+
+# ---- strings -----
+
+include "_golang_str_test.pyx"

golang/golang_str_test.py

@@ -26,6 +26,7 @@ from golang._golang import _udata, _bdata
 from golang.gcompat import qq
 from golang.strconv_test import byterange
 from golang.golang_test import readfile, assertDoc, _pyrun, dir_testprog, PIPE
+from golang import _golang_test
 from gpython import _tEarlyStrSubclass
 from pytest import raises, mark, skip
 import sys
@@ -2631,7 +2632,7 @@ def test_strings_patched_transparently():
 #
 # XXX note !gpystr_only ...
 # XXX also test bytes?
-def tests_strings_early_str_subclass():
+def test_strings_early_str_subclass():
     xstr = _tEarlyStrSubclass
 
     # .tp_new should be adjusted to point to current str
@@ -2664,6 +2665,62 @@ def tests_strings_early_str_subclass():
     # XXX more...
 
 
+# verify that all string types are accepted by getattr/setattr/delattr/hasattr & co.
+@mark.parametrize('tx', (str, bstr, ustr))
+def test_strings_wrt_xxxattr(tx):
+    x = xstr(u'мир', tx)
+    assert type(x) is tx
+
+    class C: pass
+    obj = C()
+
+    t = _golang_test
+    vgetattr = [getattr, t.CPyObject_GetAttr] + [t.CPyObject_LookupAttr] if six.PY3  else []
+    vsetattr = [setattr, t.CPyObject_SetAttr]
+    vdelattr = [delattr, t.CPyObject_DelAttr]
+    vhasattr = [hasattr, t.CPyObject_HasAttr]
+
+    value = object()
+
+    # run runs f on each element of v.
+    def run(f, v):
+        for e in v:
+            f(e)
+
+    # attr is initially missing
+    def _(ga):
+        with raises(AttributeError): ga(obj, x)
+    run(_, vgetattr)
+
+    def _(ha):
+        assert ha(obj, x) is False
+    run(_, vhasattr)
+
+    def _(da):
+        with raises(AttributeError): da(obj, x)
+    run(_, vdelattr)
+
+    # set attr -> make sure it is there -> del
+    for sa in vsetattr:
+        for da in vdelattr:
+            def _(ha):
+                assert ha(obj, x) is False
+            run(_, vhasattr)
+            sa(obj, x, value)
+            def _(ha):
+                assert ha(obj, x) is True
+            run(_, vhasattr)
+            def _(ga):
+                assert ga(obj, x) is value
+            da(obj, x)
+            def _(ha):
+                assert ha(obj, x) is False
+            run(_, vhasattr)
+            def _(ga):
+                with raises(AttributeError): ga(obj, x)
+            run(_, vgetattr)
+
+
 # ---- issues hit by users ----
 # fixes for below issues have their corresponding tests in the main part above, but
 # we also add tests with original code where problems were hit.

setup.py

@@ -510,7 +510,9 @@ setup(
                     Ext('golang._golang_test',
                         ['golang/_golang_test.pyx',
                          'golang/runtime/libgolang_test_c.c',
-                         'golang/runtime/libgolang_test.cpp']),
+                         'golang/runtime/libgolang_test.cpp'],
+                        depends = [
+                         'golang/_golang_str_test.pyx']),
 
                     Ext('golang.pyx._runtime_test',
                         ['golang/pyx/_runtime_test.pyx'],