[LLDB][Python] remove ArgInfo::count

[lawrence_danna]

Summary

This patch updates the last user of ArgInfo::count and deletes
it. I also delete GetNumInitArguments() and GetInitArgInfo().
Classess are callables and GetArgInfo() should work on them.

On python 3 it already works, of course. inspect is good.

On python 2 we have to add yet another special case. But hey if
python 2 wasn't crufty we wouln't need python 3.

I also delete is_bound_method becuase it is unused.

This path is tested in TestStepScripted.py

[labath]

At this point, should we just delete the ArgInfo struct altogether, and replace it with a plain unsigned?

[JDevlieghere]

LGTM modulo inline comment.

lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

764

Should we check has_varargs here as well?

[lawrence_danna]

lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

764

nope, it's an implementation detail now. not available here.

[JDevlieghere]

lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

764

Makes sense, sorry for the noise!

[lawrence_danna]

In D69742#1732608, @labath wrote:

At this point, should we just delete the ArgInfo struct altogether, and replace it with a plain unsigned?

Sounds like a good idea to me. Maybe I'll do that in a followon patch.

lldb/scripts/Python/python-wrapper.swig

if (!pfunc.IsAllocated()) {
         return nullptr;
     }
 
     PythonObject tp_arg(PyRefType::Owned, SBTypeToSWIGWrapper(tp_value));
 
     if (!tp_arg.IsAllocated())
         Py_RETURN_NONE;
 
+    llvm::Expected<PythonCallable::ArgInfo> arg_info = pfunc.GetArgInfo();
+    if (!arg_info) {
+        llvm::handleAllErrors(
+            arg_info.takeError(),
+            [&](PythonException &E) {
+                error_string.append(E.ReadBacktrace());
+            },
+            [&](const llvm::ErrorInfoBase &E) {
+                error_string.append(E.message());
+            });
+        Py_RETURN_NONE;
+    }
+
     PythonObject result = {};
-    size_t init_num_args = pfunc.GetNumInitArguments().count;
-    if (init_num_args == 3) {
+    if (arg_info.get().max_positional_args == 2) {
         if (args_impl != nullptr) {
            error_string.assign("args passed, but __init__ does not take an args dictionary");
            Py_RETURN_NONE;
         }
         result = pfunc(tp_arg, dict);
-    } else if (init_num_args == 4) {
+    } else if (arg_info.get().max_positional_args >= 3) {
         lldb::SBStructuredData *args_value = new lldb::SBStructuredData(args_impl);
         PythonObject args_arg(PyRefType::Owned, SBTypeToSWIGWrapper(args_value));
         result = pfunc(tp_arg, args_arg, dict);
     } else {
-        error_string.assign("wrong number of arguments in __init__, should be 1 or 2 (not including self & dict)");
+        error_string.assign("wrong number of arguments in __init__, should be 2 or 3 (not including self)");
         Py_RETURN_NONE;
     }
 
     // FIXME: At this point we should check that the class we found supports all the methods
     // that we need.
 
     if (result.IsAllocated())
         return result.release();

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h

public:
   using TypedPythonObject::TypedPythonObject;
 
   struct ArgInfo {
     /* the largest number of positional arguments this callable
      * can accept, or UNBOUNDED, ie UINT_MAX if it's a varargs
      * function and can accept an arbitrary number */
     unsigned max_positional_args;
     static constexpr unsigned UNBOUNDED = UINT_MAX; // FIXME c++17 inline
-    /* the number of positional arguments, including optional ones,
-     * and excluding varargs.  If this is a bound method, then the
-     * count will still include a +1 for self.
-     *
-     * FIXME. That's crazy.  This should be replaced with
-     * an accurate min and max for positional args.
-     */
-    int count;
-    /* does the callable have positional varargs? */
-    bool has_varargs : 1; // FIXME delete this
   };
 
   static bool Check(PyObject *py_obj);
 
   llvm::Expected<ArgInfo> GetArgInfo() const;
 
-  llvm::Expected<ArgInfo> GetInitArgInfo() const;
-
-  ArgInfo GetNumArguments() const; // DEPRECATED
-
-  // If the callable is a Py_Class, then find the number of arguments
-  // of the __init__ method.
-  ArgInfo GetNumInitArguments() const; // DEPRECATED
-
   PythonObject operator()();
 
   PythonObject operator()(std::initializer_list<PyObject *> args);
 
   PythonObject operator()(std::initializer_list<PythonObject> args);
 
   template <typename Arg, typename... Args>
   PythonObject operator()(const Arg &arg, Args... args) {

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp

 
 bool PythonCallable::Check(PyObject *py_obj) {
   if (!py_obj)
     return false;
 
   return PyCallable_Check(py_obj);
 }
 
-PythonCallable::ArgInfo PythonCallable::GetNumInitArguments() const {
-  auto arginfo = GetInitArgInfo();
-  if (!arginfo) {
-    llvm::consumeError(arginfo.takeError());
-    return ArgInfo{};
-  }
-  return arginfo.get();
-}
-
-Expected<PythonCallable::ArgInfo> PythonCallable::GetInitArgInfo() const {
-  if (!IsValid())
-    return nullDeref();
-  auto init = As<PythonCallable>(GetAttribute("__init__"));
-  if (!init)
-    return init.takeError();
-  return init.get().GetArgInfo();
-}
-
 #if PY_MAJOR_VERSION >= 3 && PY_MINOR_VERSION >= 3
 static const char get_arg_info_script[] = R"(
 from inspect import signature, Parameter, ismethod
 from collections import namedtuple
-ArgInfo = namedtuple('ArgInfo', ['count', 'has_varargs', 'is_bound_method'])
+ArgInfo = namedtuple('ArgInfo', ['count', 'has_varargs'])
 def main(f):
     count = 0
     varargs = False
     for parameter in signature(f).parameters.values():
         kind = parameter.kind
         if kind in (Parameter.POSITIONAL_ONLY,
                     Parameter.POSITIONAL_OR_KEYWORD):
             count += 1
         elif kind == Parameter.VAR_POSITIONAL:
             varargs = True
         elif kind in (Parameter.KEYWORD_ONLY,
                       Parameter.VAR_KEYWORD):
             pass
         else:
             raise Exception(f'unknown parameter kind: {kind}')
-    return ArgInfo(count, varargs, ismethod(f))
+    return ArgInfo(count, varargs)
 )";
 #endif
 
 Expected<PythonCallable::ArgInfo> PythonCallable::GetArgInfo() const {
   ArgInfo result = {};
   if (!IsValid())
     return nullDeref();
 
 #if PY_MAJOR_VERSION >= 3 && PY_MINOR_VERSION >= 3
 
   // no need to synchronize access to this global, we already have the GIL
   static PythonScript get_arg_info(get_arg_info_script);
   Expected<PythonObject> pyarginfo = get_arg_info(*this);
   if (!pyarginfo)
     return pyarginfo.takeError();
-  result.count = cantFail(As<long long>(pyarginfo.get().GetAttribute("count")));
-  result.has_varargs =
+  long long count =
+      cantFail(As<long long>(pyarginfo.get().GetAttribute("count")));
+  bool has_varargs =
       cantFail(As<bool>(pyarginfo.get().GetAttribute("has_varargs")));
-  bool is_method =
-      cantFail(As<bool>(pyarginfo.get().GetAttribute("is_bound_method")));
-  result.max_positional_args =
-      result.has_varargs ? ArgInfo::UNBOUNDED : result.count;
-
-  // FIXME emulate old broken behavior
-  if (is_method)
-    result.count++;
+  result.max_positional_args = has_varargs ? ArgInfo::UNBOUNDED : count;
 
 #else
+  PyObject *py_func_obj;
   bool is_bound_method = false;
-  PyObject *py_func_obj = m_py_obj;
+  bool is_class = false;
+
+  if (PyType_Check(m_py_obj) || PyClass_Check(m_py_obj)) {
+    auto init = GetAttribute("__init__");
+    if (!init)
+      return init.takeError();
+    py_func_obj = init.get().get();
+    is_class = true;
+  } else {
+    py_func_obj = m_py_obj;
+  }
+
   if (PyMethod_Check(py_func_obj)) {
     py_func_obj = PyMethod_GET_FUNCTION(py_func_obj);
     PythonObject im_self = GetAttributeValue("im_self");
     if (im_self.IsValid() && !im_self.IsNone())
       is_bound_method = true;
   } else {
     // see if this is a callable object with an __call__ method
     if (!PyFunction_Check(py_func_obj)) {
#else
 
   if (!py_func_obj)
     return result;
 
   PyCodeObject *code = (PyCodeObject *)PyFunction_GET_CODE(py_func_obj);
   if (!code)
     return result;
 
-  result.count = code->co_argcount;
-  result.has_varargs = !!(code->co_flags & CO_VARARGS);
-  result.max_positional_args = result.has_varargs
-                                   ? ArgInfo::UNBOUNDED
-                                   : (result.count - (int)is_bound_method);
+  auto count = code->co_argcount;
+  bool has_varargs = !!(code->co_flags & CO_VARARGS);
+  result.max_positional_args =
+      has_varargs ? ArgInfo::UNBOUNDED
+                  : (count - (int)is_bound_method) - (int)is_class;
 
 #endif
 
   return result;
 }
 
 constexpr unsigned
     PythonCallable::ArgInfo::UNBOUNDED; // FIXME delete after c++17
 
-PythonCallable::ArgInfo PythonCallable::GetNumArguments() const {
-  auto arginfo = GetArgInfo();
-  if (!arginfo) {
-    llvm::consumeError(arginfo.takeError());
-    return ArgInfo{};
-  }
-  return arginfo.get();
-}
-
 PythonObject PythonCallable::operator()() {
   return PythonObject(PyRefType::Owned, PyObject_CallObject(m_py_obj, nullptr));
 }
 
 PythonObject PythonCallable::
 operator()(std::initializer_list<PyObject *> args) {
   PythonTuple arg_tuple(args);
   return PythonObject(PyRefType::Owned,

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

auto dict = PythonModule::MainModule()
       .ResolveName<PythonDictionary>(m_dictionary_name);
   auto pfunc = PythonObject::ResolveNameWithDictionary<PythonCallable>(
       callable_name, dict);
   if (!pfunc.IsAllocated()) {
     return llvm::createStringError(
         llvm::inconvertibleErrorCode(),
         "can't find callable: %s", callable_name.str().c_str());
   }
-  PythonCallable::ArgInfo arg_info = pfunc.GetNumArguments();
-  return arg_info.max_positional_args;
+  llvm::Expected<PythonCallable::ArgInfo> arg_info = pfunc.GetArgInfo();
+  if (!arg_info)
+    return arg_info.takeError();
+  return arg_info.get().max_positional_args;
 }
 
 static std::string GenerateUniqueName(const char *base_name_wanted,
                                       uint32_t &functions_counter,
                                       const void *name_token = nullptr) {
   StreamString sstr;
 
   if (!base_name_wanted)

lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

TEST_F(PythonDataObjectsTest, TestCallable) {
 
   {
     PyObject *o = PyRun_String("lambda x : x", Py_eval_input, globals.get(),
                                globals.get());
     ASSERT_FALSE(o == NULL);
     auto lambda = Take<PythonCallable>(o);
     auto arginfo = lambda.GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
-    EXPECT_EQ(arginfo.get().count, 1);
     EXPECT_EQ(arginfo.get().max_positional_args, 1u);
-    EXPECT_EQ(arginfo.get().has_varargs, false);
   }
 
   {
     PyObject *o = PyRun_String("lambda x,y=0: x", Py_eval_input, globals.get(),
                                globals.get());
     ASSERT_FALSE(o == NULL);
     auto lambda = Take<PythonCallable>(o);
     auto arginfo = lambda.GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
-    EXPECT_EQ(arginfo.get().count, 2);
     EXPECT_EQ(arginfo.get().max_positional_args, 2u);
-    EXPECT_EQ(arginfo.get().has_varargs, false);
   }
 
   {
     PyObject *o = PyRun_String("lambda x,y=0, **kw: x", Py_eval_input,
                                globals.get(), globals.get());
     ASSERT_FALSE(o == NULL);
     auto lambda = Take<PythonCallable>(o);
     auto arginfo = lambda.GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
-    EXPECT_EQ(arginfo.get().count, 2);
     EXPECT_EQ(arginfo.get().max_positional_args, 2u);
-    EXPECT_EQ(arginfo.get().has_varargs, false);
   }
 
   {
     PyObject *o = PyRun_String("lambda x,y,*a: x", Py_eval_input, globals.get(),
                                globals.get());
     ASSERT_FALSE(o == NULL);
     auto lambda = Take<PythonCallable>(o);
     auto arginfo = lambda.GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
-    EXPECT_EQ(arginfo.get().count, 2);
     EXPECT_EQ(arginfo.get().max_positional_args,
               PythonCallable::ArgInfo::UNBOUNDED);
-    EXPECT_EQ(arginfo.get().has_varargs, true);
   }
 
   {
     PyObject *o = PyRun_String("lambda x,y,*a,**kw: x", Py_eval_input,
                                globals.get(), globals.get());
     ASSERT_FALSE(o == NULL);
     auto lambda = Take<PythonCallable>(o);
     auto arginfo = lambda.GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
-    EXPECT_EQ(arginfo.get().count, 2);
     EXPECT_EQ(arginfo.get().max_positional_args,
               PythonCallable::ArgInfo::UNBOUNDED);
-    EXPECT_EQ(arginfo.get().has_varargs, true);
   }
 
   {
     const char *script = R"(
 class Foo:
   def bar(self, x):
      return x
   @classmethod
   def classbar(cls, x):
      return x
   @staticmethod
   def staticbar(x):
      return x
   def __call__(self, x):
      return x
 obj = Foo()
 bar_bound   = Foo().bar
 bar_class   = Foo().classbar
 bar_static  = Foo().staticbar
 bar_unbound = Foo.bar
+
+
+class OldStyle:
+  def __init__(self, one, two, three):
+    pass
+
+class NewStyle(object):
+  def __init__(self, one, two, three):
+    pass
+
 )";
     PyObject *o =
         PyRun_String(script, Py_file_input, globals.get(), globals.get());
     ASSERT_FALSE(o == NULL);
     Take<PythonObject>(o);
 
     auto bar_bound = As<PythonCallable>(globals.GetItem("bar_bound"));
     ASSERT_THAT_EXPECTED(bar_bound, llvm::Succeeded());
     auto arginfo = bar_bound.get().GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
-    EXPECT_EQ(arginfo.get().count, 2); // FIXME, wrong
     EXPECT_EQ(arginfo.get().max_positional_args, 1u);
-    EXPECT_EQ(arginfo.get().has_varargs, false);
 
     auto bar_unbound = As<PythonCallable>(globals.GetItem("bar_unbound"));
     ASSERT_THAT_EXPECTED(bar_unbound, llvm::Succeeded());
     arginfo = bar_unbound.get().GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
-    EXPECT_EQ(arginfo.get().count, 2);
     EXPECT_EQ(arginfo.get().max_positional_args, 2u);
-    EXPECT_EQ(arginfo.get().has_varargs, false);
 
     auto bar_class = As<PythonCallable>(globals.GetItem("bar_class"));
     ASSERT_THAT_EXPECTED(bar_class, llvm::Succeeded());
     arginfo = bar_class.get().GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
     EXPECT_EQ(arginfo.get().max_positional_args, 1u);
-    EXPECT_EQ(arginfo.get().has_varargs, false);
 
     auto bar_static = As<PythonCallable>(globals.GetItem("bar_static"));
     ASSERT_THAT_EXPECTED(bar_static, llvm::Succeeded());
     arginfo = bar_static.get().GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
     EXPECT_EQ(arginfo.get().max_positional_args, 1u);
-    EXPECT_EQ(arginfo.get().has_varargs, false);
 
     auto obj = As<PythonCallable>(globals.GetItem("obj"));
     ASSERT_THAT_EXPECTED(obj, llvm::Succeeded());
     arginfo = obj.get().GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
     EXPECT_EQ(arginfo.get().max_positional_args, 1u);
-    EXPECT_EQ(arginfo.get().has_varargs, false);
+
+    auto oldstyle = As<PythonCallable>(globals.GetItem("OldStyle"));
+    ASSERT_THAT_EXPECTED(oldstyle, llvm::Succeeded());
+    arginfo = oldstyle.get().GetArgInfo();
+    ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
+    EXPECT_EQ(arginfo.get().max_positional_args, 3u);
+
+    auto newstyle = As<PythonCallable>(globals.GetItem("NewStyle"));
+    ASSERT_THAT_EXPECTED(newstyle, llvm::Succeeded());
+    arginfo = newstyle.get().GetArgInfo();
+    ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
+    EXPECT_EQ(arginfo.get().max_positional_args, 3u);
   }
 
 #if PY_MAJOR_VERSION >= 3 && PY_MINOR_VERSION >= 3
 
   // the old implementation of GetArgInfo just doesn't work on builtins.
 
   {
     auto builtins = PythonModule::BuiltinsModule();
     auto hex = As<PythonCallable>(builtins.GetAttribute("hex"));
     ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());
     auto arginfo = hex.get().GetArgInfo();
     ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
-    EXPECT_EQ(arginfo.get().count, 1);
     EXPECT_EQ(arginfo.get().max_positional_args, 1u);
-    EXPECT_EQ(arginfo.get().has_varargs, false);
   }
 
 #endif
 }
 
 TEST_F(PythonDataObjectsTest, TestScript) {
 
   static const char script[] = R"(

 g = foobar()
 )";
 
   r = runStringMultiLine(script, globals, globals);
   ASSERT_THAT_EXPECTED(r, llvm::Succeeded());
   auto g = As<std::string>(globals.GetItem("g"));
   ASSERT_THAT_EXPECTED(g, llvm::HasValue("foobarbaz"));
 }
 No newline at end of file