Skip to content

bpo-42482: remove reference to exc_traceback from TracebackException #23531

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

Jump to bottom
Merged
merged 7 commits into from
Dec 1, 2020
Merged

bpo-42482: remove reference to exc_traceback from TracebackException #23531

merged 7 commits into from
Dec 1, 2020

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Nov 27, 2020 •
edited by miss-islington
Loading

https://bugs.python.org/issue42482

Automerge-Triggered-By: GH:gvanrossum

gvanrossum
gvanrossum approved these changes Nov 27, 2020
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Yeah!

@iritkatriel
Copy link
Member Author

Yeah!

Wait, I made a mistake in the refcount test. The TracebackException object is an intermediate, I need to assign it to something.

gvanrossum
gvanrossum approved these changes Nov 28, 2020
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Sorry I didn't catch that. :-(

Let me know when you are confident enough that I can merge this.

@iritkatriel
Copy link
Member Author

I think it’s ready now.

@iritkatriel
Copy link
Member Author

BTW - the faulty test did fail on the old code. It must be that the exc_traceback field was closing a cycle with the test function’s frame or something like that.

@iritkatriel
Copy link
Member Author

Actually it might make more sense to move the line that prints the "traceback header" to StackSummary.format(), where the traceback itself is being formatted.

@gvanrossum
Copy link
Member

Actually it might make more sense to move the line that prints the "traceback header" to StackSummary.format(), where the traceback itself is being formatted.

Except that would change the behavior of TracebackException.format(), which feels like a public(ish) API.

@iritkatriel
Copy link
Member Author

iritkatriel commented Nov 29, 2020 •
edited
Loading

Actually it might make more sense to move the line that prints the "traceback header" to StackSummary.format(), where the traceback itself is being formatted.

Except that would change the behavior of TracebackException.format(), which feels like a public(ish) API.

Yes, I'm adding a with_header=False arg to format(). Alternatively, we could leave the header line where it is but under "if self.stack:" and remove the _print_traceback_header boolean.

@iritkatriel
Copy link
Member Author

iritkatriel commented Nov 29, 2020 •
edited
Loading

BTW - the faulty test did fail on the old code. It must be that the exc_traceback field was closing a cycle with the test function’s frame or something like that.

This is the cause - the TracebackException was not really intermediate because format() returns a generator:

(Pdb) gc.get_referrers(gc.get_referrers(exc_info[2])[2])
[<traceback.TracebackException object at 0x000001E0D48E3A90>]
(Pdb) gc.get_referrers(gc.get_referrers(gc.get_referrers(exc_info[2])[2])[0])
[<frame at 0x000001E0D2551C90, file 'C:\Users\User\src\cpython\lib\traceback.py', line 616, code format>]
(Pdb) gc.get_referrers(gc.get_referrers(gc.get_referrers(gc.get_referrers(exc_info[2])[2])[0])[0])
[<generator object TracebackException.format at 0x000001E0D49E63C0>]
(Pdb) gc.get_referrers(gc.get_referrers(gc.get_referrers(gc.get_referrers(gc.get_referrers(exc_info[2])[2])[0])[0])[0])
[{'self': <test.test_traceback.TestTracebackException testMethod=test_no_refs_to_exception_and_traceback_objects>, 'exc_info': (<class 'ZeroDivisionError'>, ZeroDivisionError('division by zero'), <traceback object at 0x000001E0D48DE880>), 'refcnt1': 2, 'refcnt2': 3, '_': <generator object TracebackException.format at 0x000001E0D49E63C0>, 'gc': <module 'gc' (built-in)>}]

@iritkatriel
Copy link
Member Author

I went for just removing the boolean. At this point moving the header line (as an opt-in) won't make the code clearer than this.

gvanrossum
gvanrossum reviewed Nov 30, 2020
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Okay, though I am curious why the original fixer didn't do it that way...

Lib/traceback.py Outdated
@@ -627,7 +626,7 @@ def format(self, *, chain=True):
not self.__suppress_context__):
yield from self.__context__.format(chain=chain)
yield _context_message
if self._print_traceback_header:
if self.stack:
yield 'Traceback (most recent call last):\n'
yield from self.stack.format()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe indent this line too then?

gvanrossum
gvanrossum approved these changes Nov 30, 2020
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Let's sit on this for a day...

@gvanrossum gvanrossum changed the title bpo-42482: remove reference to exception traceback from TracebackExce… bpo-42482: remove reference to exc_traceback from TracebackException Nov 30, 2020
@miss-islington miss-islington merged commit 427613f into python:master Dec 1, 2020
@miss-islington
Copy link
Contributor

Thanks @iritkatriel for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-23578 is a backport of this pull request to the 3.9 branch.

@bedevere-bot
Copy link

GH-23579 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 1, 2020
@iritkatriel @miss-islington
bpo-42482: remove reference to exc_traceback from TracebackException (p…
ef9426c 
…ythonGH-23531)

(cherry picked from commit 427613f)

Co-authored-by: Irit Katriel <iritkatriel@yahoo.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 1, 2020
@iritkatriel @miss-islington
bpo-42482: remove reference to exc_traceback from TracebackException (p…
279bc5e 
…ythonGH-23531)

(cherry picked from commit 427613f)

Co-authored-by: Irit Katriel <iritkatriel@yahoo.com>
miss-islington added a commit that referenced this pull request Dec 1, 2020
@miss-islington @iritkatriel
bpo-42482: remove reference to exc_traceback from TracebackException (G…
1cc5c94 
…H-23531)

(cherry picked from commit 427613f)

Co-authored-by: Irit Katriel <iritkatriel@yahoo.com>
miss-islington added a commit that referenced this pull request Dec 4, 2020
@miss-islington @iritkatriel
[3.9] bpo-42482: remove reference to exc_traceback from TracebackExce…
40b92f1 
…ption (GH-23531) (GH-23578)

(cherry picked from commit 427613f)


Co-authored-by: Irit Katriel <iritkatriel@yahoo.com>
@iritkatriel iritkatriel added the type-bug An unexpected behavior, bug, or error label Dec 23, 2020
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@iritkatriel @adorilson
bpo-42482: remove reference to exc_traceback from TracebackException (p…
027e6d8 
…ythonGH-23531)
@iritkatriel iritkatriel deleted the bpo-42482 branch March 16, 2021 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@iritkatriel @gvanrossum @miss-islington @bedevere-bot @the-knights-who-say-ni