eval julia_type input in Main instead of current_module()

[jrevels]

...since input types might not exist in current_module(). This fixes using a function which calls JLD.translate inside a module scope where JLD is not imported (but the caller does have JLD imported). This bug can be seen in the SerializationTests failure here.

I'm not sure whether this PR is really the correct fix for this issue - it might violate existing assumptions or rely on bad assumptions. For example, this fix may be broken if e is not fully qualified.

Credit to @maleadt for hunting this down.

[timholy]

Did it pass the tests locally? It didn't with CI.

[jrevels]

I just modified this PR to make eval(Main, ...) the fallback rather than the default behavior, which should fix the tests.

Is there a way to perform this check ahead of time instead of using try as control flow? Something like isdefined(current_module(), e) works when e is a Symbol, but not an expression (e.g. a partially/fully qualified type name).

I suppose I could drill down through any namespaces given in e. For example, to achieve something like isdefined(A, :(B.C.D)), I could write isdefined(A, :B) && isdefined(B, :C) && isdefined(C, :D). That might be more convoluted than it's worth, though.

[timholy]

Might JuliaCI/BenchmarkTools.jl#23 also be worked around with addrequire? https://github.com/JuliaIO/JLD.jl#types-and-their-definitions

[timholy]

Oh, wait, I see that it's an import-cycle-thing. Gack.

I suppose we can have this. I'll wait to hit merge until you decide how hard you want to work to avoid the use of try.

[simonster]

I'm not sure if I'll have time to look at this more closely, but this should probably not have been using current_module(), which returns whatever module is executing at top level and probably shouldn't be used outside of macros. It seems like this can only lead to agony, since e.g. code can work from one module, but fail when put into a function that is then called from another module.

[jrevels]

code can work from one module, but fail when put into a function that is then called from another module.

Yup, this is exactly the bug I opened this PR to work around. I originally removed current_module() in this PR, but it broke tests. Funnily enough, it seems that Main was used instead of current_module() at some point in this function's history, which is exactly what this PR now adds as a fallback for fully qualified types that aren't in the calling module's scope.

It seems like it's not worth it for me to refactor the try stuff if more knowledgable JLD folks need to refactor this method later anyway. Feel free to merge whenever (or not, if it's deemed that this isn't a good idea).

[jrevels]

Doesn't seem like there's any dissent here, good to merge?

[jrevels]

Bump, this is the only thing preventing getting picosecond benchmark resolution on Nanosoldier.

[jrevels]

Thanks!

[jrevels]

Can we tag a new version of JLD that incorporates this PR (so that JuliaCI/BenchmarkTools.jl#27 will be mergable)? Thanks again!

[tkelman]

this should really use getfield, eval'ing code that may come from a binary file without the user seeing it is a bad idea

[simonster]

I agree, but it's highly non-trivial since types can have type parameters. The is_valid_type_ex function tries to ensure that evaling the type expression will not have side effects, and is pretty conservative, which is bad because it means that arbitrary immutables cannot be used as type parameters (JuliaIO/HDF5.jl#204). The real solution is to encode the types in a way that we don't need to use eval to reconstruct them. JLD2 will do this, but it is not so easy to slap onto JLD.

[simonster]

(also note that this behavior has existed for years and did not change with this PR)