Guarantee that divmod() and PyNumber_Divmod() return a 2-tuple

[serhiy-storchaka]

BPO	34676
Nosy	@rhettinger, @mdickinson, @zware, @serhiy-storchaka, @pganssle, @tirkarthi
PRs	bpo-34676: Always return a 2-tuple from divmod() and PyNumber_Divmod(). #9301
Dependencies	bpo-34716: MagicMock.divmod should return a pair

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2018-09-14.10:13:50.305>
labels = ['interpreter-core', 'type-feature', '3.8']
title = 'Guarantee that divmod() and PyNumber_Divmod() return a 2-tuple'
updated_at = <Date 2018-09-17.19:55:24.197>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2018-09-17.19:55:24.197>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2018-09-14.10:13:50.305>
creator = 'serhiy.storchaka'
dependencies = ['34716']
files = []
hgrepos = []
issue_num = 34676
keywords = ['patch']
message_count = 6.0
messages = ['325341', '325361', '325364', '325398', '325423', '325543']
nosy_count = 6.0
nosy_names = ['rhettinger', 'mark.dickinson', 'zach.ware', 'serhiy.storchaka', 'p-ganssle', 'xtreak']
pr_nums = ['9301']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue34676'
versions = ['Python 3.8']

[serhiy-storchaka]

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 bpo-31577.

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.

[pganssle]

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.

[rhettinger]

-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.

[zware]

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?

[serhiy-storchaka]

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.

[pganssle]

@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.

[encukou]

On the PR, Serhiy wrote:

Alternatively we can provide a new public API for calling PyNumber_Divmod() and unpacking the result. It would not help the old code which uses PyNumber_Divmod() followed by PyTuple_GET_ITEM(), but it may make writing the new correct code less painful.

+1 for that. It can return two objects via PyObject ** output arguments.

The conversion idea sounds good once it'd be done, but I don't see good behaviour for the deprecation period. If users return a list, they'd need to start getting deprecation warnings best silenced by switching to tuples.