-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-122145: Handle an empty AST body when reporting tracebacks. #122161
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I don't know what to really put in the NEWS entry 😢 I'm not sure that the only way to get an AST body that is empty is only by having comments inside (and without causing a SyntaxError/Import Error) =/ (I also forgot to add credits to you Nikita). |
CC @pablogsal |
I don't think it's possible for an AST to have a node with empty body, there seems to be something else going on here. |
Actually it is if you only have comments: >>> ast.parse('#').body
[] |
Could you rewrite the test to use this ast rather than construct the intermediate structure? |
I can do it but I need to check that I'm able to hit the |
Can you create the frame summary from the python code snippet, rather than hard code it? |
Ideally no, we don't write unit tests for internal functions. |
Ok, I'll try to see how I can write my test without this (I was fixing some stuff in the meantime) |
@iritkatriel I tried to use as much public API I could but it's actually extremely difficult to trigger the bug without an explicit construction of a FrameSummary. The reason is that the column offsets are only set when invoking (Also, I don't think I can write everything in the file since otherwise the AST won't be empty...) |
So this makes me wonder - ok, it's possible to have an empty body on an AST node, but how is it possible for that to appear on the traceback of an exception? It would be compiled into something like "RESUME" followed by "RETURN_CONST None". So maybe if there is a KeyboardInterrupt while the RETURN_CONST is executing? How did an empty body end up being accessed from |
Sorry, RETURN_CONST will only be added to a function node body. For a module not even that. But the question remains - how does executing an empty module raise an exception? |
Actually, I think it's because of For the issue, I think the source is incorrectly deduced or one of the frame is actually used in the traceback itself. Except for a live reproducer with the REPL or with a linecache hack, I don't know how to do reproduce the bug. I agree that we should not have an empty AST because that would mean the file is empty or only consists of comments. Empty lines are already handled but somewhat comments only no. In the meantime, we could perhaps have this hotfix and then try to investigate more in details (or we can just take our time, though it could be annoying that your REPL crashes without you being able to do anything...). The culprit AST is in # Important: don't add things to this module, as they will end up in the REPL's
# default globals. Use _pyrepl.main instead.
if __name__ == "__main__":
from .main import interactive_console as __pyrepl_interactive_console
__pyrepl_interactive_console() |
I put a breakpoint just before the call to
neither of these is in |
It's linecache using a loader that has the wrong filename. I'll post the relevant code once I'm on my computer. |
See #122071 (comment) and surrounding context for an explanation, a possible fix and a PR targeting this. |
Right. I just landed on this possible solution before I saw your comment (not sure this is 100% correct, there's stuff here I'm not very familiar with):
|
It would be nice to have compile() feed linecache with the line it's compiling. Then we would have that in the traceback as well (not just the exec call). compile is written in C though, so it's not clear how simple this is. |
I'll add a DO-NOT-MERGE label here because we might possibly want to fix linecache instead of using this simple fix. If we fix linecache, I think we should instead have an |
@picnixz: Your PR has the DO-NOT-MERGE label. Is the PR ready for review? It already got multiple reviews :-) |
IMO this fix is needed. Fixing linecache is something different which deserves its own PR. |
This bug can occur if the Python source code is modified after a program started, which is likely while developing. So again, IMO it's worth it to fix this bug. Fixing the other bug about getting the right line in linecache is also worth it but has a lower priority. I marked this bug as a blocker issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@iritkatriel @sobolevn: I removed the DO-NOT-MERGE label. Are you still ok to merge this change? |
…ythonGH-122161) (cherry picked from commit 5cd50cb) Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
GH-124214 is a backport of this pull request to the 3.13 branch. |
Merged. Thanks @picnixz for your fix and the test. |
Credits to @sobolevn.
Should I rather try to fix the two issues simultaneously or should I just fix this one here?