updates for handling Python3.7 #29

SnoopyLane · 2018-04-22T01:01:30Z

These changes make the tests pass with Python 3.7.0b3. I made a few enhancements to tests, but the changes were mostly functional. In particular, related to changes in dis.stack_effect and compile.c.

In particular, compile.c seems to do a better job in 3.7 of computing stacksize, by leveraging new knowledge of separate stack_effect for taken vs untaken branches.

I have modeled this in bytecode by having Instr.stack_effect() take a "jump" argument as in compile.c. Note that this is supported for all versions, not just 3.7. Note also that this is a breaking change because stack_effect used to be a property.

I also rewrote most of the logic in _compute_stack_effect(). The previous code tested or several instructions, embedding taken/not-taken knowledge in this function. Since that knowledge is now exposed in Instr.stack_effect the logic here can be simpler.

I haven't yet tried running any of my own code which uses bytecode, as I'm only just starting to test under 3.7.

Note that tests do not pass yet.

BREAKING CHANGE. This turns Instr.stack_effect() into a method. It used to be a property. This commit has no functional changes other than removing the @Property and fixing all the uses. The commit is just prep work for the next commit that will be functional.

Instr.stack_effect now takes an optional parameter "jump", much like compile.c:stack_effect(). This allows the caller to request information about taken jumps vs non-taken jumps. Note that the compile.c behavior is not exposed: this is done entirely within instr.py. New _compute_stack_size() logic now leans on Instr.stack_effect instead of having lots of tests for different structions. This commit was intended to only structural changes that would pass tests on 3.6 and get things in place for 3.7 work in a later commit. But tests pass on 3.7 as well :-)

codecov-io · 2018-04-22T01:03:08Z

Codecov Report

Merging #29 into master will increase coverage by 0.55%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
+ Coverage   93.48%   94.04%   +0.55%     
==========================================
  Files          17       17              
  Lines        2564     2819     +255     
==========================================
+ Hits         2397     2651     +254     
- Misses        167      168       +1

Impacted Files	Coverage Δ
bytecode/tests/test_cfg.py	`99.68% <ø> (+0.05%)`	⬆️
bytecode/cfg.py	`96.53% <100%> (+0.06%)`	⬆️
bytecode/tests/test_instr.py	`98.55% <100%> (+0.12%)`	⬆️
bytecode/instr.py	`93.9% <100%> (+0.15%)`	⬆️
bytecode/tests/test_concrete.py	`99.59% <0%> (-0.14%)`	⬇️
bytecode/bytecode.py	`89.71% <0%> (+0.19%)`	⬆️
bytecode/tests/test_bytecode.py	`98.11% <0%> (+0.33%)`	⬆️
bytecode/concrete.py	`95.42% <0%> (+0.61%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f1e8ec...a867d51. Read the comment docs.

serhiy-storchaka · 2018-04-22T10:05:27Z

bytecode/cfg.py

        if instr.has_jump():
+            # first compute the taken-jump path
+            before_size = size
+            update_size(instr.stack_effect(1))


It would be better to use boolean value True. Or even with keyword argument: jump=True.

serhiy-storchaka · 2018-04-22T10:07:53Z

bytecode/instr.py

+}
+
+# More stack effect values that are unique to the version of Python.
+if not (sys.version_info >= (3, 7)):


Why not just sys.version_info < (3, 7)?

I originally wrote it as "sys.version_info <= (3,6)", which was the problem. Although I would prefer "<= (3,6)", it is certainly cleaner to say "< (3,7)" than "not ( >= (3,7))"

[Clang 6.0 (clang-600.0.57)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import sys >>> sys.version_info sys.version_info(major=3, minor=7, micro=0, releaselevel='beta', serial=3) >>> sys.version_info < (3,8) True >>> sys.version_info < (3,7) False >>> sys.version_info <= (3,7) False

Ahhh, I see. That was a noob mistake. Apparently "<= (3,7)" is short for "<= (3,7,0,0,0)" (or something like that) and I'm running (3,7,0,'beta',3) which is greater. So I've just convinced myself why my preference for "<= (3,6)" is just wrong. Thanks for making me think that through.

serhiy-storchaka · 2018-04-22T10:10:50Z

doc/api.rst

@@ -158,6 +153,15 @@ Instr
      .. versionchanged:: 0.3
         The *lineno* parameter has been removed.

+   .. method:: stack_effect(jump:int = -1) -> int


Needed versionchanged.

serhiy-storchaka · 2018-04-22T10:14:15Z

There are plans of exposing the jump parameter at the Python level: https://bugs.python.org/issue32455.

This isn't needed since EXTENDED_ARG has an entry in _stack_effects.

Review feedback from Serhiy Storchaka. Change stack_effect's 'jump' argument to True/False/None instead of 1/0/-1.

Review feedback from Serhiy Storchaka. Note to self: "<= (3, 6)" didn't work because running version 3.6.4 is greater than that.

serhiy-storchaka · 2018-04-22T18:05:57Z

bytecode/cfg.py


        else:  # no jump
-            update_size(instr.stack_effect())
+            update_size(instr.stack_effect(jump=None))


I think it is better to call without arguments for the default behavior.

serhiy-storchaka · 2018-04-22T18:08:27Z

doc/api.rst

@@ -153,7 +153,7 @@ Instr
      .. versionchanged:: 0.3
         The *lineno* parameter has been removed.

-   .. method:: stack_effect(jump:int = -1) -> int
+   .. method:: stack_effect(jump: bool = None) -> int


Noneas the default conflicts with the description below.

serhiy-storchaka · 2018-04-22T18:16:23Z

bytecode/instr.py

@@ -146,12 +146,10 @@ def _check_arg_int(name, arg):


 _stack_effects = {


You can simplify the definition of _stack_effects if add a simple code after it.

_stack_effects = { 'EXTENDED_ARG': (0, 0), 'JUMP_IF_TRUE_OR_POP': (-1, 0), ... } ... _stack_effects = {_opcode.opmap[name]: (a, b, max(a, b)) for name, (a, b) in _stack_effects.items()}

I've done something different that what you suggested. Since converting to True/False/None already trashed the jump-free indexing of the 3-tuple, I just converted the whole thing to a 2-tuple and using max() at runtime.

-- don't use explicit arg value when default was sufficient -- _stack_effect values are now 2-tuples -- doc fix

MatthieuDartiailh · 2018-04-28T17:37:34Z

bytecode/cfg.py

        if instr.has_jump():
+            # first compute the taken-jump path
+            before_size = size
+            update_size(instr.stack_effect(jump=True))
            target_size = size


Since we do not need anymore to update target_size to handle some special cases, I think we can completely remove it.

If we get rid off nonlocal, we could remove before_size and do

target_size, maxsize = update_size(instr.stack_effect(jump=True))```

