Skip to content

bpo-42810: Mark jumps at end of if and try statements as artificial. #24091

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 3 commits into from
Jan 4, 2021
Merged

bpo-42810: Mark jumps at end of if and try statements as artificial. #24091

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
6a99c94
Mark jumps at end of if and try statements as artificial.
markshannon Jan 4, 2021
8a856dc
Update importlib
markshannon Jan 4, 2021
deaedf6
Add comment explaining the purpose of ADDOP_JUMP_NOLINE.
markshannon Jan 4, 2021
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
42 changes: 42 additions & 0 deletions Lib/test/test_sys_settrace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,6 +874,48 @@ def func():
(5, 'line'),
(5, 'return')])

def test_nested_ifs(self):

def func():
a = b = 1
if a == 1:
if b == 1:
x = 4
else:
y = 6
else:
z = 8

self.run_and_compare(func,
[(0, 'call'),
(1, 'line'),
(2, 'line'),
(3, 'line'),
(4, 'line'),
(4, 'return')])

def test_nested_try_if(self):

def func():
x = "hello"
try:
3/0
except ZeroDivisionError:
if x == 'raise':
raise ValueError() # line 6
f = 7

self.run_and_compare(func,
[(0, 'call'),
(1, 'line'),
(2, 'line'),
(3, 'line'),
(3, 'exception'),
(4, 'line'),
(5, 'line'),
(7, 'line'),
(7, 'return')])


class SkipLineEventsTraceTestCase(TraceTestCase):
"""Repeat the trace tests, but with per-line events skipped"""
Expand Down
31 changes: 24 additions & 7 deletions Python/compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ static int compiler_next_instr(basicblock *);
static int compiler_addop(struct compiler *, int);
static int compiler_addop_i(struct compiler *, int, Py_ssize_t);
static int compiler_addop_j(struct compiler *, int, basicblock *);
static int compiler_addop_j_noline(struct compiler *, int, basicblock *);
static int compiler_error(struct compiler *, const char *);
static int compiler_warn(struct compiler *, const char *, ...);
static int compiler_nameop(struct compiler *, identifier, expr_context_ty);
Expand Down Expand Up @@ -1458,6 +1459,12 @@ compiler_addop_j(struct compiler *c, int opcode, basicblock *b)
return add_jump_to_block(c->u->u_curblock, opcode, c->u->u_lineno, b);
}

static int
compiler_addop_j_noline(struct compiler *c, int opcode, basicblock *b)
{
return add_jump_to_block(c->u->u_curblock, opcode, -1, b);
}

/* NEXT_BLOCK() creates an implicit jump from the current block
to the new block.

Expand Down Expand Up @@ -1528,6 +1535,14 @@ compiler_addop_j(struct compiler *c, int opcode, basicblock *b)
return 0; \
}

/* Add a jump with no line number.
* Used for artificial jumps that have no corresponding
* token in the source code. */
#define ADDOP_JUMP_NOLINE(C, OP, O) { \
Copy link
Member

@pablogsal pablogsal Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Small suggestion (I haven't checked all the PR in detail): we could use something like ADDOP_ARTIFICIAL_JUMP instead of ADDOP_JUMP_NOLINE so is clear the kind of jump more than how it will behave

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I prefer NOLINE to ARTIFICIAL. NOLINE explains what it does, ARTIFICIAL explains why.
I'd prefer the code to say what it does, and leave the "why" to comments.
I'll add a comment to explain why the opcode should have no line number.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense

if (!compiler_addop_j_noline((C), (OP), (O))) \
return 0; \
}

#define ADDOP_COMPARE(C, CMP) { \
if (!compiler_addcompare((C), (cmpop_ty)(CMP))) \
return 0; \
Expand Down Expand Up @@ -2560,7 +2575,7 @@ compiler_jump_if(struct compiler *c, expr_ty e, basicblock *next, int cond)
return 0;
if (!compiler_jump_if(c, e->v.IfExp.body, next, cond))
return 0;
ADDOP_JUMP(c, JUMP_FORWARD, end);
ADDOP_JUMP_NOLINE(c, JUMP_FORWARD, end);
compiler_use_next_block(c, next2);
if (!compiler_jump_if(c, e->v.IfExp.orelse, next, cond))
return 0;
Expand Down Expand Up @@ -2593,11 +2608,11 @@ compiler_jump_if(struct compiler *c, expr_ty e, basicblock *next, int cond)
basicblock *end = compiler_new_block(c);
if (end == NULL)
return 0;
ADDOP_JUMP(c, JUMP_FORWARD, end);
ADDOP_JUMP_NOLINE(c, JUMP_FORWARD, end);
compiler_use_next_block(c, cleanup);
ADDOP(c, POP_TOP);
if (!cond) {
ADDOP_JUMP(c, JUMP_FORWARD, next);
ADDOP_JUMP_NOLINE(c, JUMP_FORWARD, next);
}
compiler_use_next_block(c, end);
return 1;
Expand Down Expand Up @@ -2632,7 +2647,7 @@ compiler_ifexp(struct compiler *c, expr_ty e)
if (!compiler_jump_if(c, e->v.IfExp.test, next, 0))
return 0;
VISIT(c, expr, e->v.IfExp.body);
ADDOP_JUMP(c, JUMP_FORWARD, end);
ADDOP_JUMP_NOLINE(c, JUMP_FORWARD, end);
compiler_use_next_block(c, next);
VISIT(c, expr, e->v.IfExp.orelse);
compiler_use_next_block(c, end);
Expand Down Expand Up @@ -2719,7 +2734,7 @@ compiler_if(struct compiler *c, stmt_ty s)
}
VISIT_SEQ(c, stmt, s->v.If.body);
if (asdl_seq_LEN(s->v.If.orelse)) {
ADDOP_JUMP(c, JUMP_FORWARD, end);
ADDOP_JUMP_NOLINE(c, JUMP_FORWARD, end);
compiler_use_next_block(c, next);
VISIT_SEQ(c, stmt, s->v.If.orelse);
}
Expand Down Expand Up @@ -2978,7 +2993,7 @@ compiler_try_finally(struct compiler *c, stmt_ty s)
ADDOP(c, POP_BLOCK);
compiler_pop_fblock(c, FINALLY_TRY, body);
VISIT_SEQ(c, stmt, s->v.Try.finalbody);
ADDOP_JUMP(c, JUMP_FORWARD, exit);
ADDOP_JUMP_NOLINE(c, JUMP_FORWARD, exit);
/* `finally` block */
compiler_use_next_block(c, end);
if (!compiler_push_fblock(c, FINALLY_END, end, NULL, NULL))
Expand Down Expand Up @@ -3127,6 +3142,8 @@ compiler_try_except(struct compiler *c, stmt_ty s)
return 0;
VISIT_SEQ(c, stmt, handler->v.ExceptHandler.body);
compiler_pop_fblock(c, HANDLER_CLEANUP, cleanup_body);
/* name = None; del name; # Mark as artificial */
c->u->u_lineno = -1;
ADDOP(c, POP_EXCEPT);
ADDOP_JUMP(c, JUMP_FORWARD, end);
}
Expand Down Expand Up @@ -3940,7 +3957,7 @@ compiler_compare(struct compiler *c, expr_ty e)
basicblock *end = compiler_new_block(c);
if (end == NULL)
return 0;
ADDOP_JUMP(c, JUMP_FORWARD, end);
ADDOP_JUMP_NOLINE(c, JUMP_FORWARD, end);
compiler_use_next_block(c, cleanup);
ADDOP(c, ROT_TWO);
ADDOP(c, POP_TOP);
Expand Down
Loading