Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

JeffBezanson
Copy link
Member

Load-time eval into a different module is not allowed, since its results can't be tracked and saved. Init-time and run-time eval 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 to X while running X.__init__. That at least allows X 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:

module PackageA
using PackageB
println(PackageB.inited)
end

module PackageB
inited = false
__init__() = (global inited = true)
end

used to print true in precompiled mode, but false in --compiled-modules=no mode. The change makes it always print true, by considering any full package to be sufficiently "top level".

fixes #29059

Copy link
Member

@vtjnash vtjnash left a 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)

@JeffBezanson
Copy link
Member Author

this will allow code to run that is exactly the bad pattern it's designed to detect

Example?

to be replaced with a scan through jl_module_init_order

Is the idea to disallow eval into a module after it has had __init__ called?

@vtjnash
Copy link
Member

vtjnash commented Sep 11, 2018

The idea is to detect calls to eval for which we won't be able to handle the result. The jl_module_init_order does essentially contain that information (of which modules have had __init__ called), but I think that's more driven by common intent than definition.

@JeffBezanson
Copy link
Member Author

The idea is to detect calls to eval for which we won't be able to handle the result.

Certainly, but we need a more practical definition than that --- for example there's nothing wrong with eval(mod, 2) but it's too complicated to try to specify why that's ok but other evals aren't.

Just to make sure we're on the same page, is the code in the original issue intended to be valid?

@vtjnash
Copy link
Member

vtjnash commented Sep 11, 2018

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.

@JeffBezanson
Copy link
Member Author

Well clearly eval(:(b() = 1)) doesn't really need eval, but if it depended on, say, a random number or environment variable then it would be invalid?

@JeffBezanson
Copy link
Member Author

Superseded by #29483

@DilumAluthge DilumAluthge deleted the jb/fix29059 branch March 25, 2021 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:precompilation Precompilation of modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eval in __init__ causes incorrect precompilation warning
3 participants