Conflict between lexical scoping and name injection in __prepare__

[ethanfurman]

BPO	17853
Nosy	@warsaw, @mdickinson, @ncoghlan, @benjaminp, @alex, @durban, @ethanfurman
Files	classderef.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-04-30.13:41:49.114>
created_at = <Date 2013-04-27.08:10:01.898>
labels = ['interpreter-core', 'type-bug']
title = 'Conflict between lexical scoping and name injection in __prepare__'
updated_at = <Date 2013-04-30.13:41:49.112>
user = 'https://github.com/ethanfurman'

bugs.python.org fields:

activity = <Date 2013-04-30.13:41:49.112>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2013-04-30.13:41:49.114>
closer = 'python-dev'
components = ['Interpreter Core']
creation = <Date 2013-04-27.08:10:01.898>
creator = 'ethan.furman'
dependencies = []
files = ['30043']
hgrepos = []
issue_num = 17853
keywords = ['patch']
message_count = 16.0
messages = ['187892', '187895', '187934', '187936', '187937', '187953', '187957', '187958', '187959', '187964', '187983', '187988', '187997', '188008', '188157', '188163']
nosy_count = 8.0
nosy_names = ['barry', 'mark.dickinson', 'ncoghlan', 'benjamin.peterson', 'alex', 'daniel.urban', 'ethan.furman', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue17853'
versions = ['Python 3.4']

[ethanfurman]

In playing with metaclasses for the ongoing Enum saga, I tried having the metaclass insert an object into the custom dict (aka namespace) returned by __prepare__; this object has the same name as the to-be-created class.

An example:

class Season(Enum):
    SPRING = Season()
    SUMMER = Season()
    AUTUMN = Season()
    WINTER = Season()

When this executes as top level code it works beautifully.

However, if you have the exact same definition in a function, Bad Things happen:

---------------------------------------

--> def test():
...   class Season(Enum):
...     SPRING = Season()
... 
--> test()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 2, in test
  File "<stdin>", line 3, in Season
NameError: free variable 'Season' referenced before assignment in enclosing scope

---

So I tried creating a dummy variable to see if it would be worked around:

---------------------------------------

--> def test():
...   Season = None
...   class Season(Enum):
...     SPRING = Season()
... 
--> test()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in test
  File "<stdin>", line 4, in Season
TypeError: 'NoneType' object is not callable

---

Finally I inserted the object using a different name, and also printed 'locals()' just to see what was going on:

---------------------------------------
--> def test():
... class Season(Enum):
... print(locals())
... SPRING = S()
...
--> test()
{'S': <class 'aenum.Season'>, 'Season': <class 'aenum.Season'>, '__module__': '__main__', '__qualname__': 'test.<locals>.Season', '__locals__': {...}}
---------------------------------------

and an actual (albeit extremely ugly) work around:

---------------------------------------

>>> def test():
...   class Season(Enum):
...     Season = locals()['Season']
...     SPRING = Season()
...   print(Season)
... 
>>> test()
Season(SPRING=1)

---

It seems that the function namespace is seriously messing with the class namespace.

[ethanfurman]

One more data point to truly demonstrate that something is wrong:

--------------------------------------------
--> def test():
... class Season(Enum):
... print(locals())
... Season = locals()['Season']
... print(locals())
...
--> test()
{'Season': <class 'aenum.Season'>, '__module__': '__main__', '__locals__': {...}}
{'Season': <class 'aenum.Season'>, '__module__': '__main__', '__locals__': {...}}
--------------------------------------------

Notice the class name space *did not change in any way* before and after setting 'Season' manually.

[mdickinson]

Isn't this just a consequence of Python's usual name-lookup rules? In this code:

    def test():
        class Season(Enum):
            SPRING = Season()

the class definition in 'test' amounts to a local assignment to the name 'Season'. So the occurrence of 'Season' in the class definition is bound to that function local (at bytecode level, it's looked up using LOAD_DEREF instead of LOAD_NAME).

I find it difficult to see how one could 'fix' this without significant changes to the way that Python does name resolution.

[ethanfurman]

On 04/27/2013 02:01 PM, Mark Dickinson wrote:

Mark Dickinson added the comment:

Isn't this just a consequence of Python's usual name-lookup rules? In this code:

 def test():
     class Season(Enum):
         SPRING = Season()

the class definition in 'test' amounts to a local assignment to the name 'Season'. So the occurrence of 'Season' in the class definition is bound to that function local (at bytecode level, it's looked up using LOAD_DEREF instead of LOAD_NAME).

The key point is that class construction should take place in its own namespace (it has its own locals(), after all),
only looking to enclosing name spaces if it can't find the name in its locals() (which it won't know until it tries).
Granted, this was never a problem before __prepare__ was available, but now that it is we need to have the name space
look-up rules applied appropriately, and not have the enclosing function strong-arm its own values into the class namespace.

I find it difficult to see how one could 'fix' this without significant changes to the way that Python does name resolution.

I don't know if this helps, but here's the dis for three different runs:

--> def test1():
... class WeekDay(Enum):
... MONDAY = WeekDay(1)
...
--> dis.dis(test1)
2 0 LOAD_BUILD_CLASS
1 LOAD_CLOSURE 0 (WeekDay)
4 BUILD_TUPLE 1
7 LOAD_CONST 1 (<code object WeekDay at 0x7f60264c6608, file "<stdin>", line 2>)
10 MAKE_CLOSURE 0
13 LOAD_CONST 2 ('WeekDay')
16 LOAD_GLOBAL 0 (Enum)
19 CALL_FUNCTION 3
22 STORE_DEREF 0 (WeekDay)
25 LOAD_CONST 0 (None)
28 RETURN_VALUE

--> def test2():
... class WeekDay(Enum):
... MONDAY = enum(1)
...
--> dis.dis(test2)
2 0 LOAD_BUILD_CLASS
1 LOAD_CONST 1 (<code object WeekDay at 0x7f602490b7a0, file "<stdin>", line 2>)
4 MAKE_FUNCTION 0
7 LOAD_CONST 2 ('WeekDay')
10 LOAD_GLOBAL 0 (Enum)
13 CALL_FUNCTION 3
16 STORE_FAST 0 (WeekDay)
19 LOAD_CONST 0 (None)
22 RETURN_VALUE

--> def test3():
... class WeekDay(Enum):
... WeekDay = locals()['WeekDay']
... MONDAY = WeekDay(1)
...
--> dis.dis(test3)
2 0 LOAD_BUILD_CLASS
1 LOAD_CONST 1 (<code object WeekDay at 0x7f602490ee88, file "<stdin>", line 2>)
4 MAKE_FUNCTION 0
7 LOAD_CONST 2 ('WeekDay')
10 LOAD_GLOBAL 0 (Enum)
13 CALL_FUNCTION 3
16 STORE_FAST 0 (WeekDay)
19 LOAD_CONST 0 (None)
22 RETURN_VALUE

test1's dis should look more like test3's. Also, I find it interesting that test2's dis is exactly the same as test3's.

[ethanfurman]

I guess the question is:

Because Python can no longer know if the name has been inserted via __prepare__ it has to go through the whole CLOSURE
business, but will changing the LOAD_DEREF to a LOAD_GLOBAL (only for items inside a class def) do the trick?

[ncoghlan]

Just to clarify, the problem here isn't to do with referencing the class name in particular, it's referencing *any* lexically scoped name from the class body, when a metaclass wants to inject that as variable name in the class namespace. Here's a case where it silently looks up the wrong value:

>>> class Meta(type): pass
... 
>>> def f():
...     outer = "lexically scoped"
...     class inner(metaclass=Meta):
...         print(outer)
... 
>>> f()
lexically scoped
>>> class Meta(type):
...     def __prepare__(*args):
...         return dict(outer="from metaclass")
... 
>>> f()
lexically scoped

That second one *should* say "from metaclass", but it doesn't because the LOAD_DEREF completely ignores the local namespace. You can get the same exception noted above by moving the assignment after the inner class definition:

>>> def g():
...     class inner(metaclass=Meta):
...         print(outer)
...     outer = "This causes an exception"
... 
>>> g()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 2, in g
  File "<stdin>", line 3, in inner
NameError: free variable 'outer' referenced before assignment in enclosing scope                      

We simply missed the fact that PEP-3115 and the __prepare__ method mean that using LOAD_DEREF to resolve lexically scoped names in a nested class is now wrong. Instead, we need a new opcode that first tries the class namespace and only if that fails does it fall back to looking it up in the lexically scoped cell reference.

(I changed the affected versions, as even though this *is* a bug in all current Python 3 versions, there's no way we're going to change the behaviour of name resolution in a maintenance release)

[ethanfurman]

Perhaps you and Benjamin should discuss that. :)

I just read a thread on core-mentors about when a bug is fixed or not -- considering that the behavior is plain wrong,
that workarounds will still work even if the bug is fixed, why shouldn't we fix it even in a maintenance release?

[benjaminp]

There's no need for a discussion; the answer is no.

[ncoghlan]

In this case, we won't fix it in a maintenance release because of the kind of change required to eliminate the bug. Adding a new opcode is likely to be the simplest fix and that is necessarily a backwards incompatible change (since older versions won't understand the new opcode).

Even if we find a solution that doesn't require a new opcode, fixing the problem is going to require changes to both the compiler and the eval loop, and we simply don't touch those in maintenance releases without a *really* compelling reason. Fixing an edge case related to the interaction between two features that are already somewhat obscure on their own doesn't qualify.

If anyone does decide to tackle this, I'll note that my examples in my previous post may be useful as inspiration for a test case - the final versions of both f() and g() should report "from metaclass" as the value of "outer" inside the class body, and it should be simple enough to replace the print statements with self.assertEqual() in a unit test.

[ethanfurman]

Nick, thanks for the explanation.

Benjamin, I was referring to Nick taking 3.3 and 3.2 off the issue and leaving 3.4, and you reversing it. ;)

Sorry for the confusion -- I just reread my post and the words there didn't match the ideas in my head at the time very
well.

[benjaminp]

Nick, care to review?

[ethanfurman]

Thanks, Benjamin. Looking at that patch I realize the fix was way beyond my current core-hacking skills.