Skip to content

bpo-35021: Fix assertion failures in _datetimemodule.c. #9958

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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Oct 18, 2018

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.
@Yhg1s
Copy link
Member

Yhg1s commented Oct 19, 2018

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?

@serhiy-storchaka
Copy link
Member Author

serhiy-storchaka commented Oct 19, 2018

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:

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

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

@Yhg1s
Copy link
Member

Yhg1s commented Oct 22, 2018

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?

@serhiy-storchaka
Copy link
Member Author

See an alternate solution in #10039.

@serhiy-storchaka
Copy link
Member Author

And other alternate solution in #10040.

@serhiy-storchaka
Copy link
Member Author

Closed in favor of #10039.

@serhiy-storchaka serhiy-storchaka deleted the timedelta-int-subclass branch November 20, 2018 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting merge skip news type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants