Skip to content

bpo-37500: Make sure dead code does not generate bytecode but also detect syntax errors #14612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jul 15, 2019
Merged

bpo-37500: Make sure dead code does not generate bytecode but also detect syntax errors #14612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
23efd8e
bpo-37500: Make sure bytecode for always false conditional is not emi…
pablogsal Jul 5, 2019
b58ad27
Add more tests and skip more dead code
pablogsal Jul 6, 2019
b314b72
fixup! Add more tests and skip more dead code
pablogsal Jul 6, 2019
b47c977
Address Serhiy's feedback
pablogsal Jul 6, 2019
0662108
Add guards in other functions and remove them from compiler_add_o
pablogsal Jul 6, 2019
d6ffa31
Add more tests to test_compile
pablogsal Jul 6, 2019
169ce59
Add more tests to sys_settrace
pablogsal Jul 6, 2019
58322a5
Reorder tests
pablogsal Jul 6, 2019
125c278
Make the macros re-entrant
pablogsal Jul 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions Lib/test/test_compile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,40 @@ def test_stack_overflow(self):
# complex statements.
compile("if a: b\n" * 200000, "<dummy>", "exec")

# Multiple users rely on the fact that CPython does not generate
# bytecode for dead code blocks. See bpo-37500 for more context.
@support.cpython_only
def test_dead_blocks_do_not_generate_bytecode(self):
def unused_block_if():
if 0:
return 42

def unused_block_while():
while 0:
return 42

def unused_block_if_else():
if 1:
return None
else:
return 42

def unused_block_while_else():
while 1:
return None
else:
return 42

funcs = [unused_block_if, unused_block_while,
unused_block_if_else, unused_block_while_else]

for func in funcs:
opcodes = list(dis.get_instructions(func))
self.assertEqual(2, len(opcodes))
self.assertEqual('LOAD_CONST', opcodes[0].opname)
self.assertEqual(None, opcodes[0].argval)
self.assertEqual('RETURN_VALUE', opcodes[1].opname)


class TestExpressionStackSize(unittest.TestCase):
# These tests check that the computed stack size for a code object
Expand Down
41 changes: 35 additions & 6 deletions Lib/test/test_syntax.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,18 +697,47 @@ def test_break_outside_loop(self):
self._check_error("break", "outside loop")

def test_yield_outside_function(self):
self._check_error("if 0: yield", "outside function")
self._check_error("class C:\n if 0: yield", "outside function")
self._check_error("if 0: yield", "outside function")
self._check_error("if 0: yield\nelse: x=1", "outside function")
self._check_error("if 1: pass\nelse: yield", "outside function")
self._check_error("while 0: yield", "outside function")
self._check_error("while 0: yield\nelse: x=1", "outside function")
self._check_error("class C:\n if 0: yield", "outside function")
self._check_error("class C:\n if 1: pass\n else: yield",
"outside function")
self._check_error("class C:\n while 0: yield", "outside function")
self._check_error("class C:\n while 0: yield\n else: x = 1",
"outside function")

def test_return_outside_function(self):
self._check_error("if 0: return", "outside function")
self._check_error("class C:\n if 0: return", "outside function")
self._check_error("if 0: return", "outside function")
self._check_error("if 0: return\nelse: x=1", "outside function")
self._check_error("if 1: pass\nelse: return", "outside function")
self._check_error("while 0: return", "outside function")
self._check_error("class C:\n if 0: return", "outside function")
self._check_error("class C:\n while 0: return", "outside function")
self._check_error("class C:\n while 0: return\n else: x=1",
"outside function")
self._check_error("class C:\n if 0: return\n else: x= 1",
"outside function")
self._check_error("class C:\n if 1: pass\n else: return",
"outside function")

def test_break_outside_loop(self):
self._check_error("if 0: break", "outside loop")
self._check_error("if 0: break", "outside loop")
self._check_error("if 0: break\nelse: x=1", "outside loop")
self._check_error("if 1: pass\nelse: break", "outside loop")
self._check_error("class C:\n if 0: break", "outside loop")
self._check_error("class C:\n if 1: pass\n else: break",
"outside loop")

