clean up the implementation of PythonCallable::GetNumArguments

[lawrence_danna]

Summary

The current implementation of PythonCallable::GetNumArguments
is not exception safe, has weird semantics, and is just plain
incorrect for some kinds of functions.

Python 3.3 introduces inspect.signature, which lets us easily
query for function signatures in a sane and documented way.

This patch leaves the old implementation in place for < 3.3,
but uses inspect.signature for modern pythons. It also leaves
the old weird semantics in place, but with FIXMEs grousing about
it. We should update the callers and fix the semantics in a
subsequent patch. It also adds some tests.

[lawrence_danna]

put in missing consumeError()

[lawrence_danna]

oops, revert

[lawrence_danna]

python 2 fix

[labath]

Thanks for jumping onto this. Apart from the inline comments, I have one high level question: What are the cases that the old method is wrong for? Was it just builtin functions, or are there other cases too? Is it possible to fix it to work for builtings too? (okay, those were three questions)

What I am wondering is how important it is to maintain two methods for fetching the argument information. Builtin functions are not likely to be used as lldb callbacks, so if it's just those, it may be sufficient to just leave a TODO to use the new method once we are in a position to require python>=3.3.

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

58–61

Btw, what are the reasons that this can fail? The string object was created two lines above, so we definitely know it's a string. If the call cannot fail in this particular circumstance, then a one can express that (and make the code simpler) with return cantFail(str.AsUTF8()).

827

Make this a raw string? Among other benefits, it means clang-format will not reflow it weirdly...

864–891

Likewise, should most of these takeErrors be cantFail too ?

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

339

Maybe it's time for a utility function like py2_const_cast(format) ?

lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

643–647

