Merge pull request opencv#19312 from VadimLevin:dev/vlevin/clear-msg-for-failed-overload-resolution

Author: alalek

modules/core/include/opencv2/core/bindings_utils.hpp

@@ -64,6 +64,20 @@ String dumpString(const String& argument)
     return cv::format("String: %s", argument.c_str());
 }
 
+CV_WRAP static inline
+String testOverloadResolution(int value, const Point& point = Point(42, 24))
+{
+    return format("overload (int=%d, point=(x=%d, y=%d))", value, point.x,
+                  point.y);
+}
+
+CV_WRAP static inline
+String testOverloadResolution(const Rect& rect)
+{
+    return format("overload (rect=(x=%d, y=%d, w=%d, h=%d))", rect.x, rect.y,
+                  rect.width, rect.height);
+}
+
 CV_WRAP static inline
 AsyncArray testAsyncArray(InputArray argument)
 {

modules/python/src2/cv2.cpp

@@ -33,6 +33,7 @@
 
 #include "opencv2/core/utils/configuration.private.hpp"
 #include "opencv2/core/utils/logger.hpp"
+#include "opencv2/core/utils/tls.hpp"
 
 #include "pyopencv_generated_include.h"
 #include "opencv2/core/types_c.h"
@@ -138,6 +139,51 @@ class PyEnsureGIL
     PyGILState_STATE _state;
 };
 
+/**
+ * Light weight RAII wrapper for `PyObject*` owning references.
+ * In comparisson to C++11 `std::unique_ptr` with custom deleter, it provides
+ * implicit conversion functions that might be useful to initialize it with
+ * Python functions those returns owning references through the `PyObject**`
+ * e.g. `PyErr_Fetch` or directly pass it to functions those want to borrow
+ * reference to object (doesn't extend object lifetime) e.g. `PyObject_Str`.
+ */
+class PySafeObject
+{
+public:
+    PySafeObject() : obj_(NULL) {}
+
+    explicit PySafeObject(PyObject* obj) : obj_(obj) {}
+
+    ~PySafeObject()
+    {
+        Py_CLEAR(obj_);
+    }
+
+    operator PyObject*()
+    {
+        return obj_;
+    }
+
+    operator PyObject**()
+    {
+        return &obj_;
+    }
+
+    PyObject* release()
+    {
+        PyObject* obj = obj_;
+        obj_ = NULL;
+        return obj;
+    }
+
+private:
+    PyObject* obj_;
+
+    // Explicitly disable copy operations
+    PySafeObject(const PySafeObject*); // = delete
+    PySafeObject& operator=(const PySafeObject&); // = delete
+};
+
 static void pyRaiseCVException(const cv::Exception &e)
 {
     PyObject_SetAttrString(opencv_error, "file", PyString_FromString(e.file.c_str()));
@@ -290,6 +336,74 @@ bool parseNumpyScalar(PyObject* obj, T& value)
     return false;
 }
 
+TLSData<std::vector<std::string> > conversionErrorsTLS;
+
+inline void pyPrepareArgumentConversionErrorsStorage(std::size_t size)
+{
+    std::vector<std::string>& conversionErrors = conversionErrorsTLS.getRef();
+    conversionErrors.clear();
+    conversionErrors.reserve(size);
+}
+
+void pyRaiseCVOverloadException(const std::string& functionName)
+{
+    const std::vector<std::string>& conversionErrors = conversionErrorsTLS.getRef();
+    const std::size_t conversionErrorsCount = conversionErrors.size();
+    if (conversionErrorsCount > 0)
+    {
+        // In modern std libraries small string optimization is used = no dynamic memory allocations,
+        // but it can be applied only for string with length < 18 symbols (in GCC)
+        const std::string bullet = "\n - ";
+
+        // Estimate required buffer size - save dynamic memory allocations = faster
+        std::size_t requiredBufferSize = bullet.size() * conversionErrorsCount;
+        for (std::size_t i = 0; i < conversionErrorsCount; ++i)
+        {
+            requiredBufferSize += conversionErrors[i].size();
+        }
+
+        // Only string concatenation is required so std::string is way faster than
+        // std::ostringstream
+        std::string errorMessage("Overload resolution failed:");
+        errorMessage.reserve(errorMessage.size() + requiredBufferSize);
+        for (std::size_t i = 0; i < conversionErrorsCount; ++i)
+        {
+            errorMessage += bullet;
+            errorMessage += conversionErrors[i];
+        }
+        cv::Exception exception(CV_StsBadArg, errorMessage, functionName, "", -1);
+        pyRaiseCVException(exception);
+    }
+    else
+    {
+        cv::Exception exception(CV_StsInternal, "Overload resolution failed, but no errors reported",
+                                functionName, "", -1);
+        pyRaiseCVException(exception);
+    }
+}
+
+void pyPopulateArgumentConversionErrors()
+{
+    if (PyErr_Occurred())
+    {
+        PySafeObject exception_type;
+        PySafeObject exception_value;
+        PySafeObject exception_traceback;
+        PyErr_Fetch(exception_type, exception_value, exception_traceback);
+        PyErr_NormalizeException(exception_type, exception_value,
+                                 exception_traceback);
+
+        PySafeObject exception_message(PyObject_Str(exception_value));
+        std::string message;
+        getUnicodeString(exception_message, message);
+#ifdef CV_CXX11
+        conversionErrorsTLS.getRef().push_back(std::move(message));
+#else
+        conversionErrorsTLS.getRef().push_back(message);
+#endif
+    }
+}
+
 } // namespace
 
 typedef std::vector<uchar> vector_uchar;

modules/python/src2/gen2.py

@@ -174,6 +174,14 @@
 gen_template_rw_prop_init = Template("""
     {(char*)"${member}", (getter)pyopencv_${name}_get_${member}, (setter)pyopencv_${name}_set_${member}, (char*)"${member}", NULL},""")
 
+gen_template_overloaded_function_call = Template("""
+    {
+${variant}
+
+        pyPopulateArgumentConversionErrors();
+    }
+""")
+
 class FormatStrings:
     string = 's'
     unsigned_char = 'b'
@@ -768,8 +776,12 @@ def gen_code(self, codegen):
             # if the function/method has only 1 signature, then just put it
             code += all_code_variants[0]
         else:
-            # try to execute each signature
-            code += "    PyErr_Clear();\n\n".join(["    {\n" + v + "    }\n" for v in all_code_variants])
+            # try to execute each signature, add an interlude between function
+            # calls to collect error from all conversions
+            code += '    pyPrepareArgumentConversionErrorsStorage({});\n'.format(len(all_code_variants))
+            code += '    \n'.join(gen_template_overloaded_function_call.substitute(variant=v)
+                                  for v in all_code_variants)
+            code += '    pyRaiseCVOverloadException("{}");\n'.format(self.name)
 
         def_ret = "NULL"
         if self.isconstructor:

modules/python/test/test_misc.py

@@ -73,6 +73,44 @@ def test_error_handler(status, func_name, err_msg, file_name, line):
         except cv.error as _e:
             pass
 
+    def test_overload_resolution_can_choose_correct_overload(self):
+        val = 123
+        point = (51, 165)
+        self.assertEqual(cv.utils.testOverloadResolution(val, point),
+                         'overload (int={}, point=(x={}, y={}))'.format(val, *point),
+                         "Can't select first overload if all arguments are provided as positional")
+
+        self.assertEqual(cv.utils.testOverloadResolution(val, point=point),
+                         'overload (int={}, point=(x={}, y={}))'.format(val, *point),
+                         "Can't select first overload if one of the arguments are provided as keyword")
+
+        self.assertEqual(cv.utils.testOverloadResolution(val),
+                         'overload (int={}, point=(x=42, y=24))'.format(val),
+                         "Can't select first overload if one of the arguments has default value")
+
+        rect = (1, 5, 10, 23)
+        self.assertEqual(cv.utils.testOverloadResolution(rect),
+                         'overload (rect=(x={}, y={}, w={}, h={}))'.format(*rect),
+                         "Can't select second overload if all arguments are provided")
+
+    def test_overload_resolution_fails(self):
+        def test_overload_resolution(msg, *args, **kwargs):
+            no_exception_msg = 'Overload resolution failed without any exception for: "{}"'.format(msg)
+            wrong_exception_msg = 'Overload resolution failed with wrong exception type for: "{}"'.format(msg)
+            with self.assertRaises((cv.error, Exception), msg=no_exception_msg) as cm:
+                cv.utils.testOverloadResolution(*args, **kwargs)
+            self.assertEqual(type(cm.exception), cv.error, wrong_exception_msg)
+
+        test_overload_resolution('wrong second arg type (keyword arg)', 5, point=(1, 2, 3))
+        test_overload_resolution('wrong second arg type', 5, 2)
+        test_overload_resolution('wrong first arg', 3.4, (12, 21))
+        test_overload_resolution('wrong first arg, no second arg', 4.5)
+        test_overload_resolution('wrong args number for first overload', 3, (12, 21), 123)
+        test_overload_resolution('wrong args number for second overload', (3, 12, 12, 1), (12, 21))
+        # One of the common problems
+        test_overload_resolution('rect with float coordinates', (4.5, 4, 2, 1))
+        test_overload_resolution('rect with wrong number of coordinates', (4, 4, 1))
+
 
 class Arguments(NewOpenCVTests):