Expose stack effect calculator to Python

[larryhastings]

BPO	19722
Nosy	@brettcannon, @ncoghlan, @larryhastings, @ericsnowcurrently
Files	larry.expose.stack.effect.patch.1.diff larry.expose.stack.effect.patch.2.diff larry.expose.stack.effect.patch.3.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/larryhastings'
closed_at = <Date 2013-11-23.22:50:27.780>
created_at = <Date 2013-11-22.19:42:03.240>
labels = ['type-feature', 'library']
title = 'Expose stack effect calculator to Python'
updated_at = <Date 2014-03-10.01:35:03.426>
user = 'https://github.com/larryhastings'

bugs.python.org fields:

activity = <Date 2014-03-10.01:35:03.426>
actor = 'python-dev'
assignee = 'larry'
closed = True
closed_date = <Date 2013-11-23.22:50:27.780>
closer = 'larry'
components = ['Library (Lib)']
creation = <Date 2013-11-22.19:42:03.240>
creator = 'larry'
dependencies = []
files = ['32781', '32782', '32813']
hgrepos = []
issue_num = 19722
keywords = ['patch']
message_count = 11.0
messages = ['203845', '203847', '203856', '203857', '203922', '203931', '203937', '204028', '204113', '204122', '213006']
nosy_count = 5.0
nosy_names = ['brett.cannon', 'ncoghlan', 'larry', 'python-dev', 'eric.snow']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue19722'
versions = ['Python 3.4']

[larryhastings]

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.

[larryhastings]

(Sponging around for a reviewer ;-)

[ericsnowcurrently]

FWIW, I agree with Antoine about making those PyCompile_ functions private (leading "_").

[larryhastings]

New patch, incorporating Antoine's comments. Thanks, Antoine!

[ncoghlan]

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

[ncoghlan]

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.

[larryhastings]

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.

[ncoghlan]

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.

[larryhastings]

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.

[python-dev]

New changeset 5fe72b9ed48e by Larry Hastings in branch 'default':
Issue bpo-19722: Added opcode.stack_effect(), which accurately
http://hg.python.org/cpython/rev/5fe72b9ed48e

[python-dev]

New changeset 4a801f8b7e2d by R David Murray in branch 'default':
whatsnew: dis.stack_effect (bpo-19722).
http://hg.python.org/cpython/rev/4a801f8b7e2d