Mlj.jl: `@from_network` does more strange `eval` stuff

[ExpandingMan]

So apparently @load (thanks to @ablaom for fixing!) wasn't the only macro with dubious and scary behavior. @from_network also evals stuff directly into the module. Again, this is extremely unexpected behavior for the user, this is just not what macros normally do. It would be great if this could be fixed as well.

[ablaom]

As I understand it, all macros ultimately call __module__.eval on the return expression. As far as I can remember, __module__.eval is the only kind of evaluation going on in the @from_network, no?

Perhaps you can be more specific about the strange, dubious and scary behaviour you are experiencing. I have not experienced any problems except when the macro is called in a baremodule.

[ExpandingMan]

Doing Base.eval directly into the module can have unexpected non-local consequences that macros are not expected to have. I can only imagine that it also has scary implications for whether things are actually get compiled at compile time and not at run time.

Consider this example:

macro crazy()
    Base.eval(__module__, :(struct Crazy end))
end

macro sane()
    esc(:(struct Sane end))
end

function testcrazy()
    @crazy
    nothing
end

then

julia> testcrazy()

julia> Crazy()
Crazy()

julia> sane() = (@sane(); nothing)
ERROR: syntax: "struct" expression not at top level
Stacktrace:
 [1] top-level scope at REPL[3]:1

julia> @macroexpand @crazy()  # note that if `Crazy` were not already defined, this would have defined it unexpectedly

julia> @macroexpand @sane()
:(struct Sane
      #= /home/expandingman/src/scrap.jl:7 =#
  end)

Based on my current understanding of MLJ, I do not believe eval has any place in it apart from code generation. However, even if you are convinced I am wrong and there should be functions that use eval this way, then surely those should be functions that are marked explicitly with eval in their name so that the behavior is not so unexpected (in which case it may be better to use functions than macros).

[ExpandingMan]

Btw, I'm looking into the code a little bit as I may be able to help out, but it seems that there are evals peppered throughout this particular macro, so I can see why it will not be so easy to fix. Would you consider adding a dependency of MacroTools? I find it makes working with macros vastly easier.

[ablaom]

Okay, I see how we have not followed best practice here. Really appreciate the feedback and guidance.

It's hard for me to imagine living without these evaluations as a lot of logic depends on them. In retrospect, if we weren't constructing new types, we might have avoided the meta-programming altogether - see https://github.com/alan-turing-institute/MLJ.jl/issues/594#issuecomment-656363138 - but I think that is a complete redesign. I guess in principle all this logic could be shifted into the final expression - but it would be a bit of work.

Would you consider adding a dependency of MacroTools?

julia> @time using MacroTools
  0.389238 seconds (1.18 M allocations: 58.403 MiB, 5.23% gc time)

Sure. I should take a look.

[ExpandingMan]

Yeah, I'm not yet as convinced that @from_network is really doing anything a function shouldn't be doing, unlike with @load where I agree there was a clear need for a macro. But I'm not on some sort of anti-macro rampage here, I'm just worried they are doing very unexpected things. Worrying about whether they should be there in the first place seems like a lower priority for now.

If it really is the case that "a lot of logic depends on" the evals, we might have a number of bigger problems. Before we get there, I have looked at @from_network and I'm pretty sure that about 90% of it could easily replaced by keeping an Expr block around and adding to it as you go. That should mostly replicate the functionality you are currently getting.

For example

macro from_network(args...)
    expr = Expr(:block)

    # replace
    # eval(ex1) with
    push!(expr.args, ex1)

    # more stuff

    esc(expr)
end

Though I haven't spent much time on it yet, so there may be some major things missing which can't be easily written this way.

[ablaom]

Appreciate the code review and help!

[ExpandingMan]

I also just realized that although @pipeline returns an expression as expected, it does lots of eval in the pre-processing, so there are problems there as well. I'd like to be more helpful, but unfortunately there is quite a lot of code in @pipeline and @from_network, and I'm not even entirely sure what functionality would have to change if they get cleaned up, so I have not actually gone as far as creating a branch for a PR yet.