classification
Title: Expose stack effect calculator to Python
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: larry Nosy List: brett.cannon, eric.snow, larry, ncoghlan, python-dev
Priority: normal Keywords: patch

Created on 2013-11-22 19:42 by larry, last changed 2014-03-10 01:35 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
larry.expose.stack.effect.patch.1.diff larry, 2013-11-22 19:42 review
larry.expose.stack.effect.patch.2.diff larry, 2013-11-22 20:24 review
larry.expose.stack.effect.patch.3.diff larry, 2013-11-23 22:06 review
Messages (11)
msg203845 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-11-22 19:42
Attached is a patch exposing the old opcode_stack_effect() function to Python.  The patch does the following:

* renames opcode_stack_effect() to PyCompile_OpcodeStackEffect()
* removes the "static" modifier from PyCompile_OpcodeStackEffect()
* changes PyCompile_OpcodeStackEffect()'s behavior so it returns a magic
  value on failure
* preserves existing behavior when compiling code and encountering
  an opcode/oparg pair that results in failure
* creates a new _opcode module
* exposes PyCompile_OpcodeStackEffect() as _opcode.stack_effect()
* tests _opcode module with new test__opcode.py
* imports _opcode.stack_effect() into opcode, exposing it publically
* documents the function in dis (there is no documentation for opcode,
  and dis imports and exposes everything in opcode)

Whew!  I think it's ready to go in.
msg203847 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-11-22 19:43
(Sponging around for a reviewer ;-)
msg203856 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-11-22 20:15
FWIW, I agree with Antoine about making those PyCompile_ functions private (leading "_").
msg203857 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-11-22 20:24
New patch, incorporating Antoine's comments.  Thanks, Antoine!
msg203922 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-11-22 23:42
+1 from me. A stack_effect attribute on dis.Instruction would be a nice
bonus, but doesn't need to be part of this patch.
msg203931 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-11-23 00:37
Hmm, looking at dis.py, I'm -1 on exposing this as a public opcode module API at this point, although I'm still a fan of exposing it as opcode._stack_effect to allow advanced users access (ala sys._get_frames).

I initially thought the required addition to dis.Instruction would just be:

    @property
    def stack_effect(self):
        return opcode.stack_effect(self.opcode, self.arg)

However, that doesn't necessarily work, since self.arg may be None.

That means stack_effect has to be at least:

    def stack_effect(opcode, oparg=None):
        if oparg is None:
            if opcode >= HAVE_ARGUMENT:
                raise ValueError("This opcode needs an argument")
            oparg = 0
        return _opcode.stack_effect(opcode, oparg)

However, even that's not quite accurate, since if the previous opcode was EXTENDED_ARG, you should be adding *that* arg times 65536 to oparg in order to figure out the stack effect.

It's that need to take the previous opcode into account to correctly determine the value for "oparg" that makes this a bit tricky.

Although, I guess the latter concern would only apply to integration into the dis module - for the opcode module, it just needs to be documented that the calculation of the passed in oparg value should take EXTENDED_ARG into account.
msg203937 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-11-23 00:51
stack_effect will never know about EXTENDED_ARG.  Instead, you simply pass the already-extended arg as the second argument. 

Making the second argument support a default of None will be slightly inconvenient, but I'll do it if you think it's a big improvement.

Considering how troublesome it was to recreate this information when I wrote my assembler, I definitely think this information should be exported out of the murky heart of Python.
msg204028 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-11-23 14:23
Yeah, I argued myself into realising EXTENDED_ARG just needed to be
mentioned in the function docs, but didn't go back and fix my opening
paragraph.

The fact dis uses an arg of None (rather than zero) to indicate "no
argument" means I think the extra layer of wrapping in the pure Python
module is needed prior to 3.4 rc1, but we can live without it for beta 1.
msg204113 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-11-23 22:06
Attached is revision 3 of the patch.  I'm gonna check it in pretty soon, so as to make beta (and feature freeze).  I changed the API so the oparg is optional, and it raises if it gets one it shouldn't have or didn't get one when it should.
msg204122 - (view) Author: Roundup Robot (python-dev) Date: 2013-11-23 22:50
New changeset 5fe72b9ed48e by Larry Hastings in branch 'default':
Issue #19722: Added opcode.stack_effect(), which accurately
http://hg.python.org/cpython/rev/5fe72b9ed48e
msg213006 - (view) Author: Roundup Robot (python-dev) Date: 2014-03-10 01:35
New changeset 4a801f8b7e2d by R David Murray in branch 'default':
whatsnew: dis.stack_effect (#19722).
http://hg.python.org/cpython/rev/4a801f8b7e2d
History
Date User Action Args
2014-03-10 01:35:03python-devsetmessages: + msg213006
2013-11-23 22:50:27larrysetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2013-11-23 22:50:01python-devsetnosy: + python-dev
messages: + msg204122
2013-11-23 22:06:38larrysetfiles: + larry.expose.stack.effect.patch.3.diff

messages: + msg204113
2013-11-23 14:23:55ncoghlansetmessages: + msg204028
2013-11-23 00:51:47larrysetmessages: + msg203937
2013-11-23 00:37:59ncoghlansetmessages: + msg203931
2013-11-22 23:42:13ncoghlansetmessages: + msg203922
2013-11-22 20:24:42larrysetfiles: + larry.expose.stack.effect.patch.2.diff

messages: + msg203857
2013-11-22 20:15:56eric.snowsetnosy: + eric.snow
messages: + msg203856
2013-11-22 19:43:32larrysetmessages: + msg203847
2013-11-22 19:43:21larrysetnosy: + brett.cannon
2013-11-22 19:42:03larrycreate