Shrink recursive tracebacks

[Vgr255]

BPO	26823
Nosy	@birkenfeld, @ncoghlan, @ethanfurman, @Rosuav, @berkerpeksag, @serhiy-storchaka, @zhangyangyu, @Vgr255, @AraHaan
Files	short_tracebacks.patch short_tracebacks_2.patch short_tracebacks_3.patch short_tracebacks_4.patch short_tracebacks_5.patch short_tracebacks_6.patch short_tracebacks_7.patch short_tracebacks_doc_1.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/ncoghlan'
closed_at = <Date 2016-08-16.01:00:00.997>
created_at = <Date 2016-04-22.02:36:25.104>
labels = ['interpreter-core', 'type-bug']
title = 'Shrink recursive tracebacks'
updated_at = <Date 2016-08-20.01:06:53.338>
user = 'https://github.com/Vgr255'

bugs.python.org fields:

activity = <Date 2016-08-20.01:06:53.338>
actor = 'python-dev'
assignee = 'ncoghlan'
closed = True
closed_date = <Date 2016-08-16.01:00:00.997>
closer = 'ncoghlan'
components = ['Interpreter Core']
creation = <Date 2016-04-22.02:36:25.104>
creator = 'abarry'
dependencies = []
files = ['42561', '42562', '42570', '43646', '43843', '43863', '43866', '44120']
hgrepos = []
issue_num = 26823
keywords = ['patch']
message_count = 33.0
messages = ['263948', '263949', '263950', '263951', '263952', '263953', '263954', '263955', '263959', '264022', '265750', '269909', '271085', '271089', '271140', '271152', '271156', '271162', '272642', '272703', '272705', '272706', '272707', '272708', '272709', '272710', '272711', '272724', '272734', '272780', '272812', '272814', '273177']
nosy_count = 10.0
nosy_names = ['georg.brandl', 'ncoghlan', 'ethan.furman', 'python-dev', 'Rosuav', 'berker.peksag', 'serhiy.storchaka', 'xiang.zhang', 'abarry', 'Decorater']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue26823'
versions = ['Python 3.6']

[Vgr255]

I recently suggested on Python-ideas ( https://mail.python.org/pipermail/python-ideas/2016-April/039899.html ) to shrink long tracebacks if they were all the same noise (recursive calls). Seeing as the idea had a good reception, I went ahead and implemented a small patch for this.

It doesn't keep track of call chains, and I'm not sure if we want to implement that, as the performance decrease needed to store all of that might not be worth it. But then again, if an error happened performance is probably not a concern in this case.

I've never really coded in C before, so feedback is very much welcome.

[Rosuav]

By "doesn't keep track of call chains", you mean that it can't handle mutually-recursive functions, right?

Still useful.

[Vgr255]

Yes, can't handle mutually recursive functions. I could maybe check for the last two or three functions, but that seems like unnecessary work for something that might not happen as often (I can see it being the case with e.g. __getattr__ though).

If enough people think it should keep track of all or most functions being called, I can do it.

[ethanfurman]

If you can, give it a go. Make it a new patch, though -- don't delete the existing one.

[zhangyangyu]

With the current patch, a simple test gives the traceback:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 2, in test
  File "<stdin>", line 2, in test
  File "<stdin>", line 2, in test
  [Previous message repeated 995 more times]
RecursionError: maximum recursion depth exceeded

I think the message [Previous message repeated 995 more times] is vague. Does it refer to the previous line or the whole previous messages? Hmm, at least it is vague for me, a non-native English speaker.

[Vgr255]

The message is mostly a placeholder, but "message" is singular so I figured it would be obvious. But alas, if you are confused, others might be too. Propositions for a better message are welcome :)

I'll attempt to make it track chained calls (or mutually recursive functions) tomorrow, my sleep-deprived brain is unable to find a clean way to do this right now :)

[ncoghlan]

+1 for the simple approach, and deferring the mutual recursion support - it's desirable to keep traceback printing simple in order to minimise the chance for failures during the display process.

In addition to the C implementation of traceback printing, the standard library's traceback module would also need updating.

Regarding the ambiguity of "Previous message", I'd suggest using "Previous line" instead - that way it's accurate regardless of whether people read it as "previous line of the traceback" or "previous line of quoted source code".

[Vgr255]

Attached patch also modifies Lib/traceback.py to present identical behaviour, and changes "Previous message" to "Previous line". I'll postpone the more complex implementation of that, and might just not do it as it's indeed better to avoid bugs where we're meant to handle bugs =)

Should there be unit tests for that? I'm really not sure how to write tests for that particular case...

[ncoghlan]

For testing, you can create a recursive scenario that terminates with an exception after a defined number of iterations:

>>> def f(counter):
...     if counter:
...         f(counter-1)
...     else:
...         raise RuntimeError
... 
>>> f(3)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in f
  File "<stdin>", line 3, in f
  File "<stdin>", line 3, in f
  File "<stdin>", line 5, in f
RuntimeError

It's probably also worth checking the handling of two distinct frame repetitions by having another recursion counter that terminates itself by calling the one that terminates by raising an exception

Beyond that, https://hg.python.org/cpython/file/tip/Lib/test/test_traceback.py covers both traceback formatting implementations, so a new test case in TracebackFormatTests seems appropriate.

[Vgr255]

New version with tests now, I test both the C and Python implementations.

[Vgr255]

I realize that I haven't given any update on this since my last patch. I figured that trying to detect when an arbitrary number of functions call each other in a convoluted call chain isn't a very good idea.

Not only is it way beyond my abilities, but I think it'll overcomplicate the code too much. Furthermore, the way Python/traceback.c (and, coincidentally, Lib/traceback.py) is done - print each frame as they come and then throw it away - means that remembering more than the last frame needs a refactor of both files. This is way out of the scope of this issue, which aims to solve the common cases (a typo when messing around in the REPL, __getattribute__ and __getattr__ are what get me the most).

I think that if the idea of keeping a track of multiple frames still flies, it should be a separate issue - probably after yet another issue proposing to refactor the files for that, but I'm dropping the idea.

Except for the ambigous message, I haven't seen any comments on any of my patches, which I take as a very good thing. Is there anything else preventing this from being merged?

TL;DR - Ping

[Vgr255]

Sorry for the delay, but here's a patch with updated tests after Nick's suggestions. It should be ready to merge now.

(I'm having some failures in test_traceback, but they fail even without my patch)