-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
gh-104240: make _PyCompile_CodeGen support different compilation modes #104241
Conversation
Python/compile.c
Outdated
location loc = LOCATION(1, 1, 0, 0); | ||
switch (mod->kind) { | ||
case Module_kind: | ||
if (compiler_body(c, loc, mod->v.Module.body) < 0) { | ||
compiler_exit_scope(c); |
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.
I'm a bit confused by this; how does this not leave us with one fewer compiler_exit_scope
than we had previously? We still have the same number of compiler_enter_scope
(it's just moved from above into both callers of compiler_codegen
), and both callers still do the same number of compiler_exit_scope
that they did before (1), but we eliminate this compiler_exit_scope
. Why?
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.
I think currently we may have one too many - one exit here and then we return to the caller which exits again.
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.
Actually I'm wrong in the case of compiler_mod, I'll fix it there.
Python/compile.c
Outdated
_Py_DECLARE_STR(anon_module, "<module>"); | ||
RETURN_IF_ERROR( | ||
compiler_enter_scope(c, &_Py_STR(anon_module), COMPILER_SCOPE_MODULE, | ||
mod, 1)); | ||
|
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.
What is the purpose of moving this from here out into the (only) two callers of this function? It duplicates this line of code, but it doesn't seem like it would make any other difference, AFAICT.
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.
The purpose is to be able to make a distinction between compiler_enter_scope failed, in which case we don't need to exit the scope, and the case where compiler_enter_scope succeeded but compiler_codegen failed later, in which case we do need to exit the scope. I don't think this is currently correct in all cases in main.
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.
That makes sense! Beyond adding the compilation modes option, I wasn't clear where this PR was aiming to maintain behavior vs fixing problems in existing behavior.
Looking at the definition of compiler_exit_scope
, it seems like if we have one too many it would leave things in an observably broken state (like too small c_nestlevel
?) Is there somewhere we can add asserts to show that things are broken today, but not after this fix?
Also, it seems like it would be simplest to get the enter/exit logic correct if we do it just once, inside compiler_codegen
, rather than in both callers?
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.
I'm looking into it, I think it's a bigger mess than I previously thought. There are functions like compile_class which always exit the scope (which they did not enter). And VISIT_SEQ/VISIT_SEQ_IN_SCOPE also call compiler_exit_scope.
Also, it seems like it would be simplest to get the enter/exit logic correct if we do it just once, inside
compiler_codegen
, rather than in both callers?
I don't think we can exit the scope in compiler_codegen, because we need it until after optimize_and_assemble.
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.
I'm going to revert the commit with the enter/exit changes for now - I probably only needed it in a broken state when I was sending bad inputs to the unit tests.
This reverts commit 0630b4e.
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.
Looks good!
With this PR we can do this: