-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-46344: Fix trace bug in else of try and try-star blocks #30544
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.
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
Changes from all commits
503e79e
87c25b1
46cf04a
beb64bc
7ff6500
43aefd2
aac20fa
d95af87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -644,16 +644,25 @@ def func(): | |
4 | ||
else: | ||
6 | ||
if False: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this cover the original issue? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test did fail without the fix. But sure, I'll add a test with a non const. |
||
8 | ||
else: | ||
10 | ||
if func.__name__ == 'Fred': | ||
12 | ||
finally: | ||
8 | ||
14 | ||
|
||
self.run_and_compare(func, | ||
[(0, 'call'), | ||
(1, 'line'), | ||
(2, 'line'), | ||
(6, 'line'), | ||
(8, 'line'), | ||
(8, 'return')]) | ||
(7, 'line'), | ||
(10, 'line'), | ||
(11, 'line'), | ||
(14, 'line'), | ||
(14, 'return')]) | ||
|
||
def test_nested_loops(self): | ||
|
||
|
@@ -1222,16 +1231,25 @@ def func(): | |
4 | ||
else: | ||
6 | ||
if False: | ||
8 | ||
else: | ||
10 | ||
if func.__name__ == 'Fred': | ||
12 | ||
finally: | ||
8 | ||
14 | ||
|
||
self.run_and_compare(func, | ||
[(0, 'call'), | ||
(1, 'line'), | ||
(2, 'line'), | ||
(6, 'line'), | ||
(8, 'line'), | ||
(8, 'return')]) | ||
(7, 'line'), | ||
(10, 'line'), | ||
(11, 'line'), | ||
(14, 'line'), | ||
(14, 'return')]) | ||
|
||
def test_try_except_star_named_no_exception(self): | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 9 more opcodes generated now. Looks like that jump enabled some optimisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gvanrossum this PR fixes a regression from the except* PR and it’s all very odd, both the bug and the impact of the fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracing is Mark's black magic these days (I don't claim to understand the line number table code).
Could it be that test_dis.py wasn't properly regenerated in a while? It's always struck me as odd that there isn't a more direct way to cause this data to be regenerated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests fail if you don’t update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extracted it out of the test. I think with this PR it emits the finally body 4 times, and without it 3 times?
Old:
New:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the explicit jump prevents that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to assume that this is what happened (this jump prevented an inlining optimization), and remove it as in the first version of this PR. This will being us back to where we were before the except* PR was merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should probably merge this before the release so that we don’t get a change in behaviour between 3.11a3 and 311.a4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
This PR merely highlights the excessive duplication of finally blocks. It doesn't cause it.
Do we have an issue to investigate the cause of finally block duplication?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do now: link