From 76834ad504001aa8f38abc300ce7a4025bfb5f9d Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Fri, 8 Dec 2023 12:05:15 +0000 Subject: [PATCH 1/3] Warn if a Trait default method raises AttributeError --- traits/ctraits.c | 67 ++++++++++++++++++++++++++++++--- traits/tests/test_regression.py | 23 +++++++++++ 2 files changed, 84 insertions(+), 6 deletions(-) diff --git a/traits/ctraits.c b/traits/ctraits.c index 91c0bfc25..d2dd54366 100644 --- a/traits/ctraits.c +++ b/traits/ctraits.c @@ -1786,6 +1786,58 @@ static PyTypeObject has_traits_type = { | Returns the default value associated with a specified trait: +----------------------------------------------------------------------------*/ +// Since traits are attributes, AttributeErrors are naturally swallowed by +// some parts of the Traits machinery (e.g., trait_get). So if computation of +// the default value raises AttributeError, we issue a warning, using the +// following helper function. +// xref: https://github.com/enthought/traits/issues/1747 + +void +_warn_on_attribute_error(PyObject *result) +{ + if (result == NULL && PyErr_ExceptionMatches(PyExc_AttributeError)) { + // PyErr_WarnEx won't work correctly if there's an exception set, + // so save and clear the current exception. + PyObject *exc_type, *exc_value, *exc_traceback; + PyErr_Fetch(&exc_type, &exc_value, &exc_traceback); + if (PyErr_WarnEx(PyExc_UserWarning, + "default value resolution raised an AttributeError; " + "this may cause Traits to behave in unexpected ways", 0) == -1) { + // The warning was raised as an exception; retrieve that exception + // and set the AttributeError as its cause. + PyObject *warn_type, *warn_value, *warn_traceback; + + // Normalize the AttributeError to ensure exc_value is valid. + PyErr_NormalizeException(&exc_type, &exc_value, &exc_traceback); + if (exc_traceback != NULL) { + PyException_SetTraceback(exc_value, exc_traceback); + } + assert(exc_value != NULL); + + // Set the warning cause to the attribute error. The UserWarning + // shouldn't need to be normalized, since PyErr_WarnEx doesn't + // indulge in delayed normalization. + PyErr_Fetch(&warn_type, &warn_value, &warn_traceback); + assert(warn_value != NULL); + PyException_SetCause(warn_value, exc_value); + PyErr_Restore(warn_type, warn_value, warn_traceback); + + // Reference cleanup: PyErr_Restore stole our references to + // warn_type, warn_value and warn_traceback, and + // PyException_SetCause stole our reference to exc_value; that + // leaves us to handle exc_type and exc_traceback. + Py_DECREF(exc_type); + if (exc_traceback != NULL) { + Py_DECREF(exc_traceback); + } + } + else { + // Restore the AttributeError + PyErr_Restore(exc_type, exc_value, exc_traceback); + } + } +} + static PyObject * default_value_for(trait_object *trait, has_traits_object *obj, PyObject *name) { @@ -1820,8 +1872,10 @@ default_value_for(trait_object *trait, has_traits_object *obj, PyObject *name) if (kw == Py_None) { kw = NULL; } - return PyObject_Call( + result = PyObject_Call( PyTuple_GET_ITEM(dv, 0), PyTuple_GET_ITEM(dv, 1), kw); + _warn_on_attribute_error(result); + return result; case CALLABLE_DEFAULT_VALUE: tuple = PyTuple_Pack(1, (PyObject *)obj); if (tuple == NULL) { @@ -1834,17 +1888,18 @@ default_value_for(trait_object *trait, has_traits_object *obj, PyObject *name) if (trait->flags & TRAIT_SETATTR_ORIGINAL_VALUE) { if (value == NULL) { Py_DECREF(result); - return NULL; + result = NULL; + } else { + Py_DECREF(value); } - Py_DECREF(value); - return result; } else { Py_DECREF(result); - return value; + result = value; } } - break; + _warn_on_attribute_error(result); + return result; case TRAIT_SET_OBJECT_DEFAULT_VALUE: return call_class( TraitSetObject, trait, obj, name, trait->default_value); diff --git a/traits/tests/test_regression.py b/traits/tests/test_regression.py index 45b3f8f44..f14ccb982 100644 --- a/traits/tests/test_regression.py +++ b/traits/tests/test_regression.py @@ -31,6 +31,7 @@ Either, Enum, Expression, + Float, Instance, Int, List, @@ -556,6 +557,28 @@ def test_subclass_disallow_default_value(self): class OverrideDisallow(SubclassDefaultsSuper): disallow_default = "a default value" + def test_warn_on_attribute_error_in_default_method(self): + # Given a misspelled reference to a trait in a _default method ... + class HasBrokenDefault(HasStrictTraits): + weight = Float(12.3) + + mass = Float() + + def _mass_default(self): + # Deliberately misspelled! + return self.wieght / 9.81 + + # When we try to get all trait values on an instance, + # Then a warning is issued ... + obj = HasBrokenDefault() + with self.assertWarnsRegex( + UserWarning, "default value resolution raised an AttributeError" + ): + traits = obj.trait_get() + + # ... and the returned dictionary does not include the mass. + self.assertEqual(traits, {"weight": 12.3}) + class NestedContainerClass(HasTraits): # Used in regression test for changes to nested containers From 45a2e1d2a523c5845e1b43d9a5983d913ee3a121 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Thu, 14 Dec 2023 17:01:33 +0000 Subject: [PATCH 2/3] Suppress an instance of the new warning from another test --- traits/tests/test_property_notifications.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/traits/tests/test_property_notifications.py b/traits/tests/test_property_notifications.py index 05d22d13a..c8f543c7f 100644 --- a/traits/tests/test_property_notifications.py +++ b/traits/tests/test_property_notifications.py @@ -407,9 +407,12 @@ def test_property_with_cache(self): def test_property_default_initializer_with_value_in_init(self): # With "depends_on", instantiation will fail. # enthought/traits#709 - with self.assertRaises(AttributeError): - ClassWithPropertyDependsOnInit( - info_without_default=PersonInfo(age=30)) + with self.assertWarnsRegex( + UserWarning, "default value resolution raised an AttributeError" + ): + with self.assertRaises(AttributeError): + ClassWithPropertyDependsOnInit( + info_without_default=PersonInfo(age=30)) # With "observe", initialization works. instance = ClassWithPropertyObservesInit( From 4819297c90ed73f92e12c456398872a7d7a87843 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Thu, 14 Dec 2023 17:02:29 +0000 Subject: [PATCH 3/3] Add test that covers the CALLABLE_AND_ARGS case --- traits/tests/test_regression.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/traits/tests/test_regression.py b/traits/tests/test_regression.py index f14ccb982..ee6601d90 100644 --- a/traits/tests/test_regression.py +++ b/traits/tests/test_regression.py @@ -579,6 +579,26 @@ def _mass_default(self): # ... and the returned dictionary does not include the mass. self.assertEqual(traits, {"weight": 12.3}) + def test_warn_on_attribute_error_in_factory(self): + + def bad_range(start, stop): + self.this_attribute_doesnt_exist + return range(start, stop) + + class HasBrokenFactory(HasStrictTraits): + ticks = Instance(range, factory=bad_range, args=(0, 10)) + + # When we try to get all trait values on an instance, + # Then a warning is issued ... + obj = HasBrokenFactory() + with self.assertWarnsRegex( + UserWarning, "default value resolution raised an AttributeError" + ): + traits = obj.trait_get() + + # ... and the returned dictionary does not include the bad trait + self.assertEqual(traits, {}) + class NestedContainerClass(HasTraits): # Used in regression test for changes to nested containers