classification
Title: int.from_bytes() is broken for subclasses
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Arfrever, barry, bru, eli.bendersky, eltoder, ethan.furman, facundobatista, iyogeshjoshi, juggernaut, mark.dickinson, python-dev, r.david.murray, rhettinger, serhiy.storchaka, skrah, vstinner
Priority: high Keywords: patch

Created on 2015-03-11 13:34 by serhiy.storchaka, last changed 2016-05-12 12:23 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
issue23640.stoneleaf.01.patch ethan.furman, 2015-03-11 15:24 review
0001-int.from_bytes-calls-constructor-for-subclasses.patch bru, 2015-03-11 18:09 review
0002-int.from_bytes-calls-constructor-for-subclasses.patch serhiy.storchaka, 2015-09-28 07:26 review
Messages (24)
msg237864 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-11 13:34
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_'
msg237881 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2015-03-11 16:17
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?
msg237887 - (view) Author: Bruno Cauet (bru) * Date: 2015-03-11 16:58
Hi,
I feel like this behaviour does not only affect IntEnum & related but anything that inherits from int.

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:
- always return an int, and not the type. deleting Objects/longobjects.c:4845,4866 does the job.
- instantiate the required type with the value as the (sole?) argument, correctly initializing the object to be returned. In Serhyi's example this results in x == AddressFamily.AF_UNIX. the same lines are concerned.
msg237888 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-11 17:03
Not all classmethods are constructors.

I see two solutions:

1) Override from_bytes() in IntEnum and every int subclass that needs this.

2) Change int.from_bytes() to call __new__.
msg237891 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2015-03-11 17:26
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.
msg237896 - (view) Author: Bruno Cauet (bru) * Date: 2015-03-11 18:09
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.
msg239619 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2015-03-30 16:02
'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.
msg239622 - (view) Author: Bruno Cauet (bru) * Date: 2015-03-30 16:35
I'm not sure how you can have both, those two seem opposite to me:
- if 'from_bytes' returns the same type as the class it is called on then
the instantiation of the result object should go through its constructor
(patch proposed)
- if the subclass should override base class behaviour then there is no
reason for Enum.from_bytes to return the same type as the class it is
called on, and therefore it should be made a classmethod.

2015-03-30 18:02 GMT+02:00 Ethan Furman <report@bugs.python.org>:

>
> Ethan Furman added the comment:
>
> '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.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue23640>
> _______________________________________
>
msg240053 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2015-04-04 09:08
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.
msg240065 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-04-04 11:19
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.
msg240085 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-04 17:31
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
msg251750 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-28 07:21
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.
msg251751 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-28 07:26
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.
msg251833 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-29 09:46
0002-int.from_bytes-calls-constructor-for-subclasses.patch looks good to me, but see my review on Rietveld for 2 minor comments.
msg251863 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-09-29 15:14
> 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__.

Could you create new issues? I need a summary. :)
msg252091 - (view) Author: ashutosh (juggernaut) Date: 2015-10-02 08:16
hi

i am ashutosh. I am running kubuntu 14.04.
before applying the patch. It failed in 2 test i.e

test___all__ 
test_re

After applying patch. It again gave me 2 error
test___all__ 
test_re

So,it worked for me.
msg252092 - (view) Author: Yogesh Joshi (iyogeshjoshi) Date: 2015-10-02 08:32
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:
test___all__ test_calendar test_re test_socket
msg265104 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2016-05-07 23:18
Not sure what I was thinking at the time, but several of my comments were supportive of `classmethod`s not calling subclass' __new__; I actually do not think that, and am +1 on the patch.
msg265334 - (view) Author: Eugene Toder (eltoder) Date: 2016-05-11 18:53
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'
msg265352 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-05-12 00:13
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).
msg265355 - (view) Author: Eugene Toder (eltoder) Date: 2016-05-12 01:19
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
msg265372 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-05-12 07:40
New changeset 0af15b8ef3b2 by Serhiy Storchaka in branch '3.5':
Issue #23640: int.from_bytes() no longer bypasses constructors for subclasses.
https://hg.python.org/cpython/rev/0af15b8ef3b2

