Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
50 changes: 50 additions & 0 deletions Lib/test/test_compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -1633,6 +1633,56 @@ def test_remove_redundant_nop_edge_case(self):
def f():
a if (1 if b else c) else d

def test_while_continue_try_except_exception_table(self):
# The try region's exception table must include the backward jump emitted
# for 'continue', so pending exceptions (e.g. KeyboardInterrupt) and
# tracing behave like other try-body instructions.
src = textwrap.dedent('''\
def f():
while True:
try:
continue
except KeyboardInterrupt:
break
''')
f_code = compile(src, '<string>', 'exec').co_consts[0]
jump_offsets = [
i.offset for i in dis.get_instructions(f_code)
if i.opname == 'JUMP_BACKWARD'
]
self.assertEqual(len(jump_offsets), 1)
joff = jump_offsets[0]
entries = dis._parse_exception_table(f_code)
self.assertTrue(
any(e.start <= joff < e.end for e in entries),
f'offset {joff} not covered by {entries!r}',
)

def test_try_literal_stmt_exception_table(self):
# Like try + continue, a try body that is only a literal statement must
# not leave the "invisible" result-discard outside the exception table.
src = textwrap.dedent('''\
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems a little manual rather than actually facing the problem itself. Let's say if it was:

try:
      x = 1
      42 # or continue

then it would be a problem because the line would just go back to the previous line instead of the try block, which means that this isn't the right fix for the problem, it's just a workaround for a specific fix.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall I think the code is not only fixing a specific problem, but it also seems a bit excessive the way it is trying to tackle the issue of:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I may be wrong, I think we should do some tests and such for this, like regression tests.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As feedback, I would like to suggest a possible simplification, I think there could be more room for simplification here, also maybe apart of the function names as well, a bit repetitive in my opinion, however it is your choice to keep that. I also would like to add my thought on the code logic itself, which I think does not broadly apply to all cases, so we can try further discussing some ways on improving the code so that it reaches more use cases more reliably.

def f():
try:
42
except:
pass
''')
f_code = compile(src, '<string>', 'exec').co_consts[0]
body_offsets = [
i.offset for i in dis.get_instructions(f_code)
if i.positions is not None
and i.positions.lineno == 3
and i.opname not in ('RESUME', 'NOP')
]
self.assertNotEqual(body_offsets, [])
entries = dis._parse_exception_table(f_code)
for off in body_offsets:
self.assertTrue(
any(e.start <= off < e.end for e in entries),
f'offset {off} not covered by {entries!r}',
)

def test_lineno_propagation_empty_blocks(self):
# Smoke test. See gh-138714.
def f():
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Fix exception table coverage for ``try`` blocks: pseudo ``SETUP_FINALLY`` and
``POP_BLOCK`` instructions are now labeled with the active handler, so
``continue`` inside ``try``/``except`` and other minimal try bodies handle
pending exceptions and tracing like the rest of the protected region.
9 changes: 9 additions & 0 deletions Python/flowgraph.c
Original file line number Diff line number Diff line change
Expand Up @@ -932,8 +932,17 @@ label_exception_targets(basicblock *entryblock) {
todo++;
}
handler = push_except_block(except_stack, instr);
/* Exception coverage for this instruction must match the try
* region it opens, so tracing and pending exceptions while
* executing it are handled like the following protected
* instructions. */
instr->i_except = handler;
}
else if (instr->i_opcode == POP_BLOCK) {
/* POP_BLOCK ends a protected region but must still be covered by
* that region's handler until the pop completes (e.g. continue
* inside try). */
instr->i_except = except_stack_top(except_stack);
handler = pop_except_block(except_stack);
INSTR_SET_OP0(instr, NOP);
}
Expand Down
Loading