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
int.from_bytes() is broken for subclasses #67828
Comments
Example: >>> import socket
>>> x = socket.AddressFamily.from_bytes(b'\1', 'big')
>>> type(x)
<enum 'AddressFamily'>
>>> int(x)
1
>>> str(x)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/serhiy/py/cpython/Lib/enum.py", line 464, in __str__
return "%s.%s" % (self.__class__.__name__, self._name_)
AttributeError: 'AddressFamily' object has no attribute '_name_'
>>> repr(x)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/serhiy/py/cpython/Lib/enum.py", line 461, in __repr__
self.__class__.__name__, self._name_, self._value_)
AttributeError: 'AddressFamily' object has no attribute '_name_' |
The only solution that is coming to mind is to have EnumMeta go through the other base classes, wrap any classmethods it finds, and ensure that they return their appropriate type and not an Enum type. Any other ideas? |
Hi, Example:
>>> class foo(int):
... def __init__(self, value, base=10):
... if value == 2:
... raise ValueError("not that!!")
... super(foo, self).__init__(value, base=base)
...
>>> x = foo.from_bytes(b"\2", "big")
>>> x
2
>>> type(x)
foo 2 solutions come to mind:
|
Not all classmethods are constructors. I see two solutions:
|
I think the classmethod-as-constructor behavior is correct, so it's up to IntEnum (or EnumMeta, or foo, or ...), to work around the issue. |
I took the liberty of putting together a small patch which makes from_bytes() call & return type(value_found) if type != long (Serhiy's & my 2nd solution). Ethan: in that case, type.from_bytes() would always return an int, and not resort to build-a-pseudo-int, right? That way not overriding from_bytes() would not result in strange behaviour. |
'from_bytes' is a classmethod. As such, it should return the same type as the class it is called on. If that wasn't the intent it would be a staticmethod instead. It is the responsibility of the subclass to override base class behavior, not the other way around. |
I'm not sure how you can have both, those two seem opposite to me:
2015-03-30 18:02 GMT+02:00 Ethan Furman <report@bugs.python.org>:
|
With the patch: --> import enum
--> class Huh(enum.IntEnum):
... blah = 2
...
--> Huh.blah.from_bytes(b'\04', 'big')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/ethan/source/python/issue23640/Lib/enum.py", line 222, in __call__
return cls.__new__(cls, value)
File "/home/ethan/source/python/issue23640/Lib/enum.py", line 457, in __new__
raise ValueError("%r is not a valid %s" % (value, cls.__name__))
ValueError: 4 is not a valid Huh This is not the correct behavior. An IntEnum should act like an int, and in cases where it can't and still be an IntEnum, it becomes an int. But this behavior is Enum specific, and I would not expect other int subclasses to need or want that behavior. Also, in cases where class methods are alternate constructors there is no requirement that they go through the main __new__/init constructors to do their job. In other words, if IntEnum.from_bytes (which is inherited) is not behaving correctly, it is up to IntEnum to fix it -- it is not the job of int, and this is not a bug in int. |
The fact that derived_int.from_bytes() doesn't call the derived constructor clearly sounds like a bug to me, regardless of whether IntEnum also has its own bugs. |
This bug allows to create new bool instances. >>> false = bool.from_bytes(b'\0', 'big')
>>> true = bool.from_bytes(b'\1', 'big')
>>> bool(false)
False
>>> bool(true)
True
>>> false is False
False
>>> true is True
False
>>> false
False
>>> true
False |
There are similar issues with Decimal.from_float() (C implementation only), chain.from_iterable(), epoll.fromfd() and kqueue.fromfd(). All these alternative constructors don't call __new__ or __init__. But float.fromhex() calls the constructor. >>> import enum
>>> class M(float, enum.Enum):
... PI = 3.14
...
>>> M.PI
<M.PI: 3.14>
>>> M.fromhex((3.14).hex())
<M.PI: 3.14>
>>> M.fromhex((2.72).hex())
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/serhiy/py/cpython/Lib/enum.py", line 241, in __call__
return cls.__new__(cls, value)
File "/home/serhiy/py/cpython/Lib/enum.py", line 476, in __new__
raise ValueError("%r is not a valid %s" % (value, cls.__name__))
ValueError: 2.72 is not a valid M And this behavior looks correct to me. |
Here is simplified Bruno's patch with additional tests for float, int, bool and enum. But I'm not sure that correct solution is so simple. |
0002-int.from_bytes-calls-constructor-for-subclasses.patch looks good to me, but see my review on Rietveld for 2 minor comments. |
Could you create new issues? I need a summary. :) |
hi i am ashutosh. I am running kubuntu 14.04. test___all__ After applying patch. It again gave me 2 error So,it worked for me. |
I applied patch to module and ran all the tested on Mac Os El Captain, only 4 tests were failing with or without applying patch, following are the tests failing: |
Not sure what I was thinking at the time, but several of my comments were supportive of |
There's a similar issue with replace() methods on date/time/datetime classes. They create instances of derived types without calling derived __new__/init, thus potentially leaving those uninitialized. >>> from datetime import date
>>> class D(date):
... def __init__(self, y, m, d):
... self.y = y
>>> D(2016,1,1).y
2016
>>> D(2016,1,1).replace(2015).y
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'D' object has no attribute 'y' |
No, they do not return instances of the derived type, they return instances of the base type. This is an intentional design decision that has been discussed elsewhere (including, I think, somewhere in this tracker). |
They sure do. Check the example in my comment, or if you prefer the source code, it's pretty clear as well: https://hg.python.org/cpython/file/fff0c783d3db/Modules/_datetimemodule.c#l2770 |
New changeset 0af15b8ef3b2 by Serhiy Storchaka in branch '3.5': New changeset df14ea17a517 by Serhiy Storchaka in branch 'default': |
Discussion on Python-Dev: http://comments.gmane.org/gmane.comp.python.devel/157649 . The conclusion is that alternate constructors should return an instance of the subclass, and either call constructor or use pickling machinery for creating an object in correct state. Pushed the fix for int.from_bytes() and tests. I'll open separate issues for other alternate constructors (for example bpo-27006 -- for Decimal.from_float()). Yes, there is similar bug in date.replace(). C implementation doesn't match Python implementation and bypasses the constructor. Please open separate issue for this Eugene. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: