Switch suitable constants in the socket module to IntEnum

[elibendersky]

BPO	18720
Nosy	@gvanrossum, @warsaw, @ncoghlan, @pitrou, @tiran, @ethanfurman, @serhiy-storchaka
Files	socket-intenum-af.1.patch socket-intenum-af.2.patch socket-intenum-af.3.patch socket-intenum-af.4.patch socket-intenum-af.5.patch socket-intenum-af-type.6.patch

^{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 = None
closed_at = <Date 2013-08-31.22:13:55.642>
created_at = <Date 2013-08-12.21:10:49.284>
labels = ['type-feature', 'library']
title = 'Switch suitable constants in the socket module to IntEnum'
updated_at = <Date 2013-08-31.22:21:55.668>
user = 'https://bugs.python.org/elibendersky'

bugs.python.org fields:

activity = <Date 2013-08-31.22:21:55.668>
actor = 'eli.bendersky'
assignee = 'eli.bendersky'
closed = True
closed_date = <Date 2013-08-31.22:13:55.642>
closer = 'python-dev'
components = ['Library (Lib)']
creation = <Date 2013-08-12.21:10:49.284>
creator = 'eli.bendersky'
dependencies = []
files = ['31261', '31264', '31291', '31313', '31468', '31521']
hgrepos = []
issue_num = 18720
keywords = ['patch']
message_count = 30.0
messages = ['195018', '195027', '195126', '195129', '195145', '195146', '195149', '195153', '195156', '195157', '195161', '195163', '195165', '195166', '195167', '195169', '195316', '195328', '195330', '195397', '195425', '195426', '195441', '196173', '196241', '196539', '196554', '196678', '196680', '196682']
nosy_count = 10.0
nosy_names = ['gvanrossum', 'barry', 'ncoghlan', 'pitrou', 'christian.heimes', 'eli.bendersky', 'neologix', 'ethan.furman', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue18720'
versions = ['Python 3.4']

[elibendersky]

As part of original plan and since issue bpo-18264 has been resolved, it's time to dust off some old patches I have for the socket.* module. The socket.AF_* and socket.SOCK_* constants are good candidates for IntEnum conversion.

I'm attaching an initial patch that handles socket.AF_* (as a proof-of-concept; socket.SOCK_* should get identical treatment); it only touches Lib/socket.py and all regrtest tests pass without changes. Result:

>>> import socket
>>> socket.AF_INET
<AddressFamily.AF_INET: 2>
>>> s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
>>> s.family
<AddressFamily.AF_INET: 2>
>>> '{}'.format(s.family)
'AddressFamily.AF_INET'

The code in the patch is pretty well commented, and I also want to point out a couple of decision points that came up:

1. The underlying socketmodule.c has no idea of IntEnums, and neither IMHO it should. These constants are essentially one-way, going from Python into C. They are only exposed back through read-only accessors (e.g. s.family above), at which point the Python code can wrap them back into the enum. The alternative, making socketmodule.c use enums is probably way more complicated than really necessary.
2. As a followup to (1), the constants are actually wrapped into an IntEnum at the Python level and exposed back to the user. My hacking of globals() there may be a bit rough - suggestions for a better way are welcome.
3. A bunch of AF_* constants exported by Python built on my x64 Ubuntu are not documented in socket.rst; I'm still wrapping them all in enums; I basically went over all PyModule_AddIntMacro(m, AF_*) in PyInit__socket.

PTAL

[elibendersky]

Attaching updated patch answering Ethan's review. Also added a tiny sanity test that makes sure that AF_INET is indeed an enum. A more comprehensive test probably won't hurt for the final patch.

[serhiy-storchaka]

What about SOCK_STREAM, SOCK_DGRAM, and SOCK_RAW?

[pitrou]

I'm not really thrilled by the current implementation. The way it is done leads to duplication among socket constants. Also, if I create a socket from a numeric value (because a well-known socket family is known yet known by Python), then socket.family will fail.

[elibendersky]

I'm not really thrilled by the current implementation. The way it is done
leads to duplication among socket constants.

Can you clarify which duplication you mean specifically? In the code review
tool, Serhiy proposed that instead of listing all the constants explicitly
in the Python code, we can just do:

AddressFamily = IntEnum('AddressFamily',
           {name: value for name, value in _moduledict.items()
            if name.startswith('AF_')})

Is this less duplication?

Also, if I create a socket from a numeric value (because a well-known
socket family is known yet known by Python), then socket.family will fail.

Well, the documentation of socket.socket says it should be one of the AF_*
constants. One thing we can do is check in the socket.socket constructor
whether it's a known AF_* constants and fail early with a meaningful error
message. Would that be better?

[elibendersky]

What about SOCK_STREAM, SOCK_DGRAM, and SOCK_RAW?

This is a proof of concept. As I mentioned in the original message starting
this issue - the SOCK_* constants will get the same treatment as AF_*
constants, once we decide what the right treatment is.

[pitrou]

Can you clarify which duplication you mean specifically? In the code
review
tool, Serhiy proposed that instead of listing all the constants
explicitly
in the Python code, we can just do:

AddressFamily = IntEnum('AddressFamily',
{name: value for name, value in _moduledict.items()
if name.startswith('AF_')})

Yes, that's better.

> Also, if I create a socket from a numeric value (because a
> well-known
> socket family is known yet known by Python), then socket.family
> will fail.
>

Well, the documentation of socket.socket says it should be one of the
AF_*
constants. One thing we can do is check in the socket.socket
constructor
whether it's a known AF_* constants and fail early with a meaningful
error
message. Would that be better?

Well, no. We should still allow creating sockets with unknown numeric
values. The .family attribute should then return the raw integer.

[serhiy-storchaka]

I'm surprised that test_repr is not failed. '%i' % socket.AddressFamily.AF_INET returns 'AddressFamily.AF_INET' instead of '2' as I expected. Is not this a bug in IntEnum implementation?

[elibendersky]

Attaching a new patch that uses Serhiy's suggestion to avoid the duplication, and uses Antoine's suggestion to keep socket.family working even if the family is an unknown numeric constant.

Note that the latter is challenging to test - I need families the OS knows and socket doesn't. Any suggestions are welcome. For the time being the test I cooked up is an abomination ;-)

[elibendersky]

On Wed, Aug 14, 2013 at 6:24 AM, Serhiy Storchaka <report@bugs.python.org>wrote:

Serhiy Storchaka added the comment:

I'm surprised that test_repr is not failed. '%i' %
socket.AddressFamily.AF_INET returns 'AddressFamily.AF_INET' instead of '2'
as I expected. Is not this a bug in IntEnum implementation?

I did notice it when playing with the implementation. Perhaps %i doesn't
call enum? Need to look into it more carefully...

[ethanfurman]

bpo-18738 created.

[tiran]

The manual list of AF_ prefixes looks ugly. We are going to run into the same problem for every module. How about an additional constructor that takes a name, dict and string prefix?

class IntEnum(int, Enum):
    @classmethod
    def from_moduledict(cls, name, moddict, prefix):
        members = {name: value for name, value in moddict.items()
                   if name.startswith(prefix)}
        return cls(name, members)

>>> import socket, enum, pprint
>>> AddressFamily = enum.IntEnum.from_moduledict("AddressFamily", socket.__dict__, "AF_")
>>> pprint.pprint(dict(AddressFamily.__members__))
{'AF_APPLETALK': <AddressFamily.AF_APPLETALK: 5>,
 'AF_ASH': <AddressFamily.AF_ASH: 18>,
 'AF_ATMPVC': <AddressFamily.AF_ATMPVC: 8>,
 'AF_ATMSVC': <AddressFamily.AF_ATMSVC: 20>,
 'AF_AX25': <AddressFamily.AF_AX25: 3>,
 'AF_BLUETOOTH': <AddressFamily.AF_BLUETOOTH: 31>,
 'AF_BRIDGE': <AddressFamily.AF_BRIDGE: 7>,
 'AF_CAN': <AddressFamily.AF_CAN: 29>,
 'AF_DECnet': <AddressFamily.AF_DECnet: 12>,
 'AF_ECONET': <AddressFamily.AF_ECONET: 19>,
 'AF_INET': <AddressFamily.AF_INET: 2>,
 'AF_INET6': <AddressFamily.AF_INET6: 10>,
 'AF_IPX': <AddressFamily.AF_IPX: 4>,
 'AF_IRDA': <AddressFamily.AF_IRDA: 23>,
 'AF_KEY': <AddressFamily.AF_KEY: 15>,
 'AF_LLC': <AddressFamily.AF_LLC: 26>,
 'AF_NETBEUI': <AddressFamily.AF_NETBEUI: 13>,
 'AF_NETLINK': <AddressFamily.AF_NETLINK: 16>,
 'AF_NETROM': <AddressFamily.AF_NETROM: 6>,
 'AF_PACKET': <AddressFamily.AF_PACKET: 17>,
 'AF_PPPOX': <AddressFamily.AF_PPPOX: 24>,
 'AF_RDS': <AddressFamily.AF_RDS: 21>,
 'AF_ROSE': <AddressFamily.AF_ROSE: 11>,
 'AF_ROUTE': <AddressFamily.AF_NETLINK: 16>,
 'AF_SECURITY': <AddressFamily.AF_SECURITY: 14>,
 'AF_SNA': <AddressFamily.AF_SNA: 22>,
 'AF_TIPC': <AddressFamily.AF_TIPC: 30>,
 'AF_UNIX': <AddressFamily.AF_UNIX: 1>,
 'AF_UNSPEC': <AddressFamily.AF_UNSPEC: 0>,
 'AF_WANPIPE': <AddressFamily.AF_WANPIPE: 25>,
 'AF_X25': <AddressFamily.AF_X25: 9>}