indeed. i've made some other simplifications too

MatthieuDartiailh · 2018-04-28T17:39:58Z

bytecode/cfg.py

@@ -53,6 +53,15 @@ def _compute_stack_size(block, size, maxsize):
    if block.seen or block.startsize >= size:
        return maxsize

+    def update_size(delta):
+        nonlocal size


I must say I am not a big fan on nonlocal. It seems to me it makes the code harder to follow. Couldn't we simply return the new size and maxsize ?

MatthieuDartiailh · 2018-04-28T17:42:07Z

bytecode/instr.py

@@ -144,6 +145,33 @@ def _check_arg_int(name, arg):
                         % name)


+_stack_effects = {
+    # NOTE: the entries are all 3-tuples.  Entry[0/False] is non-taken jumps.


This is not true anymore.

Yikes! Fixed

MatthieuDartiailh · 2018-04-28T17:45:08Z

bytecode/instr.py

@@ -269,15 +297,19 @@ def lineno(self):
    def lineno(self, lineno):
        self._set(self._name, self._arg, lineno)

-    @property
-    def stack_effect(self):
+    def stack_effect(self, jump=None):


I do agree this is necessary, but I do not like to break backward compatibility and I tend to prefer verbs for method. But I cannot find a nice one (eval_stack_effect could work). WDYT ? If we change the method name we could have a deprecation warning (FutureWarning for visibility).

Well you asked what I think so... Shortly after Stuart Feldman invented "make" somebody pointed out that the tab syntax was horrible. He said that he agreed, but he didn't want to change because 12 people were already using it.

More seriously: if we're fans of properties then we could add stack_effect_with_jump and stack_effect_without_jump. But I dislike that: the work done in stack_effect is beyond what I think a property ought to do.

We could have a new method with a different name, as you suggested. And also deprecate stack_effect. And then once stack_effect is gone we put it back as a method.

I'm not a fan of "eval_stack_effect" in particular, the name sounds like I might pass it evaluates a stack effect object ("eval" is a verb, "stackeffect" the verb's object). I'd need to go read the docs to understand the difference between the two.

Let me think on this for a few hours, maybe I'll come up with something more constructive.

BTW, +1 for your comment "I tend to prefer verbs for method".

How about "get_stack_method()" ? I like "get" better than "eval", but the irony is that "get" screams "make this a property" :-).

I think breaking backward compatibility is not a problem while the version < 1.0.

Based on review feedback from Matthieu Dartiailh. _compute_stack_size simplied to not use nonlocals, and non-jump instructions share code with non-taken path of jumps. Fix doc string on _stack_effects.

SnoopyLane · 2018-04-29T21:38:01Z

I just pushed changes for your comments except about stack_size back-compat. If you think back compat is more important then I've got that change ready to go as well. However, I think have two ways to get stack_effect, one of which promotes ignorance, is not ideal. And as Serhiy mentions, this isn't 1.0.

MatthieuDartiailh · 2018-05-01T08:56:32Z

Since we do not have a good alternative name for the method, and since this part of the API is not meant to be used very often so the backward incompatibility should be a minor annoyance, I will merge as is. Thanks @SnoopyLane for your work and @serhiy-storchaka for your reviews.

serhiy-storchaka · 2018-05-01T13:25:56Z

Likely this workaround will be not needed in Python 3.8: see bpo-33334 and bpo-32455.

SnoopyLane added 5 commits April 21, 2018 09:24

tox.ini: add 'py37'

193405d

Note that tests do not pass yet.

remove logging/debug code

a142c66

test_stack_effects: verify all opcodes are handled

da5cb7b

serhiy-storchaka reviewed Apr 22, 2018

View reviewed changes

SnoopyLane added 3 commits April 22, 2018 09:12

remove stack_effect special treatment of EXTENDED_ARG

f6ff638

This isn't needed since EXTENDED_ARG has an entry in _stack_effects.

jump arg to stack_effect is now bool

7447ace

Review feedback from Serhiy Storchaka. Change stack_effect's 'jump' argument to True/False/None instead of 1/0/-1.

fix comparison to sys.version_info

4145ea6

Review feedback from Serhiy Storchaka. Note to self: "<= (3, 6)" didn't work because running version 3.6.4 is greater than that.

serhiy-storchaka reviewed Apr 22, 2018

View reviewed changes

changes from review feedback

716f189

-- don't use explicit arg value when default was sufficient -- _stack_effect values are now 2-tuples -- doc fix

serhiy-storchaka approved these changes Apr 23, 2018

View reviewed changes

MatthieuDartiailh requested changes Apr 28, 2018

View reviewed changes

Simplify _compute_stack_size, minor tweaks

a867d51

Based on review feedback from Matthieu Dartiailh. _compute_stack_size simplied to not use nonlocals, and non-jump instructions share code with non-taken path of jumps. Fix doc string on _stack_effects.

MatthieuDartiailh approved these changes May 1, 2018

View reviewed changes

MatthieuDartiailh merged commit a7cc7a5 into MatthieuDartiailh:master May 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updates for handling Python3.7 #29

updates for handling Python3.7 #29

SnoopyLane commented Apr 22, 2018

codecov-io commented Apr 22, 2018 •

edited

serhiy-storchaka Apr 22, 2018

serhiy-storchaka Apr 22, 2018

SnoopyLane Apr 22, 2018 •

edited

serhiy-storchaka Apr 22, 2018

serhiy-storchaka commented Apr 22, 2018

serhiy-storchaka Apr 22, 2018

serhiy-storchaka Apr 22, 2018

serhiy-storchaka Apr 22, 2018

SnoopyLane Apr 23, 2018

MatthieuDartiailh Apr 28, 2018

SnoopyLane Apr 29, 2018

MatthieuDartiailh Apr 28, 2018

MatthieuDartiailh Apr 28, 2018

SnoopyLane Apr 29, 2018

MatthieuDartiailh Apr 28, 2018

SnoopyLane Apr 28, 2018 •

edited

SnoopyLane Apr 28, 2018 •

edited

serhiy-storchaka Apr 29, 2018

SnoopyLane commented Apr 29, 2018

MatthieuDartiailh commented May 1, 2018

serhiy-storchaka commented May 1, 2018

		@@ -146,12 +146,10 @@ def _check_arg_int(name, arg):


		_stack_effects = {

updates for handling Python3.7 #29

updates for handling Python3.7 #29

Conversation

SnoopyLane commented Apr 22, 2018

codecov-io commented Apr 22, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SnoopyLane Apr 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SnoopyLane Apr 28, 2018 • edited

Choose a reason for hiding this comment

SnoopyLane Apr 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SnoopyLane commented Apr 29, 2018

MatthieuDartiailh commented May 1, 2018

serhiy-storchaka commented May 1, 2018

codecov-io commented Apr 22, 2018 •

edited

SnoopyLane Apr 22, 2018 •

edited

SnoopyLane Apr 28, 2018 •

edited

SnoopyLane Apr 28, 2018 •

edited