-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix #29059, unnecessary warning in precompile for eval
in __init__
#29134
Conversation
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.
No, this will allow code to run that is exactly the bad pattern it's designed to detect.
The current_module
handling everywhere is supposed to go away with eventual deprecation cleanup (to be replaced with a scan through jl_module_init_order
to implement this warning, and I think it will also drop some of the other try/catch functions that wrap eval
)
Example?
Is the idea to disallow eval into a module after it has had |
The idea is to detect calls to |
Certainly, but we need a more practical definition than that --- for example there's nothing wrong with Just to make sure we're on the same page, is the code in the original issue intended to be valid? |
No, that’s specifically the example this is here to catch Yes, there’s nothing wrong with that. We just make the observation that you don’t need eval for doing permitted actions, so while we could attempt to write a classifier (like we have for pure), it’s unnecessary. |
Well clearly |
Superseded by #29483 |
Load-time
eval
into a different module is not allowed, since its results can't be tracked and saved. Init-time and run-timeeval
are allowed.__init__
methods are the only code expected to run during precompilation that are not "load time" code. So technically, the eval warning could be disabled at init-time. We don't have a good way to do that though, so instead I chose to set current_module toX
while runningX.__init__
. That at least allowsX
to eval into itself, which is the most sensible case.Incidentally, I believe this warning is the only remaining use for the
current_module
stuff. I'll make a separate PR to remove unnecessary setting/resetting of it.I also made a slight change to the logic controlling when to run
__init__
methods. Previously we did it (1) after loading a precompiled module, or (2) once we are back outside of any module. E.g. these modules:used to print
true
in precompiled mode, butfalse
in--compiled-modules=no
mode. The change makes it always printtrue
, by considering any full package to be sufficiently "top level".fixes #29059