-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[mypyc] Do not emit tracebacks with negative line numbers #20641
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
base: master
Are you sure you want to change the base?
Conversation
JukkaL
left a comment
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.
Thanks for fixing so many missing line numbers! I've also seen the pytest exception, and it was pretty difficult to debug the related test failure.
mypyc/irbuild/builder.py
Outdated
| ) -> Value: | ||
| if isinstance(target, Value): | ||
| if line != -1: | ||
| target.line = line |
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.
Should this only be done if the original line (target.line) is -1? This might otherwise plausibly break line numbers.
It's not clear why this is needed. In this just a defensive measure, or does this fix some real issue? Maybe add a comment?
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.
this was kind of a hack as the real issue was that Value objects representing literals in the Python code inserted by the IR builder did not have line numbers set. but i have changed that in the meantime to fix other cases of -1 tracebacks found while compiling mypy so this isn't necessary anymore, thanks for spotting.
mypyc/irbuild/main.py
Outdated
|
|
||
| # Make sure we have a builtins import | ||
| builder.gen_import("builtins", -1) | ||
| builder.gen_import("builtins", 0) |
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.
Use line 1 here, since builtins import could be understood to happen at the beginning of a file?
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.
sure, makes sense. done.
264c574 to
0049c52
Compare
mypyc may add tracebacks in the generated code with line number -1 when the AST expression that generates an error has no line set. This is usually the case for expressions that are not directly linked with a line in the Python code but are generated internally by mypyc, like helper functions generated for
asyncfunctions.These tracebacks become a problem when running a test that calls a compiled function with pytest. The line number ends up being interpreted by Python as
Noneinstead of anintand pytest crashes when trying to do math on it. So instead of a stack trace pointing to the compiled function we get an internal pytest error which makes debugging more difficult.Changes in this PR ensure that AST nodes that may produce tracebacks have their line numbers set. For AST nodes generated by mypyc this usually means the line number of the function they are associated with, so an internal attribute access node inside a helper function generated for an
async def my_fun()will have the line number ofmy_fun.I didn't add a specific test for this because getting an exception from an internally generated node means that there is a bug in mypyc that we should fix but I tested it with one such error that I'm working on and pytest no longer crashes. I have also added assertions so that new changes to mypyc don't cause it to generate tracebacks with negative line numbers.
For now the assertions are only enabled in tests since there are cases where mypyc emits tracebacks that we don't have tests for, and the new assertions could break compilation that currently succeeds without them. Once we remove all cases where tracebacks with negative line numbers are possible we can enable them by default.