New changeset df14ea17a517 by Serhiy Storchaka in branch 'default':
Issue #23640: int.from_bytes() no longer bypasses constructors for subclasses.
https://hg.python.org/cpython/rev/df14ea17a517
msg265373 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-12 07:49
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 issue27006 -- 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.
msg265397 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-05-12 12:23
@eltoder: Ah, you are right, I was looking at the python version.  There's an open issue about this, #20371, which indicates that the design decision was to support subclasses.  Clearly I was remembering the conversation backward based on looking at the wrong code :)
History
Date User Action Args
2016-05-12 12:23:16pitrousetnosy: - pitrou
2016-05-12 12:23:00r.david.murraysetmessages: + msg265397
2016-05-12 07:49:10serhiy.storchakasetstatus: open -> closed

assignee: serhiy.storchaka
components: + Interpreter Core, - Library (Lib)
versions: + Python 3.6, - Python 3.4
resolution: fixed
messages: + msg265373
stage: needs patch -> resolved
2016-05-12 07:40:08python-devsetnosy: + python-dev
messages: + msg265372
2016-05-12 01:19:20eltodersetmessages: + msg265355
2016-05-12 00:13:17r.david.murraysetnosy: + r.david.murray
messages: + msg265352
2016-05-11 18:53:27eltodersetnosy: + eltoder
messages: + msg265334
2016-05-08 15:50:22ethan.furmansetmessages: + msg240053
2016-05-08 15:50:12ethan.furmansetmessages: + msg239619
2016-05-08 15:50:02ethan.furmansetmessages: + msg237891
2016-05-07 23:21:08ethan.furmansetmessages: - msg240053
2016-05-07 23:20:48ethan.furmansetmessages: - msg239619
2016-05-07 23:19:47ethan.furmansetmessages: - msg237891
2016-05-07 23:18:48ethan.furmansetnosy: + ethan.furman
messages: + msg265104
2015-10-02 08:32:24iyogeshjoshisetnosy: + iyogeshjoshi
messages: + msg252092
2015-10-02 08:16:07juggernautsetnosy: + juggernaut
messages: + msg252091
2015-09-29 15:14:06skrahsetmessages: + msg251863
2015-09-29 09:46:57vstinnersetnosy: + vstinner
messages: + msg251833
2015-09-28 15:34:39Arfreversetnosy: + Arfrever
2015-09-28 07:26:06serhiy.storchakasetfiles: + 0002-int.from_bytes-calls-constructor-for-subclasses.patch

messages: + msg251751
2015-09-28 07:21:22serhiy.storchakasetnosy: + rhettinger, facundobatista, mark.dickinson, skrah
messages: + msg251750
2015-07-21 08:12:49ethan.furmansetassignee: ethan.furman -> (no value)
title: Enum.from_bytes() is broken -> int.from_bytes() is broken for subclasses
nosy: - ethan.furman
2015-04-04 17:31:25serhiy.storchakasetpriority: normal -> high

messages: + msg240085
2015-04-04 11:19:54pitrousetnosy: + pitrou
messages: + msg240065
2015-04-04 09:08:10ethan.furmansetassignee: ethan.furman
messages: + msg240053
2015-03-30 16:35:05brusetmessages: + msg239622
2015-03-30 16:02:01ethan.furmansetmessages: + msg239619
2015-03-11 18:09:19brusetfiles: + 0001-int.from_bytes-calls-constructor-for-subclasses.patch

messages: + msg237896
2015-03-11 17:26:02ethan.furmansetmessages: + msg237891
2015-03-11 17:03:51serhiy.storchakasetmessages: + msg237888
2015-03-11 16:58:04brusetnosy: + bru
messages: + msg237887
2015-03-11 16:17:38ethan.furmansetmessages: + msg237881
2015-03-11 15:24:10ethan.furmansetfiles: + issue23640.stoneleaf.01.patch
keywords: + patch
stage: test needed -> needs patch
2015-03-11 15:15:15ethan.furmansetstage: test needed
2015-03-11 13:34:36serhiy.storchakacreate