bpo-35021: Fix assertion failures in _datetimemodule.c. by serhiy-storchaka · Pull Request #9958 · python/cpython (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation6 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

The result of multiplication and division of an integer and
an instance of integer subclass can be not exact integer.
The type of the right operand takes precedence when it is
a subclass of the type of the left operand.
Use PyLong slots explicitly to get an exact integer.

https://bugs.python.org/issue35021

The result of multiplication and division of an integer and an instane of integer subclass can be not exact integer. The type of the right operand takes precedence when it is a subclass of the type of the left operand. Use PyLong slots explicitly to get an exact integer.

This is a behaviour change, and not suitable for 3.7 and earlier.

I also don't understand why. What's the problem you're solving? Why would you want to silently bypass behaviour redefined in a subclass? Is the problem the actual types, or are you expecting the result to be in certain bounds? Notice that the old code had assertions, which are ignored unless you explicitly enable them (in a --with-pydebug build, or if you enable them separately -- which is how I ran into them). That means existing code can already be passing subclasses with different behaviour and relying on that behaviour.

If it's important that only 'real' ints are passed to datetime.timedelta, convert them when they come in, or raise an exception if they're the wrong type. That would be a behaviour change, though, and again not suitable for a released Python. If it's just that some values are unexpected, why not do bounds checking after the calculations?

The behavior was already changed in bpo-31752 (#3947). The behavior redefined in a subclass is silently bypassed for float subclasses. Also this is where minor details are different in Python and C implementations.

The problem that I tried to solve in bpo-31752 is that the C code expected that divmod() returns a pair of integers, and the value of the remainder is between 0 and the value of the divider. All of these expectations can be false if allow the behavior to be redefined in int subclasses.

There are two ways of solving this issue:

Exact emulation of the Python code in C. Remove all asserts, and be ready that every operation can fail or return unexpected result. This is not particularly efficient and will need to rewrite a lot of code in maintained branches.
Ignore the behavior redefined in subclasses. Perform all operations as on exact ints and floats. This is already done for float subclasses. We can just convert all input from int sublasses to exact int (2a). Or perform operations which ignore the behavior redefined in subclasses (e.g. int.__mul__(a, b) instead of a * b) (2b). The former way required adding a little more code and was a tiny bit less efficient, so I choose the latter way, but made an error in the implementation.

This PR fixes errors in the (2b) implementation. If you wish, I can implement (2a) and (1) and compare solutions.

That the behaviour was already changed doesn't mean it's open season for behaviour changes :) PEP 6 still applies. It's still not clear to me why it's necessary to change the behaviour, and I have real-world (proprietary) code that triggers these asserts. The assertions are the only problem with the code -- it otherwise works fine -- which suggests the assumptions in the old code (about the types) are simply invalid. Why is it better to silently ignore subclass behaviour rather than raise an appropriate exception when the subclass returns the wrong type?

See an alternate solution in #10039.

And other alternate solution in #10040.