From 937a3dbefcf549b571e45e4a32081437ad55b49f Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 8 Jul 2022 16:16:40 -0700 Subject: [PATCH 1/6] Add failing regression test --- Lib/test/test_compile.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/Lib/test/test_compile.py b/Lib/test/test_compile.py index ab1685d3e5b8a9..cc80dad01a6c6c 100644 --- a/Lib/test/test_compile.py +++ b/Lib/test/test_compile.py @@ -1181,6 +1181,26 @@ def test_complex_single_line_expression(self): self.assertOpcodeSourcePositionIs(compiled_code, 'BINARY_OP', line=1, end_line=1, column=0, end_column=27, occurrence=4) + def test_multiline_assert_rewritten_as_method_call(self): + # GH-94694 + tree = ast.parse("assert (\n42\n)") + old_node = tree.body[0] + new_node = ast.Expr( + ast.Call( + ast.Attribute( + ast.Name("spam", ast.Load()), + "eggs", + ast.Load(), + ), + [], + [], + ) + ) + ast.copy_location(new_node, old_node) + ast.fix_missing_locations(new_node) + tree.body[0] = new_node + compile(tree, "", "exec") + class TestExpressionStackSize(unittest.TestCase): # These tests check that the computed stack size for a code object From dd5a39195c2159afa4868ddbee06ed4e482b44a6 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 8 Jul 2022 16:16:54 -0700 Subject: [PATCH 2/6] Fix invalid column math --- Python/compile.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/Python/compile.c b/Python/compile.c index f36a6e85a54c20..f6afc00400be06 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -4737,7 +4737,19 @@ update_location_to_match_attr(struct compiler *c, expr_ty meth) if (meth->lineno != meth->end_lineno) { // Make start location match attribute c->u->u_loc.lineno = meth->end_lineno; - c->u->u_loc.col_offset = meth->end_col_offset - (int)PyUnicode_GetLength(meth->v.Attribute.attr)-1; + int len = (int)PyUnicode_GET_LENGTH(meth->v.Attribute.attr); + // The dot may or not be on this line. Don't try to include it in the + // column span, it's more trouble than it's worth: + if (len <= meth->end_col_offset) { + // |---- end_col_offset + // .method(...) + // |---------- new col_offset + c->u->u_loc.col_offset = meth->end_col_offset - len; + } + else { + // GH-94694: Somebody's compiling weird ASTs. Just drop the columns: + c->u->u_loc.col_offset = c->u->u_loc.end_col_offset = -1; + } } } From ffd3413fe84c2900095c7eae366d8c7da2edffe2 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 8 Jul 2022 16:35:44 -0700 Subject: [PATCH 3/6] Add additional tests for column locations --- Lib/test/test_traceback.py | 51 ++++++++++++++++++++++++++++++++++++++ Python/compile.c | 2 +- 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_traceback.py b/Lib/test/test_traceback.py index 722c265a6a8a51..72d67bf6ef1407 100644 --- a/Lib/test/test_traceback.py +++ b/Lib/test/test_traceback.py @@ -702,6 +702,57 @@ class A: pass ) self.assertEqual(result_lines, expected_error.splitlines()) + def test_multiline_method_call_a(self): + def f(): + (None + .method + )() + actual = self.get_exception(f) + expected = [ + f"Traceback (most recent call last):", + f" File \"{__file__}\", line {self.callable_line}, in get_exception", + f" callable()", + f" ^^^^^^^^^^", + f" File \"{__file__}\", line {f.__code__.co_firstlineno + 2}, in f", + f" .method", + f" ^^^^^^", + ] + self.assertEqual(actual, expected) + + def test_multiline_method_call_b(self): + def f(): + (None. + method + )() + actual = self.get_exception(f) + expected = [ + f"Traceback (most recent call last):", + f" File \"{__file__}\", line {self.callable_line}, in get_exception", + f" callable()", + f" ^^^^^^^^^^", + f" File \"{__file__}\", line {f.__code__.co_firstlineno + 2}, in f", + f" method", + f" ^^^^^^", + ] + self.assertEqual(actual, expected) + + def test_multiline_method_call_c(self): + def f(): + (None + . method + )() + actual = self.get_exception(f) + expected = [ + f"Traceback (most recent call last):", + f" File \"{__file__}\", line {self.callable_line}, in get_exception", + f" callable()", + f" ^^^^^^^^^^", + f" File \"{__file__}\", line {f.__code__.co_firstlineno + 2}, in f", + f" . method", + f" ^^^^^^", + ] + self.assertEqual(actual, expected) + @cpython_only @requires_debug_ranges() class CPythonTracebackErrorCaretTests(TracebackErrorLocationCaretTests): diff --git a/Python/compile.c b/Python/compile.c index f6afc00400be06..fd93aa1d57dd50 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -4738,7 +4738,7 @@ update_location_to_match_attr(struct compiler *c, expr_ty meth) // Make start location match attribute c->u->u_loc.lineno = meth->end_lineno; int len = (int)PyUnicode_GET_LENGTH(meth->v.Attribute.attr); - // The dot may or not be on this line. Don't try to include it in the + // We have no idea where the dot is. Don't try to include it in the // column span, it's more trouble than it's worth: if (len <= meth->end_col_offset) { // |---- end_col_offset From 53ba4445a038e3430b1f6609c167f973c943699d Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 8 Jul 2022 16:48:53 -0700 Subject: [PATCH 4/6] blurb add --- .../2022-07-08-16-44-11.gh-issue-94694.VkL2CM.rst | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-07-08-16-44-11.gh-issue-94694.VkL2CM.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-07-08-16-44-11.gh-issue-94694.VkL2CM.rst b/Misc/NEWS.d/next/Core and Builtins/2022-07-08-16-44-11.gh-issue-94694.VkL2CM.rst new file mode 100644 index 00000000000000..6434788140f420 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-07-08-16-44-11.gh-issue-94694.VkL2CM.rst @@ -0,0 +1,4 @@ +Fix an issue that could cause code with multi-line method lookups to have +misleading or incorrect column offset information. In some cases (when +compiling a hand-built AST) this could have resulted in a hard crash of the +interpreter. From 33dc4d8a911512fa310dc262b1c641f26c89cc9d Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 8 Jul 2022 16:49:01 -0700 Subject: [PATCH 5/6] Update comment --- Lib/test/test_compile.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_compile.py b/Lib/test/test_compile.py index cc80dad01a6c6c..3887a87c6e073d 100644 --- a/Lib/test/test_compile.py +++ b/Lib/test/test_compile.py @@ -1182,7 +1182,8 @@ def test_complex_single_line_expression(self): line=1, end_line=1, column=0, end_column=27, occurrence=4) def test_multiline_assert_rewritten_as_method_call(self): - # GH-94694 + # GH-94694: Copying location information from a "real" node to a + # handwritten one should always be valid! tree = ast.parse("assert (\n42\n)") old_node = tree.body[0] new_node = ast.Expr( From 13c07ff7f39a51803872a3d07eacc473bce68f78 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Sat, 9 Jul 2022 16:00:42 -0700 Subject: [PATCH 6/6] Feedback from code review --- Lib/test/test_compile.py | 4 ++-- Python/compile.c | 7 +------ 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_compile.py b/Lib/test/test_compile.py index 3887a87c6e073d..bc022a9f5cbe71 100644 --- a/Lib/test/test_compile.py +++ b/Lib/test/test_compile.py @@ -1182,8 +1182,8 @@ def test_complex_single_line_expression(self): line=1, end_line=1, column=0, end_column=27, occurrence=4) def test_multiline_assert_rewritten_as_method_call(self): - # GH-94694: Copying location information from a "real" node to a - # handwritten one should always be valid! + # GH-94694: Don't crash if pytest rewrites a multiline assert as a + # method call with the same location information: tree = ast.parse("assert (\n42\n)") old_node = tree.body[0] new_node = ast.Expr( diff --git a/Python/compile.c b/Python/compile.c index fd93aa1d57dd50..427fb2a369481f 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -4736,14 +4736,9 @@ update_location_to_match_attr(struct compiler *c, expr_ty meth) { if (meth->lineno != meth->end_lineno) { // Make start location match attribute - c->u->u_loc.lineno = meth->end_lineno; + c->u->u_loc.lineno = c->u->u_loc.end_lineno = meth->end_lineno; int len = (int)PyUnicode_GET_LENGTH(meth->v.Attribute.attr); - // We have no idea where the dot is. Don't try to include it in the - // column span, it's more trouble than it's worth: if (len <= meth->end_col_offset) { - // |---- end_col_offset - // .method(...) - // |---------- new col_offset c->u->u_loc.col_offset = meth->end_col_offset - len; } else {