gh-106922: Support multi-line error locations in traceback (attempt 2) #112097

williamwen42 · 2023-11-15T01:34:05Z

Attempt 2 at #109589 - the C changes are no longer required, so only python files and tests are touched. Otherwise, the method to output multi-line errors remains unchanged compared to the original PR.

Issue: Support multi-line error locations in traceback and other related improvements (PEP-657, 3.11) #106922

bedevere-app · 2023-11-15T01:34:12Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

williamwen42 · 2023-11-15T01:34:47Z

cc @pablogsal @1st1 @lysnikolaou

pablogsal · 2023-11-17T03:43:39Z

@williamwen42 Seems that there are a bunch of doctests that still fail, can you take a look?

pablogsal

This looks very good. Great work. I made a first pass over the code.

Lib/traceback.py

pablogsal · 2023-11-18T16:28:02Z

Lib/traceback.py

@@ -328,13 +332,27 @@ def _original_line(self):
        self.line
        return self._line

+    @property
+    def _dedented_lines(self):


This also strips the line, not only dedents, no?

@williamwen42 ?

Lib/traceback.py

pablogsal · 2023-11-18T16:44:28Z

Lib/traceback.py

+                result = []
+
+                # only display first line, last line, and lines around anchor start/end
+                significant_lines = {0, len(all_lines) - 1}


I am not very convinced about this significant_lines treatment. I have the following concerns:

It increases the complexity of the traceback

There are plenty of cases where this hides seemingly arbitrary lines. Consider this code:

def foo(a,b,c,d,e,f,g): bar(b) def bar(b): 1/b foo( 1, 0, 3, 4, 5, 6, 7)

Currently this shows:

Traceback (most recent call last): File "/Users/pgalindo3/github/python/main/lel.py", line 7, in <module> foo( ~~~^ 1, ^^ ...<4 lines>... 6, ^^ 7) ^^ File "/Users/pgalindo3/github/python/main/lel.py", line 2, in foo bar(b) ~~~^^^ File "/Users/pgalindo3/github/python/main/lel.py", line 5, in bar 1/b ~^~ ZeroDivisionError: division by zero

but it's not clear to me why we are hiding 4 lines there. Why are the arguments provided there not important? If you look closely what's causing the exception is that b is 0, but that's not being shown in the call to foo, but is the most important part of the call.

That's true, although I agree with the general direction of the code to hide some lines, in case the error spans many lines and the traceback becomes unreadable. Should we maybe think of another heuristic to figure out which lines to show and which not to show?

We will need some kind of heuristic - I encountered a case where a traceback through a with statement results in the entire block being printed because the instruction being traced points to the entire block.

Perhaps we can show all lines only for binary ops/subscripts/function calls?

Do you have an example of the with statement out of curiosity?

Code:

class C: def __enter__(self): raise RuntimeError("fail") def __exit__(self, *excinfo): pass with C(): # comment # comment # comment # comment # comment # comment # comment # comment # comment pass

Output:

Traceback (most recent call last): File "/data/users/williamwen/cpython/local_test.py", line 8, in <module> with C(): ...<9 lines>... pass File "/data/users/williamwen/cpython/local_test.py", line 3, in __enter__ raise RuntimeError("fail") RuntimeError: fail

So if we don't skip lines, we would print out all those comments.

Hummm, I think this may be a bug in the code locations more than a problem in the implementation. But I am fine going with this version initially and refine it if people find it confusing. @lysnikolaou WDYT?

Agreed, let's go with this and we can refine it in the future if the need arises.

Indeed, looks like all the bytecode instruction for the with include all the comments:

[] Instruction(opname='RESUME', opcode=151, arg=0, argval=0, argrepr='', offset=0, starts_line=0, is_jump_target=False, positions=Positions(lineno=0, end_lineno=1, col_offset=0, end_col_offset=0)) ['class C:', ' def __enter__(self):', ' raise RuntimeError("fail")', '', ' def __exit__(self, *excinfo):'] Instruction(opname='PUSH_NULL', opcode=2, arg=None, argval=None, argrepr='', offset=2, starts_line=1, is_jump_target=False, positions=Positions(lineno=1, end_lineno=6, col_offset=0, end_col_offset=12)) ['class C:', ' def __enter__(self):', ' raise RuntimeError("fail")', '', ' def __exit__(self, *excinfo):'] Instruction(opname='LOAD_BUILD_CLASS', opcode=71, arg=None, argval=None, argrepr='', offset=4, starts_line=None, is_jump_target=False, positions=Positions(lineno=1, end_lineno=6, col_offset=0, end_col_offset=12)) ['class C:', ' def __enter__(self):', ' raise RuntimeError("fail")', '', ' def __exit__(self, *excinfo):'] Instruction(opname='LOAD_CONST', opcode=100, arg=0, argval=<code object C at 0x1049211b0, file "<string>", line 1>, argrepr='<code object C at 0x1049211b0, file "<string>", line 1>', offset=6, starts_line=None, is_jump_target=False, positions=Positions(lineno=1, end_lineno=6, col_offset=0, end_col_offset=12)) ['class C:', ' def __enter__(self):', ' raise RuntimeError("fail")', '', ' def __exit__(self, *excinfo):'] Instruction(opname='MAKE_FUNCTION', opcode=132, arg=0, argval=0, argrepr='', offset=8, starts_line=None, is_jump_target=False, positions=Positions(lineno=1, end_lineno=6, col_offset=0, end_col_offset=12)) ['class C:', ' def __enter__(self):', ' raise RuntimeError("fail")', '', ' def __exit__(self, *excinfo):'] Instruction(opname='LOAD_CONST', opcode=100, arg=1, argval='C', argrepr="'C'", offset=10, starts_line=None, is_jump_target=False, positions=Positions(lineno=1, end_lineno=6, col_offset=0, end_col_offset=12)) ['class C:', ' def __enter__(self):', ' raise RuntimeError("fail")', '', ' def __exit__(self, *excinfo):'] Instruction(opname='PRECALL', opcode=166, arg=2, argval=2, argrepr='', offset=12, starts_line=None, is_jump_target=False, positions=Positions(lineno=1, end_lineno=6, col_offset=0, end_col_offset=12)) ['class C:', ' def __enter__(self):', ' raise RuntimeError("fail")', '', ' def __exit__(self, *excinfo):'] Instruction(opname='CALL', opcode=171, arg=2, argval=2, argrepr='', offset=16, starts_line=None, is_jump_target=False, positions=Positions(lineno=1, end_lineno=6, col_offset=0, end_col_offset=12)) ['class C:', ' def __enter__(self):', ' raise RuntimeError("fail")', '', ' def __exit__(self, *excinfo):'] Instruction(opname='STORE_NAME', opcode=90, arg=0, argval='C', argrepr='C', offset=26, starts_line=None, is_jump_target=False, positions=Positions(lineno=1, end_lineno=6, col_offset=0, end_col_offset=12)) [] Instruction(opname='PUSH_NULL', opcode=2, arg=None, argval=None, argrepr='', offset=28, starts_line=8, is_jump_target=False, positions=Positions(lineno=8, end_lineno=8, col_offset=5, end_col_offset=6)) [] Instruction(opname='LOAD_NAME', opcode=101, arg=0, argval='C', argrepr='C', offset=30, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=8, col_offset=5, end_col_offset=6)) [] Instruction(opname='PRECALL', opcode=166, arg=0, argval=0, argrepr='', offset=32, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=8, col_offset=5, end_col_offset=8)) [] Instruction(opname='CALL', opcode=171, arg=0, argval=0, argrepr='', offset=36, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=8, col_offset=5, end_col_offset=8)) ['with C():', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment'] Instruction(opname='BEFORE_WITH', opcode=53, arg=None, argval=None, argrepr='', offset=46, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=18, col_offset=0, end_col_offset=8)) ['with C():', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment'] Instruction(opname='POP_TOP', opcode=1, arg=None, argval=None, argrepr='', offset=48, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=18, col_offset=0, end_col_offset=8)) [] Instruction(opname='NOP', opcode=9, arg=None, argval=None, argrepr='', offset=50, starts_line=18, is_jump_target=False, positions=Positions(lineno=18, end_lineno=18, col_offset=4, end_col_offset=8)) ['with C():', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment'] Instruction(opname='LOAD_CONST', opcode=100, arg=2, argval=None, argrepr='None', offset=52, starts_line=8, is_jump_target=False, positions=Positions(lineno=8, end_lineno=18, col_offset=0, end_col_offset=8)) ['with C():', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment'] Instruction(opname='LOAD_CONST', opcode=100, arg=2, argval=None, argrepr='None', offset=54, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=18, col_offset=0, end_col_offset=8)) ['with C():', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment'] Instruction(opname='LOAD_CONST', opcode=100, arg=2, argval=None, argrepr='None', offset=56, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=18, col_offset=0, end_col_offset=8)) ['with C():', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment'] Instruction(opname='PRECALL', opcode=166, arg=2, argval=2, argrepr='', offset=58, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=18, col_offset=0, end_col_offset=8)) ['with C():', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment'] Instruction(opname='CALL', opcode=171, arg=2, argval=2, argrepr='', offset=62, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=18, col_offset=0, end_col_offset=8)) ['with C():', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment'] Instruction(opname='POP_TOP', opcode=1, arg=None, argval=None, argrepr='', offset=72, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=18, col_offset=0, end_col_offset=8)) ['with C():', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment'] Instruction(opname='LOAD_CONST', opcode=100, arg=2, argval=None, argrepr='None', offset=74, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=18, col_offset=0, end_col_offset=8)) ['with C():', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment'] Instruction(opname='RETURN_VALUE', opcode=83, arg=None, argval=None, argrepr='', offset=76, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=18, col_offset=0, end_col_offset=8)) ['with C():', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment'] Instruction(opname='PUSH_EXC_INFO', opcode=35, arg=None, argval=None, argrepr='', offset=78, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=18, col_offset=0, end_col_offset=8)) ['with C():', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment'] Instruction(opname='WITH_EXCEPT_START', opcode=49, arg=None, argval=None, argrepr='', offset=80, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=18, col_offset=0, end_col_offset=8)) ['with C():', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment'] Instruction(opname='POP_JUMP_FORWARD_IF_TRUE', opcode=115, arg=4, argval=92, argrepr='to 92', offset=82, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=18, col_offset=0, end_col_offset=8)) ['with C():', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment', ' # comment'] Instruction(opname='RERAISE', opcode=119, arg=2, argval=2, argrepr='', offset=84, starts_line=None, is_jump_target=False, positions=Positions(lineno=8, end_lineno=18, col_offset=0, end_col_offset=8))

