classification
Title: Enum members are easily replaced
Type: behavior Stage: resolved
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ethan.furman Nosy List: barry, eli.bendersky, eric.snow, ethan.furman, pitrou, python-dev
Priority: normal Keywords: buildbot

Created on 2013-09-04 19:47 by ethan.furman, last changed 2013-09-06 14:17 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
issue18924.stoneleaf.patch.01 ethan.furman, 2013-09-05 05:25 review
Messages (16)
msg196945 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-04 19:47
Python 3.4.0a1+ (default:33727fbb4668+, Aug 31 2013, 12:34:55) 
[GCC 4.7.3] on linux
Type "help", "copyright", "credits" or "license" for more information.
--> import enum
--> class Test(enum.Enum):
...   this = 'that'
... 
--> Test.this
<Test.this: 'that'>
--> Test.this = 9
--> Test.this
9

I'm pretty sure we don't want that.
msg196946 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-04 19:55
On Sep 04, 2013, at 07:47 PM, Ethan Furman wrote:

>I'm pretty sure we don't want that.

Agreed, although a "we're all consenting adults" argument could be made.
msg196948 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-04 20:12
I'm not suggesting we try to make it impossible, just tougher (akin to what we did with the name and value attributes on 
an Enum member -- at Guido's behest, no less ;).
msg196949 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2013-09-04 20:18
Time for your friendly devil's advocate... We're using and loving this language:

>>> class Foo:
...   bar = 2
... 
>>> f = Foo()
>>> f.bar
2
>>> f.bar = 42
>>> f.bar
42
>>> 


So let's stop trying to make enums even more alien. This is a non-issue in Python.

[Barry, how come your name in the tracker is linked to your website? me wants...]
msg196950 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-04 20:23
On Sep 04, 2013, at 08:18 PM, Eli Bendersky wrote:

>[Barry, how come your name in the tracker is linked to your website? me
>wants...]

Go to "Your Details" in the left sidebar and enter a "Homepage".
msg196951 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2013-09-04 20:34
"You do not have permission to edit user" - in a red banner, no less. Blasphemy.
msg196954 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-04 20:57
Eli Bendersky added the comment:
>
> So let's stop trying to make enums even more alien. This is a non-issue in Python.

Enumerations are supposed to be constant.  Since this is Python there is actually very little that cannot be changed, 
but we can make objects better reflect our intent.

For Enum members Guido had me change the `value` and `name` attributes to properties because the value and name should 
also be constant.  Can they still be changed?  Yes, but you have to know what you're doing.  (Enum.member._name_ = ... )

I'm proposing we do the same thing for the Enum class that we did for the Enum member.

To me, an Enumeration that lets you change its constants higgledy-piggledy is way more alien than one that tries to 
stay, um, /constant/.
msg196955 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2013-09-04 21:08
On Wed, Sep 4, 2013 at 1:57 PM, Ethan Furman <report@bugs.python.org> wrote:

>
> Ethan Furman added the comment:
>
> Eli Bendersky added the comment:
> >
> > So let's stop trying to make enums even more alien. This is a non-issue
> in Python.
>
> Enumerations are supposed to be constant.  Since this is Python there is
> actually very little that cannot be changed,
> but we can make objects better reflect our intent.
>
> For Enum members Guido had me change the `value` and `name` attributes to
> properties because the value and name should
> also be constant.  Can they still be changed?  Yes, but you have to know
> what you're doing.  (Enum.member._name_ = ... )
>
> I'm proposing we do the same thing for the Enum class that we did for the
> Enum member.
>
> To me, an Enumeration that lets you change its constants higgledy-piggledy
> is way more alien than one that tries to
> stay, um, /constant/.
>

I can empathize with your reasoning, but I'm still -1. There's an infinite
amount of tweaks you can think of to make Enum "safer", in a language that
by design is unsafe for such operations. You will keep finding new holes,
because the language will keep fighting you. And the end result? A bit more
theoretical purity, hardly any tangible gain. But more complex code, which
makes it more prone to bugs and more difficult to understand.
msg196961 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-09-04 21:57
I'm also -1, though I do appreciate the "indicating intent" argument.  What's the risk that someone will accidentally overwrite an enum item?  Also, is there other enum functionality that relies on the continued existence of the initial enum items?  If not then I'm in the "consenting adults" camp.  Eli makes a good point about the potential for (ultimately) unnecessary complexity and what that costs us.

However, now's the time to come to a conclusion--before the 3.4 release (and likely beta 1) lock in the API.
msg196966 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-04 22:05
Yes, as a matter of fact:

