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

equality not symmetric for subclasses of datetime.date and datetime.datetime #49766

Closed
jessaustin mannequin opened this issue Mar 19, 2009 · 22 comments
Closed

equality not symmetric for subclasses of datetime.date and datetime.datetime #49766

jessaustin mannequin opened this issue Mar 19, 2009 · 22 comments
Assignees
Labels
3.13 bugs and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@jessaustin
Copy link
Mannequin

jessaustin mannequin commented Mar 19, 2009

BPO 5516
Nosy @ncoghlan, @abalkin, @jackdied, @ericsnowcurrently, @pganssle
Files
  • issue5516.diff: patch against trunk for datetimemodule.c and test_datetime.py
  • issue5516_trunk.diff: udiff against 2.7 trunk
  • 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 = 'https://github.com/abalkin'
    closed_at = None
    created_at = <Date 2009-03-19.00:51:23.421>
    labels = ['3.7', 'type-bug', 'library']
    title = 'equality not symmetric for subclasses of datetime.date and datetime.datetime'
    updated_at = <Date 2018-07-05.15:58:15.282>
    user = 'https://bugs.python.org/jessaustin'

    bugs.python.org fields:

    activity = <Date 2018-07-05.15:58:15.282>
    actor = 'p-ganssle'
    assignee = 'belopolsky'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2009-03-19.00:51:23.421>
    creator = 'jess.austin'
    dependencies = []
    files = ['13403', '13419']
    hgrepos = []
    issue_num = 5516
    keywords = []
    message_count = 19.0
    messages = ['83798', '84028', '84184', '103854', '103864', '103881', '103886', '104433', '106497', '106501', '106503', '106505', '106507', '106508', '108603', '108684', '195030', '195034', '195035']
    nosy_count = 7.0
    nosy_names = ['ncoghlan', 'belopolsky', 'jackdied', 'jess.austin', 'ysj.ray', 'eric.snow', 'p-ganssle']
    pr_nums = []
    priority = 'low'
    resolution = 'postponed'
    stage = 'needs patch'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue5516'
    versions = ['Python 3.7']

    Linked PRs

    @jessaustin
    Copy link
    Mannequin Author

    jessaustin mannequin commented Mar 19, 2009

    While the datetime.date and datetime.datetime classes consistently
    handle mixed-type comparison, their subclasses do not:

    >>> from datetime import date, datetime, time
    >>> d = date.today()
    >>> dt = datetime.combine(d, time())
    >>> d == dt
    False
    >>> dt == d
    False
    >>> class D(date):
    ...     pass
    ... 
    >>> class DT(datetime):
    ...     pass
    ... 
    >>> d = D.today()
    >>> dt = DT.combine(d, time())
    >>> d == dt
    True
    >>> dt == d
    False

    I think this is due to the premature "optimization" of using memcmp() in
    date_richcompare().

    @jessaustin jessaustin mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Mar 19, 2009
    @jessaustin jessaustin mannequin changed the title equality not reflixive for subclasses of datetime.date and datetime.datetime equality not reflexive for subclasses of datetime.date and datetime.datetime Mar 19, 2009
    @jessaustin jessaustin mannequin changed the title equality not reflexive for subclasses of datetime.date and datetime.datetime equality not symmetric for subclasses of datetime.date and datetime.datetime Mar 19, 2009
    @jessaustin
    Copy link
    Mannequin Author

    jessaustin mannequin commented Mar 23, 2009

    The attached patch fixes this issue, and updates the tests. Contrary to
    my initial impression, it seems that a previous developer knew of this
    behavior and thought it correct; see the comment of the test I deleted.
    I left memcmp() in.

    @jackdied
    Copy link
    Contributor

    +1

    Patch and tests work for me. Uploaded a patch that is identical except
    the file paths are fixed.

    Was the old behavior stable across compilers anyway? It memcmpared two
    different structs and IIRC only the first item of each struct is
    guaranteed to be at the start of the memory location. No?

    With this patch only same-struct objects are memcmpared.

    @ncoghlan ncoghlan self-assigned this Jan 13, 2010
    @AlexanderBelopolsky
    Copy link
    Mannequin

    AlexanderBelopolsky mannequin commented Apr 21, 2010

    There is another inconsistency that this patch does not seem to cure. With patch applied and D and DT defined as in OP,

    >>> D(1900,1,1) > DT(1900,1,1)
    True

    but

    >>> DT(1900,1,1) < D(1900,1,1)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: can't compare DT to D

    and

    >>> date(1900,1,1) < datetime(1900,1,1)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: can't compare datetime.datetime to datetime.date

    Note that without the patch,

    >>> D(1900,1,1) > DT(1900,1,1)
    False

    but both behaviors seem to be wrong.

    @AlexanderBelopolsky
    Copy link
    Mannequin

    AlexanderBelopolsky mannequin commented Apr 21, 2010

    Upon further reflection I am -1 on this patch. First, as implemented this patch changes behavior of an explicit invocation of date.__eq__. The patch proposes to remove the following tests:

    •    # Neverthelss, comparison should work with the base-class (date)
      
    •    # projection if use of a date method is forced.
      
    •    self.assert_(as_date.\_\_eq__(as_datetime))
      
    •    different_day = (as_date.day + 1) % 20 + 1
      
    •    self.assert_(not as_date.\_\_eq__(as_datetime.replace(day=
      
    •                                                 different_day)))
      

    Second, the patch introduces dependence of the baseclass method (date_richcompare) on a particular subclass (PyDateTime_Check). This is against OOP principles.

    I am not sure how the "equality not symmetric" issue can be fixed. In my opinion current datetime implementation is fighting OOP and violating the substitution principle in an attempt to prevent date to datetime comparison. I would prefer seeing one of two things: either datetime not inheriting from date or making datetime to date comparison compare date part alone. Once you stop fighting OOP principles, symmetry of equality for subclasses will follow from that for the base class automatically.

    Given that either of these solutions means a major change, I think it is best to leave the things as they are now.

    @jessaustin
    Copy link
    Mannequin Author

    jessaustin mannequin commented Apr 21, 2010

    To be systematic, without the patch:

    >>> D(1900, 1, 1) > DT(1900, 1, 1)
    False
    >>> D(1900, 1, 1) < DT(1900, 1, 1)
    False
    >>> DT(1900, 1, 1) > D(1900, 1, 1)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: can't compare DT to D
    >>> DT(1900, 1, 1) < D(1900, 1, 1)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: can't compare DT to D
    with the patch:
    >>> D(1900, 1, 1) > DT(1900, 1, 1)
    True
    >>> D(1900, 1, 1) < DT(1900, 1, 1)
    False
    >>> DT(1900, 1, 1) > D(1900, 1, 1)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: can't compare DT to D
    >>> DT(1900, 1, 1) < D(1900, 1, 1)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: can't compare DT to D

    It might seem like the latter behavior is marginally better, but really this is just a mess, since a date-datetime comparison TypeErrors in all directions. I appreciate Alexander's more experienced perspective, but it's not obvious to me that this problem is insoluble simply due to OOP algebra. I'm going to keep tinkering with this to see if there isn't a way to satisfy his concerns AND fix these bugs WITHOUT breaking the established (and admittedly anti-OOP) behavior that dates are not equal to datetimes.

    (Incidentally, the test I removed still seems to be an arbitrary ad-hoc requirement that subclasses of date behave differently than date itself. I don't see how one could rely on this given the other inconsistencies with date subclasses, and so violating this in order to fix more serious problems seems acceptable.)

    I'm reminded of the set and frozenset situation, which seems almost dual to this one. set and frozenset don't inherit from each other, but they do compare. This seems to bite you only when you try to redefine comparison in subclasses.

    @AlexanderBelopolsky
    Copy link
    Mannequin

    AlexanderBelopolsky mannequin commented Apr 21, 2010

    On Wed, Apr 21, 2010 at 2:33 PM, Jess Austin <[email protected]> wrote:
    ..

    It might seem like the latter behavior is marginally better, but really this is just a mess, since a date-datetime comparison TypeErrors in all
    directions.  I appreciate Alexander's more experienced perspective, but it's not obvious to me that this problem is insoluble simply due to OOP
    algebra.  I'm going to keep tinkering with this to see if there isn't a way to satisfy his concerns AND fix these bugs WITHOUT breaking the
    established (and admittedly anti-OOP) behavior that dates are not equal to datetimes.

    I certainly don't have a proof that this is impossible, so best of
    luck. Note, however that the problematic behavior is due to D/DT
    classes implementor's choice not to derive DT from D. Whether
    resulting violation of the symmetry of equality is a bug in python or
    D/DT implementation is at least an open question.

    @ncoghlan
    Copy link
    Contributor

    I think I'll concur with the "this is a mess" assessment.

    Given that state of affairs, punting on this until 3.2 (at the earliest).

    @ncoghlan ncoghlan removed their assignment Apr 28, 2010
    @abalkin
    Copy link
    Member

    abalkin commented May 26, 2010

    I am leaning towards "won't fix". Any objections?

    @abalkin abalkin self-assigned this May 26, 2010
    @jessaustin
    Copy link
    Mannequin Author

    jessaustin mannequin commented May 26, 2010

    Could you provide some reasoning for such a resolution? I had thought that "won't fix" indicated that the issue wasn't actually an error in behavior.

    I grant that most people will never see this particular error, but it is an error.

    @abalkin
    Copy link
    Member

    abalkin commented May 26, 2010

    Could you provide some reasoning for such a resolution?
    I had thought that "won't fix" indicated that the issue
    wasn't actually an error in behavior.

    No, that would be "invalid." IMO, "won't fix" is for bugs were cost of fixing them outweighs the benefits. Here is a typical example: bpo-8309 "Sin(x) is Wrong".

    Here, however I am torn between "won't fix" and "invalid." As I said in my previous comment:

    """
    Note, however that the problematic behavior is due to D/DT
    classes implementor's choice not to derive DT from D. Whether
    resulting violation of the symmetry of equality is a bug in python or
    D/DT implementation is at least an open question.
    """

    I don't mind keeping this open if there is a hope that someone will come up with a working solution. The current patch is not a solution.

    @ncoghlan
    Copy link
    Contributor

    I'd suggest leaving it open - the current situation is definitely suboptimal, but it is likely to take some close scrutiny to get it to behave nicely.

    @ysjray
    Copy link
    Mannequin

    ysjray mannequin commented May 26, 2010

    Date and Datetime comparison is not defined and not documented, and until we find a nice way to implement the comparison, we should just let this comparison raise NotImpelemented.

    @ncoghlan
    Copy link
    Contributor

    Deprecating the feature for 3.x is certainly an option. May be a little drastic though.

    @abalkin
    Copy link
    Member

    abalkin commented Jun 25, 2010

    Deprecating the feature for 3.x is certainly an option.
    May be a little drastic though.

    How drastic would be to stop subclassing datetime from date in 3.2? After all, we don't subclass float form int.

    @ncoghlan
    Copy link
    Contributor

    If you can articulate the benefits of splitting them apart, it would be worth making the case for doing so on python-dev. I'm having a hard time picturing what anyone could be doing such that it would break their code, but it's still definitely a backwards incompatible change.

    @ericsnowcurrently
    Copy link
    Member

    This bit me today (under 2.7).

    @abalkin
    Copy link
    Member

    abalkin commented Aug 12, 2013

    Eric,

    Could you share details of your use-case? My experience with subclassing from basic python types including date/time has been mostly negative. The problem is that when I subclass, I want to inherit the rich set of operations such as +, -, *, etc., and add a few methods of my own. After that, I want to always use instances of my subclass instead of the stdlib one. This does not work because adding instances of my subclass returns an instance of the superclass unless I override __add__ explicitly. (See bpo-2267.) This kills all benefits of subclassing as compared to containment.

    These days I try to stay away from subclassing date/time, int/float, or anything like that and thus have little incentive to resolve this issue. And it does not look like we have a workable solution.

    @ericsnowcurrently
    Copy link
    Member

    I'm doing some string-based serialization of datetimes and need to be able to specify the type somewhat declaratively. So I'm using a datetime subclass. This is more or less the code I'm using:

    class Timestamp(datetime.datetime):
    
        def __new__(cls, raw_value, *args, **kwargs):
            if not args and not kwargs:
                return cls.fromtimestamp(int(raw_value))
            else:
                return super(Timestamp, cls).__new__(cls, raw_value,
                                                     *args, **kwargs)
    
        def __str__(self):
            return str(int(time.mktime(self.timetuple())))

    Incidently, the whole equality testing thing didn't actually cause a problem. It was comparing against the result of datetime.utcnow() which has microseconds (and my Timestamp instance didn't). Clearing out the microseconds resolved the failure so I wasn't actually bitten by this issue after all.

    @abalkin abalkin added the 3.7 (EOL) end of life label Sep 10, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @iritkatriel
    Copy link
    Member

    Reproduced on 3.13:

    Python 3.13.0a0 (heads/main:ecc05e23a1, Jul 28 2023, 20:21:40) [Clang 14.0.3 (clang-1403.0.22.14.1)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> from datetime import date, datetime, time
    >>> d = date.today()
    >>> dt = datetime.combine(d, time())
    >>> d == dt
    False
    >>> dt == d
    False
    >>> class D(date): pass
    ... 
    >>> class DT(datetime): pass
    ... 
    >>> d = D.today()
    >>> dt = DT.combine(d, time())
    >>> d == dt
    True
    >>> dt == d
    False
    >>> 
    

    @iritkatriel iritkatriel added 3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes and removed 3.7 (EOL) end of life labels Jul 29, 2023
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jan 30, 2024
    …ible
    
    Now the special comparison methods like `__eq__` and `__lt__` return
    NotImplemented if one of comparands is date and other is datetime
    instead of ignoring the time part and the time zone or forcefully
    return "not equal" or raise TypeError.
    
    It makes comparison of date and datetime subclasses more symmetric
    and allows to change the default behavior by overriding
    the special comparison methods in subclasses.
    
    It is now the same as if date and datetime was independent classes.
    @serhiy-storchaka
    Copy link
    Member

    @ncoghlan, @ericsnowcurrently, you participated in this discussion, can you take a look at #114760? It does not only fixes this issue, it makes the code simpler.

    serhiy-storchaka added a commit that referenced this issue Feb 11, 2024
    …H-114760)
    
    Now the special comparison methods like `__eq__` and `__lt__` return
    NotImplemented if one of comparands is date and other is datetime
    instead of ignoring the time part and the time zone or forcefully
    return "not equal" or raise TypeError.
    
    It makes comparison of date and datetime subclasses more symmetric
    and allows to change the default behavior by overriding
    the special comparison methods in subclasses.
    
    It is now the same as if date and datetime was independent classes.
    @serhiy-storchaka
    Copy link
    Member

    It may be too serious change for backporting.

    @serhiy-storchaka serhiy-storchaka removed 3.11 only security fixes 3.12 bugs and security fixes labels Feb 11, 2024
    fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this issue Feb 14, 2024
    …ible (pythonGH-114760)
    
    Now the special comparison methods like `__eq__` and `__lt__` return
    NotImplemented if one of comparands is date and other is datetime
    instead of ignoring the time part and the time zone or forcefully
    return "not equal" or raise TypeError.
    
    It makes comparison of date and datetime subclasses more symmetric
    and allows to change the default behavior by overriding
    the special comparison methods in subclasses.
    
    It is now the same as if date and datetime was independent classes.
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.13 bugs and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    Archived in project
    Development

    No branches or pull requests

    6 participants