@iritkatriel I think this qualifies as a bug. Fo example the CALL for C() in with C() has end_lineno setted to the last comment in the block.

pablogsal · 2023-11-18T16:48:19Z

Lib/traceback.py

+        # Binary ops, subscripts, and calls are expressions, so
+        # we can wrap them with parentheses to parse them as
+        # (possibly multi-line) expressions.
+        tree = ast.parse("(\n" + segment + "\n)")


I am not sure I understand the problem here, can you provide some examples. Also, isn't this something we can solve with mode= "eval"?

The problem is that we might have to parse lines such as

x = ( a + b )

The anchor parsing code will only parse

a + b

which is not a valid expression because of the newline, so not even using eval mode will help.
We need to wrap in parentheses so that the segment is a proper expression.

( a + b )

But isn't this going to alter the line locations for single-line expressions?

Add the example to the comment so is clearer

Let's simplify to:

Suggested change

tree = ast.parse("(\n" + segment + "\n)")

tree = ast.parse(f"(\n{segment}\n)")

But isn't this going to alter the line locations for single-line expressions?

My understanding is that there's some post-processing of the tree, which handles this, which looks okay to me.

Lib/traceback.py

pablogsal · 2023-11-18T16:53:15Z

@lysnikolaou Can you take a look when you have some time? This is a considerable ammount of code and I think it needs extra eyes

lysnikolaou

This look very good in general indeed. Thanks @williamwen42! I left some comments, some require further discussion.

lysnikolaou · 2023-11-20T11:37:45Z

Lib/traceback.py

    @property
    def line(self):
        if self._line is None:
            if self.lineno is None:
                return None
-            self._line = linecache.getline(self.filename, self.lineno)
-        return self._line.strip()
+            end_lineno = self.lineno if self.end_lineno is None else self.end_lineno


This is probably not needed, since it's being handled in __init__, and self.end_lineno can be used directly, right?

Lib/traceback.py

lysnikolaou · 2023-11-20T12:15:12Z

Lib/traceback.py

+                start_offset -= dedent_characters
+                end_offset -= dedent_characters
+                start_offset = max(0, start_offset)
+                end_offset = max(0, end_offset)


Suggested change

start_offset -= dedent_characters

end_offset -= dedent_characters

start_offset = max(0, start_offset)

end_offset = max(0, end_offset)

start_offset = max(0, start_offset - dedent_characters)

end_offset = max(0, end_offset - dedent_characters)

lysnikolaou · 2023-11-20T12:17:06Z

Lib/traceback.py

+                segment = segment[start_offset:len(segment) - (len(all_lines[-1]) - end_offset)]
+
+                # attempt to parse for anchors
+                anchors: Optional[_Anchors] = None


Let's not put type hints in a completely untyped part of the codebase. Optional is also undefined here.

Lib/test/test_traceback.py

williamwen42 · 2023-11-27T18:51:52Z

@pablogsal @lysnikolaou can I get a re-review?

pablogsal · 2023-11-27T19:36:42Z

Will make another pass this week. Thanks for your patience @williamwen42!

pablogsal

LGTM

This is a fantastic job @williamwen42 thanks a lot for your patience and for following along with us. You rock 🤘

I left a nit comment but after @lysnikolaou takes a look I think we can land it.

A heads up: it's possible that we may require some changes from now to the first beta if some users or core devs find some aspects of this confusing or not very legible, but we can address those once we have more concrete aspects to discuss.

pablogsal · 2023-12-01T13:45:02Z

@lysnikolaou can you take a look when you have some time?

lysnikolaou · 2023-12-01T13:45:26Z

@lysnikolaou can you take a look when you have some time?

On it.

lysnikolaou

Let's merge this! Thanks @williamwen42 for doing this. Great work! 🚀

pablogsal · 2023-12-01T22:18:50Z

Merged! Congratulations @williamwen42! Thanks a lot for your contribution

…empt 2) (python#112097)

williamwen42 requested a review from iritkatriel as a code owner November 15, 2023 01:34

bedevere-app bot added the awaiting review label Nov 15, 2023

bedevere-app bot mentioned this pull request Nov 15, 2023

Support multi-line error locations in traceback and other related improvements (PEP-657, 3.11) #106922

Closed

pablogsal requested review from pablogsal, lysnikolaou and isidentical November 15, 2023 16:13

pablogsal self-assigned this Nov 15, 2023

williamwen42 force-pushed the multiline-traceback-attempt-2 branch from 9d9a510 to 4094c93 Compare November 17, 2023 19:32

pablogsal reviewed Nov 18, 2023

View reviewed changes

lysnikolaou reviewed Nov 20, 2023

View reviewed changes

williamwen42 and others added 4 commits November 21, 2023 14:04

Support multi-line error locations in traceback (attempt 2)

2644341

📜🤖 Added by blurb_it.

c1f8d0b

fix lint and doc test failures

3152844

address pr comments

425c874

williamwen42 force-pushed the multiline-traceback-attempt-2 branch from 4094c93 to 425c874 Compare November 21, 2023 22:04

williamwen42 requested review from pablogsal and lysnikolaou November 22, 2023 00:01

pablogsal approved these changes Dec 1, 2023

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Dec 1, 2023

lysnikolaou approved these changes Dec 1, 2023

View reviewed changes

Merge branch 'main' into multiline-traceback-attempt-2

873501f

add more comments, address some nits

99094bb

pablogsal merged commit 939fc6d into python:main Dec 1, 2023
29 checks passed

bedevere-app bot removed the awaiting merge label Dec 1, 2023

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-106922: Support multi-line error locations in traceback (att…

72da3a8

…empt 2) (python#112097)

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythongh-106922: Support multi-line error locations in traceback (att…

88e3c6c

…empt 2) (python#112097)

cretz mentioned this pull request Dec 18, 2024

Drop Python 3.8, add 3.13 temporalio/sdk-python#694

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-106922: Support multi-line error locations in traceback (attempt 2) #112097

gh-106922: Support multi-line error locations in traceback (attempt 2) #112097

williamwen42 commented Nov 15, 2023 •

edited by bedevere-app bot

Loading

bedevere-app bot commented Nov 15, 2023

williamwen42 commented Nov 15, 2023

pablogsal commented Nov 17, 2023

pablogsal left a comment

pablogsal Nov 18, 2023

pablogsal Dec 1, 2023

pablogsal Nov 18, 2023

lysnikolaou Nov 20, 2023

williamwen42 Nov 20, 2023 •

edited

Loading

pablogsal Nov 20, 2023

williamwen42 Nov 21, 2023

pablogsal Dec 1, 2023

lysnikolaou Dec 1, 2023

pablogsal Dec 1, 2023

pablogsal Nov 18, 2023

williamwen42 Nov 21, 2023 •

edited

Loading

pablogsal Dec 1, 2023

lysnikolaou Dec 1, 2023

pablogsal commented Nov 18, 2023

lysnikolaou left a comment

lysnikolaou Nov 20, 2023

lysnikolaou Nov 20, 2023

lysnikolaou Nov 20, 2023

williamwen42 commented Nov 27, 2023

pablogsal commented Nov 27, 2023

pablogsal left a comment

pablogsal commented Dec 1, 2023

lysnikolaou commented Dec 1, 2023

lysnikolaou left a comment

pablogsal commented Dec 1, 2023

	tree = ast.parse("(\n" + segment + "\n)")
	tree = ast.parse(f"(\n{segment}\n)")

gh-106922: Support multi-line error locations in traceback (attempt 2) #112097

gh-106922: Support multi-line error locations in traceback (attempt 2) #112097

Conversation

williamwen42 commented Nov 15, 2023 • edited by bedevere-app bot Loading

bedevere-app bot commented Nov 15, 2023

williamwen42 commented Nov 15, 2023

pablogsal commented Nov 17, 2023

pablogsal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamwen42 Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamwen42 Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal commented Nov 18, 2023

lysnikolaou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamwen42 commented Nov 27, 2023

pablogsal commented Nov 27, 2023

pablogsal left a comment

Choose a reason for hiding this comment

pablogsal commented Dec 1, 2023

lysnikolaou commented Dec 1, 2023

lysnikolaou left a comment

Choose a reason for hiding this comment

pablogsal commented Dec 1, 2023

williamwen42 commented Nov 15, 2023 •

edited by bedevere-app bot

Loading

williamwen42 Nov 20, 2023 •

edited

Loading

williamwen42 Nov 21, 2023 •

edited

Loading