--> Test.this
<Test.this: 'that'>
--> Test.this = 'other'
--> Test.this
'other'
--> Test('that')
<Test.this: 'that'>
--> list(Test)
[<Test.this: 'that'>]

As you can see, the Test Enum becomes inconsistent if this is allowed.
msg197008 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2013-09-05 15:54
On Wed, Sep 4, 2013 at 3:05 PM, Ethan Furman <report@bugs.python.org> wrote:

>
> Ethan Furman added the comment:
>
> Yes, as a matter of fact:
>
> --> Test.this
> <Test.this: 'that'>
> --> Test.this = 'other'
> --> Test.this
> 'other'
> --> Test('that')
> <Test.this: 'that'>
> --> list(Test)
> [<Test.this: 'that'>]
>
> As you can see, the Test Enum becomes inconsistent if this is allowed.
>

Again, this is fully in accordance to the Python philosophy of allowing
monkey-patching in the first place. There's any number of way
monkey-patching objects can lead to inconsistent states, because the
internal invariants are only guaranteed to be preserved through public,
documented interfaces.

I'm still -1.
msg197024 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-05 20:56
I think Ethan has a point that the inconsistency when overriding a member can hide subtle bugs. I would agree with Eli and Eric if it wasn't for that problem. Also, we can first forbid overriding, then change our decision later on if someone comes with a use case (doing the converse would be more annoying as it would break compatibility).
msg197040 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-09-06 01:50
> I would agree with Eli and Eric if it wasn't for that problem.

Agreed.  That was the gist of my question that led to Ethan's example.  If it's easy to accidentally break an enum, particularly in a subtle way, then it may not be worth taking a consenting adults approach.

Usually in consenting adults cases, it's simply not worth adding the extra complexity (or taking the time) to lock down an API or cover all the cases--it won't be a problem in practice since normal usage doesn't bear enough risk to worry about it.

In the case of enums, particularly in how they re-purpose classes and yet allow non-item attributes, there's a higher risk of accidentally breaking something during normal usage.  They quack like a class, but maybe they need to look less like a duck.  The question is, is the risk (and associated cost) of accidental breakage high enough to warrant the cost of being more heavy-handed?

> Also, we can first forbid overriding, then change our decision later
> on if someone comes with a use case (doing the converse would be more
> annoying as it would break compatibility).

+1
msg197046 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-06 02:48
Heavy-handed would be having the metaclass turn all the enum members into read-only properties.  The solution I have proposed is more like a wagging of one's finger saying, "No, no, that's not the right way!"  ;)
msg197073 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2013-09-06 14:13
In retrospect the read-only properties would not be any more difficult to get around than the __setattr__ solution, and it would conflict with our use of _RouteClassAttributeToGetattr.

To properly replace an enum member one has to change two internal data structures:

    _member_map_      -> 'enum_name' : member
    _member2value_map -> enum_value  : member   # if hashable

To actually create a real (non-mock) member is even more work.
msg197074 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-09-06 14:17
New changeset 1d88d04aade2 by Ethan Furman in branch 'default':
Close #18924:  Block naive attempts to change an Enum member.
http://hg.python.org/cpython/rev/1d88d04aade2
History
Date User Action Args
2013-09-06 14:17:19python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg197074

resolution: fixed
stage: patch review -> resolved
2013-09-06 14:13:35ethan.furmansetmessages: + msg197073
2013-09-06 02:48:48ethan.furmansetmessages: + msg197046
2013-09-06 01:50:55eric.snowsetmessages: + msg197040
2013-09-05 20:56:37pitrousetnosy: + pitrou
messages: + msg197024
2013-09-05 15:54:24eli.benderskysetmessages: + msg197008
2013-09-05 05:25:49ethan.furmansetfiles: + issue18924.stoneleaf.patch.01
stage: patch review
2013-09-04 22:05:18ethan.furmansetmessages: + msg196966
2013-09-04 21:57:04eric.snowsetnosy: + eric.snow
messages: + msg196961
2013-09-04 21:08:14eli.benderskysetmessages: + msg196955
2013-09-04 20:57:37ethan.furmansetmessages: + msg196954
2013-09-04 20:34:36eli.benderskysetmessages: + msg196951
2013-09-04 20:23:19barrysetmessages: + msg196950
2013-09-04 20:18:37eli.benderskysetkeywords: + buildbot

messages: + msg196949
2013-09-04 20:12:46ethan.furmansetmessages: + msg196948
2013-09-04 19:55:49barrysetmessages: + msg196946
2013-09-04 19:47:27ethan.furmancreate