Skip to content
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

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

Closed
williamwen42 opened this issue Jul 20, 2023 · 15 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@williamwen42
Copy link
Contributor

williamwen42 commented Jul 20, 2023

Feature or enhancement

We propose a few improvements and fixes to PEP-657, namely:

  1. Support underlining errors that span across multiple lines instead of only showing the first line
  2. Use caret anchors for function calls as well
  3. Fix bracket/binary op heuristic in the caret anchor computation function

Pitch

We already implemented these items in PyTorch here: pytorch/pytorch#104676. We're seeing if these may be worth adding to CPython.

Rationale for 1

Multi-line expressions can negate the utility of PEP-657, for example:

really_long_expr_1 = 1
really_long_expr_2 = 2
really_long_expr_3 = 0
really_long_expr_4 = 4

y = (
    (
        really_long_expr_1 +
        really_long_expr_2
    ) /
    really_long_expr_2 /
    really_long_expr_3
)

Current traceback:

Traceback (most recent call last):
  File "/scratch/williamwen/work/pytorch/playground5.py", line 25, in <module>
    (
ZeroDivisionError: float division by zero

Better traceback:

Traceback (most recent call last):
  File "/scratch/williamwen/work/pytorch/playground5.py", line 25, in <module>
    (
    ~
        really_long_expr_1 +
        ~~~~~~~~~~~~~~~~~~~~
        really_long_expr_2
        ~~~~~~~~~~~~~~~~~~
    ) /
    ~~^
    really_long_expr_2 /
    ~~~~~~~~~~~~~~~~~~
ZeroDivisionError: float division by zero

Rationale for 2

Helpful for identifying function calls that cause errors in chained function calls. We may as well do it since we're already doing it for subscripts. For example:

def f1(x1):
    def f2(x2):
        raise RuntimeError()
    return f2
y = f1(1)(2)(3)

Current traceback:

Traceback (most recent call last):
  File "/scratch/williamwen/work/pytorch/playground5.py", line 22, in <module>
    y = f1(1)(2)(3)
        ^^^^^^^^
  File "/scratch/williamwen/work/pytorch/playground5.py", line 6, in f2
    raise RuntimeError()
RuntimeError

Better traceback:

Traceback (most recent call last):
  File "/scratch/williamwen/work/pytorch/playground5.py", line 22, in <module>
    y = f1(1)(2)(3)
        ~~~~~^^^
  File "/scratch/williamwen/work/pytorch/playground5.py", line 6, in f2
    raise RuntimeError()
RuntimeError

Rationale for 3

The binary op anchors are computed by taking the next non-space character after the left subexpression. This is incorrect in some simple cases:

    x = (3) / 0
        ~~^~~~~
ZeroDivisionError: division by zero

We should expect

    x = (3) / 0
        ~~~~^~~
ZeroDivisionError: division by zero

The fix is that we should continue advancing the anchor until we find a non-space character that is also not in )\# (for \#, we should move the anchor to the next line).

Subscript has a similar issue as well. We should continue advancing the left anchor until we find [, and the right anchor should be the end of the entire subscript expression.

cc @pablogsal @isidentical @ammaraskar @iritkatriel @ezyang

Linked PRs

@williamwen42 williamwen42 added the type-feature A feature request or enhancement label Jul 20, 2023
@pablogsal
Copy link
Member

pablogsal commented Jul 22, 2023

Hi @williamwen42! Thanks a lot for opening the issue. I think all of these make sense and would be great improvements! Do you plan to contribute these improvements or the issue is just for the proposal itself?

@williamwen42
Copy link
Contributor Author

williamwen42 commented Jul 24, 2023

@pablogsal I can contribute these improvements since I already have prototypes written. I also want to double check if we're okay with changing the default traceback behavior from one line to multiline.

@pablogsal
Copy link
Member

pablogsal commented Jul 28, 2023

@pablogsal I can contribute these improvements since I already have prototypes written.

Ok, let's do it then. Please, ping me in the PR as soon as is available! Just to make this clear: this needs changes in both the traceback module and the default traceback logic in C.

I also want to double-check if we're okay with changing the default traceback behavior from one line to multiline.

I will be supportive and have the feeling that it won't be controversial but we may need to discuss it. In any case the discussion would be much easier if we have a PR to play with. Maybe, if is not too bothersome, you can split that particular aspect in a separate PR, but is not a requirement.

Thanks a lot for the help ❤️

@pablogsal
Copy link
Member

@williamwen42 any news on this?

@pablogsal pablogsal self-assigned this Sep 5, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 8, 2023
…rentheses (pythonGH-108959)

(cherry picked from commit 6275c67)

Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 8, 2023
…rentheses (pythonGH-108959)

(cherry picked from commit 6275c67)

Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
pablogsal added a commit that referenced this issue Sep 8, 2023
…arentheses (GH-108959) (#109148)

Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
pablogsal added a commit that referenced this issue Sep 8, 2023
…arentheses (GH-108959) (#109147)

Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
@williamwen42
Copy link
Contributor Author

@pablogsal I'm still working on the C implementation. I've been distracted with work/travel in the last month but I'm planning to return to complete this in the near future.

@pablogsal
Copy link
Member

pablogsal commented Sep 8, 2023

Ok, just be aware that I have treated (3) as a bugfix in #108959 so that's not required anymore. I can take care of (2) if I get some spare cycles but I will certainly leave (1) to you :)

@pablogsal
Copy link
Member

@williamwen42 I think there is one thing we should consider. Think of the following example:

def baz(*args):
    return 1/0

def bar():
    return baz(1,
           2, 3
           , 4) + baz(1,
           2, 3
           , 4)

bar()

This currently shows:

Traceback (most recent call last):
  File "/Users/pgalindo3/github/python/main/lel.py", line 11, in <module>
    bar()
    ~~~^^
  File "/Users/pgalindo3/github/python/main/lel.py", line 5, in bar
    return baz(1,
           ~~~^^^
           2, 3
           ^^^^
           , 4)
           ^^^^
  File "/Users/pgalindo3/github/python/main/lel.py", line 2, in baz
    return 1/0
           ~^~
ZeroDivisionError: division by zero

but that is odd as the line that shows , 4) doesn't show the rest of the line , 4) + baz(1,. I think we should ensure we show the actual line to show what happens on the single line case.

@williamwen42
Copy link
Contributor Author

@pablogsal hmm, I currently get

Traceback (most recent call last):
  File "/data/users/williamwen/cpython/local_test.py", line 11, in <module>
    bar()
    ~~~^^
  File "/data/users/williamwen/cpython/local_test.py", line 5, in bar
    return baz(1,
           ~~~^^^
           2, 3
           ^^^^
           , 4) + baz(1,
           ^^^^
  File "/data/users/williamwen/cpython/local_test.py", line 2, in baz
    return 1/0
           ~^~
ZeroDivisionError: division by zero

on main

@pablogsal
Copy link
Member

Ah, apologies. I was testing this on top of #112732 and looks I screwed up something on that PR because that part is missing.

This means this is working on main but we don't cover it with a test. Would you like to add a test for it?

@williamwen42
Copy link
Contributor Author

I think test_caret_for_call_multiline in test_traceback covers it:

            'Traceback (most recent call last):\n'
            f'  File "{__file__}", line {self.callable_line}, in get_exception\n'
            '    callable()\n'
            '    ~~~~~~~~^^\n'
            f'  File "{__file__}", line {lineno_f+8}, in f_with_call\n'
            '    a = (g(1).y)(\n'
            '        ~~~~~~~~~\n'
            '        2\n'
            '        ~\n'
            '    )(3)(4)\n'
            '    ~^^^\n'
            f'  File "{__file__}", line {lineno_f+4}, in f\n'
            '    raise RuntimeError("fail")\n'

@pablogsal
Copy link
Member

I think test_caret_for_call_multiline in test_traceback covers it:

            'Traceback (most recent call last):\n'

            f'  File "{__file__}", line {self.callable_line}, in get_exception\n'

            '    callable()\n'

            '    ~~~~~~~~^^\n'

            f'  File "{__file__}", line {lineno_f+8}, in f_with_call\n'

            '    a = (g(1).y)(\n'

            '        ~~~~~~~~~\n'

            '        2\n'

            '        ~\n'

            '    )(3)(4)\n'

            '    ~^^^\n'

            f'  File "{__file__}", line {lineno_f+4}, in f\n'

            '    raise RuntimeError("fail")\n'

I don't think it does because my PR was showing the wrong output and the CI was passing. I can look closer tomorrow

@pablogsal
Copy link
Member

pablogsal commented Dec 6, 2023

Ah forget that, the problem is that the colorising code in my PR was just being used outside most tests in the test suite. This means I should add more tests on my side. 😅

Sorry for the misunderstanding

aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
@indiVar0508
Copy link

Hi
i stumbled upon this issue from 72da3a8, which seem to fix issue i am facing in below mentioned discussion in py3.13
https://discuss.python.org/t/how-can-i-pass-linecache-over-an-exec-mthod-local-global-scope/51192/2

tl;dr: for python3 < 3.13 frameSummary.line attribute becomes None when i create a method using exec with given code string due to which i couldn't see content of traceback line in my error traceback.
i tried adding linecache but when my codestring runs as task in distributed system source becomes None and FrameSummary.line is empty.

Wanted to know if you have any suggestion for solving this to point me in right direction

Thanks

@williamwen42
Copy link
Contributor Author

@pablogsal will #112097 be backported to 3.11 and 3.12?

@pablogsal
Copy link
Member

Unfortunately no since it's a new feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants