Scope bug and generators

[akaptur]

This PR:

• Fixes the implementation of generators by giving each frame its own data stack, not having one data stack for the VM (thanks to @rntz for pairing on this). I think this is consistent with the implementation of CPython. See, e.g., this blog post:

  Finally, inside each frame object in the call stack there’s a reference to two frame-specific stacks ... the value stack and the block stack."

• Exposes and fixes a bug around nested scoping that allowed inner function scopes to clobber outer function scopes if they had identically-named variables (this was part of the problem with generators, which set up a local variable named ".0"). The fix is to not pass through the local variables from one frame to another. I think this solution is true to the implementation of CPython and not just an ugly hack.

• Exposes problems with class inheritance - I think this was only ever working because of the bad scoping bug above. I've added a test of dynamic attribute lookup that shows our old implementation wasn't right. This PR doesn't fix the class inheritance problems in Python 2, but I have spent a lot of time reading through object.c, typeobject.c, and classobject.c, so maybe I'll make some headway on that eventually.

• Adds "if PY2" around the object and class implementation to make clear that we're not using them in Python 3. (We're not putting 'if PY2' and 'if PY3' around every bytecode or function that's specific to one or the other, but I think it's worth it here - I didn't realize until a few days ago that we're falling back to the builtin objects and classes in Python 3.)

[nedbat]

Wow, this is amazing! I'm humbled to learn that I had the data stack wrong, and a 5-minute look at frameobject.{h,c} confirms it!

I'm confused about the PY2-only Object, etc classes. I don't remember deciding to use them differently in Py2 and Py3. Why is that?

Thanks so much for this, I'm going to review it in detail.

[akaptur]

It took me considerably longer than that on frameobject!

On the classes and objects, our code was bypassed because Py3 doesn't emit BUILD_CLASS. Instead it emits LOAD_BUILD_CLASS, which pushes a new builtin called __build_class__ on to the stack. Looking at the output of tox -- -s tests.test_basic:TestIt.test_calling_subclass_methods is interesting here, since it allows us to examine the stack.

Our creation of a new Class only happens in BUILD_CLASS, which is simply never invoked by Py3, so we were accidentally falling back to the default internal mechanics. If we want to avoid doing that we can probably shadow the builtin or write LOAD_BUILD_CLASS to load something else.

Py2:

>>> def cls():
...     class Foo(object):
...         pass
... 
>>> dis.dis(cls)
  2           0 LOAD_CONST               1 ('Foo')
              3 LOAD_GLOBAL              0 (object)
              6 BUILD_TUPLE              1
              9 LOAD_CONST               2 (<code object Foo at 0x10e246730, file "<stdin>", line 2>)
             12 MAKE_FUNCTION            0
             15 CALL_FUNCTION            0
             18 BUILD_CLASS         
             19 STORE_FAST               0 (Foo)
             22 LOAD_CONST               0 (None)
             25 RETURN_VALUE  

Py3:

PY3 >>> def cls():
PY3 ...     class Foo(object):
PY3 ...         pass
PY3 ... 
PY3 >>> dis.dis(cls)
  2           0 LOAD_BUILD_CLASS     
              1 LOAD_CONST               1 (<code object Foo at 0x107780540, file "<stdin>", line 2>) 
              4 LOAD_CONST               2 ('Foo') 
              7 MAKE_FUNCTION            0 
             10 LOAD_CONST               2 ('Foo') 
             13 LOAD_GLOBAL              0 (object) 
             16 CALL_FUNCTION            3 (3 positional, 0 keyword pair) 
             19 STORE_FAST               0 (Foo) 
             22 LOAD_CONST               0 (None) 
             25 RETURN_VALUE   

[akaptur]

code review welcome.

[nedbat]

Am I doing something wrong? When I run the tests on Py2.7, I get three test failures (captured stdout and logging removed):

(byterun)((58e6038...)) ~/byterun $ tox -e py27
GLOB sdist-make: /Users/ned/byterun/setup.py
py27 inst-nodeps: /Users/ned/byterun/.tox/dist/Byterun-1.0.zip
py27 runtests: commands[0] | nosetests
.........F.......................FF........................................................
======================================================================
FAIL: test_calling_subclass_methods (tests.test_basic.TestIt)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ned/byterun/tests/test_basic.py", line 311, in test_calling_subclass_methods
    """)
  File "/Users/ned/byterun/tests/vmtest.py", line 86, in assert_ok
    self.assert_same_exception(vm_exc, py_exc)
  File "/Users/ned/byterun/tests/vmtest.py", line 96, in assert_same_exception
    self.assertEqual(str(e1), str(e2))
AssertionError: "'SubThing' object has no attribute 'foo'" != 'None'

======================================================================
FAIL: test_subclass_attribute (tests.test_basic.TestIt)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ned/byterun/tests/test_basic.py", line 322, in test_subclass_attribute
    """)
  File "/Users/ned/byterun/tests/vmtest.py", line 86, in assert_ok
    self.assert_same_exception(vm_exc, py_exc)
  File "/Users/ned/byterun/tests/vmtest.py", line 96, in assert_same_exception
    self.assertEqual(str(e1), str(e2))
AssertionError: "'SubThing' object has no attribute 'foo'" != 'None'

======================================================================
FAIL: test_subclass_attributes_dynamic (tests.test_basic.TestIt)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ned/byterun/tests/test_basic.py", line 345, in test_subclass_attributes_dynamic
    """)
  File "/Users/ned/byterun/tests/vmtest.py", line 86, in assert_ok
    self.assert_same_exception(vm_exc, py_exc)
  File "/Users/ned/byterun/tests/vmtest.py", line 96, in assert_same_exception
    self.assertEqual(str(e1), str(e2))
AssertionError: "'Bar' object has no attribute 'baz'" != 'None'

----------------------------------------------------------------------
Ran 91 tests in 0.920s

FAILED (failures=3)
ERROR: InvocationError: '/Users/ned/byterun/.tox/py27/bin/nosetests'
_______________________________________________________________ summary _______________________________________________________________
ERROR:   py27: commands failed

[nedbat]

On the question of our Class object vs. builtin classes: it should work the same in Py2 and Py3, either use builtins in both, or our own objects in both, I think. Don't you? The purpose of the objects was to make their behavior explicit, though it was awfully fiddly getting the boundary between our objects and the builtins right.

[akaptur]

The three test failures are consistent with what I'm seeing. One of them - test_subclass_attributes_dynamic - will fail on master, too.

I think the class inheritance tests only ever passed because of the scoping bug, when locals in the parent frame were passed through blindly to the child, but I'm not sure about this. I haven't been able so far to write a test like test_nested_names with class inheritance.

[akaptur]

I spent more time looking at the object system (again with @rntz, thanks rntz). We found several ways in which the existing attribute lookup code isn't true to the implementation of CPython, particularly around descriptors. (See this branch for the detailed attempt, which we didn't get working, and this commit for the tests.)

The good news is that it's almost trivial to sidestep the whole object system by implementing BUILD_CLASS as a call to type. That's what this branch is now doing, and all the tests pass. (Of course, a lot of these tests are now testing code that we didn't write - CPython's underlying object system - but I think it's worth leaving them in the codebase so that future attempts to write an object system will have a record of some of the fiddly edge cases.)

[nedbat]

I'm a little confused: I think your last comment said, "We fixed a number of bugs in XYZ, and we also completely obsoleted XYZ." (where XYZ == Python implementation of objects, classes, attribute lookup, etc). I think you are also recommending getting rid of that stuff. We still need a some of it, right? For generators and closures? Will you be working up a pull request to show what you intend?

[akaptur]

No. We exposed, but did not fix, a number of bugs in the implementation of user-defined classes and objects. We then obsoleted the user-defined classes and objects implementation by removing Class and Object.

We have still implemented everything other than user-defined objects. The implementation of generators and closures is independent of these.

[akaptur]

The file diff on this PR shows is where I intended to end up (although I'd be happy to rebase).

My comment from above would now read:

This PR:

• Fixes the implementation of generators by giving each frame its own data stack, not having one data stack for the VM.
• Exposes and fixes a bug around nested scoping that allowed inner function scopes to clobber outer function scopes if they had identically-named variables (this was part of the problem with generators, which set up a local variable named ".0"). The fix is to not pass through the local variables from one frame to another. I think this solution is true to the implementation of CPython and not just an ugly hack.
• Exposes problems with class inheritance and attribute lookup in the old implementation of Class and Object. See e.g. here, here, and here, all of which fail on master.
• Removes the byterun implementation of Class and Object by using the type builtin in byte_BUILD_CLASS. This brings the Py2 byterun implementation in sync with the Py3 implementation. Because Python 3 does not emit BUILD_CLASS, our custom class code was never reached.

[akaptur]

Let me know if I can clarify anything in these proposed changes. It looks like #12 includes correct attribute handling, so quite possibly that's the better way to go.

[arthurp]

I didn't even look for other in progress work on this. Sorry about that! I don't know how much time I will have to help with any of this but I'll keep an eye on this, and you should feel free to ping me if you have any questions about my request or the changes I made.