def test_continue_outside_loop(self):
self._check_error("if 0: continue", "not properly in loop")
self._check_error("if 0: continue", "not properly in loop")
self._check_error("if 0: continue\nelse: x=1", "not properly in loop")
self._check_error("if 1: pass\nelse: continue", "not properly in loop")
self._check_error("class C:\n if 0: continue", "not properly in loop")
self._check_error("class C:\n if 1: pass\n else: continue",
"not properly in loop")

def test_unexpected_indent(self):
self._check_error("foo()\n bar()\n", "unexpected indent",
Expand Down
48 changes: 41 additions & 7 deletions Lib/test/test_sys_settrace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,22 +53,52 @@ def basic():
# following that clause?


# The entire "while 0:" statement is optimized away. No code
# exists for it, so the line numbers skip directly from "del x"
# to "x = 1".
def arigo_example():
# Some constructs like "while 0:", "if 0:" or "if 1:...else:..." are optimized
# away. No code # exists for them, so the line numbers skip directly from
# "del x" to "x = 1".
def arigo_example0():
x = 1
del x
while 0:
pass
x = 1

arigo_example.events = [(0, 'call'),
arigo_example0.events = [(0, 'call'),
(1, 'line'),
(2, 'line'),
(5, 'line'),
(5, 'return')]

def arigo_example1():
x = 1
del x
if 0:
pass
x = 1

arigo_example1.events = [(0, 'call'),
(1, 'line'),
(2, 'line'),
(5, 'line'),
(5, 'return')]

def arigo_example2():
x = 1
del x
if 1:
x = 1
else:
pass
return None

arigo_example2.events = [(0, 'call'),
(1, 'line'),
(2, 'line'),
(4, 'line'),
(7, 'line'),
(7, 'return')]


# check that lines consisting of just one instruction get traced:
def one_instr_line():
x = 1
Expand Down Expand Up @@ -349,8 +379,12 @@ def fn(*args):

def test_01_basic(self):
self.run_test(basic)
def test_02_arigo(self):
self.run_test(arigo_example)
def test_02_arigo0(self):
self.run_test(arigo_example0)
def test_02_arigo1(self):
self.run_test(arigo_example1)
def test_02_arigo2(self):
self.run_test(arigo_example2)
def test_03_one_instr(self):
self.run_test(one_instr_line)
def test_04_no_pop_blocks(self):
Expand Down
69 changes: 64 additions & 5 deletions Python/compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,11 @@ struct compiler {
int c_optimize; /* optimization level */
int c_interactive; /* true if in interactive mode */
int c_nestlevel;
int c_do_not_emit_bytecode; /* The compiler won't emit any bytecode
if this value is different from zero.
This can be used to temporarily visit
nodes without emitting bytecode to
check only errors. */

PyObject *c_const_cache; /* Python dict holding all constants,
including names tuple */
Expand Down Expand Up @@ -340,6 +345,7 @@ PyAST_CompileObject(mod_ty mod, PyObject *filename, PyCompilerFlags *flags,
c.c_flags = flags;
c.c_optimize = (optimize == -1) ? config->optimization_level : optimize;
c.c_nestlevel = 0;
c.c_do_not_emit_bytecode = 0;

if (!_PyAST_Optimize(mod, arena, c.c_optimize)) {
goto finally;
Expand Down Expand Up @@ -1152,6 +1158,9 @@ compiler_addop(struct compiler *c, int opcode)
struct instr *i;
int off;
assert(!HAS_ARG(opcode));
if (c->c_do_not_emit_bytecode) {
return 1;
}
off = compiler_next_instr(c, c->u->u_curblock);
if (off < 0)
return 0;
Expand Down Expand Up @@ -1305,6 +1314,10 @@ merge_consts_recursive(struct compiler *c, PyObject *o)
static Py_ssize_t
compiler_add_const(struct compiler *c, PyObject *o)
{
if (c->c_do_not_emit_bytecode) {
return 0;
}

PyObject *key = merge_consts_recursive(c, o);
if (key == NULL) {
return -1;
Expand All @@ -1318,6 +1331,10 @@ compiler_add_const(struct compiler *c, PyObject *o)
static int
compiler_addop_load_const(struct compiler *c, PyObject *o)
{
if (c->c_do_not_emit_bytecode) {
return 1;
}

Py_ssize_t arg = compiler_add_const(c, o);
if (arg < 0)
return 0;
Expand All @@ -1328,6 +1345,10 @@ static int
compiler_addop_o(struct compiler *c, int opcode, PyObject *dict,
PyObject *o)
{
if (c->c_do_not_emit_bytecode) {
return 1;
}

Py_ssize_t arg = compiler_add_o(c, dict, o);
if (arg < 0)
return 0;
Expand All @@ -1339,6 +1360,11 @@ compiler_addop_name(struct compiler *c, int opcode, PyObject *dict,
PyObject *o)
{
Py_ssize_t arg;

if (c->c_do_not_emit_bytecode) {
return 1;
}

PyObject *mangled = _Py_Mangle(c->u->u_private, o);
if (!mangled)
return 0;
Expand All @@ -1359,6 +1385,10 @@ compiler_addop_i(struct compiler *c, int opcode, Py_ssize_t oparg)
struct instr *i;
int off;

if (c->c_do_not_emit_bytecode) {
return 1;
}

/* oparg value is unsigned, but a signed C int is usually used to store
it in the C code (like Python/ceval.c).

Expand All @@ -1385,6 +1415,10 @@ compiler_addop_j(struct compiler *c, int opcode, basicblock *b, int absolute)
struct instr *i;
int off;

if (c->c_do_not_emit_bytecode) {
return 1;
}

assert(HAS_ARG(opcode));
assert(b != NULL);
off = compiler_next_instr(c, c->u->u_curblock);
Expand Down Expand Up @@ -1519,6 +1553,17 @@ compiler_addop_j(struct compiler *c, int opcode, basicblock *b, int absolute)
} \
}

/* These macros allows to check only for errors and not emmit bytecode
* while visiting nodes.
*/

#define BEGIN_DO_NOT_EMIT_BYTECODE { \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BEGIN_DO_NOT_EMIT_BYTECODE and END_DO_NOT_EMIT_BYTECODE always surround VISIT_SEQ. I would add a new macros which expands to

c->c_do_not_emit_bytecode++;
VISIT_SEQ(...);
c->c_do_not_emit_bytecode--;

instead of BEGIN_DO_NOT_EMIT_BYTECODE and END_DO_NOT_EMIT_BYTECODE.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update the PR soon.

c->c_do_not_emit_bytecode++;

#define END_DO_NOT_EMIT_BYTECODE \
c->c_do_not_emit_bytecode--; \
}

/* Search if variable annotations are present statically in a block. */

static int
Expand Down Expand Up @@ -2546,13 +2591,23 @@ compiler_if(struct compiler *c, stmt_ty s)
return 0;

constant = expr_constant(s->v.If.test);
/* constant = 0: "if 0" Leave the optimizations to
* the pephole optimizer to check for syntax errors
* in the block.
/* constant = 0: "if 0"
* constant = 1: "if 1", "if 2", ...
* constant = -1: rest */
if (constant == 1) {
if (constant == 0) {
BEGIN_DO_NOT_EMIT_BYTECODE
VISIT_SEQ(c, stmt, s->v.If.body);
END_DO_NOT_EMIT_BYTECODE
if (s->v.If.orelse) {
VISIT_SEQ(c, stmt, s->v.If.orelse);
}
} else if (constant == 1) {
VISIT_SEQ(c, stmt, s->v.If.body);
if (s->v.If.orelse) {
BEGIN_DO_NOT_EMIT_BYTECODE
VISIT_SEQ(c, stmt, s->v.If.orelse);
END_DO_NOT_EMIT_BYTECODE
}
} else {
if (asdl_seq_LEN(s->v.If.orelse)) {
next = compiler_new_block(c);
Expand Down Expand Up @@ -2662,8 +2717,12 @@ compiler_while(struct compiler *c, stmt_ty s)
int constant = expr_constant(s->v.While.test);

if (constant == 0) {
if (s->v.While.orelse)
BEGIN_DO_NOT_EMIT_BYTECODE
VISIT_SEQ(c, stmt, s->v.While.body);
END_DO_NOT_EMIT_BYTECODE
if (s->v.While.orelse) {
VISIT_SEQ(c, stmt, s->v.While.orelse);
}
return 1;
}
loop = compiler_new_block(c);
Expand Down