From 67e8334ac82a0aa80667abeea8f68960824e1019 Mon Sep 17 00:00:00 2001 From: William S Fulton Date: Sun, 5 Jan 2020 14:06:03 +0000 Subject: [PATCH] Python -builtin constructors silently ignored keyword arguments. Instead of silenty ignoring them, now a "TypeError: f() takes no keyword arguments" exception is thrown if keyword arguments are used. Hence constructors and normal methods/functions behave in the same way. Closes issue #1595 --- CHANGES.current | 7 + Examples/test-suite/common.mk | 1 + Examples/test-suite/cpp_parameters.i | 46 +++ .../test-suite/python/cpp_parameters_runme.py | 296 ++++++++++++++++++ Lib/python/pyrun.swg | 13 + Source/Modules/python.cxx | 17 +- 6 files changed, 376 insertions(+), 4 deletions(-) create mode 100644 Examples/test-suite/cpp_parameters.i create mode 100644 Examples/test-suite/python/cpp_parameters_runme.py diff --git a/CHANGES.current b/CHANGES.current index 09facd90b..e4f18ca41 100644 --- a/CHANGES.current +++ b/CHANGES.current @@ -7,6 +7,13 @@ the issue number to the end of the URL: https://github.com/swig/swig/issues/ Version 4.0.2 (in progress) =========================== +2020-01-13: wsfulton + [Python] #1595 Python -builtin constructors silently ignored keyword arguments. + Instead of silenty ignoring them, now a "TypeError: f() takes no keyword arguments" + exception is thrown if keyword arguments are used. Hence constructors and normal methods/ + functions behave in the same way. Note, -keyword should be used with -builtin to obtain + keyword argument support. + 2019-12-10: wsfulton #1679 Fix parsing of C++11 identifiers with special meaning (final and override) when they are used as part of the scope name of an identifier, such as a namespace name. diff --git a/Examples/test-suite/common.mk b/Examples/test-suite/common.mk index c817bdf80..00aa4922d 100644 --- a/Examples/test-suite/common.mk +++ b/Examples/test-suite/common.mk @@ -159,6 +159,7 @@ CPP_TEST_CASES += \ cpp_enum \ cpp_namespace \ cpp_nodefault \ + cpp_parameters \ cpp_static \ cpp_typedef \ cpp14_binary_integer_literals \ diff --git a/Examples/test-suite/cpp_parameters.i b/Examples/test-suite/cpp_parameters.i new file mode 100644 index 000000000..e8a4c94fd --- /dev/null +++ b/Examples/test-suite/cpp_parameters.i @@ -0,0 +1,46 @@ +%module cpp_parameters + +%{ +// For Perl +#ifdef Zero +#undef Zero +#endif +%} +%inline %{ + +// Zero arguments +struct Zero { + Zero() {} + int zero() { return 0; } + static int stat_zero() { return 0; } +}; +// One mandatory argument +struct One { + One(int a) {} + int one(int a) { return a; } + static int stat_one(int a) { return a; } +}; +// Two mandatory arguments +struct Two { + Two(int a, int b) {} + int two(int a, int b) { return a + b; } + static int stat_two(int a, int b) { return a + b; } +}; +// Single optional argument +struct Single { + Single(int a=0) {} + int single(int a=0) { return a; } + static int stat_single(int a=0) { return a; } +}; + +int global_zero() { return 0; } +int global_one(int a) { return a; } +int global_two(int a, int b) { return a + b; } +int global_single(int a=0) { return a; } + +#ifdef SWIGPYTHON_BUILTIN +bool is_python_builtin() { return true; } +#else +bool is_python_builtin() { return false; } +#endif +%} diff --git a/Examples/test-suite/python/cpp_parameters_runme.py b/Examples/test-suite/python/cpp_parameters_runme.py new file mode 100644 index 000000000..99d14ad74 --- /dev/null +++ b/Examples/test-suite/python/cpp_parameters_runme.py @@ -0,0 +1,296 @@ +from cpp_parameters import * + +# Testing correct and incorrect parameter counts being passed (kwargs and non-kwargs) +# Note that the implementation depends a lot on whether zero, one, two or more args are being wrapped + +def is_python_fastproxy(): + """Return True if SWIG is generating Python code using -fastproxy.""" + import cpp_parameters + # Note: _swig_new_instance_method is only generated when using -fastproxy + return hasattr(cpp_parameters, "_swig_new_instance_method") + +# Zero parameters expected +x = Zero() +try: + x = Zero(z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + x = Zero(0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +try: + x.zero(z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + x.zero(0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +try: + Zero.stat_zero(z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + Zero.stat_zero(0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +try: + global_zero(z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + global_zero(0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +# One mandatory parameter expected +x = One(1) +try: + x = One(a=1, z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + x = One(1, 0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +try: + x.one(a=1, z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + x.one(1, 0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +try: + One.stat_one(a=1, z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + One.stat_one(1, 0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +try: + global_one(a=1, z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + global_one(1, 0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +# Two mandatory parameters expected +x = Two(1, 2) +try: + x = Two(a=1, b=2, z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + x = Two(1, 2, 0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +try: + x.two(a=1, b=2, z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + x.two(1, 2, 0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +try: + Two.stat_two(a=1, b=2, z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + Two.stat_two(1, 2, 0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +try: + global_two(a=1, b=2, z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + global_two(1, 2, 0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +# Single optional parameter expected +x = Single(1) +try: + x = Single(a=1, z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + x = Single(1, 0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +try: + x.single(a=1, z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + x.single(1, 0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +try: + Single.stat_single(a=1, z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + Single.stat_single(1, 0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +try: + global_single(a=1, z=0) + raise RuntimeError("Missed throw") +except TypeError: + pass +try: + global_single(1, 0) + raise RuntimeError("Missed throw") +except TypeError: + pass + +# Test that -builtin option throws TypeError if kwargs are used even when they look like they should work, kwargs are not supported unless using -keyword. +# Also same for -fastproxy option except that kwargs are supported by default for constructors. TODO: Fix inconsistency. + +if is_python_builtin() or is_python_fastproxy(): + # One mandatory parameter in API + x = One(1) + if is_python_fastproxy(): + x = One(a=1) + else: + try: + x = One(a=1) + raise RuntimeError("Missed throw") + except TypeError: + pass + try: + x.one(a=1) + raise RuntimeError("Missed throw") + except TypeError: + pass + try: + One.stat_one(a=1) + raise RuntimeError("Missed throw") + except TypeError: + pass + try: + global_one(a=1) + raise RuntimeError("Missed throw") + except TypeError: + pass + + # Two mandatory parameters in API + x = Two(1, 2) + if is_python_fastproxy(): + x = Two(a=1, b=2) + else: + try: + x = Two(a=1, b=2) + raise RuntimeError("Missed throw") + except TypeError: + pass + try: + x.two(a=1, b=2) + raise RuntimeError("Missed throw") + except TypeError: + pass + try: + Two.stat_two(a=1, b=2) + raise RuntimeError("Missed throw") + except TypeError: + pass + try: + global_two(a=1, b=2) + raise RuntimeError("Missed throw") + except TypeError: + pass + + # Single optional parameter in API + x = Single(1) + if is_python_fastproxy(): + x = Single(a=1) + else: + try: + x = Single(a=1) + raise RuntimeError("Missed throw") + except TypeError: + pass + try: + x.single(a=1) + raise RuntimeError("Missed throw") + except TypeError: + pass + try: + Single.stat_single(a=1) + raise RuntimeError("Missed throw") + except TypeError: + pass + try: + global_single(a=1) + raise RuntimeError("Missed throw") + except TypeError: + pass + +else: + # Non-builtin should work as expected + # One mandatory parameter in API + x = One(a=1) + x.one(a=1) + One.stat_one(a=1) + global_one(a=1) + + # Two mandatory parameters in API + x = Two(a=1, b=2) + x.two(a=1, b=2) + Two.stat_two(a=1, b=2) + global_two(a=1, b=2) + + # Single optional parameter in API + x = Single(a=1) + x.single(a=1) + Single.stat_single(a=1) + global_single(a=1) diff --git a/Lib/python/pyrun.swg b/Lib/python/pyrun.swg index 82859b887..218018177 100644 --- a/Lib/python/pyrun.swg +++ b/Lib/python/pyrun.swg @@ -183,6 +183,19 @@ SWIG_Python_UnpackTuple(PyObject *args, const char *name, Py_ssize_t min, Py_ssi } } +SWIGINTERN int +SWIG_Python_CheckNoKeywords(PyObject *kwargs, const char *name) { + int no_kwargs = 1; + if (kwargs) { + assert(PyDict_Check(kwargs)); + if (PyDict_Size(kwargs) > 0) { + PyErr_Format(PyExc_TypeError, "%s() does not take keyword arguments", name); + no_kwargs = 0; + } + } + return no_kwargs; +} + /* A functor is a function object with one single object argument */ #define SWIG_Python_CallFunctor(functor, obj) PyObject_CallFunctionObjArgs(functor, obj, NULL); diff --git a/Source/Modules/python.cxx b/Source/Modules/python.cxx index 4792090ea..1e211fb8b 100644 --- a/Source/Modules/python.cxx +++ b/Source/Modules/python.cxx @@ -2493,7 +2493,7 @@ public: String *symname = Getattr(n, "sym:name"); String *wname = Swig_name_wrapper(symname); - const char *builtin_kwargs = builtin_ctor ? ", PyObject *SWIGUNUSEDPARM(kwargs)" : ""; + const char *builtin_kwargs = builtin_ctor ? ", PyObject *kwargs" : ""; Printv(f->def, linkage, builtin_ctor ? "int " : "PyObject *", wname, "(PyObject *self, PyObject *args", builtin_kwargs, ") {", NIL); Wrapper_add_local(f, "argc", "Py_ssize_t argc"); @@ -2503,6 +2503,9 @@ public: if (!fastunpack) { Wrapper_add_local(f, "ii", "Py_ssize_t ii"); + if (builtin_ctor) + Printf(f->code, "if (!SWIG_Python_CheckNoKeywords(kwargs, \"%s\")) SWIG_fail;\n", symname); + if (maxargs - (add_self ? 1 : 0) > 0) { Append(f->code, "if (!PyTuple_Check(args)) SWIG_fail;\n"); Append(f->code, "argc = PyObject_Length(args);\n"); @@ -2518,8 +2521,9 @@ public: if (add_self) Append(f->code, "argc++;\n"); } else { - String *iname = Getattr(n, "sym:name"); - Printf(f->code, "if (!(argc = SWIG_Python_UnpackTuple(args, \"%s\", 0, %d, argv%s))) SWIG_fail;\n", iname, maxargs, add_self ? "+1" : ""); + if (builtin_ctor) + Printf(f->code, "if (!SWIG_Python_CheckNoKeywords(kwargs, \"%s\")) SWIG_fail;\n", symname); + Printf(f->code, "if (!(argc = SWIG_Python_UnpackTuple(args, \"%s\", 0, %d, argv%s))) SWIG_fail;\n", symname, maxargs, add_self ? "+1" : ""); if (add_self) Append(f->code, "argv[0] = self;\n"); else @@ -2713,7 +2717,7 @@ public: Append(wname, overname); } - const char *builtin_kwargs = builtin_ctor ? ", PyObject *SWIGUNUSEDPARM(kwargs)" : ""; + const char *builtin_kwargs = builtin_ctor ? ", PyObject *kwargs" : ""; if (!allow_kwargs || overname) { if (!varargs) { Printv(f->def, linkage, wrap_return, wname, "(PyObject *", self_param, ", PyObject *args", builtin_kwargs, ") {", NIL); @@ -2886,6 +2890,7 @@ public: funpack = 0; } else { Clear(parse_args); + if (funpack) { Clear(f->def); if (overname) { @@ -2898,6 +2903,8 @@ public: } else { int is_tp_call = Equal(Getattr(n, "feature:python:slot"), "tp_call"); Printv(f->def, linkage, wrap_return, wname, "(PyObject *", self_param, ", PyObject *args", builtin_kwargs, ") {", NIL); + if (builtin_ctor) + Printf(parse_args, "if (!SWIG_Python_CheckNoKeywords(kwargs, \"%s\")) SWIG_fail;\n", iname); if (onearg && !builtin_ctor && !is_tp_call) { Printf(parse_args, "if (!args) SWIG_fail;\n"); Append(parse_args, "swig_obj[0] = args;\n"); @@ -2908,6 +2915,8 @@ public: } } } else { + if (builtin_ctor) + Printf(parse_args, "if (!SWIG_Python_CheckNoKeywords(kwargs, \"%s\")) SWIG_fail;\n", iname); if (builtin && in_class && tuple_arguments == 0) { Printf(parse_args, " if (args && PyTuple_Check(args) && PyTuple_GET_SIZE(args) > 0) SWIG_exception_fail(SWIG_TypeError, \"%s takes no arguments\");\n", iname); } else {