8000 bpo-44446: support lineno being None in traceback.FrameSummary by FFY00 · Pull Request #26781 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-44446: support lineno 8000 being None in traceback.FrameSummary #26781

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

Jump to bottom
Merged
merged 1 commit into from
Jul 8, 2021
Merged

bpo-44446: support lineno being None in traceback.FrameSummary #26781

merged 1 commit into from
Jul 8, 2021

Conversation

FFY00
Copy link
Member
@FFY00 FFY00 commented Jun 17, 2021 •
edited
Loading

As of 088a15c, lineno is None instead of -1 if there is no line number.

Signed-off-by: Filipe Laíns lains@riseup.net

https://bugs.python.org/issue44446

@FFY00 FFY00 requested a review from iritkatriel June 17, 2021 21:41
@FFY00 FFY00 force-pushed the bpo-44446 branch 2 times, most recently from 70c0a93 to b8963bf Compare June 17, 2021 21:45
@graingert
Copy link
Contributor

Might be worth including a regression test formatting a Traceback containing [v async for v in vs()]?

@FFY00
Copy link
Member Author
FFY00 commented Jun 17, 2021

Maybe, though I do not know where such test should belong. I'd hope that is already being tested 😁

iritkatriel
iritkatriel reviewed Jun 17, 2021
View reviewed changes
Copy link
Member
@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

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

Might be worth including a regression test formatting a Traceback containing [v async for v in vs()]?

Yes, might be nice to have also a small test for StackSummary.format() with a None in one of the FrameSummary's.

@iritkatriel
Copy link
Member

Maybe, though I do not know where such test should belong. I'd hope that is already being tested 😁

It would belong here, in TracebackException's tests. If it was already tested we wouldn't be here!

@graingert
Copy link
Contributor

Why does the [v async for v in vs()] frame not have a lineno? Is that also a bug?

@iritkatriel
Copy link
Member

Why does the [v async for v in vs()] frame not have a lineno? Is that also a bug?

No idea. Probably worth asking in a separate bpo.

@FFY00
Copy link
Member Author
FFY00 commented Jun 18, 2021

See my comment on bpo. That test will have to wait for #26782. Do you want to wait or should it be done in a separate PR?

@iritkatriel
Copy link
Member

See my comment on bpo. That test will have to wait for #26782. Do you want to wait or should it be done in a separate PR?

Why does the test depend on that? Traceback shouldn’t blow up with -1 or None.

@graingert
Copy link
Contributor

See my comment on bpo. That test will have to wait for #26782. Do you want to wait or should it be done in a separate PR?

Why does the test depend on that? Traceback shouldn’t blow up with -1 or None.

Right but I'm assuming @FFY00 wants to assert on the correct output rather than just it not raising

@iritkatriel
Copy link
Member

Right but I'm assuming @FFY00 wants to assert on the correct output rather than just it not raising

I assumed it was None in both cases, because linecache won't find anything for line -1 (but I didn't check).

@FFY00
Copy link
Member Author
FFY00 commented Jun 20, 2021

I mean, I can just add a check making sure it doesn't raise. Maybe it makes sense given that the other PR might take a while, and this PR might be enough to remove the release blocker status. @pablogsal what do you think?

@pablogsal
Copy link
Member

I mean, I can just add a check making sure it doesn't raise. Maybe it makes sense given that the other PR might take a while, and this PR might be enough to remove the release blocker status. @pablogsal what do you think?

The release blocker is for both, including the regression in a previously valid line number. Also, my gut feeling is that there is also a ownership issue with the locals after some of the latest changes in the frames and the error reporting, but I need time to investigate. Hopefully @markshannon can take a look before.

@FFY00
Copy link
Member Author
FFY00 commented Jun 20, 2021 •
edited
Loading

I have some time, if this is something you think I could help with. I just need some pointers 😅

@pablogsal
Copy link
Member

I have some time, if this is something you think I could help with. I just need some pointers 😅

Unfortunately I have no more pointers than the regression information and my gut feeling on some underlying problem. I would try to create a situation where the locals are accessed when getting the line number of the generator to prove this.

@FFY00
Copy link
Member Author
FFY00 commented Jun 20, 2021

Okay. Did you see my other PR (#26782)? Perhaps you could comment there if you have anything to add.

@pablogsal
Copy link
Member

Okay. Did you see my other PR (#26782)? Perhaps you could comment there if you have anything to add.

Haha, I did not see your other PR, but that confirms my theory: there is a ownership issue with the locals. I did not debug it but I bet that if you check what's wrong you are accessing a locals dict with 0 refcount.

@pablogsal pablogsal mentioned this pull request Jun 20, 2021
Closed
@pablogsal
Copy link
Member

Haha, I did not see your other PR, but that confirms my theory: there is a ownership issue with the locals. I did not debug it but I bet that if you check what's wrong you are accessing a locals dict with 0 refcount.

I checked, and indeed my gut feeling is correct:

#26782 (comment)

@FFY00
bpo-44446: support lineno being None in traceback.FrameSummary
541b7ad 
As of 088a15c, lineno is None instead
of -1 if there is no line number.

Signed-off-by: Filipe Laíns <lains@riseup.net>
@FFY00
Copy link
Member Author
FFY00 commented Jul 8, 2021

Sorry, I don't have much time today so I just rushed to rebase this to unblock the new release. I can open a new PR for the test discussed here later when I am able to get back, I think that'd also make sense from an organizational standpoint given the test is not logically related to this change, but rather the underlying issue, which got fixed separatedly.

@pablogsal pablogsal added the needs backport to 3.10 only security fixes label Jul 8, 2021
@pablogsal pablogsal merged commit 91a8f8c into python:main Jul 8, 2021
@miss-islington
Copy link
Contributor

Thanks @FFY00 for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @FFY00 and @pablogsal, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 91a8f8c16ca9a7e2466a8241d9b41769ef97d094 3.10

@pablogsal
Copy link
Member

Thanks @FFY00 for your contribution!

6D40
pablogsal pushed a commit to pablogsal/cpython that referenced this pull request Jul 8, 2021
@FFY00 @pablogsal
[3.10] bpo-44446: support lineno being None in traceback.FrameSummary (…
931a85e 
…pythonGH-26781)

As of 088a15c, lineno is None instead
of -1 if there is no line number.

Signed-off-by: Filipe Laíns <lains@riseup.net>.
(cherry picked from commit 91a8f8c)

Co-authored-by: Filipe Laíns <lains@riseup.net>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 8, 2021
@bedevere-bot
Copy link

GH-27072 is a backport of this pull request to the 3.10 branch.

pablogsal added a commit that referenced this pull request Jul 8, 2021
@pablogsal @FFY00
[3.10] bpo-44446: support lineno being None in traceback.FrameSummary (…
61eb9b5 
…GH-26781) (GH-27072)

As of 088a15c, lineno is None instead
of -1 if there is no line number.

Signed-off-by: Filipe Laíns <lains@riseup.net>.
(cherry picked from commit 91a8f8c)

Co-authored-by: Filipe Laíns <lains@riseup.net>

Co-authored-by: Filipe Laíns <lains@riseup.net>
@pablogsal pablogsal mentioned this pull request Jul 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iritkatriel iritkatriel iritkatriel left review comments

@graingert graingert graingert approved these changes

@pablogsal pablogsal pablogsal approved these changes

Assignees

@pablogsal pablogsal

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@FFY00 @graingert @iritkatriel @pablogsal @miss-islington @bedevere-bot @the-knights-who-say-ni
0