Skip to content

bpo-1875, bpo-32477: Raise SyntaxError in invalid blocks that will be optimized away. #14116

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

Closed
serhiy-storchaka wants to merge 3 commits into from
Closed

bpo-1875, bpo-32477: Raise SyntaxError in invalid blocks that will be optimized away. #14116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
775fda6
bpo-1875, bpo-32477: Raise SyntaxError in invalid blocks that will be…
serhiy-storchaka Jun 15, 2019
578ccd1
Merge branch 'master' into optimize-jumps2
serhiy-storchaka Mar 22, 2020
8be08e4
Merge branch 'master' into optimize-jumps2
serhiy-storchaka Apr 3, 2020
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
10 changes: 5 additions & 5 deletions Lib/test/test_dis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,22 +140,22 @@ def bug708901():


def bug1333982(x=[]):
assert 0, ([s for s in x] +
assert x, ([s for s in x] +
1)
pass

dis_bug1333982 = """\
%3d 0 LOAD_CONST 1 (0)
%3d 0 LOAD_FAST 0 (x)
2 POP_JUMP_IF_TRUE 26
4 LOAD_ASSERTION_ERROR
6 LOAD_CONST 2 (<code object <listcomp> at 0x..., file "%s", line %d>)
8 LOAD_CONST 3 ('bug1333982.<locals>.<listcomp>')
6 LOAD_CONST 1 (<code object <listcomp> at 0x..., file "%s", line %d>)
8 LOAD_CONST 2 ('bug1333982.<locals>.<listcomp>')
10 MAKE_FUNCTION 0
12 LOAD_FAST 0 (x)
14 GET_ITER
16 CALL_FUNCTION 1

%3d 18 LOAD_CONST 4 (1)
%3d 18 LOAD_CONST 3 (1)

%3d 20 BINARY_ADD
22 CALL_FUNCTION 1
Expand Down
4 changes: 2 additions & 2 deletions Misc/NEWS.d/3.8.0b1.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,8 @@ Add NamedExpression kind support to ast_unparse.c
.. nonce: 9oxXFX
.. section: Core and Builtins

A :exc:`SyntaxError` is now raised if a code blocks that will be optimized
away (e.g. if conditions that are always false) contains syntax errors.
A :exc:`SyntaxError` is now raised if the "if" code blocks that will be optimized
away (if the conditions is always false) contains syntax errors.
Patch by Pablo Galindo.

..
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2019-06-15-19-01-05.bpo-1875.IfLQg1.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
A :exc:`SyntaxError` is now always raised if a code blocks that will be
Copy link
Member

@pablogsal pablogsal Jun 15, 2019

Choose a reason for hiding this comment

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

'if code blocks' or 'if a code block'

optimized away (e.g. if conditions that are always false) contains syntax
errors.
119 changes: 32 additions & 87 deletions Python/compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ static int compiler_slice(struct compiler *, expr_ty);

static int inplace_binop(operator_ty);
static int are_all_items_const(asdl_seq *, Py_ssize_t, Py_ssize_t);
static int expr_constant(expr_ty);

static int compiler_with(struct compiler *, stmt_ty, int);
static int compiler_async_with(struct compiler *, stmt_ty, int);
Expand Down Expand Up @@ -2555,10 +2554,22 @@ compiler_jump_if(struct compiler *c, expr_ty e, basicblock *next, int cond)
/* fallback to general implementation */
break;
}
default:
default: {
if (e->kind == Constant_kind) {
Copy link
Member

@pablogsal pablogsal Jun 16, 2019

Choose a reason for hiding this comment

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

Shouldn't this be it's on case in the previous switch?

(case Constant_kind: )

Copy link
Member Author

@serhiy-storchaka serhiy-storchaka Jun 16, 2019

Choose a reason for hiding this comment

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

Good point!

int is_true = PyObject_IsTrue(e->v.Constant.value);
if (is_true < 0) {
return 0;
}
if (is_true == cond) {
ADDOP_JABS(c, JUMP_ABSOLUTE, next);
NEXT_BLOCK(c);
}
return 1;
}
/* fallback to general implementation */
break;
}
}

/* general implementation */
VISIT(c, expr, e);
Expand Down Expand Up @@ -2646,48 +2657,24 @@ static int
compiler_if(struct compiler *c, stmt_ty s)
{
basicblock *end, *next;
int constant;
assert(s->kind == If_kind);
end = compiler_new_block(c);
next = end = compiler_new_block(c);
if (end == NULL)
return 0;

constant = expr_constant(s->v.If.test);
/* constant = 0: "if 0"
* constant = 1: "if 1", "if 2", ...
* constant = -1: rest */
if (constant == 0) {
BEGIN_DO_NOT_EMIT_BYTECODE
Copy link
Member

@pablogsal pablogsal Apr 3, 2020

Choose a reason for hiding this comment

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

You can remove this macro and the attribute in the compiler struct if is not used anymore

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);
if (next == NULL)
return 0;
}
else {
next = end;
}
if (!compiler_jump_if(c, s->v.If.test, next, 0)) {
if (asdl_seq_LEN(s->v.If.orelse)) {
next = compiler_new_block(c);
if (next == NULL)
return 0;
}
VISIT_SEQ(c, stmt, s->v.If.body);
if (asdl_seq_LEN(s->v.If.orelse)) {
ADDOP_JREL(c, JUMP_FORWARD, end);
compiler_use_next_block(c, next);
VISIT_SEQ(c, stmt, s->v.If.orelse);
}
}
if (!compiler_jump_if(c, s->v.If.test, next, 0)) {
return 0;
}
VISIT_SEQ(c, stmt, s->v.If.body);
if (asdl_seq_LEN(s->v.If.orelse)) {
ADDOP_JREL(c, JUMP_FORWARD, end);
compiler_use_next_block(c, next);
VISIT_SEQ(c, stmt, s->v.If.orelse);
}
compiler_use_next_block(c, end);
return 1;
Expand Down Expand Up @@ -2777,59 +2764,32 @@ compiler_async_for(struct compiler *c, stmt_ty s)
static int
compiler_while(struct compiler *c, stmt_ty s)
{
basicblock *loop, *orelse, *end, *anchor = NULL;
int constant = expr_constant(s->v.While.test);
basicblock *loop, *orelse = NULL, *end, *anchor;

if (constant == 0) {
BEGIN_DO_NOT_EMIT_BYTECODE
// Push a dummy block so the VISIT_SEQ knows that we are
// inside a while loop so it can correctly evaluate syntax
// errors.
if (!compiler_push_fblock(c, WHILE_LOOP, NULL, NULL, NULL)) {
return 0;
}
VISIT_SEQ(c, stmt, s->v.While.body);
// Remove the dummy block now that is not needed.
compiler_pop_fblock(c, WHILE_LOOP, NULL);
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);
end = compiler_new_block(c);
if (constant == -1) {
anchor = compiler_new_block(c);
if (anchor == NULL)
return 0;
}
if (loop == NULL || end == NULL)
anchor = compiler_new_block(c);
if (loop == NULL || end == NULL || anchor == NULL)
return 0;
if (s->v.While.orelse) {
orelse = compiler_new_block(c);
if (orelse == NULL)
return 0;
}
else
orelse = NULL;

compiler_use_next_block(c, loop);
if (!compiler_push_fblock(c, WHILE_LOOP, loop, end, NULL))
return 0;
if (constant == -1) {
if (!compiler_jump_if(c, s->v.While.test, anchor, 0))
return 0;
}
if (!compiler_jump_if(c, s->v.While.test, anchor, 0))
return 0;
VISIT_SEQ(c, stmt, s->v.While.body);
ADDOP_JABS(c, JUMP_ABSOLUTE, loop);

/* XXX should the two POP instructions be in a separate block
if there is no else clause ?
*/

if (constant == -1)
compiler_use_next_block(c, anchor);
compiler_use_next_block(c, anchor);
compiler_pop_fblock(c, WHILE_LOOP, loop);

if (orelse != NULL) /* what if orelse is just pass? */
Expand Down Expand Up @@ -4669,21 +4629,6 @@ compiler_visit_keyword(struct compiler *c, keyword_ty k)
return 1;
}

/* Test whether expression is constant. For constants, report
whether they are true or false.

Return values: 1 for true, 0 for false, -1 for non-constant.
*/

static int
expr_constant(expr_ty e)
{
if (e->kind == Constant_kind) {
return PyObject_IsTrue(e->v.Constant.value);
}
return -1;
}

static int
compiler_with_except_finish(struct compiler *c) {
basicblock *exit;
Expand Down
16 changes: 0 additions & 16 deletions Python/peephole.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,24 +300,8 @@ PyCode_Optimize(PyObject *code, PyObject* consts, PyObject *names,
cumlc = 0;

switch (opcode) {
/* Skip over LOAD_CONST trueconst
POP_JUMP_IF_FALSE xx. This improves
"while 1" performance. */
case LOAD_CONST:
cumlc = lastlc + 1;
if (nextop != POP_JUMP_IF_FALSE ||
!ISBASICBLOCK(blocks, op_start, i + 1)) {
break;
}
PyObject* cnt = PyList_GET_ITEM(consts, get_arg(codestr, i));
int is_true = PyObject_IsTrue(cnt);
if (is_true == -1) {
goto exitError;
}
if (is_true == 1) {
fill_nops(codestr, op_start, nexti + 1);
cumlc = 0;
}
break;

/* Try to fold tuples of constants.
Expand Down