If you implemented operator== and operator<<(raw_ostream&) for the ArgInfo class, you could write this as EXPECT_THAT_EXPECTED(lambda.GetArgInfo(), llvm::HasValue(ArgInfo(1, false, false))

[lawrence_danna]

review fixes

[lawrence_danna]

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

58–61

It is possible!

chr(0xDFFF).encode('utf8')

864–891

Yea, i guess given that we control the script there's really no reasonable way these can fail if the first check doesn't.

lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

643–647

eh, do i have to? I actually like having them each on their own line. It makes the patches nicer when members are added or removed, and i'd like to remove all the members except max_positional_args.

[lawrence_danna]

In D68995#1710594, @labath wrote:

Thanks for jumping onto this. Apart from the inline comments, I have one high level question: What are the cases that the old method is wrong for? Was it just builtin functions, or are there other cases too? Is it possible to fix it to work for builtings too? (okay, those were three questions)

What I am wondering is how important it is to maintain two methods for fetching the argument information. Builtin functions are not likely to be used as lldb callbacks, so if it's just those, it may be sufficient to just leave a TODO to use the new method once we are in a position to require python>=3.3.

Looks like the old implementation also doesn't work for class or static methods, or for objects with __call__ method. I added a bunch of tests for script commands with various callable types in the followon patch.

[lawrence_danna]

fixup: VAR_KEYWORD

[labath]

In D68995#1711831, @lawrence_danna wrote:

In D68995#1710594, @labath wrote:

Thanks for jumping onto this. Apart from the inline comments, I have one high level question: What are the cases that the old method is wrong for? Was it just builtin functions, or are there other cases too? Is it possible to fix it to work for builtings too? (okay, those were three questions)

What I am wondering is how important it is to maintain two methods for fetching the argument information. Builtin functions are not likely to be used as lldb callbacks, so if it's just those, it may be sufficient to just leave a TODO to use the new method once we are in a position to require python>=3.3.

Looks like the old implementation also doesn't work for class or static methods, or for objects with __call__ method. I added a bunch of tests for script commands with various callable types in the followon patch.

Ok, I see... Would it be hard/impossible to fix that using the previous method, so that we have a unified implementation? Or would that result in a bunch of ifdefs too ? I'm sorry if that's obvious, but I don't know much about the C python api..

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

58–61

Right. Thanks for explaining.

864–891

Yes, that's what I was thinking.

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

317

Could you also add a comment that this is because py2 apis accept strings as "char *", and that one should be careful to not use it if the function can actually modify the string.

lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

643–647

No, you don't. These are mostly fine. The main thing I don't like is when people then try to "reduce duplication" and create a function like compare_arginfo, with the EXPECTs inside. Then, when the checks fail, the error message points you to that function and you're left guessing which of the compare invocations did this fail for. The only thing that can be said here is that if the ASSERT_THAT_EXPECTED macro fails it will terminate the execution of all the subsequent checks too, whereas if this was just one EXPECT call, then you'd be able to see results of all other tests too (and maybe notice a pattern). But that's something I can live with...

695

I don't see this defined anywhere. I guess it leaked from the other patch?

700

Raw string too?

[lawrence_danna]

In D68995#1712387, @labath wrote:

In D68995#1711831, @lawrence_danna wrote:

In D68995#1710594, @labath wrote:

Thanks for jumping onto this. Apart from the inline comments, I have one high level question: What are the cases that the old method is wrong for? Was it just builtin functions, or are there other cases too? Is it possible to fix it to work for builtings too? (okay, those were three questions)

What I am wondering is how important it is to maintain two methods for fetching the argument information. Builtin functions are not likely to be used as lldb callbacks, so if it's just those, it may be sufficient to just leave a TODO to use the new method once we are in a position to require python>=3.3.

Looks like the old implementation also doesn't work for class or static methods, or for objects with __call__ method. I added a bunch of tests for script commands with various callable types in the followon patch.

Ok, I see... Would it be hard/impossible to fix that using the previous method, so that we have a unified implementation? Or would that result in a bunch of ifdefs too ? I'm sorry if that's obvious, but I don't know much about the C python api..

I did fix the previous method for python2, at least for the callable types I could think of to test.

There must be some way to get something resembling the old method to work for python3, because inspect does it. But it would be harder to get right, and a lot less likely to keep being correct in future versions of python. And I don't think it's possible to have a single implementation like that that works for both 2 and 3 without ifdefs.

I was thinking that since python2 is EOL in like two months anyway, the best thing to do is to just keep the old implementation around until we drop python 2 support, and then delete it.

[lawrence_danna]

fixed

[lawrence_danna]

fixed

[labath]

In D68995#1712396, @lawrence_danna wrote:

Looks like the old implementation also doesn't work for class or static methods, or for objects with __call__ method. I added a bunch of tests for script commands with various callable types in the followon patch.

Ok, I see... Would it be hard/impossible to fix that using the previous method, so that we have a unified implementation? Or would that result in a bunch of ifdefs too ? I'm sorry if that's obvious, but I don't know much about the C python api..

I did fix the previous method for python2, at least for the callable types I could think of to test.

There must be some way to get something resembling the old method to work for python3, because inspect does it. But it would be harder to get right, and a lot less likely to keep being correct in future versions of python. And I don't think it's possible to have a single implementation like that that works for both 2 and 3 without ifdefs.

Right, that's what I was thinking/fearing. I think I am satisfied with that explanation. Thanks for explaining.

I was thinking that since python2 is EOL in like two months anyway, the best thing to do is to just keep the old implementation around until we drop python 2 support, and then delete it.

Yeah, though I don't know if that means that lldb will drop support for it straight away. I haven't heard of any plans like that, though the next weeks dev meeting will probably be a good place to talk about it. Will you be there by any chance?

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

821

static const char get_arg_info_script[] (one pointer more efficient)

[lawrence_danna]

char * -> char[]

[lawrence_danna]

fixed

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

 protected:
   static llvm::Error nullDeref() {
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "A NULL PyObject* was dereferenced");
   }
   static llvm::Error exception(const char *s = nullptr) {
     return llvm::make_error<PythonException>(s);
   }
+  static llvm::Error keyError() {
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "key not in dict");
+  }
 
 public:
   template <typename... T>
   llvm::Expected<PythonObject> CallMethod(const char *name,
                                           const T &... t) const {
     const char format[] = {'(', PythonFormat<T>::format..., ')', 0};
 #if PY_MAJOR_VERSION < 3
     PyObject *obj = PyObject_CallMethod(m_py_obj, const_cast<char *>(name),
                                         const_cast<char *>(format),
                                         PythonFormat<T>::get(t)...);
 #else
     PyObject *obj =
         PyObject_CallMethod(m_py_obj, name, format, PythonFormat<T>::get(t)...);
 #endif
     if (!obj)
       return exception();
     return python::Take<PythonObject>(obj);
   }
 
+  template <typename... T>
+  llvm::Expected<PythonObject> Call(const T &... t) const {
+    const char format[] = {'(', PythonFormat<T>::format..., ')', 0};
+#if PY_MAJOR_VERSION < 3
+    PyObject *obj = PyObject_CallFunction(m_py_obj, const_cast<char *>(format),
+                                          PythonFormat<T>::get(t)...);
+#else
+    PyObject *obj =
+        PyObject_CallFunction(m_py_obj, format, PythonFormat<T>::get(t)...);
+#endif
+    if (!obj)
+      return exception();
+    return python::Take<PythonObject>(obj);
+  }
+
   llvm::Expected<PythonObject> GetAttribute(const char *name) const {
     if (!m_py_obj)
       return nullDeref();
     PyObject *obj = PyObject_GetAttrString(m_py_obj, name);
     if (!obj)
       return exception();
     return python::Take<PythonObject>(obj);
   }
template <typename T> llvm::Expected<T> As(llvm::Expected<PythonObject> &&obj) {
   return T(PyRefType::Borrowed, std::move(obj.get().get()));
 }
 
 template <> llvm::Expected<bool> As<bool>(llvm::Expected<PythonObject> &&obj);
 
 template <>
 llvm::Expected<long long> As<long long>(llvm::Expected<PythonObject> &&obj);
 
+template <>
+llvm::Expected<std::string> As<std::string>(llvm::Expected<PythonObject> &&obj);
+
 } // namespace python
 
 template <class T> class TypedPythonObject : public PythonObject {
 public:
   // override to perform implicit type conversions on Reset
   // This can be eliminated once we drop python 2 support.
   static void Convert(PyRefType &type, PyObject *&py_obj) {}
 
public:
   explicit PythonDictionary(PyInitialValue value);
 
   static bool Check(PyObject *py_obj);
 
   uint32_t GetSize() const;
 
   PythonList GetKeys() const;
 
-  PythonObject GetItemForKey(const PythonObject &key) const;
-  void SetItemForKey(const PythonObject &key, const PythonObject &value);
+  PythonObject GetItemForKey(const PythonObject &key) const; // DEPRECATED
+  void SetItemForKey(const PythonObject &key,
+                     const PythonObject &value); // DEPRECATED
+
+  llvm::Expected<PythonObject> GetItem(const PythonObject &key) const;
+  llvm::Expected<PythonObject> GetItem(const char *key) const;
+  llvm::Error SetItem(const PythonObject &key, const PythonObject &value) const;
+  llvm::Error SetItem(const char *key, const PythonObject &value) const;
 
   StructuredData::DictionarySP CreateStructuredDictionary() const;
 };
 
 class PythonModule : public TypedPythonObject<PythonModule> {
 public:
   using TypedPythonObject::TypedPythonObject;
 
public:
   PythonDictionary GetDictionary() const;
 };
 
 class PythonCallable : public TypedPythonObject<PythonCallable> {
 public:
   using TypedPythonObject::TypedPythonObject;
 
   struct ArgInfo {
-    size_t count;
-    bool is_bound_method : 1;
-    bool has_varargs : 1;
-    bool has_kwargs : 1;
+    /* 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
+    /* is the callable a bound method written in python? */
+    bool is_bound_method : 1; // FIXME delete this
   };
 
   static bool Check(PyObject *py_obj);
 
-  ArgInfo GetNumArguments() const;
+  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;
+  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>

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

 
 template <>
 Expected<long long> python::As<long long>(Expected<PythonObject> &&obj) {
   if (!obj)
     return obj.takeError();
   return obj.get().AsLongLong();
 }
 
+template <>
+Expected<std::string> python::As<std::string>(Expected<PythonObject> &&obj) {
+  if (!obj)
+    return obj.takeError();
+  PyObject *str_obj = PyObject_Str(obj.get().get());
+  if (!obj)
+    return llvm::make_error<PythonException>();
+  auto str = Take<PythonString>(str_obj);
+  auto utf8 = str.AsUTF8();
+  if (!utf8)
+    return utf8.takeError();
+  return utf8.get();
+}
+
 void StructuredPythonObject::Serialize(llvm::json::OStream &s) const {
   s.value(llvm::formatv("Python Obj: {0:X}", GetValue()).str());
 }
 
 // PythonObject
 
 void PythonObject::Dump(Stream &strm) const {
   if (m_py_obj) {

 
 PythonList PythonDictionary::GetKeys() const {
   if (IsValid())
     return PythonList(PyRefType::Owned, PyDict_Keys(m_py_obj));
   return PythonList(PyInitialValue::Invalid);
 }
 
 PythonObject PythonDictionary::GetItemForKey(const PythonObject &key) const {
-  if (IsAllocated() && key.IsValid())
-    return PythonObject(PyRefType::Borrowed,
-                        PyDict_GetItem(m_py_obj, key.get()));
+  auto item = GetItem(key);
+  if (!item) {
+    llvm::consumeError(item.takeError());
     return PythonObject();
   }
+  return std::move(item.get());
+}
+
+Expected<PythonObject>
+PythonDictionary::GetItem(const PythonObject &key) const {
+  if (!IsValid())
+    return nullDeref();
+#if PY_MAJOR_VERSION >= 3
+  PyObject *o = PyDict_GetItemWithError(m_py_obj, key.get());
+  if (PyErr_Occurred())
+    return exception();
+#else
+  PyObject *o = PyDict_GetItem(m_py_obj, key.get());
+#endif
+  if (!o)
+    return keyError();
+  return Retain<PythonObject>(o);
+}
+
+Expected<PythonObject> PythonDictionary::GetItem(const char *key) const {
+  if (!IsValid())
+    return nullDeref();
+  PyObject *o = PyDict_GetItemString(m_py_obj, key);
+  if (PyErr_Occurred())
+    return exception();
+  if (!o)
+    return keyError();
+  return Retain<PythonObject>(o);
+}
+
+Error PythonDictionary::SetItem(const PythonObject &key,
+                                const PythonObject &value) const {
+  if (!IsValid() || !value.IsValid())
+    return nullDeref();
+  int r = PyDict_SetItem(m_py_obj, key.get(), value.get());
+  if (r < 0)
+    return exception();
+  return Error::success();
+}
+
+Error PythonDictionary::SetItem(const char *key,
+                                const PythonObject &value) const {
+  if (!IsValid() || !value.IsValid())
+    return nullDeref();
+  int r = PyDict_SetItemString(m_py_obj, key, value.get());
+  if (r < 0)
+    return exception();
+  return Error::success();
+}
 
 void PythonDictionary::SetItemForKey(const PythonObject &key,
                                      const PythonObject &value) {
-  if (IsAllocated() && key.IsValid() && value.IsValid())
-    PyDict_SetItem(m_py_obj, key.get(), value.get());
+  Error error = SetItem(key, value);
+  if (error)
+    llvm::consumeError(std::move(error));
 }
 
 StructuredData::DictionarySP
 PythonDictionary::CreateStructuredDictionary() const {
   StructuredData::DictionarySP result(new StructuredData::Dictionary);
   PythonList keys(GetKeys());
   uint32_t num_keys = keys.GetSize();
   for (uint32_t i = 0; i < num_keys; ++i) {

 bool PythonCallable::Check(PyObject *py_obj) {
   if (!py_obj)
     return false;
 
   return PyCallable_Check(py_obj);
 }
 
 PythonCallable::ArgInfo PythonCallable::GetNumInitArguments() const {
-  ArgInfo result = {0, false, false, false};
-  if (!IsValid())
-    return result;
-
-  PythonObject __init__ = GetAttributeValue("__init__");
-  if (__init__.IsValid() ) {
-    auto __init_callable__ = __init__.AsType<PythonCallable>();
-    if (__init_callable__.IsValid())
-      return __init_callable__.GetNumArguments();
+  auto arginfo = GetInitArgInfo();
+  if (!arginfo) {
+    llvm::consumeError(arginfo.takeError());
+    return ArgInfo{};
   }
-  return result;
+  return arginfo.get();
 }
 
-PythonCallable::ArgInfo PythonCallable::GetNumArguments() const {
-  ArgInfo result = {0, false, false, false};
+Expected<PythonCallable::ArgInfo> PythonCallable::GetInitArgInfo() const {
   if (!IsValid())
-    return result;
+    return nullDeref();
+  auto init = As<PythonCallable>(GetAttribute("__init__"));
+  if (!init)
+    return init.takeError();
+  return init.get().GetArgInfo();
+}
+
+Expected<PythonCallable::ArgInfo> PythonCallable::GetArgInfo() const {
+  ArgInfo result = {};
+  if (!IsValid())
+    return nullDeref();
+
+#if PY_MAJOR_VERSION >= 3 && PY_MINOR_VERSION >= 3
+
+  const char *script =
+      "from inspect import signature, Parameter, ismethod \n"
+      "from collections import namedtuple \n"
+      "ArgInfo = namedtuple('ArgInfo', ['count', 'has_varargs', "
+      "'is_bound_method']) \n"
+      "def get_arg_info(f): \n"
+      "    count = 0 \n"
+      "    varargs = False  \n"
+      "    for parameter in signature(f).parameters.values():  \n"
+      "        kind = parameter.kind  \n"
+      "        if kind in (Parameter.POSITIONAL_ONLY,   \n"
+      "                    Parameter.POSITIONAL_OR_KEYWORD):  \n"
+      "            count += 1  \n"
+      "        elif kind == Parameter.VAR_POSITIONAL:  \n"
+      "            varargs = True  \n"
+      "        elif kind in (Parameter.KEYWORD_ONLY,  \n"
+      "                      Parameter.KEYWORD_ONLY):  \n"
+      "            pass  \n"
+      "        else:  \n"
+      "            raise Exception(f'unknown parameter kind: {kind}')  \n"
+      "    return ArgInfo(count, varargs, ismethod(f))  \n";
+
+  // this global is protected by the GIL
+  static PythonCallable get_arg_info;
+
+  if (!get_arg_info.IsValid()) {
+    PythonDictionary globals(PyInitialValue::Empty);
+
+    auto builtins = PythonModule::BuiltinsModule();
+    Error error = globals.SetItem("__builtins__", builtins);
+    if (error)
+      return std::move(error);
+    PyObject *o =
+        PyRun_String(script, Py_file_input, globals.get(), globals.get());
+    if (!o)
+      return exception();
+    Take<PythonObject>(o);
+    auto function = As<PythonCallable>(globals.GetItem("get_arg_info"));
+    if (!function)
+      return function.takeError();
+    get_arg_info = std::move(function.get());
+  }
+
+  Expected<PythonObject> pyarginfo = get_arg_info.Call(*this);
+  if (!pyarginfo)
+    return pyarginfo.takeError();
+  Expected<long long> count =
+      As<long long>(pyarginfo.get().GetAttribute("count"));
+  if (!count)
+    return count.takeError();
+  result.count = count.get();
+  Expected<bool> has_varargs =
+      As<bool>(pyarginfo.get().GetAttribute("has_varargs"));
+  if (!has_varargs)
+    return has_varargs.takeError();
+  result.has_varargs = has_varargs.get();
+  Expected<bool> is_method =
+      As<bool>(pyarginfo.get().GetAttribute("is_bound_method"));
+  if (!is_method)
+    return is_method.takeError();
+  result.is_bound_method = is_method.get();
+
+  // FIXME emulate old broken behavior
+  if (result.is_bound_method)
+    result.count++;
+
+#else
 
   PyObject *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())
       result.is_bound_method = true;
   } 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.has_kwargs = !!(code->co_flags & CO_VARKEYWORDS);
+
+#endif
+
   return result;
 }
 
+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/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/lldb-enumerations.h"
 #include "llvm/Testing/Support/Error.h"
 
 #include "PythonTestSuite.h"
 
 using namespace lldb_private;
+using namespace lldb_private::python;
 
 class PythonDataObjectsTest : public PythonTestSuite {
 public:
   void SetUp() override {
     PythonTestSuite::SetUp();
 
     PythonString sys_module("sys");
     m_sys_module.Reset(PyRefType::Owned, PyImport_Import(sys_module.get()));
if (structured_dict_ptr) {
           structured_dict_ptr->GetValueForKey(key_name);
       EXPECT_TRUE((bool)structured_addr_value_sp);
       const uint64_t extracted_value =
           structured_addr_value_sp->GetIntegerValue(123);
       EXPECT_TRUE(extracted_value == value);
     }
   }
 }
+
+TEST_F(PythonDataObjectsTest, TestCallable) {
+
+  PythonDictionary globals(PyInitialValue::Empty);
+  auto builtins = PythonModule::BuiltinsModule();
+  llvm::Error error = globals.SetItem("__builtins__", builtins);
+  ASSERT_FALSE(error);
+
+  {
+    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().has_varargs, false);
+    EXPECT_EQ(arginfo.get().is_bound_method, 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().has_varargs, false);
+    EXPECT_EQ(arginfo.get().is_bound_method, 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().has_varargs, true);
+    EXPECT_EQ(arginfo.get().is_bound_method, false);
+  }
+
+  {
+    const char *script = "class Foo: \n"
+                         "  def bar(self, x):\n"
+                         "     return x \n"
+                         "bar_bound   = Foo().bar \n"
+                         "bar_unbound = Foo.bar \n";
+    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().has_varargs, false);
+    EXPECT_EQ(arginfo.get().is_bound_method, true);
+
+    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().has_varargs, false);
+    EXPECT_EQ(arginfo.get().is_bound_method, false);
+  }
+
+#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().has_varargs, false);
+    EXPECT_EQ(arginfo.get().is_bound_method, false);
+  }
+
+#endif
+}
 No newline at end of file