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

gh-106922: Support multi-line error locations in traceback #109589

Closed
wants to merge 14 commits into from

Conversation

williamwen42
Copy link
Contributor

@williamwen42 williamwen42 commented Sep 20, 2023

Complete implementation for #106922. Implementing (1) requires substantial changes for an implementation of (2) or (3), so I just did it all in one go.

Some changes compared to the initial issue:
We do not always output all lines that correspond to an error:

  • If locations are missing, we only output the first line (missing locations assumes we only want to output one line)
  • If there are too many lines to output, in order to declutter output, we only output "significant lines" - the first and last lines, and the beginning and the end of the anchors, and 1 line above and below these lines (see tests for examples, e.g. test_many_lines_binary_op).

Order of looking things in the PR:

  1. traceback.py
  • Changes to FrameSummary fields
  • Changes to the traceback printing algorithm, primarily format_frame_summary and _extract_caret_anchors_from_line_segment - comments should sufficiently explain the algorithm
  1. traceback.c
  • Algorithm essentially mirrors traceback.py - comments should sufficiently explain the algorithm
  • I needed to write my own implementation for dedent and for tracking significant lines (struct SignificantLines i.e. a set). If there are existing implementations, then I would be happy to use those instead.
  1. test_traceback.c
  • Change some tests to check for multi-line outputs. Add some new multi-line tests.
  1. Other testing files
  • Change some tests to check for multi-line outputs.

cc @pablogsal @isidentical @ammaraskar @iritkatriel @ezyang

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Sep 20, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Sep 20, 2023

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@bedevere-app
Copy link

bedevere-app bot commented Sep 20, 2023

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@pablogsal
Copy link
Member

Thanks for the PR @williamwen42! I will review this shortly but two initial comments:

  • I think you mistakenly commited two lib2to3 files.
  • We are missing a NEWS entry, can you add one following these instructions
  • I think we may be able to review everything in one go, but in general I would prefer separate PRs because big PRs are notoriously more difficult to review

@pablogsal
Copy link
Member

I am also a bit concerned over how much C code the required implementation requires. I need to review it but my initial impression is that this may be a bit too much. What do others think? @isidentical @ammaraskar @iritkatriel

@bedevere-app
Copy link

bedevere-app bot commented Sep 21, 2023

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@williamwen42
Copy link
Contributor Author

@pablogsal any idea when you can take a look at this PR? There are some failing tests but those should just be small fixes that don't have a large impact on the PR. LMK if you would like an even more detailed description of the changes.

@pablogsal
Copy link
Member

pablogsal commented Oct 13, 2023

@williamwen42 We have refactored the whole traceback machinery so CPython uses mainly the traceback module (in Python) and falls back to the C implementation but we ripped out all the error locations in the C implementation so is easier to maintain. This means that we can remove the C parts of this PRs. You may have some conflicts on the Python part yet, but they shouldn't be too big.

I know you probably spent a lot of time doing the C part, but this change was involving a lot of C code that we woudn't be able to merge because the complexity outweighs what we are gaining from it, but given that now is only on the Python side, then we can consider it.

@pablogsal
Copy link
Member

CC: @lysnikolaou

@terryjreedy
Copy link
Member

Since 'attempt 2' is merged, close this?

@pablogsal pablogsal closed this Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants