Repository: cython/cython
Pull request: [RFC] Enable overloaded extern static methods (GH #1851)
Closes #1851

Not sure if the approach I'm taking here is the best, but I tracked the issue down to overloads not being properly accounted for in `AttributeNode.analyse_as_type_attribute`.

Problem statement:
Multiple static functions, with same name and different parameters in a class will only expose one
Example:

c++:
```c++
class SqBufferIoStream : public SqIoStream {
public:
	static std::shared_ptr<SqBufferIoStream> Open(const std::string& data);
	static std::shared_ptr<SqBufferIoStream> Open();
};
```


cython:
```python
cdef extern from "SqBufferIoStream.h":
    cdef cppclass SqBufferIoStream:
        @staticmethod
        shared_ptr[SqBufferIoStream] Open(const string s)
        @staticmethod
        shared_ptr[SqBufferIoStream] Open()
```

```
Error compiling Cython file:
------------------------------------------------------------
        for command, description in c_map:
            p_map[command] = description
        return p_map

    def executeCommand(self, command, parameters):
        cdef shared_ptr[SqInputStream] b = dynamic_pointer_cast[SqInputStream, SqBufferIoStream](SqBufferIoStream.Open(_json.dumps(parameters)))
                                                                                                                     ^
------------------------------------------------------------

SqFoundationTypes.pyx:200:118: Call with wrong number of arguments (expected 0, got 1)
```

If I change to:
```python 
cdef extern from "SqBufferIoStream.h":
    cdef cppclass SqBufferIoStream:
        @staticmethod
        shared_ptr[SqBufferIoStream] Open()
        @staticmethod
        shared_ptr[SqBufferIoStream] Open(const string s)
```

```
Error compiling Cython file:
------------------------------------------------------------

    def executeCommand(self, command, parameters):
        cdef shared_ptr[SqInputStream] b = dynamic_pointer_cast[SqInputStream, SqBufferIoStream](SqBufferIoStream.Open(_json.dumps(parameters)))
        cdef SqKeyValueList data = SqJsonParser.ParseAsSqValue(b).get().GetKeyValueList()
        cdef SqKeyValueList c_kvl = self.c_this.get().ExecuteCommand(command, data)
        cdef shared_ptr[SqBufferIoStream] buf = SqBufferIoStream.Open()
                                                                    ^
------------------------------------------------------------

SqFoundationTypes.pyx:203:69: Call with wrong number of arguments (expected 1, got 0)
```

Version:
```
cython --version
Cython version 0.25.2
```

Hints from issue discussion:

Commit message: Remove cpp11 tag

Patch to review:
diff --git a/Cython/Compiler/ExprNodes.py b/Cython/Compiler/ExprNodes.py
index 2714d3e4d7a..b3e1e10d5e0 100644
--- a/Cython/Compiler/ExprNodes.py
+++ b/Cython/Compiler/ExprNodes.py
@@ -7012,29 +7012,11 @@ def analyse_as_type_attribute(self, env):
                             return None
                         ubcm_entry = entry
                     else:
-                        # Create a temporary entry describing the C method
-                        # as an ordinary function.
-                        if entry.func_cname and not hasattr(entry.type, 'op_arg_struct'):
-                            cname = entry.func_cname
-                            if entry.type.is_static_method or (
-                                    env.parent_scope and env.parent_scope.is_cpp_class_scope):
-                                ctype = entry.type
-                            elif type.is_cpp_class:
-                                error(self.pos, "%s not a static member of %s" % (entry.name, type))
-                                ctype = PyrexTypes.error_type
-                            else:
-                                # Fix self type.
-                                ctype = copy.copy(entry.type)
-                                ctype.args = ctype.args[:]
-                                ctype.args[0] = PyrexTypes.CFuncTypeArg('self', type, 'self', None)
-                        else:
-                            cname = "%s->%s" % (type.vtabptr_cname, entry.cname)
-                            ctype = entry.type
-                        ubcm_entry = Symtab.Entry(entry.name, cname, ctype)
-                        ubcm_entry.is_cfunction = 1
-                        ubcm_entry.func_cname = entry.func_cname
-                        ubcm_entry.is_unbound_cmethod = 1
-                        ubcm_entry.scope = entry.scope
+                        ubcm_entry = self._create_ubcm_entry(entry, env)
+                        ubcm_entry.overloaded_alternatives = [
+                            self._create_ubcm_entry(overloaded_alternative, env) for overloaded_alternative in
+                            entry.overloaded_alternatives
+                        ]
                     return self.as_name_node(env, ubcm_entry, target=False)
             elif type.is_enum or type.is_cpp_enum:
                 if self.attribute in type.values:
@@ -7047,6 +7029,32 @@ def analyse_as_type_attribute(self, env):
                     error(self.pos, "%s not a known value of %s" % (self.attribute, type))
         return None
 
+    def _create_ubcm_entry(self, entry, env):
+        # Create a temporary entry describing the unbound C method in `entry`
+        # as an ordinary function.
+        if entry.func_cname and not hasattr(entry.type, 'op_arg_struct'):
+            cname = entry.func_cname
+            if entry.type.is_static_method or (
+                    env.parent_scope and env.parent_scope.is_cpp_class_scope):
+                ctype = entry.type
+            elif type.is_cpp_class:
+                error(self.pos, "%s not a static member of %s" % (entry.name, type))
+                ctype = PyrexTypes.error_type
+            else:
+                # Fix self type.
+                ctype = copy.copy(entry.type)
+                ctype.args = ctype.args[:]
+                ctype.args[0] = PyrexTypes.CFuncTypeArg('self', type, 'self', None)
+        else:
+            cname = "%s->%s" % (type.vtabptr_cname, entry.cname)
+            ctype = entry.type
+        ubcm_entry = Symtab.Entry(entry.name, cname, ctype)
+        ubcm_entry.is_cfunction = 1
+        ubcm_entry.func_cname = entry.func_cname
+        ubcm_entry.is_unbound_cmethod = 1
+        ubcm_entry.scope = entry.scope
+        return ubcm_entry
+
     def analyse_as_type(self, env):
         module_scope = self.obj.analyse_as_module(env)
         if module_scope:
diff --git a/tests/run/cpp_static_method_overload.pyx b/tests/run/cpp_static_method_overload.pyx
new file mode 100644
index 00000000000..25ce5bbb4d2
--- /dev/null
+++ b/tests/run/cpp_static_method_overload.pyx
@@ -0,0 +1,49 @@
+# mode: run
+# tag: cpp
+
+cdef extern from *:
+    """
+    struct Foo
+    {
+
+      static const char* bar(int x, int y) {
+        return "second";
+      }
+
+      static const char* bar(int x) {
+        return "first";
+      }
+
+      const char* baz(int x, int y) {
+        return "second";
+      }
+
+      const char* baz(int x) {
+        return "first";
+      }
+    };
+    """
+    cppclass Foo:
+        @staticmethod
+        const char* bar(int x)
+
+        @staticmethod
+        const char* bar(int x, int y)
+
+        const char* baz(int x)
+        const char* baz(int x, int y)
+
+def test_normal_method_overload():
+    """
+    >>> test_normal_method_overload()
+    """
+    cdef Foo f
+    assert f.baz(1) == b"first"
+    assert f.baz(1, 2) == b"second"
+
+def test_static_method_overload():
+    """
+    >>> test_static_method_overload()
+    """
+    assert Foo.bar(1) == b"first"
+    assert Foo.bar(1, 2) == b"second"


Code review comments:
diff_hunk: @@ -7012,29 +7012,11 @@ def analyse_as_type_attribute(self, env):
                             return None
                         ubcm_entry = entry
                     else:
-                        # Create a temporary entry describing the C method
-                        # as an ordinary function.
-                        if entry.func_cname and not hasattr(entry.type, 'op_arg_struct'):
-                            cname = entry.func_cname
-                            if entry.type.is_static_method or (
-                                    env.parent_scope and env.parent_scope.is_cpp_class_scope):
-                                ctype = entry.type
-                            elif type.is_cpp_class:
-                                error(self.pos, "%s not a static member of %s" % (entry.name, type))
-                                ctype = PyrexTypes.error_type
-                            else:
-                                # Fix self type.
-                                ctype = copy.copy(entry.type)
-                                ctype.args = ctype.args[:]
-                                ctype.args[0] = PyrexTypes.CFuncTypeArg('self', type, 'self', None)
-                        else:
-                            cname = "%s->%s" % (type.vtabptr_cname, entry.cname)
-                            ctype = entry.type
-                        ubcm_entry = Symtab.Entry(entry.name, cname, ctype)
-                        ubcm_entry.is_cfunction = 1
-                        ubcm_entry.func_cname = entry.func_cname
-                        ubcm_entry.is_unbound_cmethod = 1
-                        ubcm_entry.scope = entry.scope
+                        ubcm_entry = self._create_ubcm_entry(entry, env)
+                        ubcm_entry.overloaded_alternatives = [
+                            self._create_ubcm_entry(overloaded_alternative, env) for overloaded_alternative in
+                            entry.overloaded_alternatives
path: Cython/Compiler/ExprNodes.py
text: @user1:
I find this more readable since it visibly separates the target item from the loop that creates it. Especially for complex item code, easily spotting the `for` is crucial.
```suggestion
                            self._create_ubcm_entry(overloaded_alternative, env)
                            for overloaded_alternative in entry.overloaded_alternatives
```

diff_hunk: @@ -7047,6 +7029,32 @@ def analyse_as_type_attribute(self, env):
                     error(self.pos, "%s not a known value of %s" % (self.attribute, type))
         return None
 
+    def _create_ubcm_entry(self, entry, env):
+        # Create a temporary entry describing the unbound C method in `entry`
+        # as an ordinary function.
+        if entry.func_cname and not hasattr(entry.type, 'op_arg_struct'):
path: Cython/Compiler/ExprNodes.py
text: @user1:
I'm aware that you only copied this code, but `hasattr()` has a code smell. If we need to test whether this attribute exists in order to do something generic, maybe it should just always exist? And possibly just be `None` by default? Conditionally setting an attribute that would otherwise not exist breaks assumptions about object types/classes.

@author:
I replaced the `hasattr()` with a check against `None`, and added a class attribute `op_args_struct` to `CFuncType`. I don't believe there any other instances of doing `hasattr(..., "op_arg_struct")`, and CI seems to have passed.