Enum._convert shadows members named _convert

[orlnub123]

BPO	34282
Nosy	@vstinner, @ethanfurman, @orlnub123
PRs	bpo-34282: Fix Enum._convert shadowing members named _convert #8568 [3.7] bpo-34282: Fix Enum._convert shadowing members named _convert #9034 [3.6] bpo-34282: Fix Enum._convert method shadowing members named _convert #9229 bpo-34282: Remove deprecated _convert method #13823

^{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/ethanfurman'
closed_at = <Date 2018-10-21.18:33:39.855>
created_at = <Date 2018-07-30.17:36:39.221>
labels = ['3.7', '3.8', 'type-bug', 'library']
title = 'Enum._convert shadows members named _convert'
updated_at = <Date 2019-06-04.21:03:23.805>
user = 'https://github.com/orlnub123'

bugs.python.org fields:

activity = <Date 2019-06-04.21:03:23.805>
actor = 'vstinner'
assignee = 'ethan.furman'
closed = True
closed_date = <Date 2018-10-21.18:33:39.855>
closer = 'orlnub123'
components = ['Library (Lib)']
creation = <Date 2018-07-30.17:36:39.221>
creator = 'orlnub123'
dependencies = []
files = []
hgrepos = []
issue_num = 34282
keywords = ['patch']
message_count = 10.0
messages = ['322681', '324415', '324460', '324466', '324471', '324472', '324933', '325148', '327214', '344642']
nosy_count = 3.0
nosy_names = ['vstinner', 'ethan.furman', 'orlnub123']
pr_nums = ['8568', '9034', '9229', '13823']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue34282'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[orlnub123]

If an enum has a member named _convert it gets shadowed by the _convert method as shown below.

>>> import enum
>>>
>>> class Test(enum.Enum):
...     _convert = enum.auto()
...
>>> Test._convert
<bound method Enum._convert of <enum 'Test'>>

I've came up with a couple of solutions:

1. Add _convert to the invalid names list, next to mro
2. Rename _convert to _convert_ as sunder names are reserved
3. Move _convert to the metaclass

I think the first solution would be the worst as it would break existing enums that have _convert as a member (although unusable).

The problem with the second solution would be breaking external code that uses it although I think that's a moot point as it's meant for internal use. Another shortcoming would be having to update all the stdlib code that uses it.

The third solution might be a bit confusing on its own if an existing enum with a _convert member suddenly removed it leaving you with a bound method instead of raising an AttributeError.

[ethanfurman]

For versions 3.6 and 3.7 the solution is to modify the shadow check at line 236 so only DynamicClassAttributes are /not/ shadowed (meaning the _convert method would be shadowed by an _convert member).

For 3.8 we can move _convert to the metaclass: I wasn't originally supportive of this idea, but I can see it being useful for other Enum mix-ins such as str. However, I will want to include a check that such a mix-in is there -- probably by checking that the Enum type is a subclass of the first member's type... something like:

    if not issubclass(cls, members[0]):
       raise TypeError(...)

While we're making that change we can also check that members is not empty and issue a friendlier error message.

We can also rename _convert to _convert_ in 3.8, but we'll need to have _convert also on the metaclass and have it trigger a warning that _convert_ is now the right way, and _convert will go away in 3.9.

[orlnub123]

For versions 3.6 and 3.7 the solution is to modify the shadow check at line 236 so only DynamicClassAttributes are /not/ shadowed (meaning the _convert method would be shadowed by an _convert member).

Doing that might not be backwards-compatible. An enum with a subclassed behavior and member with the same name would shadow the behavior in favor of the member with the change. I propose adding an extra if statement under it that sets the member if it's named _convert and the _convert method on the class belongs to Enum (so it's not a behavior).

For 3.8 we can move _convert to the metaclass: I wasn't originally supportive of this idea, but I can see it being useful for other Enum mix-ins such as str. However, I will want to include a check that such a mix-in is there -- probably by checking that the Enum type is a subclass of the first member's type... something like:

if not issubclass(cls, members[0]):
   raise TypeError(...)

My thought for moving _convert to the metaclass was that it didn't make much sense for it to be accessible through a member. Could you elaborate on how mix-ins come into play?

While we're making that change we can also check that members is not empty and issue a friendlier error message.

I don't quite follow, what would members be in this case?

We can also rename _convert to _convert_ in 3.8, but we'll need to have _convert also on the metaclass and have it trigger a warning that _convert_ is now the right way, and _convert will go away in 3.9.

Sounds good.

[ethanfurman]

On 09/01/2018 01:21 AM, orlnub123 wrote:

On 08/31/2018 03:01 ethan.furman wrote:

> For versions 3.6 and 3.7 the solution is to modify the shadow check at
> line 236 so only DynamicClassAttributes are /not/ shadowed (meaning
> the _convert method would be shadowed by an _convert member).

