bpo-43950: Specialize tracebacks for subscripts/binary ops

[isidentical]

Examples;

 $ ./python t.py
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: unsupported operand type(s) for +: 'int' and 'str'

Traceback (most recent call last):
  File "/home/isidentical/cpython/cpython/t2.py", line 10, in <module>
    response['data']['segment1']['segment2']['segment3']
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^
TypeError: 'NoneType' object is not subscriptable

https://bugs.python.org/issue43950

[isidentical]

For the traceback.py changes (and tests) I guess we should wait for @ammaraskar's changes on the traceback.py module.

[iritkatriel]

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.

[isidentical]

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

[ammaraskar]

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?

[pablogsal]

@isidentical this may need rebasing now that the other PR is merged

[pablogsal]

Let's focus on land this one next @ammaraskar @isidentical

[isidentical]

@ammaraskar after your _format_traceback (or even before) can you try to port this to the traceback.py? I think it is going to look something like this but haven't integrated printing logic / tests yet.

+def _extract_anchors_from_segment(segment):
+    import ast
+
+    tree = ast.parse(segment)
+    if len(tree.body) == 1:
+        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]
+                        operator_offset = len(operator_str) - len(operator_str.lstrip())
+                        return operator_offset, operator_offset + 1
+                    case ast.Subscript():
+                        return expr.value.end_col_offset, expr.slice.col_offset
+    return -1, -1

[ammaraskar]

Just some basic stylistic suggestions mostly, looks good to me.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @isidentical for commit 26430d4 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[ammaraskar]

Ping @pablogsal

[isidentical]

Apparently ASAN is failing, though not sure if it is relevant.

[pablogsal]

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?

[pablogsal]

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

[pablogsal]

@isidentical I have pushed 081738d to greatly simplify the code structure. I think it reads better now. Please, check it out

[isidentical]

Thanks @pablogsal, it LGTM. Feel free to merge!

[pablogsal]

@isidentical Please, check out #27037 (comment)

[isidentical]

Ah, I think that is not relevant. I just checked again, and it seems like source_line should be always non-NULL, though since we are dealing with value propagation over functions it might be still wiser to just replace the if (source_line) { ... } with assert(source_line); ....

[isidentical]

Also I am not really sure if we get any cases where primary/secondary will be different, but if so I think it would be nicer to make amendments to the traceback.py logic to do the same.

[pablogsal]

Also I am not really sure if we get any cases where primary/secondary will be different, but if so I think it would be nicer to make amendments to the traceback.py logic to do the same.

In this PR or in a different one?

[pablogsal]

Ah, I think that is not relevant. I just checked again, and it seems like source_line should be always non-NULL, though since we are dealing with value propagation over functions it might be still wiser to just replace the if (source_line) { ... } with assert(source_line); ....

We should also add some tests where we fail to parse the source. Is this covered currently?

[isidentical]

In this PR or in a different one?

This PR.

[pablogsal]

Also I am not really sure if we get any cases where primary/secondary will be different, but if so I think it would be nicer to make amendments to the traceback.py logic to do the same.

Also, I am not very sure what you refer to with "will be different". Can you elaborate?

We should also add some tests where we fail to parse the source. Is this covered currently?

I don't see this covered. Could you add a test for this?

[isidentical]

Also, I am not very sure what you refer to with "will be different". Can

you elaborate? With the latest commit you pass primary/secondary (^/~) characters to the AST visitors. But for both of the cases they get assigned the same values so that is what I meant by 'will be different' (are there any other specializations that you plan would change the values for these 2 variables). I am currently unavailable to add a test, but can try to write something off tonight.

…

On Mon, Jul 12, 2021, 3:38 PM Pablo Galindo Salgado < ***@***.***> wrote: Also I am not really sure if we get any cases where primary/secondary will be different, but if so I think it would be nicer to make amendments to the traceback.py logic to do the same. Also, I am not very sure what you refer to with "will be different". Can you elaborate? We should also add some tests where we fail to parse the source. Is this covered currently? I don't see this covered. Could you add a test for this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27037 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALJKHQJO6MPLFWJU5R75MZ3TXLO37ANCNFSM473CEDAA> .

[ammaraskar]

We now have a test for when parsing fails and the source_line conditional is gone. Anything else to do here?

[pablogsal]

We now have a test for when parsing fails and the source_line conditional is gone. Anything else to do here?

Yes, the most important step: celebrate 🎉

(also pray for the buildbots to not fail 😉 )