Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
pablogsal merged 7 commits intopython:mainfromcolnotab:traceback-specialization
Jul 12, 2021

Conversation

@isidentical
Copy link
Member

@isidenticalisidentical commentedJul 5, 2021•
edited
Loading

Examples;

 $ ./python t.pyTraceback (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
Copy link
MemberAuthor

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

Copy link
Member

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
MemberAuthor

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
Member

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 likePrecision traceback orPointing traceback because we're pointing to a specific part in the expression or trying to make it more precise?

@pablogsal
Copy link
Member

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

@pablogsal
Copy link
Member

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

@isidentical
Copy link
MemberAuthor

@ammaraskar after your_format_traceback (or even before) can you try to port this to thetraceback.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):+importast++tree=ast.parse(segment)+iflen(tree.body)==1:+statement=tree.body[0]+matchstatement:+caseast.Expr(expr):+matchexpr:+caseast.BinOp():+operator_str=segment[expr.left.end_col_offset:expr.right.col_offset]+operator_offset=len(operator_str)-len(operator_str.lstrip())+returnoperator_offset,operator_offset+1+caseast.Subscript():+returnexpr.value.end_col_offset,expr.slice.col_offset+return-1,-1

@ammaraskarammaraskarforce-pushed thetraceback-specialization branch from94801f4 to26e900eCompareJuly 8, 2021 18:11
@isidenticalisidentical marked this pull request as ready for reviewJuly 8, 2021 19:12
Copy link
Member

@ammaraskarammaraskar left a comment

Choose a reason for hiding this comment

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

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

@isidenticalisidenticalforce-pushed thetraceback-specialization branch from4a9a967 to26430d4CompareJuly 10, 2021 17:54
@isidenticalisidentical added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelJul 10, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@isidentical for commit26430d4 🤖

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelJul 10, 2021
@ammaraskar
Copy link
Member

Ping@pablogsal

@isidentical
Copy link
MemberAuthor

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

err=PyFile_WriteString(" ",f);
if (err<0) {
gotodone;
if (source_line) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this guarded bysource_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
Member

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?

@pablogsal
Copy link
Member

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

@isidentical
Copy link
MemberAuthor

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

@pablogsal
Copy link
Member

@isidentical Please, check out#27037 (comment)

@isidentical
Copy link
MemberAuthor

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

@isidentical
Copy link
MemberAuthor

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
Copy link
Member

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
Copy link
Member

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

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

isidentical reacted with thumbs up emoji

@isidentical
Copy link
MemberAuthor

In this PR or in a different one?

This PR.

@pablogsal
Copy link
Member

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
Copy link
MemberAuthor

isidentical commentedJul 12, 2021 via email

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 theAST visitors. But for both of the cases they get assigned the same valuesso that is what I meant by 'will be different' (are there any otherspecializations that you plan would change the values for these 2variables).I am currently unavailable to add a test, but can try to write somethingoff 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> .
pablogsal reacted with thumbs up emoji

@isidenticalisidenticalforce-pushed thetraceback-specialization branch from5d2036a toa67c8aaCompareJuly 12, 2021 17:28
@isidenticalisidenticalforce-pushed thetraceback-specialization branch froma67c8aa to045c491CompareJuly 12, 2021 18:03
@ammaraskar
Copy link
Member

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

@pablogsal
Copy link
Member

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

Yes, the most important step: celebrate 🎉

(also pray for the buildbots to not fail 😉 )

ammaraskar reacted with laugh emoji

@pablogsalpablogsal deleted the traceback-specialization branchJuly 12, 2021 19:33
jeanas added a commit to jeanas/pygments that referenced this pull requestDec 28, 2021
Sincepython/cpython#27037, they can includetildes in addition to the carets.
Anteru pushed a commit to pygments/pygments that referenced this pull requestDec 29, 2021
Sincepython/cpython#27037, they can includetildes in addition to the carets.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ammaraskarammaraskarammaraskar approved these changes

@iritkatrieliritkatrieliritkatriel left review comments

@pablogsalpablogsalpablogsal approved these changes

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@isidentical@pablogsal@bedevere-bot@ammaraskar@iritkatriel@the-knights-who-say-ni
[8]ページ先頭
©2009-2025 Movatter.jp