From 7beb20c6296b6c4154d769b021a4828ad3e27e39 Mon Sep 17 00:00:00 2001 From: Midhun PM Date: Thu, 20 Feb 2020 10:06:30 -0600 Subject: [PATCH 01/10] Fix ref count error issue #906 --- traits/ctraits.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/traits/ctraits.c b/traits/ctraits.c index af53e4ece..1703f91cf 100644 --- a/traits/ctraits.c +++ b/traits/ctraits.c @@ -4094,7 +4094,7 @@ validate_trait_complex( case 22: /* Callable check: */ { if (PyCallable_Check(value)) { - return value; + goto done; } else if (value == Py_None) { int allow_none; @@ -4102,8 +4102,7 @@ validate_trait_complex( //Handle callables without allow_none, default to allow None if (tuple_size < 2) { - Py_INCREF(value); - return value; + goto done; } allow_none = PyObject_IsTrue(PyTuple_GET_ITEM(trait->py_validate, 1)); @@ -4113,7 +4112,7 @@ validate_trait_complex( } else if (allow_none) { - return value; + goto done; } } break; From c053dc993744b0062be2c82ec3efd3aa932ddeff Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Thu, 27 Feb 2020 10:02:52 +0000 Subject: [PATCH 02/10] Regression test for #906 --- traits/tests/test_callable.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/traits/tests/test_callable.py b/traits/tests/test_callable.py index a33309c66..098f31813 100644 --- a/traits/tests/test_callable.py +++ b/traits/tests/test_callable.py @@ -91,6 +91,21 @@ def test_callable_in_complex_trait(self): a.callable_or_str = value self.assertEqual(a.callable_or_str, old_value) + def test_compound_callable_refcount(self): + # Regression test for enthought/traits#906. + + def my_function(): + pass + + a = MyCallable() + + string_value = "some string" + callable_value = my_function + + for _ in range(10): + a.callable_or_str = string_value + a.callable_or_str = callable_value + def test_disallow_none(self): class MyNewCallable(HasTraits): From 9d273246491bfcc136daed05115d2d33d2bd7eab Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Thu, 27 Feb 2020 13:54:19 +0000 Subject: [PATCH 03/10] Expanded tests; cleanup of implementation --- traits/ctraits.c | 69 ++++++++++++++++++++--------------- traits/tests/test_callable.py | 51 +++++++++++++++++++++++--- 2 files changed, 85 insertions(+), 35 deletions(-) diff --git a/traits/ctraits.c b/traits/ctraits.c index 1703f91cf..30c50f927 100644 --- a/traits/ctraits.c +++ b/traits/ctraits.c @@ -2921,7 +2921,7 @@ trait_new(PyTypeObject *trait_type, PyObject *args, PyObject *kw) } PyErr_Format( - TraitError, + TraitError, "Invalid argument to trait constructor. The argument `kind` " "must be an integer between 0 and 8 but a value of %d was provided.", kind); @@ -3679,33 +3679,40 @@ validate_trait_callable( trait_object *trait, has_traits_object *obj, PyObject *name, PyObject *value) { - if (PyCallable_Check(value)) { - Py_INCREF(value); - return value; - } - else if (value == Py_None) { + PyObject *type_info = trait->py_validate; + + if (value == Py_None) { int allow_none; - int tuple_size = PyTuple_GET_SIZE(trait->py_validate); - //Handle callables without allow_none, default to allow None - if (tuple_size < 2) { - Py_INCREF(value); - return value; + /* Backwards compatibility with the old Callable: + allow None by default. */ + if (PyTuple_GET_SIZE(type_info) < 2) { + goto done; } - allow_none = PyObject_IsTrue(PyTuple_GET_ITEM(trait->py_validate, 1)); - + allow_none = PyObject_IsTrue( + PyTuple_GET_ITEM(type_info, 1)); if (allow_none == -1) { + /* propagate any exception arising from + conversion to bool */ return NULL; } - - else if (allow_none) { - Py_INCREF(value); - return value; + if (allow_none) { + goto done; } } + else if (PyCallable_Check(value)) { + /* No need to check return value: PyCallable_Check + always succeeds. */ + goto done; + } - return raise_trait_error(trait, obj, name, value); + raise_trait_error(trait, obj, name, value); + return NULL; + + done: + Py_INCREF(value); + return value; } /*----------------------------------------------------------------------------- @@ -4093,28 +4100,32 @@ validate_trait_complex( case 22: /* Callable check: */ { - if (PyCallable_Check(value)) { - goto done; - } - else if (value == Py_None) { + if (value == Py_None) { int allow_none; - int tuple_size = PyTuple_GET_SIZE(trait->py_validate); - //Handle callables without allow_none, default to allow None - if (tuple_size < 2) { + /* Backwards compatibility with the old Callable: + allow None by default. */ + if (PyTuple_GET_SIZE(type_info) < 2) { goto done; } - allow_none = PyObject_IsTrue(PyTuple_GET_ITEM(trait->py_validate, 1)); - + allow_none = PyObject_IsTrue( + PyTuple_GET_ITEM(type_info, 1)); if (allow_none == -1) { + /* propagate any exception arising from + conversion to bool */ return NULL; } - - else if (allow_none) { + if (allow_none) { goto done; } } + else if (PyCallable_Check(value)) { + /* No need to check return value: PyCallable_Check + always succeeds. */ + goto done; + } + break; } diff --git a/traits/tests/test_callable.py b/traits/tests/test_callable.py index 098f31813..2a0e96b25 100644 --- a/traits/tests/test_callable.py +++ b/traits/tests/test_callable.py @@ -34,10 +34,37 @@ def instance_method(self): pass +class OldCallable(BaseCallable): + """ + Old-style Callable, whose validation tuple doesn't include + the allow_none field. + + We only care about this case because it's possible that old pickles + could include Callable instances whose validation tuple has length 1. + """ + def __init__(self, value=None, **metadata): + self.fast_validate = (ValidateTrait.callable,) + super().__init__(value, **metadata) + + +class Unbool: + """ + Object that can't be interpreted as a bool, for testing purposes. + """ + def __bool__(self): + raise ZeroDivisionError() + + class MyCallable(HasTraits): value = Callable() - callable_or_str = Either(Callable(), Str) + callable_or_str = Either(Callable, Str) + + old_callable_or_str = Either(OldCallable, Str) + + bad_allow_none = Either(Callable(allow_none=Unbool()), Str) + + non_none_callable_or_str = Either(Callable(allow_none=False), Str) class MyBaseCallable(HasTraits): @@ -144,12 +171,24 @@ class MyNewCallable2(HasTraits): obj.a_non_none_union = None obj.a_allow_none_union = None - def test_old_style_callable(self): - class OldCallable(Callable): - def __init__(self, value=None, **metadata): - self.fast_validate = (ValidateTrait.callable,) - super(BaseCallable, self).__init__(value, **metadata) + def test_implicitly_allowed_none_in_compound(self): + obj = MyCallable() + obj.old_callable_or_str = "bob" + obj.old_callable_or_str = None + self.assertIsNone(obj.old_callable_or_str) + + def test_non_bool_allow_none(self): + obj = MyCallable() + obj.bad_allow_none = "a string" + with self.assertRaises(ZeroDivisionError): + obj.bad_allow_none = None + def test_none_not_allowed_in_compound(self): + obj = MyCallable() + with self.assertRaises(TraitError): + obj.non_none_callable_or_str = None + + def test_old_style_callable(self): class MyCallable(HasTraits): # allow_none flag should be ineffective value = OldCallable() From 955a2ae4933576c8b59ec29adac689d594e94757 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Thu, 27 Feb 2020 14:39:58 +0000 Subject: [PATCH 04/10] Spacing consistency tweak --- traits/ctraits.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/traits/ctraits.c b/traits/ctraits.c index 30c50f927..7e2db77ac 100644 --- a/traits/ctraits.c +++ b/traits/ctraits.c @@ -3685,7 +3685,7 @@ validate_trait_callable( int allow_none; /* Backwards compatibility with the old Callable: - allow None by default. */ + allow None by default. */ if (PyTuple_GET_SIZE(type_info) < 2) { goto done; } From 34541798377488e1601512723a2bcbc289d63f3a Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Thu, 27 Feb 2020 16:54:09 +0000 Subject: [PATCH 05/10] Refactor to remove the repetition --- traits/ctraits.c | 89 +++++++++++++++++++++--------------------------- 1 file changed, 38 insertions(+), 51 deletions(-) diff --git a/traits/ctraits.c b/traits/ctraits.c index 7e2db77ac..3f88bf3bc 100644 --- a/traits/ctraits.c +++ b/traits/ctraits.c @@ -3674,45 +3674,50 @@ validate_trait_function( | Verifies a Python value is a callable (or None): +----------------------------------------------------------------------------*/ -static PyObject * -validate_trait_callable( - trait_object *trait, has_traits_object *obj, PyObject *name, - PyObject *value) -{ - PyObject *type_info = trait->py_validate; +/* Internal function for validation: - if (value == Py_None) { - int allow_none; + Return 1 if the value is valid, 0 if it is invalid, and return -1 + and set an error condition if anything goes wrong. +*/ - /* Backwards compatibility with the old Callable: - allow None by default. */ +static int +_validate_trait_callable( + PyObject *type_info, PyObject *value) +{ + if (value == Py_None) { if (PyTuple_GET_SIZE(type_info) < 2) { - goto done; + /* Backwards compatibility with old Callable */ + return 1; } - - allow_none = PyObject_IsTrue( - PyTuple_GET_ITEM(type_info, 1)); - if (allow_none == -1) { - /* propagate any exception arising from - conversion to bool */ - return NULL; - } - if (allow_none) { - goto done; + else { + /* 2nd element of tuple determines whether None is allowed */ + return PyObject_IsTrue(PyTuple_GET_ITEM(type_info, 1)); } } else if (PyCallable_Check(value)) { - /* No need to check return value: PyCallable_Check - always succeeds. */ - goto done; + return 1; + } + else { + return 0; } +} - raise_trait_error(trait, obj, name, value); - return NULL; - done: - Py_INCREF(value); - return value; +static PyObject * +validate_trait_callable( + trait_object *trait, has_traits_object *obj, PyObject *name, + PyObject *value) +{ + int valid = _validate_trait_callable(trait->py_validate, value); + + if (valid == -1) { + return NULL; + } + if (valid == 1) { + Py_INCREF(value); + return value; + } + return raise_trait_error(trait, obj, name, value); } /*----------------------------------------------------------------------------- @@ -4100,32 +4105,14 @@ validate_trait_complex( case 22: /* Callable check: */ { - if (value == Py_None) { - int allow_none; - - /* Backwards compatibility with the old Callable: - allow None by default. */ - if (PyTuple_GET_SIZE(type_info) < 2) { - goto done; - } + int valid = _validate_trait_callable(type_info, value); - allow_none = PyObject_IsTrue( - PyTuple_GET_ITEM(type_info, 1)); - if (allow_none == -1) { - /* propagate any exception arising from - conversion to bool */ - return NULL; - } - if (allow_none) { - goto done; - } + if (valid == -1) { + return NULL; } - else if (PyCallable_Check(value)) { - /* No need to check return value: PyCallable_Check - always succeeds. */ + if (valid == 1) { goto done; } - break; } From 2ac9c58dd938b6303909f515ff0137fcb3efd7c3 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Thu, 27 Feb 2020 16:57:12 +0000 Subject: [PATCH 06/10] Fix unnecessary line break --- traits/ctraits.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/traits/ctraits.c b/traits/ctraits.c index 3f88bf3bc..39a2d5413 100644 --- a/traits/ctraits.c +++ b/traits/ctraits.c @@ -3681,8 +3681,7 @@ validate_trait_function( */ static int -_validate_trait_callable( - PyObject *type_info, PyObject *value) +_validate_trait_callable(PyObject *type_info, PyObject *value) { if (value == Py_None) { if (PyTuple_GET_SIZE(type_info) < 2) { From d44d752c4e6a68f8b513eafb622f80dffd7f6bb9 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Thu, 27 Feb 2020 17:09:11 +0000 Subject: [PATCH 07/10] Simplify a check --- traits/ctraits.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/traits/ctraits.c b/traits/ctraits.c index 39a2d5413..a2816ecf1 100644 --- a/traits/ctraits.c +++ b/traits/ctraits.c @@ -3693,11 +3693,8 @@ _validate_trait_callable(PyObject *type_info, PyObject *value) return PyObject_IsTrue(PyTuple_GET_ITEM(type_info, 1)); } } - else if (PyCallable_Check(value)) { - return 1; - } else { - return 0; + return PyCallable_Check(value); } } From be519cab6089b079ed1bf8cec4956f045b9773a3 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Fri, 28 Feb 2020 08:40:05 +0000 Subject: [PATCH 08/10] Add comment about number of repetitions needed --- traits/tests/test_callable.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/traits/tests/test_callable.py b/traits/tests/test_callable.py index 2a0e96b25..52ea96474 100644 --- a/traits/tests/test_callable.py +++ b/traits/tests/test_callable.py @@ -129,7 +129,9 @@ def my_function(): string_value = "some string" callable_value = my_function - for _ in range(10): + # We seem to need at least 3 repetitions to reliably produce + # a crash. Let's hoik it up to 5 to be safe. + for _ in range(5): a.callable_or_str = string_value a.callable_or_str = callable_value From 7ebd6e8b65e49b69d2bdf3b79bdf7bbdf654e077 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Fri, 28 Feb 2020 08:40:42 +0000 Subject: [PATCH 09/10] Spelling --- traits/tests/test_callable.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/traits/tests/test_callable.py b/traits/tests/test_callable.py index 52ea96474..ad06301f0 100644 --- a/traits/tests/test_callable.py +++ b/traits/tests/test_callable.py @@ -130,7 +130,7 @@ def my_function(): callable_value = my_function # We seem to need at least 3 repetitions to reliably produce - # a crash. Let's hoik it up to 5 to be safe. + # a crash. Let's hoick it up to 5 to be safe. for _ in range(5): a.callable_or_str = string_value a.callable_or_str = callable_value From f705c78b651ac411d196c69721155e52bed573a1 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Fri, 28 Feb 2020 08:50:26 +0000 Subject: [PATCH 10/10] Add an assertion, just to make sure the function is still usable --- traits/tests/test_callable.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/traits/tests/test_callable.py b/traits/tests/test_callable.py index ad06301f0..ce077836c 100644 --- a/traits/tests/test_callable.py +++ b/traits/tests/test_callable.py @@ -122,7 +122,7 @@ def test_compound_callable_refcount(self): # Regression test for enthought/traits#906. def my_function(): - pass + return 72 a = MyCallable() @@ -135,6 +135,8 @@ def my_function(): a.callable_or_str = string_value a.callable_or_str = callable_value + self.assertEqual(a.callable_or_str(), 72) + def test_disallow_none(self): class MyNewCallable(HasTraits):