Scope bug and generators #10

Merged
merged 9 commits into from Aug 10, 2014

Conversation

Projects
None yet
3 participants
@akaptur
Contributor

akaptur commented Jun 12, 2014

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.)

akaptur added some commits Jun 12, 2014

Clarify that we're only doing PY2 classes, and add tests
In PY3, we're falling back to the builtin implementation by relying on __build_class__.

Add tests that show that something is going wrong with attribute lookup.
Fix scoping bug.
This causes the PY2 class inheiritance tests to start failing - they were only working because of this scoping bug, I think.
Fix generator implementation
I've come to believe that the right implementation of the VM has one data stack per frame, not one data stack for the VM.  This is consistent with halting and resuming frames using generators.  The bug appeared to be around nested generators, but in fact it should arise with any generator resuming a frame if the stack has been modified in the meantime.
@nedbat

This comment has been minimized.

Show comment
Hide comment
@nedbat

nedbat Jun 12, 2014

Owner

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.

Owner

nedbat commented Jun 12, 2014

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

This comment has been minimized.

Show comment
Hide comment
@akaptur

akaptur Jun 12, 2014

Contributor

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   
Contributor

akaptur commented Jun 12, 2014

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

This comment has been minimized.

Show comment
Hide comment
@akaptur

akaptur Jun 12, 2014

Contributor

code review welcome.

Contributor

akaptur commented Jun 12, 2014

code review welcome.

@nedbat

This comment has been minimized.

Show comment
Hide comment
@nedbat

nedbat Jun 14, 2014

Owner

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
Owner

nedbat commented Jun 14, 2014

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

This comment has been minimized.

Show comment
Hide comment
@nedbat

nedbat Jun 14, 2014

Owner

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.

Owner

nedbat commented Jun 14, 2014

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

This comment has been minimized.

Show comment
Hide comment
@akaptur

akaptur Jun 14, 2014

Contributor

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.

Contributor

akaptur commented Jun 14, 2014

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

This comment has been minimized.

Show comment
Hide comment
@akaptur

akaptur Jul 2, 2014

Contributor

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.)

Contributor

akaptur commented Jul 2, 2014

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

This comment has been minimized.

Show comment
Hide comment
@nedbat

nedbat Jul 3, 2014

Owner

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?

Owner

nedbat commented Jul 3, 2014

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

This comment has been minimized.

Show comment
Hide comment
@akaptur

akaptur Jul 5, 2014

Contributor

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.

Contributor

akaptur commented Jul 5, 2014

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

This comment has been minimized.

Show comment
Hide comment
@akaptur

akaptur Jul 5, 2014

Contributor

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.
Contributor

akaptur commented Jul 5, 2014

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

This comment has been minimized.

Show comment
Hide comment
@akaptur

akaptur Aug 4, 2014

Contributor

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.

Contributor

akaptur commented Aug 4, 2014

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

This comment has been minimized.

Show comment
Hide comment
@arthurp

arthurp Aug 4, 2014

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.

arthurp commented Aug 4, 2014

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.

@nedbat nedbat merged commit 056d6bf into nedbat:master Aug 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment