bpo-44446: support lineno being None in traceback.FrameSummary#26781
bpo-44446: support lineno being None in traceback.FrameSummary#26781pablogsal merged 1 commit intopython:mainfrom
Conversation
FFY00
force-pushed
the
bpo-44446
branch
2 times, most recently
from
70c0a93 to
b8963bf
Compare
|
Might be worth including a regression test formatting a Traceback containing |
|
Maybe, though I do not know where such test should belong. I'd hope that is already being tested 😁 |
iritkatriel
left a comment
iritkatriel
left a comment
There was a problem hiding this comment.
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.
It would belong here, in TracebackException's tests. If it was already tested we wouldn't be here! |
|
Why does the |
No idea. Probably worth asking in a separate bpo. |
|
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 |
I assumed it was None in both cases, because linecache won't find anything for line -1 (but I didn't check). |
|
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. |
|
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. |
|
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. |
I checked, and indeed my gut feeling is correct: |
As of 088a15c, lineno is None instead of -1 if there is no line number. Signed-off-by: Filipe Laíns <lains@riseup.net>
|
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. |
|
Thanks @FFY00 for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10. |
|
Sorry @FFY00 and @pablogsal, I had trouble checking out the |
|
Thanks @FFY00 for your contribution! |
…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>
|
GH-27072 is a backport of this pull request to the 3.10 branch. |
7 participants
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