| msg325341 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
Date: 2018-09-14 10:13 |
| It is documented that divmod() returns a pair of numbers, but the implementation doesn't enforce this. Actually divmod() just returns the result of __divmod__() or __rdivmod__() without checking it. PyNumber_Divmod() is documented as the C equivalent of divmod(). But since it returns a 2-tuple in all implemented cases, the user code can expect that it always return a 2-tuple. This can lead to hidden bugs similar to . I think there are no reasons of returning anything except a 2-tuple from divmod(). Forcing this conditions can save third-party code from crashes. The following PR make divmod() and PyNumber_Divmod() always returning a 2-tuple. |
|
|
| msg325361 - (view) |
Author: Paul Ganssle (p-ganssle) * |
Date: 2018-09-14 16:43 |
| I would be somewhat worried that this might break something like numpy (though not numpy specifically) which might be counting on the ability to write a wrapper that overloads this behavior. Another concern is that people could be playing it fast-and-loose with actual types, and are returning something that fits the bill for a collections.abc.Sequence with length 2 (e.g. return [a, b]) but isn't an actual Tuple. I would at least think that this should go through a deprecation cycle. |
|
|
| msg325364 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2018-09-14 16:58 |
| -0 Perhaps this should be left as-is. There is no known problem that we would be solving and it would add an extra step to a fine-grained function. In general, I think we have these guards in places where it is known that downstream callers rely on a particular type signature for the return value. |
|
|
| msg325398 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2018-09-14 20:57 |
| I share Paul's concern about somebody using this (mis-?)feature and being unnecessarily broken. However, such a restriction would have prevented the segfault that was reported and fixed in bpo-31577, and would save any other users of PyNumber_Divmod from having to implement the same kind of checks. Does anyone have an example of a legitimate use for the current behavior? |
|
|
| msg325423 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-09-15 04:53 |
| An alternate idea: convert the result of __divmod__ and __rdivmod__ to tuple instead raising an error. This will support the case of returning a list. I didn't implement this initially because I think this case is very unlikely occurred in real code. Similar changes were made in past in PyMapping_Keys(). In Python 2 it just calls the keys() method and returns the result. In Python 3 it initially converted the result to list or tuple. But since the caller code often expects a list and use PyList API with the result, later PyMapping_Keys() was fixed but making it always returning a list. |
|
|
| msg325543 - (view) |
Author: Paul Ganssle (p-ganssle) * |
Date: 2018-09-17 13:37 |
| @Serhiy I like the "convert to a tuple" idea before returning *better*, though I think I'd prefer to just check that it's tuple-like, something like: if not all(hasattr(return_val, attr) for attr in ['__getitem__', '__len__']) or len(return_val) != 2: warnings.warn("divmod must return a Sequence of length 2. " + "This will be an exception in Python 3.9+", Deprecationwarning) Or some similar equivalent to `if not isinstance(return_val, collections.abc.Sequence) or len(return_val) != 2`. That said, the PR seems to have already run into an issue, which is that `unittest.mock.MagicMock` returns a `unittest.mock.MagicMock` from `divmod`, which is not a 2-tuple. |
|
|