Doing that might not be backwards-compatible. An enum with a subclassed
behavior and member with the same name would shadow the behavior in
favor of the member with the change. I propose adding an extra if
statement under it that sets the member if it's named _convert and the
_convert method on the class belongs to Enum (so it's not a behavior).

Fixing bugs is often not backwards-compatible. ;) I think the number
of affected people here will be low, though, because anybody who had an
_convert member should have noticed that they couldn't use it.

> For 3.8 we can move _convert to the metaclass: I wasn't originally
> supportive of this idea, but I can see it being useful for other Enum
> mix-ins such as str. However, I will want to include a check that
> such a mix-in is there -- probably by checking that the Enum type is a
> subclass of the first member's type... something like:
>
> if not issubclass(cls, members[0]):
> raise TypeError(...)

My thought for moving _convert to the metaclass was that it didn't make
much sense for it to be accessible through a member. Could you
elaborate on how mix-ins come into play?

Blocking access through the member is a low priority. For example,
originally Enum members were not available from each other (so
Color.Red.Blue didn't work), but that made accessing Color.Red _very_
slow. The fix for the slow made it so Color.Red.Blue now "works",
although it is not recommended.

Also, moving _convert to the metaclass means it is now available on Enum,
Flag, and IntFlag, and every user-defined non-IntEnum class where it
wasn't before. (This part will be in 3.8.)

The reason _convert exists at all is to be able to convert sets of global
constants into a drop-in replacement Enum, but that is only possible if
the drop-in Enum is compatible with the values being replaced -- so if
the values are integers (such as in the re module) then the replacement
Enum must be an IntEnum; if they were strings, then the replacement Enum
must be a StrEnum -- and the two cannot be mixed.

> While we're making that change we can also check that members is not
> empty and issue a friendlier error message.

I don't quite follow, what would members be in this case?

_convert is given a list of potential objects and a filter function to
select the ones to keep -- it is possible, most likely due to a bug in
user code, that the incoming list of objects is empty, or the filter
discards all of them, leaving members empty.

[orlnub123]

Fixing bugs is often not backwards-compatible. ;) I think the number
of affected people here will be low, though, because anybody who had an
_convert member should have noticed that they couldn't use it.

I think we had a misunderstanding, my proposal should be fully backwards-compatible. Maybe code can talk clearer than words? I'll attempt to turn it into a patch for 3.7 that can be backported to 3.6.

Blocking access through the member is a low priority. For example,
originally Enum members were not available from each other (so
Color.Red.Blue didn't work), but that made accessing Color.Red _very_
slow. The fix for the slow made it so Color.Red.Blue now "works",
although it is not recommended.

That's honestly very interesting, thanks for the insight.

Also, moving _convert to the metaclass means it is now available on Enum,
Flag, and IntFlag, and every user-defined non-IntEnum class where it
wasn't before. (This part will be in 3.8.)

In my testings _convert has always been available to them as they all subclass Enum where it's defined. Was it originally meant to be defined on IntEnum?

The reason _convert exists at all is to be able to convert sets of global
constants into a drop-in replacement Enum, but that is only possible if
the drop-in Enum is compatible with the values being replaced -- so if
the values are integers (such as in the re module) then the replacement
Enum must be an IntEnum; if they were strings, then the replacement Enum
must be a StrEnum -- and the two cannot be mixed.

I understand and agree with the need for a check now however because of what I talked about above there should be a deprecation period similar to the one for renaming _convert. Would it be okay to add one?

_convert is given a list of potential objects and a filter function to
select the ones to keep -- it is possible, most likely due to a bug in
user code, that the incoming list of objects is empty, or the filter
discards all of them, leaving members empty.

My confusion came from the fact that it doesn't raise any errors currently if they're empty. Are you suggesting that it should raise one?

[ethanfurman]

orlnub123 wrote:
---------------

In my testings _convert has always been available to them as they all
subclass Enum where it's defined.

Wow, I really shouldn't check for things in the middle of the night. On the bright side, no further changes are needed to _convert besides the rename.

orlnub123 wrote:
---------------

I think we had a misunderstanding, my proposal should be fully
backwards-compatible. Maybe code can talk clearer than words? I'll
attempt to turn it into a patch for 3.7 that can be backported to 3.6.

Please do. I'm curious to see what you come up with.

[ethanfurman]

New changeset c0d63bf by Ethan Furman (orlnub123) in branch '3.7':
[3.7] bpo-34282: Fix Enum._convert method shadowing members named _convert (GH-9034)
c0d63bf

[ethanfurman]

New changeset 0fb9fad by Ethan Furman (orlnub123) in branch 'master':
bpo-34282: Fix Enum._convert shadowing members named _convert (GH-8568)
0fb9fad

[ethanfurman]

New changeset 22e86fb by Ethan Furman (Miss Islington (bot)) in branch '3.6':
[3.7] bpo-34282: Fix Enum._convert method shadowing members named _convert (GH-9034) (GH-9229)
22e86fb