From d7d826aeb9dd0c22e3dd9620e94aafb4dde6ac27 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Mon, 6 Apr 2020 18:21:23 -0500 Subject: [PATCH 1/2] Fix #140 by turning OverflowError into TypeError. Fingers crossed for 32-bit builds on Windows. --- BTrees/BTreeModuleTemplate.c | 19 +++++++++---- BTrees/BTreeTemplate.c | 21 ++++++++++---- BTrees/BucketTemplate.c | 19 +++++++++---- BTrees/_datatypes.py | 4 +-- BTrees/intkeymacros.h | 22 +++++++++++---- BTrees/intvaluemacros.h | 8 +++--- BTrees/tests/common.py | 50 ++++++++++++++++++++++++--------- BTrees/tests/testBTrees.py | 14 ++++----- BTrees/tests/test__datatypes.py | 4 +-- CHANGES.rst | 10 ++++++- docs/overview.rst | 10 +++---- 11 files changed, 126 insertions(+), 55 deletions(-) diff --git a/BTrees/BTreeModuleTemplate.c b/BTrees/BTreeModuleTemplate.c index ee7aef1..f2ee248 100644 --- a/BTrees/BTreeModuleTemplate.c +++ b/BTrees/BTreeModuleTemplate.c @@ -82,9 +82,11 @@ longlong_handle_overflow(PY_LONG_LONG result, int overflow) { if (overflow) { - /* Python 3 tends to have an exception already set, Python 2 not so much */ - if (!PyErr_Occurred()) - PyErr_SetString(PyExc_OverflowError, "couldn't convert integer to C long long"); + PyErr_Clear(); + /* Python 3 tends to have an exception already set, Python 2 not so + much. We want to consistently raise a TypeError. + */ + PyErr_SetString(PyExc_TypeError, "couldn't convert integer to C long long"); return 0; } else if (result == -1 && PyErr_Occurred()) @@ -106,7 +108,7 @@ ulonglong_check(PyObject *ob) long tmp; tmp = PyInt_AS_LONG(ob); if (tmp < 0) { - PyErr_SetString(PyExc_OverflowError, "unsigned value less than 0"); + PyErr_SetString(PyExc_TypeError, "unsigned value less than 0"); return 0; } return 1; @@ -166,7 +168,7 @@ ulonglong_convert(PyObject *ob, unsigned PY_LONG_LONG *value) long tmp; tmp = PyInt_AS_LONG(ob); if (tmp < 0) { - PyErr_SetString(PyExc_OverflowError, "unsigned value less than 0"); + PyErr_SetString(PyExc_TypeError, "unsigned value less than 0"); return 0; } (*value) = (unsigned PY_LONG_LONG)tmp; @@ -182,7 +184,14 @@ ulonglong_convert(PyObject *ob, unsigned PY_LONG_LONG *value) val = PyLong_AsUnsignedLongLong(ob); if (val == (unsigned long long)-1 && PyErr_Occurred()) + { + if (PyErr_ExceptionMatches(PyExc_OverflowError)) + { + PyErr_Clear(); + PyErr_SetString(PyExc_TypeError, "overflow error converting int to C long long"); + } return 0; + } (*value) = val; return 1; } diff --git a/BTrees/BTreeTemplate.c b/BTrees/BTreeTemplate.c index cd61adb..d894202 100644 --- a/BTrees/BTreeTemplate.c +++ b/BTrees/BTreeTemplate.c @@ -1971,12 +1971,6 @@ BTree_getm(BTree *self, PyObject *args) return d; } -static PyObject * -BTree_has_key(BTree *self, PyObject *key) -{ - return _BTree_get(self, key, 1, _BGET_REPLACE_TYPE_ERROR); -} - static PyObject * BTree_setdefault(BTree *self, PyObject *args) { @@ -2081,6 +2075,21 @@ BTree_contains(BTree *self, PyObject *key) return result; } +static PyObject * +BTree_has_key(BTree *self, PyObject *key) +{ + int result = -1; + result = BTree_contains(self, key); + if (result == -1) { + return NULL; + } + + if (result) + Py_RETURN_TRUE; + Py_RETURN_FALSE; +} + + static PyObject * BTree_addUnique(BTree *self, PyObject *args) { diff --git a/BTrees/BucketTemplate.c b/BTrees/BucketTemplate.c index c38d836..c8444f8 100644 --- a/BTrees/BucketTemplate.c +++ b/BTrees/BucketTemplate.c @@ -1367,11 +1367,6 @@ bucket_setstate(Bucket *self, PyObject *state) return Py_None; } -static PyObject * -bucket_has_key(Bucket *self, PyObject *key) -{ - return _bucket_get(self, key, 1); -} static PyObject * bucket_setdefault(Bucket *self, PyObject *args) @@ -1475,6 +1470,20 @@ bucket_contains(Bucket *self, PyObject *key) return result; } +static PyObject * +bucket_has_key(Bucket *self, PyObject *key) +{ + int result = -1; + result = bucket_contains(self, key); + if (result == -1) { + return NULL; + } + + if (result) + Py_RETURN_TRUE; + Py_RETURN_FALSE; +} + /* ** bucket_getm ** diff --git a/BTrees/_datatypes.py b/BTrees/_datatypes.py index b3a5e4d..5035144 100644 --- a/BTrees/_datatypes.py +++ b/BTrees/_datatypes.py @@ -55,7 +55,7 @@ def __call__(self, item): """ Convert *item* into the correct format and return it. - If this cannot be done, raise an appropriate exception. + If this cannot be done, raise a :exc:`TypeError`. """ raise NotImplementedError @@ -239,7 +239,7 @@ def __call__(self, item): # PyPy can raise ValueError converting a negative number to a # unsigned value. if isinstance(item, int_types): - raise OverflowError("Value out of range", item) + raise TypeError("Value out of range", item) raise TypeError(self._error_description) return self._as_python_type(item) diff --git a/BTrees/intkeymacros.h b/BTrees/intkeymacros.h index 7f2e45e..217fc56 100644 --- a/BTrees/intkeymacros.h +++ b/BTrees/intkeymacros.h @@ -23,9 +23,15 @@ #define COPY_KEY_FROM_ARG(TARGET, ARG, STATUS) \ if (INT_CHECK(ARG)) { \ long vcopy = INT_AS_LONG(ARG); \ - if (PyErr_Occurred()) { (STATUS)=0; (TARGET)=0; } \ + if (PyErr_Occurred()) { \ + if (PyErr_ExceptionMatches(PyExc_OverflowError)) { \ + PyErr_Clear(); \ + PyErr_SetString(PyExc_TypeError, "integer out of range"); \ + } \ + (STATUS)=0; (TARGET)=0; \ + } \ else if ((int)vcopy != vcopy) { \ - PyErr_SetString(PyExc_OverflowError, "integer out of range"); \ + PyErr_SetString(PyExc_TypeError, "integer out of range"); \ (STATUS)=0; (TARGET)=0; \ } \ else TARGET = vcopy; \ @@ -55,13 +61,19 @@ #define COPY_KEY_FROM_ARG(TARGET, ARG, STATUS) \ if (INT_CHECK(ARG)) { \ long vcopy = INT_AS_LONG(ARG); \ - if (PyErr_Occurred()) { (STATUS)=0; (TARGET)=0; } \ + if (PyErr_Occurred()) { \ + if (PyErr_ExceptionMatches(PyExc_OverflowError)) { \ + PyErr_Clear(); \ + PyErr_SetString(PyExc_TypeError, "integer out of range"); \ + } \ + (STATUS)=0; (TARGET)=0; \ + } \ else if (vcopy < 0) { \ - PyErr_SetString(PyExc_OverflowError, "can't convert negative value to unsigned int"); \ + PyErr_SetString(PyExc_TypeError, "can't convert negative value to unsigned int"); \ (STATUS)=0; (TARGET)=0; \ } \ else if ((unsigned int)vcopy != vcopy) { \ - PyErr_SetString(PyExc_OverflowError, "integer out of range"); \ + PyErr_SetString(PyExc_TypeError, "integer out of range"); \ (STATUS)=0; (TARGET)=0; \ } \ else TARGET = vcopy; \ diff --git a/BTrees/intvaluemacros.h b/BTrees/intvaluemacros.h index 93a8da3..2af2e58 100644 --- a/BTrees/intvaluemacros.h +++ b/BTrees/intvaluemacros.h @@ -31,7 +31,7 @@ long vcopy = INT_AS_LONG(ARG); \ if (PyErr_Occurred()) { (STATUS)=0; (TARGET)=0; } \ else if ((int)vcopy != vcopy) { \ - PyErr_SetString(PyExc_OverflowError, "integer out of range"); \ + PyErr_SetString(PyExc_TypeError, "integer out of range"); \ (STATUS)=0; (TARGET)=0; \ } \ else TARGET = vcopy; \ @@ -64,11 +64,11 @@ long vcopy = INT_AS_LONG(ARG); \ if (PyErr_Occurred()) { (STATUS)=0; (TARGET)=0; } \ else if (vcopy < 0) { \ - PyErr_SetString(PyExc_OverflowError, "can't convert negative value to unsigned int"); \ + PyErr_SetString(PyExc_TypeError, "can't convert negative value to unsigned int"); \ (STATUS)=0; (TARGET)=0; \ - } \ + } \ else if ((unsigned int)vcopy != vcopy) { \ - PyErr_SetString(PyExc_OverflowError, "integer out of range"); \ + PyErr_SetString(PyExc_TypeError, "integer out of range"); \ (STATUS)=0; (TARGET)=0; \ } \ else TARGET = vcopy; \ diff --git a/BTrees/tests/common.py b/BTrees/tests/common.py index 09ba53b..c40fb1d 100644 --- a/BTrees/tests/common.py +++ b/BTrees/tests/common.py @@ -449,6 +449,19 @@ def testGetReturnsDefault(self): self.assertEqual(self._makeOne().get(1), None) self.assertEqual(self._makeOne().get(1, 'foo'), 'foo') + def testGetReturnsDefaultWrongTypes(self): + self.assertIsNone(self._makeOne().get('abc')) + self.assertEqual(self._makeOne().get('abc', 'def'), 'def') + + def testGetReturnsDefaultOverflowRanges(self): + too_big = 2 ** 64 + 1 + self.assertIsNone(self._makeOne().get(too_big)) + self.assertEqual(self._makeOne().get(too_big, 'def'), 'def') + + too_small = -too_big + self.assertIsNone(self._makeOne().get(too_small)) + self.assertEqual(self._makeOne().get(too_small, 'def'), 'def') + def testSetItemGetItemWorks(self): t = self._makeOne() t[1] = 1 @@ -475,14 +488,24 @@ def testLen(self): self.assertEqual(len(t), len(addl), len(t)) def testHasKeyWorks(self): - from .._compat import PY2 t = self._makeOne() t[1] = 1 - if PY2: - self.assertTrue(t.has_key(1)) - self.assertTrue(1 in t) - self.assertTrue(0 not in t) - self.assertTrue(2 not in t) + self.assertTrue(t.has_key(1)) + + self.assertIn(1, t) + self.assertNotIn(0, t) + self.assertNotIn(2, t) + + def testHasKeyOverflowAndTypes(self): + t = self._makeOne() + + too_big = 2 ** 64 + 1 + too_small = -too_big + self.assertNotIn(too_big, t) + self.assertNotIn(too_small, t) + self.assertFalse(t.has_key(too_big)) + self.assertFalse(t.has_key(too_small)) + self.assertFalse(t.has_key('abc')) def testValuesWorks(self): t = self._makeOne() @@ -1061,10 +1084,11 @@ def testKeyAndValueOverflow(self): # for values that are in range on most other platforms. And on Python 2, # PyInt_Check can fail with a TypeError starting at small values # like 2147483648. So we look for small longs and catch those errors - # even when we think we should be in range. + # even when we think we should be in range. In all cases, our code + # catches the unexpected error (OverflowError) and turns it into TypeError. long_is_32_bit = struct.calcsize('@l') < 8 - in_range_errors = (OverflowError, TypeError) if long_is_32_bit else () - out_of_range_errors = (OverflowError, TypeError) if long_is_32_bit and PY2 else (OverflowError,) + in_range_errors = TypeError + out_of_range_errors = TypeError def trial(i): i = int(i) @@ -2143,11 +2167,11 @@ def testLongIntKeysOutOfRange(self): o1, o2 = self.getTwoValues() t = self._makeOne() k1 = SMALLEST_POSITIVE_65_BITS if self.SUPPORTS_NEGATIVE_KEYS else 2**64 + 1 - with self.assertRaises(OverflowError): + with self.assertRaises(TypeError): t[k1] = o1 t = self._makeOne() - with self.assertRaises(OverflowError): + with self.assertRaises(TypeError): t[LARGEST_NEGATIVE_65_BITS] = o1 class TestLongIntValues(TestLongIntSupport): @@ -2175,11 +2199,11 @@ def testLongIntValuesOutOfRange(self): k1, k2 = self.getTwoKeys() t = self._makeOne() v1 = SMALLEST_POSITIVE_65_BITS if self.SUPPORTS_NEGATIVE_VALUES else 2**64 + 1 - with self.assertRaises(OverflowError): + with self.assertRaises(TypeError): t[k1] = v1 t = self._makeOne() - with self.assertRaises(OverflowError): + with self.assertRaises(TypeError): t[k1] = LARGEST_NEGATIVE_65_BITS diff --git a/BTrees/tests/testBTrees.py b/BTrees/tests/testBTrees.py index b289cf3..8ad3e5f 100644 --- a/BTrees/tests/testBTrees.py +++ b/BTrees/tests/testBTrees.py @@ -114,14 +114,14 @@ def testBasicOps(self): t, keys = self._build_degenerate_tree() self.assertEqual(len(t), len(keys)) self.assertEqual(list(t.keys()), keys) - # has_key actually returns the depth of a bucket. - self.assertEqual(t.has_key(1), 4) - self.assertEqual(t.has_key(3), 4) - self.assertEqual(t.has_key(5), 6) - self.assertEqual(t.has_key(7), 5) - self.assertEqual(t.has_key(11), 5) + + self.assertTrue(t.has_key(1)) + self.assertTrue(t.has_key(3)) + self.assertTrue(t.has_key(5)) + self.assertTrue(t.has_key(7)) + self.assertTrue(t.has_key(11)) for i in 0, 2, 4, 6, 8, 9, 10, 12: - self.assertTrue(i not in t) + self.assertNotIn(i, t) def _checkRanges(self, tree, keys): self.assertEqual(len(tree), len(keys)) diff --git a/BTrees/tests/test__datatypes.py b/BTrees/tests/test__datatypes.py index ed5be3d..869a184 100644 --- a/BTrees/tests/test__datatypes.py +++ b/BTrees/tests/test__datatypes.py @@ -36,7 +36,7 @@ def test_to_int_w_long_in_range(self): pass def test_to_int_w_overflow(self): - self.assertRaises(OverflowError, to_int, 2**64) + self.assertRaises(TypeError, to_int, 2**64) def test_to_int_w_invalid(self): self.assertRaises(TypeError, to_int, ()) @@ -60,7 +60,7 @@ def test_to_long_w_long_in_range(self): pass def test_to_long_w_overflow(self): - self.assertRaises(OverflowError, to_long, 2**64) + self.assertRaises(TypeError, to_long, 2**64) def test_to_long_w_invalid(self): self.assertRaises(TypeError, to_long, ()) diff --git a/CHANGES.rst b/CHANGES.rst index 2779f53..5101d63 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -5,7 +5,15 @@ 4.7.2 (unreleased) ================== -- Nothing changed yet. +- Fix more cases of C and Python inconsistency. The C implementation + now behaves like the Python implementation when it comes to integer + overflow for the integer keys for ``in``, ``get`` and ``has_key``. + Now they return False, the default value, and False, respectively in + both versions if the tested value would overflow or underflow. + Previously, the C implementation would raise ``OverflowError`` or + ``KeyError``, while the Python implementation functioned as + expected. See `issue 140 + `_. 4.7.1 (2020-03-22) diff --git a/docs/overview.rst b/docs/overview.rst index 60fc9ca..aabe5fa 100644 --- a/docs/overview.rst +++ b/docs/overview.rst @@ -168,10 +168,10 @@ exclusive of the range's endpoints. [1, 2, 3, 4] >>> [pair for pair in t.iteritems()] # new in ZODB 3.3 [(1, 'red'), (2, 'green'), (3, 'blue'), (4, 'spades')] - >>> t.has_key(4) # returns a true value, but exactly what undefined - 2 + >>> t.has_key(4) # returns a true value + True >>> t.has_key(5) - 0 + False >>> 4 in t # new in ZODB 3.3 True >>> 5 in t # new in ZODB 3.3 @@ -256,10 +256,10 @@ example, lists supply a total ordering, and then >>> list(s.keys()) # note that the lists are in sorted order [[1], [2], [3]] >>> s.has_key([3]) # and [3] is in the set - 1 + True >>> L2[0] = 5 # horrible -- the set is insane now >>> s.has_key([3]) # for example, it's insane this way - 0 + False >>> s OOSet([[1], [5], [3]]) >>> From 5f9ce957d952ba9765dc5cc7c92c07410b8228e5 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Tue, 7 Apr 2020 06:10:29 -0500 Subject: [PATCH 2/2] The value macros for ints also need to convert OverflowError into TypeError. This only showed up on Windows (because it follows the LLP64 data model). See https://ci.appveyor.com/project/mgedmin/btrees/builds/31998447/job/m3xs7q1d8b4u5u3b --- BTrees/intvaluemacros.h | 16 ++++++++++++++-- BTrees/tests/common.py | 16 ++++++++-------- CHANGES.rst | 4 ++++ 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/BTrees/intvaluemacros.h b/BTrees/intvaluemacros.h index 2af2e58..21e0562 100644 --- a/BTrees/intvaluemacros.h +++ b/BTrees/intvaluemacros.h @@ -29,7 +29,13 @@ #define COPY_VALUE_FROM_ARG(TARGET, ARG, STATUS) \ if (INT_CHECK(ARG)) { \ long vcopy = INT_AS_LONG(ARG); \ - if (PyErr_Occurred()) { (STATUS)=0; (TARGET)=0; } \ + if (PyErr_Occurred()) { \ + if (PyErr_ExceptionMatches(PyExc_OverflowError)) { \ + PyErr_Clear(); \ + PyErr_SetString(PyExc_TypeError, "integer out of range"); \ + } \ + (STATUS)=0; (TARGET)=0; \ + } \ else if ((int)vcopy != vcopy) { \ PyErr_SetString(PyExc_TypeError, "integer out of range"); \ (STATUS)=0; (TARGET)=0; \ @@ -62,7 +68,13 @@ #define COPY_VALUE_FROM_ARG(TARGET, ARG, STATUS) \ if (INT_CHECK(ARG)) { \ long vcopy = INT_AS_LONG(ARG); \ - if (PyErr_Occurred()) { (STATUS)=0; (TARGET)=0; } \ + if (PyErr_Occurred()) { \ + if (PyErr_ExceptionMatches(PyExc_OverflowError)) { \ + PyErr_Clear(); \ + PyErr_SetString(PyExc_TypeError, "integer out of range"); \ + } \ + (STATUS)=0; (TARGET)=0; \ + } \ else if (vcopy < 0) { \ PyErr_SetString(PyExc_TypeError, "can't convert negative value to unsigned int"); \ (STATUS)=0; (TARGET)=0; \ diff --git a/BTrees/tests/common.py b/BTrees/tests/common.py index c40fb1d..0893b22 100644 --- a/BTrees/tests/common.py +++ b/BTrees/tests/common.py @@ -390,7 +390,7 @@ class NonSub(object): self.assertFalse(issubclass(NonSub, type(t))) self.assertFalse(isinstance(NonSub(), type(t))) -class MappingBase(Base): +class MappingBase(Base): # pylint:disable=too-many-public-methods # Tests common to mappings (buckets, btrees) SUPPORTS_NEGATIVE_VALUES = True @@ -722,7 +722,7 @@ def testEmptyRangeSearches(self): self.assertEqual(list(keys), []) self.assertEqual(list(t.iterkeys(max=50, min=200)), []) - def testSlicing(self): + def testSlicing(self): # pylint:disable=too-many-locals # Test that slicing of .keys()/.values()/.items() works exactly the # same way as slicing a Python list with the same contents. # This tests fixes to several bugs in this area, starting with @@ -832,7 +832,7 @@ def testIterators(self): self.assertEqual(list(t.iteritems()), list(t.items())) @uses_negative_keys_and_values - def testRangedIterators(self): + def testRangedIterators(self): # pylint:disable=too-many-locals t = self._makeOne() for keys in [], [-2], [1, 4], list(range(-170, 2000, 13)): @@ -1074,7 +1074,6 @@ def testKeyAndValueOverflow(self): self.skipTest("Needs bounded key and value") import struct - from .._compat import PY2 good = set() b = self._makeOne() @@ -1995,7 +1994,7 @@ class ModuleTest(object): key_type = None value_type = None def _getModule(self): - pass + raise NotImplementedError def testNames(self): names = ['Bucket', 'BTree', 'Set', 'TreeSet'] @@ -2223,8 +2222,8 @@ def setUp(self): super(SetResult, self).setUp() _skip_if_pure_py_and_py_test(self) - self.Akeys = [1, 3, 5, 6] - self.Bkeys = [ 2, 3, 4, 6, 7] + self.Akeys = [1, 3, 5, 6] # pylint:disable=bad-whitespace + self.Bkeys = [ 2, 3, 4, 6, 7] # pylint:disable=bad-whitespace self.As = [makeset(self.Akeys) for makeset in self.builders()] self.Bs = [makeset(self.Bkeys) for makeset in self.builders()] self.emptys = [makeset() for makeset in self.builders()] @@ -2336,7 +2335,7 @@ def testDifference(self): else: self.assertEqual(list(C), want) - def testLargerInputs(self): + def testLargerInputs(self): # pylint:disable=too-many-locals from BTrees.IIBTree import IISet # pylint:disable=no-name-in-module from random import randint MAXSIZE = 200 @@ -2592,6 +2591,7 @@ class ConflictTestBase(SignedMixin, object): # Tests common to all types: sets, buckets, and BTrees storage = None + db = None def setUp(self): super(ConflictTestBase, self).setUp() diff --git a/CHANGES.rst b/CHANGES.rst index 5101d63..9a1cad5 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -15,6 +15,10 @@ expected. See `issue 140 `_. + .. note:: + The unspecified true return values of ``has_key`` + have changed. + 4.7.1 (2020-03-22) ==================