Skip to content
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

C-extension datetime.timezone is not acceptable as a base class; pure-Python version is #112451

Closed
mattip opened this issue Nov 27, 2023 · 11 comments
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@mattip
Copy link
Contributor

mattip commented Nov 27, 2023

Bug report

Bug description:

Datetime has two modes. There is divergent behavior between the pure python and the c-extension timezone class, as found in django

>>> import datetime, _pydatetime
>>> class timezone(_pydatetime.timezone): pass  # Works fine...
... 
>>> class timezone(datetime.timezone): pass  # Fails to subclass...
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: type 'datetime.timezone' is not an acceptable base type

CPython versions tested on:

3.12

Operating systems tested on:

Linux

Linked PRs

@ngnpope
Copy link
Contributor

ngnpope commented Nov 27, 2023

Just to clarify: It is acceptable as a base class in Pure Python, but not in the C extension.

@AlexWaygood AlexWaygood added the extension-modules C modules in the Modules dir label Nov 27, 2023
@AlexWaygood AlexWaygood changed the title pure python datetime.timezone is not acceptable as a base class C-extension datetime.timezone is not acceptable as a base class Nov 27, 2023
@felixxm
Copy link
Contributor

felixxm commented Nov 29, 2023

Working on patch

@AlexWaygood AlexWaygood changed the title C-extension datetime.timezone is not acceptable as a base class C-extension datetime.timezone is not acceptable as a base class; pure-Python version is Nov 29, 2023
felixxm added a commit to felixxm/cpython that referenced this issue Nov 29, 2023
felixxm added a commit to felixxm/cpython that referenced this issue Nov 29, 2023
felixxm added a commit to felixxm/cpython that referenced this issue Nov 29, 2023
@pganssle
Copy link
Member

pganssle commented Nov 29, 2023

I think @abalkin would know better, but I think the no-subclassing behavior was deliberate. If that is the case we should probably fix it by preventing inheritance in the pure Python module.

@felixxm
Copy link
Contributor

felixxm commented Nov 29, 2023

I think @abalkin would know better, but I think the no-subclassing behavior was deliberate. If that is the case we should probably fix it by preventing inheritance in the pure Python module.

I'm fine with making them consistent in any way. Just let me know and I'll update my patch.

felixxm pushed a commit to django/django that referenced this issue Nov 29, 2023
…upported().

This test relied on the behavior of subclassing `datetime.timezone`
which is not permitted by the C-extension version of CPython's
`datetime` module. This restriction isn't enforced by the pure
Python version, nor by PyPy.

See python/cpython#112451

It's not critical, and doesn't test any Django behavior, so just
remove it.
@felixxm
Copy link
Contributor

felixxm commented Jan 12, 2024

I think @abalkin would know better, but I think the no-subclassing behavior was deliberate. If that is the case we should probably fix it by preventing inheritance in the pure Python module.

As far as I'm aware, there are reasonable use cases for timezone subclasses, e.g. enums:

class AvailableTimezone(datetime.timezone, enum.Enum):
    ...

@erlend-aasland

This comment was marked as outdated.

felixxm added a commit to felixxm/cpython that referenced this issue Jan 15, 2024
@serhiy-storchaka
Copy link
Member

One of advantage of non-inheritable C-extension classes is that you can replace __init__ constructor with __new__ constructor. __new__ is more convenient in C-extension classes, because it makes easier to guarantee that all fields are properly initialized. On other hand, __init__ is more convenient in Python classes. Replacing __init__ with __new__ can break subclasses with their own constructor.

Fortunately, both timezone implementations use __new__.

But when make a class inheritable, you should check a lot of other details. Test with overridden __init__(). Test with overridden methods. Test with additional attributes. How non-trivial subclasses work with pickling and shalllow and deep copying? How instances of different subclasses are compared?

@pganssle
Copy link
Member

Here is Alexander's original justification for not allowing timezone to be subclassed: #49344 (comment)

I tend to agree with his reasoning, but I don't entirely understand the enum example given by @felixxm here. Why exactly does it need to be a subclass?

@erlend-aasland
Copy link
Contributor

Thanks for chiming in @pganssle and @serhiy-storchaka. I see this is not as straight-forward as I thought. If any change is to be done, it would have to be to follow up Paul's suggestion to disallow subclassing of the Python class. We can also just leave it as it is; there are plenty of cases where the C implementation of a module differs from the Python implementation.

@felixxm
Copy link
Contributor

felixxm commented Jan 17, 2024

I tend to agree with his reasoning, but I don't entirely understand the enum example given by @felixxm here. Why exactly does it need to be a subclass?

True, it's not probably not as useful as IntEnum or StrEnum.

there are plenty of cases where the C implementation of a module differs from the Python implementation.

Granted, but these small differences make things trickier when you try to support Python and PyPy at the same time.

felixxm added a commit to felixxm/cpython that referenced this issue Jan 17, 2024
This is consistent with C-extension datetime.timezone.
@felixxm
Copy link
Contributor

felixxm commented Jan 17, 2024

I've submitted a new PR to make both implementation consistent, #114190.

felixxm added a commit to felixxm/cpython that referenced this issue Jan 17, 2024
This is consistent with C-extension datetime.timezone.
felixxm added a commit to felixxm/cpython that referenced this issue Jan 17, 2024
This is consistent with C-extension datetime.timezone.
felixxm added a commit to felixxm/cpython that referenced this issue Jan 17, 2024
This is consistent with C-extension datetime.timezone.
felixxm added a commit to felixxm/cpython that referenced this issue Jan 24, 2024
This is consistent with C-extension datetime.timezone.
felixxm added a commit to felixxm/cpython that referenced this issue Jan 24, 2024
felixxm added a commit to felixxm/cpython that referenced this issue Jan 26, 2024
erlend-aasland pushed a commit that referenced this issue Jan 26, 2024
This is consistent with C-extension datetime.timezone.
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
…14190)

This is consistent with C-extension datetime.timezone.
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
…14190)

This is consistent with C-extension datetime.timezone.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
Projects
Archived in project
Development

No branches or pull requests

7 participants