Skip to content

bpo-43950: Specialize tracebacks for subscripts/binary ops #27037

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

Merged
pablogsal merged 7 commits into from
Jul 12, 2021
+366 −46
Merged

bpo-43950: Specialize tracebacks for subscripts/binary ops #27037

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0a825ea
bpo-43950: Specialize tracebacks for subscripts/binary ops
isidentical Jul 5, 2021
d79d365
Port new specializations to traceback.py
ammaraskar Jul 10, 2021
26430d4
pass segmen_str around
isidentical Jul 10, 2021
081738d
Refactor and simplify the traceback code
pablogsal Jul 12, 2021
045c491
make amendments
isidentical Jul 12, 2021
012ae63
Use TESTFN instead of NamedTemporaryFile
ammaraskar Jul 12, 2021
825e3b0
Remove tempfile import
ammaraskar Jul 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Doc/library/traceback.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ The output for the example would look similar to this:
['Traceback (most recent call last):\n',
' File "<doctest default[0]>", line 10, in <module>\n lumberjack()\n ^^^^^^^^^^^^\n',
' File "<doctest default[0]>", line 4, in lumberjack\n bright_side_of_death()\n ^^^^^^^^^^^^^^^^^^^^^^\n',
' File "<doctest default[0]>", line 7, in bright_side_of_death\n return tuple()[0]\n ^^^^^^^^^^\n',
' File "<doctest default[0]>", line 7, in bright_side_of_death\n return tuple()[0]\n ~~~~~~~^^^\n',
'IndexError: tuple index out of range\n']
*** extract_tb:
[<FrameSummary file <doctest...>, line 10 in <module>>,
Expand All @@ -482,7 +482,7 @@ The output for the example would look similar to this:
*** format_tb:
[' File "<doctest default[0]>", line 10, in <module>\n lumberjack()\n ^^^^^^^^^^^^\n',
' File "<doctest default[0]>", line 4, in lumberjack\n bright_side_of_death()\n ^^^^^^^^^^^^^^^^^^^^^^\n',
' File "<doctest default[0]>", line 7, in bright_side_of_death\n return tuple()[0]\n ^^^^^^^^^^\n']
' File "<doctest default[0]>", line 7, in bright_side_of_death\n return tuple()[0]\n ~~~~~~~^^^\n']
*** tb_lineno: 10


Expand Down
57 changes: 56 additions & 1 deletion Lib/test/test_traceback.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,61 @@ def f_with_multiline():
result_lines = self.get_exception(f_with_multiline)
self.assertEqual(result_lines, expected_f.splitlines())

def test_caret_for_binary_operators(self):
def f_with_binary_operator():
divisor = 20
return 10 + divisor / 0 + 30

lineno_f = f_with_binary_operator.__code__.co_firstlineno
expected_error = (
'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+2}, in f_with_binary_operator\n'
' return 10 + divisor / 0 + 30\n'
' ~~~~~~~~^~~\n'
)
result_lines = self.get_exception(f_with_binary_operator)
self.assertEqual(result_lines, expected_error.splitlines())

def test_caret_for_binary_operators_two_char(self):
def f_with_binary_operator():
divisor = 20
return 10 + divisor // 0 + 30

lineno_f = f_with_binary_operator.__code__.co_firstlineno
expected_error = (
'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+2}, in f_with_binary_operator\n'
' return 10 + divisor // 0 + 30\n'
' ~~~~~~~~^^~~\n'
)
result_lines = self.get_exception(f_with_binary_operator)
self.assertEqual(result_lines, expected_error.splitlines())

def test_caret_for_subscript(self):
def f_with_subscript():
some_dict = {'x': {'y': None}}
return some_dict['x']['y']['z']

lineno_f = f_with_subscript.__code__.co_firstlineno
expected_error = (
'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+2}, in f_with_subscript\n'
" return some_dict['x']['y']['z']\n"
' ~~~~~~~~~~~~~~~~~~~^^^^^\n'
)
result_lines = self.get_exception(f_with_subscript)
self.assertEqual(result_lines, expected_error.splitlines())



@cpython_only
@requires_debug_ranges()
Expand Down Expand Up @@ -1615,7 +1670,7 @@ def f():
self.assertEqual(
output.getvalue().split('\n')[-5:],
[' x/0',
' ^^^',
' ~^~',
' x = 12',
'ZeroDivisionError: division by zero',
''])
Expand Down
49 changes: 48 additions & 1 deletion Lib/traceback.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,9 +494,23 @@ def format(self):
colno = _byte_offset_to_character_offset(frame._original_line, frame.colno)
end_colno = _byte_offset_to_character_offset(frame._original_line, frame.end_colno)

try:
anchors = _extract_caret_anchors_from_line_segment(
frame._original_line[colno - 1:end_colno]
)
except Exception:
anchors = None

row.append(' ')
row.append(' ' * (colno - stripped_characters))
row.append('^' * (end_colno - colno))

if anchors:
row.append('~' * (anchors[0]))
row.append('^' * (anchors[1] - anchors[0]))
row.append('~' * (end_colno - colno - anchors[1]))
else:
row.append('^' * (end_colno - colno))

row.append('\n')

if frame.locals:
Expand All @@ -520,6 +534,39 @@ def _byte_offset_to_character_offset(str, offset):
return len(as_utf8[:offset + 1].decode("utf-8"))


def _extract_caret_anchors_from_line_segment(segment):
import ast

try:
tree = ast.parse(segment)
except SyntaxError:
return None

if len(tree.body) != 1:
return None

statement = tree.body[0]
match statement:
case ast.Expr(expr):
match expr:
case ast.BinOp():
operator_str = segment[expr.left.end_col_offset:expr.right.col_offset]
Comment thread
isidentical marked this conversation as resolved.
Outdated
operator_offset = len(operator_str) - len(operator_str.lstrip())

left_anchor = expr.left.end_col_offset + operator_offset
right_anchor = left_anchor + 1
if (
operator_offset + 1 < len(operator_str)
and not operator_str[operator_offset + 1].isspace()
):
right_anchor += 1
return left_anchor, right_anchor
case ast.Subscript():
return expr.value.end_col_offset, expr.slice.end_col_offset + 1

return None


class TracebackException:
"""An exception ready for rendering.

Expand Down
184 changes: 170 additions & 14 deletions Python/traceback.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
#include "pycore_interp.h" // PyInterpreterState.gc
#include "frameobject.h" // PyFrame_GetBack()
#include "pycore_frame.h" // _PyFrame_GetCode()
#include "pycore_pyarena.h" // _PyArena_Free()
#include "pycore_ast.h" // asdl_seq_*
#include "pycore_compile.h" // _PyAST_Optimize
#include "pycore_parser.h" // _PyParser_ASTFromString
#include "../Parser/pegen.h" // _PyPegen_byte_offset_to_character_offset()
#include "structmember.h" // PyMemberDef
#include "osdefs.h" // SEP
Expand Down Expand Up @@ -512,8 +516,136 @@ _Py_DisplaySourceLine(PyObject *f, PyObject *filename, int lineno, int indent, i
return err;
}

/* AST based Traceback Specialization
Copy link
Copy Markdown
Member

@iritkatriel iritkatriel Jul 6, 2021

Choose a reason for hiding this comment

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

I thought from the title of the PR that it's related to Mark's bytecode specializations. Are you settled on using this word? Seems a bit overloaded now.

Copy link
Copy Markdown
Member Author

@isidentical isidentical Jul 6, 2021

Choose a reason for hiding this comment

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

They are not relevant at all, but I can see the confusion. Any suggestions on alternative renaming?

Copy link
Copy Markdown
Member

@ammaraskar ammaraskar Jul 7, 2021

Choose a reason for hiding this comment

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

Here's how clang's documentation talks about their carets: https://clang.llvm.org/diagnostics.html

So following from there maybe something like Precision traceback or Pointing traceback because we're pointing to a specific part in the expression or trying to make it more precise?

*
* When displaying a new traceback line, for certain syntactical constructs
* (e.g a subscript, an arithmetic operation) we try to create a representation
* that separates the primary source of error from the rest.
*
* Example specialization of BinOp nodes:
* Traceback (most recent call last):
* File "/home/isidentical/cpython/cpython/t.py", line 10, in <module>
* add_values(1, 2, 'x', 3, 4)
* ^^^^^^^^^^^^^^^^^^^^^^^^^^^
* File "/home/isidentical/cpython/cpython/t.py", line 2, in add_values
* return a + b + c + d + e
* ~~~~~~^~~
* TypeError: 'NoneType' object is not subscriptable
*/

#define IS_WHITESPACE(c) (((c) == ' ') || ((c) == '\t') || ((c) == '\f'))

static int
extract_anchors_from_expr(const char *segment_str, expr_ty expr, int *left_anchor, int *right_anchor)
{
switch (expr->kind) {
case BinOp_kind: {
expr_ty left = expr->v.BinOp.left;
expr_ty right = expr->v.BinOp.right;
for (int i = left->end_col_offset + 1; i < right->col_offset; i++) {
if (IS_WHITESPACE(segment_str[i])) {
continue;
}

*left_anchor = i;
*right_anchor = i + 1;

// Check whether if this a two-character operator (e.g //)
if (i + 1 < right->col_offset && !IS_WHITESPACE(segment_str[i + 1])) {
++*right_anchor;
}
break;
}
return 1;
}
case Subscript_kind: {
*left_anchor = expr->v.Subscript.value->end_col_offset;
*right_anchor = expr->v.Subscript.slice->end_col_offset + 1;
return 1;
}
default:
return 0;
}
}

static int
extract_anchors_from_stmt(const char *segment_str, stmt_ty statement, int *left_anchor, int *right_anchor)
{
switch (statement->kind) {
case Expr_kind: {
return extract_anchors_from_expr(segment_str, statement->v.Expr.value, left_anchor, right_anchor);
}
default:
return 0;
}
}

static int
extract_anchors_from_line(PyObject *filename, PyObject *line,
Py_ssize_t start_offset, Py_ssize_t end_offset,
int *left_anchor, int *right_anchor)
{
int res = -1;
PyArena *arena = NULL;
PyObject *segment = PyUnicode_Substring(line, start_offset, end_offset);
if (!segment) {
goto done;
}

const char *segment_str = PyUnicode_AsUTF8(segment);
if (!segment) {
goto done;
}

arena = _PyArena_New();
if (!arena) {
goto done;
}

PyCompilerFlags flags = _PyCompilerFlags_INIT;

_PyASTOptimizeState state;
Comment thread
isidentical marked this conversation as resolved.
state.optimize = _Py_GetConfig()->optimization_level;
state.ff_features = 0;

mod_ty module = _PyParser_ASTFromString(segment_str, filename, Py_file_input,
&flags, arena);
if (!module) {
goto done;
}
if (!_PyAST_Optimize(module, arena, &state)) {
Comment thread
isidentical marked this conversation as resolved.
goto done;
}

assert(module->kind == Module_kind);
if (asdl_seq_LEN(module->v.Module.body) == 1) {
stmt_ty statement = asdl_seq_GET(module->v.Module.body, 0);
res = extract_anchors_from_stmt(segment_str, statement, left_anchor, right_anchor);
} else {
res = 0;
}

done:
Py_XDECREF(segment);
if (arena) {
_PyArena_Free(arena);
}
return res;
}

#define _TRACEBACK_SOURCE_LINE_INDENT 4

static inline int
ignore_source_errors(void) {
if (PyErr_Occurred()) {
if (PyErr_ExceptionMatches(PyExc_KeyboardInterrupt)) {
return -1;
}
PyErr_Clear();
}
return 0;
}

static int
tb_displayline(PyTracebackObject* tb, PyObject *f, PyObject *filename, int lineno,
PyFrameObject *frame, PyObject *name)
Expand Down Expand Up @@ -544,7 +676,7 @@ tb_displayline(PyTracebackObject* tb, PyObject *f, PyObject *filename, int linen
int start_col_byte_offset;
int end_col_byte_offset;
if (!PyCode_Addr2Location(code, code_offset, &start_line, &start_col_byte_offset,
&end_line, &end_col_byte_offset)) {
&end_line, &end_col_byte_offset)) {
goto done;
}
if (start_line != end_line) {
Expand All @@ -554,29 +686,53 @@ tb_displayline(PyTracebackObject* tb, PyObject *f, PyObject *filename, int linen
if (start_col_byte_offset < 0 || end_col_byte_offset < 0) {
goto done;
}

// Convert the utf-8 byte offset to the actual character offset so we
// print the right number of carets.
Py_ssize_t start_offset = _PyPegen_byte_offset_to_character_offset(source_line, start_col_byte_offset);
Py_ssize_t end_offset = _PyPegen_byte_offset_to_character_offset(source_line, end_col_byte_offset);
Py_ssize_t start_offset = (Py_ssize_t)start_col_byte_offset;
Py_ssize_t end_offset = (Py_ssize_t)end_col_byte_offset;

char offset = truncation;
while (++offset <= start_offset) {
err = PyFile_WriteString(" ", f);
if (err < 0) {
goto done;
if (source_line) {
Copy link
Copy Markdown
Member

@pablogsal pablogsal Jul 12, 2021

Choose a reason for hiding this comment

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

Why is this guarded by source_line? How we can correctly calculate the offsets if we cannot get the line to calculate the characters? How can tHe code path when this is false and we continue with the bytes be corrent?

Copy link
Copy Markdown
Member

@pablogsal pablogsal Jul 12, 2021

Choose a reason for hiding this comment

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

@isidentical This still applies, could you please check out what's going on here?

start_offset = _PyPegen_byte_offset_to_character_offset(source_line, start_col_byte_offset);
end_offset = _PyPegen_byte_offset_to_character_offset(source_line, end_col_byte_offset);
}

const char *primary, *secondary;
primary = secondary = "^";

int left_end_offset = Py_SAFE_DOWNCAST(end_offset, Py_ssize_t, int) - Py_SAFE_DOWNCAST(start_offset, Py_ssize_t, int);
int right_start_offset = left_end_offset;

if (source_line) {
int res = extract_anchors_from_line(filename, source_line, start_offset, end_offset,
&left_end_offset, &right_start_offset);
if (res < 0) {
err = ignore_source_errors();
if (err < 0) {
goto done;
}
} else if (res > 0) {
primary = "^";
secondary = "~";
}
}
while (++offset <= end_offset + 1) {
err = PyFile_WriteString("^", f);
if (err < 0) {
goto done;

char offset = truncation;
while (++offset <= end_offset) {
if (offset <= start_offset) {
err = PyFile_WriteString(" ", f);
} else if (offset <= left_end_offset + start_offset) {
err = PyFile_WriteString(secondary, f);
} else if (offset <= right_start_offset + start_offset) {
err = PyFile_WriteString(primary, f);
} else {
err = PyFile_WriteString(secondary, f);
}
}
err = PyFile_WriteString("\n", f);
}

else {
Comment thread
isidentical marked this conversation as resolved.
Outdated
PyErr_Clear();
err = ignore_source_errors();
}

done:
Expand Down