Skip to content
This repository has been archived by the owner on May 31, 2020. It is now read-only.

Issue #181: python36 bytecode compatibility (for dicts and others) #536

Closed
wants to merge 12 commits into from
Closed

Issue #181: python36 bytecode compatibility (for dicts and others) #536

wants to merge 12 commits into from

Conversation

cjrh
Copy link

@cjrh cjrh commented May 23, 2017

This PR is still a WIP for discussion, since several details still need to be addressed:

Note that this PR also includes implementations for the __matmul__ and __imatmul__ methods for the BINARY_MATRIX_MULTIPLY and INPLACE_MATRIX_MULTIPLY special methods. These changes are not required for 3.6 compat but they were easy to add. They can be moved into a separate PR if desired.

Another subtlety: the WITH_CLEANUP bytecode name is now, at least in 3.6, separated into WITH_CLEANUP_START and WITH_CLEANUP_END and so I've changed these in dis.js. The Batavia code handling this opcode in the VM code is currently commented out, so I've gone ahead and changed WITH_CLEANUP into WITH_CLEANUP_START inside the commented-out code.

Before too much more work is done, we should discuss whether this is the right approach or not.

@cjrh
Copy link
Author

cjrh commented May 24, 2017

So it turns out I was referring to a quite out-of-date branch of cpython. I'll have to go through my updates all over again 🙃

@cjrh
Copy link
Author

cjrh commented May 24, 2017

Updated the opcodes to the correct list for CPython 3.6. The next two actions are to (a) deal with 16bit opcodes, and (b) implement the new, empty, opcode handlers.

This issue from b.p.o. explains in quite a lot of detail the rationale behind having 16-bit opcodes.

@glasnt
Copy link
Member

glasnt commented May 24, 2017

Hi @cjrh

I'm running this against 3.6.1 in https://circleci.com/gh/pybee/batavia/834 (against the topic/cjrh-issue-181-bytecode-python36 branch). Let's see how this goes!

Specifically, I had to change the circleci config.

@cjrh
Copy link
Author

cjrh commented May 24, 2017

@glasnt Excellent, thanks!

@cjrh
Copy link
Author

cjrh commented May 24, 2017

For convenience, I'm duplicating the link to the 16-bit opcode change in CPython, here.

@glasnt
Copy link
Member

glasnt commented May 24, 2017

@cjrh there are a bunch of issues that the CI tests picked up. Take a look at the results and see what happened.

@cjrh
Copy link
Author

cjrh commented May 25, 2017

Doing a spot-check on this test: ./batavia/tests/builtins/test_abs.py. I'm using a debugger to examine the section in tests.utils.TranspileTestCase#assertCodeExecution where py_out is being compare to js_out (and I'm running this PR, under CPython 3.6.1).

In the scenario I'm looking at, the input code is:

try:
    print(\\'>>> f = abs\\')
    print(\\'>>> x = None\\')
    print(\\'>>> f(x)\\')
    f = abs
    x = None
    print(f(x))
except Exception as e:
    print(type(e), \\':\\', e)
print()

So, the py_out is

>>> f = abs
>>> x = None
>>> f(x)
<class 'TypeError'> : bad operand type for abs(): 'NoneType'

while the js_out is

>>> f = abs
>>> x = None
>>> f(x)
Traceback (most recent call last):
  File "test.py", line 7, in <module>
TypeError: bad operand type for abs(): 'NoneType'

Now I'm no batavia expert, but it seems to me that the correct TypeError was raised in both, it's just that the text formatting is...different. I guess this means we'll also need to change batavia's error reporting to match what cpython 3.6.1 produces, is that correct?

I expect such differences probably account for many of the failed tests in this PR right now.

@cjrh
Copy link
Author

cjrh commented May 25, 2017

The regex for doing cleanse on the python output will not match the output from 3.6.1, apparently. That regex is at tests/utils.py:172, and is

PYTHON_EXCEPTION = re.compile('Traceback \(most recent call last\):\r?\n(  File "(?P<file>.*)", line (?P<line>\d+), in .*\r?\n    .*\r?\n)+(?P<exception>.*?): (?P<message>.*\r?\n)')  # NOQA

This will need to be changed too (for cleanse to work).

@cjrh
Copy link
Author

cjrh commented May 25, 2017

Following discussion with @freakboy3742, it seems that the js_out above is incorrect, since the exception should be getting handled and a traceback should not be printed. This must be a regression introduced by the changes in this PR, so this must be investigated.

@cjrh
Copy link
Author

cjrh commented May 25, 2017

Testing this code snippet:

try:
    f = abs
    x = None
    print(f(x))
except Exception as e:
    print(type(e), ':', e)

Produces the correct output in the Batavia testbed in the browser. Continuing to look for why the tests are different.

@cjrh
Copy link
Author

cjrh commented May 25, 2017

For interest sake, here is the side-by-side diff of the bytecode for py35 and py36 for the code in the previous comment:

image

@cjrh
Copy link
Author

cjrh commented May 27, 2017

I'm going to refactor the unpack_code() function in batavia/VirtualMachine.js so that the code for Python 3.5.x and Python 3.6.x are in separate functions. The logic is too complex to deal with using these nested if statements testing for constants.BATAVIA_MAGIC, and also, deeply nested if's are a smell.

@cjrh
Copy link
Author

cjrh commented Aug 7, 2017

I may have successfully convinced @bspink to have a look at this too!

@cjrh
Copy link
Author

cjrh commented Nov 4, 2017

@glasnt Did @abonie complete 3.6 compat as part of SOC? If so this PR should be closed.

@freakboy3742
Copy link
Member

@cjrh Yes, he did. There are still some test failures due to message format, but those exist on 3.5 as well; so I'm going close this ticket. Thanks for your work on this, even though it didn't end up being merged!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants