PyCLIF behavior changes: Safer Python-to-C++ std::vector, std::set, std::map conversions.

**Please request a rollback only as a last resort**, to buy time for working out a forward fix. Forward fixes are almost certainly trivial: Based on the Python traceback, insert an explicit conversion to `tuple()`, `set()`, or `dict()` (e.g. cl/538602196), or change a Python `set` or `list` literal to the correct type (e.g. cl/538563104).

Motivations for the behavior changes:

* Improves safety / prevents accidents:

  * For C++ `std::set` (and `std::unordered_set`): The potentially reducing conversion from a Python sequence (e.g. Python `list` or `tuple`) to a C++ set must be explicit.

  * For C++ `std::vector` (and `std::array`): In very rare cases (see b/284333274#comment11) the conversion from a Python iterable must be explicit.

  * For C++ `std::map` (and `std::unordered_map`): The conditions added in this CL (`clif::PyObjectTypeIsConvertibleToStdMap()`) did not break any existing unit tests.

* Improves readability: Python literals to be passed as C++ sets must be set literals.

  * Note for completeness: A Python set can still be passed to a C++ `std::vector`. The rationale is that this conversion is not reducing. Implicit conversions of this kind are also fairly commonly used in google3, therefore enforcing explicit conversions has an unfavorable cost : benefit ratio.

* Original trigger for this work: Converge pybind11 & PyCLIF behavior (go/pyclif_pybind11_fusion).

  * Note that pybind11 is very far on the strict side of the spectrum. See also pybind/pybind11#4686.

  * PyCLIF was originally extremely far on the permissive side of the spectrum. This CL is already the 2nd cleanup step: prior to cl/525187106 a conversion from any zero-length iterator (e.g. the `{}` empty dict literal) to a C++ `std::vector` or `std::set` was possible.

This CL was started from cl/535028028, but the changes under google3/third_party/absl/python were backed out, the change for `std::array` in stltypes.h was added.

OSS build tested interactively with copybara to local directory & cmake commands in Ubuntu 20.04 / Python 3.9 docker container.

PiperOrigin-RevId: 538823846

Author: rwgk

clif/python/runtime.cc

@@ -14,6 +14,7 @@
 
 #include "clif/python/runtime.h"
 
+#include <initializer_list>
 #include <system_error>  // NOLINT(build/c++11)
 
 #include "clif/python/pickle_support.h"
@@ -384,4 +385,42 @@ PyObject* ReduceExImpl(PyObject* self, PyObject* args, PyObject* kw) {
   return ReduceExCore(self, protocol);
 }
 
+namespace {
+
+bool PyObjectIsInstanceWithOneOfTpNames(
+    PyObject* obj, std::initializer_list<const char*> tp_names) {
+  if (PyType_Check(obj)) {
+    return false;
+  }
+  const char* obj_tp_name = Py_TYPE(obj)->tp_name;
+  for (const auto* tp_name : tp_names) {
+    if (strcmp(obj_tp_name, tp_name) == 0) {
+      return true;
+    }
+  }
+  return false;
+}
+
+}  // namespace
+
+bool PyObjectTypeIsConvertibleToStdVector(PyObject* obj) {
+  if (PySequence_Check(obj) != 0) {
+    return !PyUnicode_Check(obj) && !PyBytes_Check(obj);
+  }
+  return (PyGen_Check(obj) != 0) || (PyAnySet_Check(obj) != 0) ||
+         PyObjectIsInstanceWithOneOfTpNames(
+             obj, {"dict_keys", "dict_values", "dict_items", "map", "zip"});
+}
+
+bool PyObjectTypeIsConvertibleToStdSet(PyObject* obj) {
+  return (PyAnySet_Check(obj) != 0) ||
+         PyObjectIsInstanceWithOneOfTpNames(obj, {"dict_keys"});
+}
+
+bool PyObjectTypeIsConvertibleToStdMap(PyObject* obj) {
+  return (PyDict_Check(obj) != 0) ||
+         ((PyMapping_Check(obj) != 0) &&
+          (PyObject_HasAttrString(obj, "items") != 0));
+}
+
 }  // namespace clif

clif/python/runtime.h

@@ -185,6 +185,10 @@ bool ensure_no_args_and_kw_args(const char* func, PyObject* args, PyObject* kw);
 // https://docs.python.org/3/library/pickle.html#object.__reduce_ex__
 PyObject* ReduceExImpl(PyObject* self, PyObject* args, PyObject* kw);
 
+bool PyObjectTypeIsConvertibleToStdVector(PyObject* obj);
+bool PyObjectTypeIsConvertibleToStdSet(PyObject* obj);
+bool PyObjectTypeIsConvertibleToStdMap(PyObject* obj);
+
 }  // namespace clif
 
 #endif  // CLIF_PYTHON_RUNTIME_H_

clif/python/stltypes.h

@@ -222,7 +222,6 @@ typename std::enable_if<(I < sizeof...(U)), bool>::type  //
 PyObjAsVariantTryAll(PyObject* py, ::std::variant<U...>* v) {
   {
     bool success = PyObjAsVariantAt<I>(py, v);
-    DCHECK(success || PyErr_Occurred() != nullptr);
     if (success) {
       return true;
     }
@@ -936,10 +935,13 @@ bool IterToCont(PyObject* py, Inserter add, bool accept_dict = false) {
 // to a C++ container via functor add.
 template <typename T, typename U, typename F>
 bool ItemsToMap(PyObject* py, F add) {
-  py = PyObject_CallMethod(py, "items", nullptr);
-  if (py == nullptr) return false;
-  bool ok = py::IterToCont<std::pair<T, U>>(py, add, true);
-  Py_DECREF(py);
+  if (!PyObjectTypeIsConvertibleToStdMap(py)) {
+    return false;
+  }
+  PyObject* items = PyObject_CallMethod(py, "items", nullptr);
+  if (items == nullptr) return false;
+  bool ok = py::IterToCont<std::pair<T, U>>(items, add, true);
+  Py_DECREF(items);
   return ok;
 }
 }  // namespace py
@@ -948,6 +950,9 @@ bool ItemsToMap(PyObject* py, F add) {
 template <typename T, typename... Args>
 bool Clif_PyObjAs(PyObject* py, std::vector<T, Args...>* c) {
   CHECK(c != nullptr);
+  if (!PyObjectTypeIsConvertibleToStdVector(py)) {
+    return false;
+  }
   c->clear();
   return py::IterToCont<T>(py, [&c](T&& i) {  // NOLINT: build/c++11
     c->push_back(std::move(i));
@@ -957,7 +962,9 @@ bool Clif_PyObjAs(PyObject* py, std::vector<T, Args...>* c) {
 template <typename T, std::size_t N>
 bool Clif_PyObjAs(PyObject* py, std::array<T, N>* c) {
   CHECK(c != nullptr);
-
+  if (!PyObjectTypeIsConvertibleToStdVector(py)) {
+    return false;
+  }
   int index = 0;
   bool rval = py::IterToCont<T>(py, [&c, &index](T&& i) {
     if (index < N) {
@@ -976,6 +983,9 @@ bool Clif_PyObjAs(PyObject* py, std::array<T, N>* c) {
 template <typename T, typename... Args>
 bool Clif_PyObjAs(PyObject* py, std::unordered_set<T, Args...>* c) {
   CHECK(c != nullptr);
+  if (!PyObjectTypeIsConvertibleToStdSet(py)) {
+    return false;
+  }
   c->clear();
   return py::IterToCont<T>(py, [&c](T&& i) {  // NOLINT: build/c++11
     c->insert(std::move(i));
@@ -984,6 +994,9 @@ bool Clif_PyObjAs(PyObject* py, std::unordered_set<T, Args...>* c) {
 template <typename T, typename... Args>
 bool Clif_PyObjAs(PyObject* py, std::set<T, Args...>* c) {
   CHECK(c != nullptr);
+  if (!PyObjectTypeIsConvertibleToStdSet(py)) {
+    return false;
+  }
   c->clear();
   return py::IterToCont<T>(py, [&c](T&& i) {  // NOLINT: build/c++11
     c->insert(std::move(i));

clif/testing/python/lambda_expressions_test.py

@@ -149,31 +149,36 @@ def test_extended_ctor_accept_iterable_as_vector(self, iterable):
     self.assertCountEqual(iterable, obj.value)
 
   def test_accept_iterable_as_set(self, iterable):
-    self.assertEqual(lambda_expressions.takes_set(iterable), 3)
-    self.assertEqual(lambda_expressions.takes_unordered_set(iterable), 3)
-    self.assertEqual(lambda_expressions.takes_set((i for i in iterable)), 3)
-    self.assertEqual(lambda_expressions.takes_unordered_set(
-        (i for i in iterable)), 3)
+    # Explicit set() added with cl/536221955.
+    self.assertEqual(lambda_expressions.takes_set(set(iterable)), 3)
+    self.assertEqual(lambda_expressions.takes_unordered_set(set(iterable)), 3)
+    self.assertEqual(lambda_expressions.takes_set(set(i for i in iterable)), 3)
+    self.assertEqual(
+        lambda_expressions.takes_unordered_set(set(i for i in iterable)), 3
+    )
 
   def test_ctor_accept_iterable_as_set(self, iterable):
-    obj = lambda_expressions.CtorTakesSet(iterable)
+    # Explicit set() added with cl/536221955.
+    obj = lambda_expressions.CtorTakesSet(set(iterable))
     self.assertCountEqual(iterable, obj.value)
-    obj = lambda_expressions.CtorTakesUnorderedSet(iterable)
+    obj = lambda_expressions.CtorTakesUnorderedSet(set(iterable))
     self.assertCountEqual(iterable, obj.value)
-    obj = lambda_expressions.CtorTakesSet((i for i in iterable))
+    obj = lambda_expressions.CtorTakesSet(set(i for i in iterable))
     self.assertCountEqual(iterable, obj.value)
-    obj = lambda_expressions.CtorTakesUnorderedSet((i for i in iterable))
+    obj = lambda_expressions.CtorTakesUnorderedSet(set(i for i in iterable))
     self.assertCountEqual(iterable, obj.value)
 
   def test_extended_ctor_accept_iterable_as_set(self, iterable):
-    obj = lambda_expressions.ExtendedCtorTakesSet(iterable)
+    # Explicit set() added with cl/536221955.
+    obj = lambda_expressions.ExtendedCtorTakesSet(set(iterable))
     self.assertCountEqual(iterable, obj.value)
-    obj = lambda_expressions.ExtendedCtorTakesUnorderedSet(iterable)
+    obj = lambda_expressions.ExtendedCtorTakesUnorderedSet(set(iterable))
     self.assertCountEqual(iterable, obj.value)
-    obj = lambda_expressions.ExtendedCtorTakesSet((i for i in iterable))
+    obj = lambda_expressions.ExtendedCtorTakesSet(set(i for i in iterable))
     self.assertCountEqual(iterable, obj.value)
     obj = lambda_expressions.ExtendedCtorTakesUnorderedSet(
-        (i for i in iterable))
+        set(i for i in iterable)
+    )
     self.assertCountEqual(iterable, obj.value)
 
 

clif/testing/python/std_containers.clif

@@ -16,6 +16,8 @@ use `std::string` as str
 
 from "clif/testing/std_containers.h":
   def PassVectorInt(v: list<int>) -> int
+  def PassArrayInt2(a: `std::array<int, 2>` as list) -> int
+  def PassVectorPairInt(v: list<tuple<int, int>>) -> int
   def PassSetInt(v: `std::set` as set<int>) -> int
   def Mul(v: list<int>, m: int) -> list<int>
   def Div(v: `std::array<int, 2>` as list, m: int) -> `std::array<int, 2>` as list

clif/testing/python/std_containers_test.py

@@ -15,23 +15,44 @@
 """Tests for clif.testing.python.std_containers."""
 
 from absl.testing import absltest
+from absl.testing import parameterized
 
 from clif.testing.python import std_containers
 
 
-class StdContainersTest(absltest.TestCase):
+class StdContainersTest(parameterized.TestCase):
 
-  def testPassVectorInt(self):
-    self.assertEqual(std_containers.PassVectorInt([7, 13]), 140)
-    self.assertEqual(std_containers.PassVectorInt(set([9, 12, 9])), 142)
+  @parameterized.parameters(
+      (std_containers.PassVectorInt, 0), (std_containers.PassArrayInt2, 1)
+  )
+  def testPassVectorInt(self, fn, offset):
+    self.assertEqual(fn([7, 13]), 140 + offset)
+    self.assertEqual(fn({6, 2}), 116 + offset)
+    self.assertEqual(fn({'x': 8, 'y': 11}.values()), 138 + offset)
+    self.assertEqual(fn({3: None, 9: None}.keys()), 124 + offset)
+    self.assertEqual(fn(i for i in [4, 17]), 142 + offset)
+    self.assertEqual(fn(map(lambda i: i * 3, [8, 7])), 190 + offset)
     with self.assertRaises(TypeError):
-      std_containers.PassVectorInt({})
+      fn({'x': 0, 'y': 1})
+    with self.assertRaises(TypeError):
+      fn({})
+
+  def testPassVectorPairInt(self):
+    fn = std_containers.PassVectorPairInt
+    self.assertEqual(fn(zip([5, 17], [13, 9])), 2222)
 
   def testPassSetInt(self):
-    self.assertEqual(std_containers.PassSetInt(set([3, 15, 3])), 254)
-    self.assertEqual(std_containers.PassSetInt([5, 17, 5]), 266)
+    fn = std_containers.PassSetInt
+    self.assertEqual(fn({3, 15}), 254)
+    self.assertEqual(fn({5: None, 12: None}.keys()), 251)
+    with self.assertRaises(TypeError):
+      fn([])
+    with self.assertRaises(TypeError):
+      fn({})
+    with self.assertRaises(TypeError):
+      fn({}.values())
     with self.assertRaises(TypeError):
-      std_containers.PassSetInt({})
+      fn(i for i in [])
 
   def testVector(self):
     self.assertEqual(std_containers.Mul([1, 2, 3], 2), [2, 4, 6])

clif/testing/std_containers.h

@@ -35,6 +35,18 @@ inline int PassVectorInt(const std::vector<int>& v) {
   return zum;
 }
 
+inline int PassArrayInt2(const std::array<int, 2>& a) {
+  return PassVectorInt(std::vector<int>(a.begin(), a.end())) + 1;
+}
+
+inline int PassVectorPairInt(const std::vector<std::pair<int, int>>& v) {
+  int zum = 0;
+  for (const auto& ij : v) {
+    zum += ij.first * 100 + ij.second;
+  }
+  return zum;
+}
+
 inline int PassSetInt(const std::set<int>& s) {
   int zum = 200;
   for (const int i : s) {