Change in name resolution leads to clash with Base.open

[jstrube]

With the new version, I'm getting

ERROR: LoadError: error in method definition: function Base.open must be explicitly imported to be extended

I hadn't seen that before, and the code works fine against CxxWrap 0.9. I searched for exported open functions in CxxWrap, but I couldn't find any, so I'm not exactly sure what triggers this. Did anything change in terms of name resolution between 0.9 and 0.10?

[rgcv]

I'm guessing I found a potential culprit over here.

https://github.com/jstrube/LCIO_Julia_Wrapper/blob/cd518a065551007aed90cc73026f32f143202c44/src/lciowrap.cc#L234-L236

As far as I can tell, I only recall seeing changes to the way how one must explicitly set in which module a method is being created/overriden using set_override_module/unset_override_module.

Nonetheless, you might also be experiencing a combination of what I just mentioned and the fact that this commit changed the way function names are processed.

You could try surrounding the above excerpt of code with the (un)set_override_module combination of functions, overriding Base i.e. jl_base_module, and check if it worked for you. I'm guessing @barche may have more pertinent information on the subject. I'm shooting from the hip here.

[jstrube]

Nice! I just found that myself and was going to edit this item here :-) You beat me at my own code by 15 minutes 😞
I think you're right in terms of the override module flag. I remember having seen this, but I thought I didn't have to worry about it since I don't want to override the method. But now I have to set something that I don't want to override. This is a bit tricky, because now the C++ code needs to know all of the methods that are exported in Base and either avoid name clashes or set the flags correctly.
I wouldn't mind so much, but now this has become a two-repository problem, which makes this more annoying to test...
Anyway, I'll look into it over the next couple of days. Thanks for jumping on this so quickly.

[jstrube]

Oh, I've just read the README again and it looks like I mis-characterized how this functionality is being used. The flags don't need to be set by method as I said above. They work on regions.
That's good, so this doesn't require the coder to know all of the Base methods. In my case, I only have two getindex methods that I expose to Base. Afterwards I call unset_override_module again. See here:
https://github.com/jstrube/LCIO_Julia_Wrapper/blob/master/src/lciowrap.cc#L280-L287

I can try explicitly attaching the open method to the current module, but I don't really understand the context of the commit you point to above, or why this requires me to take this additional step.

[jstrube]

Oh. I do understand, actually. Now this just returns the bare name, without the module name to protect it.
But don't I always want to stay in the name space of the module?
So in my case, I can work around the issue by defining a method
open() = nothing before the @wrapmodule call. Then isdefined returns true, and the open method is exported into the right module namespace.
But this is very hacky. I would prefer if the old behavior could be restored.

[barche]

The way it is defined now looks fine to me, is it still giving errors? I just tried adding an open function in one of the test modules, and it works without problems for me?

[jstrube]

Hmm, weird. So, you add an open method to your module, then import that into a Julia module, which also defines open, and then use that Julia module in another Julia module, and it works for you? All I know is that this open function had been there for a while, but the error only occurs with CxxWrap 0.10. I would really prefer not adding the silly `open()=nothing` to my code now.

…

On Sun, Apr 12, 2020, 3:57 AM Bart Janssens ***@***.***> wrote: The way it is defined now looks fine to me, is it still giving errors? I just tried adding an open function in one of the test modules, and it works without problems for me? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#226 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACX57X5LHOVTHQMU4CNQU3RMGNABANCNFSM4MGIRVSQ> .

[barche]

Just took another look, for me, changing LCIO.jl line 59 into:

function LCIO.open(f::Function, fn::AbstractString)

fixes it.

I must say that I didn't realize the following code returns true, and it's probably what causes the change in behavior here.

module Foo end
isdefined(Foo,:open)

[rgcv]

I think I can see why it would return true, because we're talking about a module here. I know it seems silly, but I say this because module in julia is just a baremodule with Base included in it! Refer to the docs for more details.

In short,

module Foo end

approximately equates to

baremodule Foo
using Base
end

So, within a module, Base functions/methods will already be defined, hopefully explaining why isdefined(Foo, :open) holds.

EDIT: Here's an example, using a module and a baremodule:

julia> module Foo end
Main.Foo

julia> isdefined(Foo, :open)
true

julia> baremodule Bar end
Main.Bar

julia> isdefined(Bar, :open)
false

EDIT 2: For the sake of completeness, a baremodule using Base:

julia> baremodule Baz; using Base end
Main.Baz

julia> isdefined(Baz, :open)
true

[jstrube]

@rgcv Oh, very interesting! I didn't know about this...
@barche The LCIO.open idea is a good one. I hadn't thought of that. Unfortunately, this still doesn't work for me. I can include the module but the tests fail.

ERROR: LoadError: MethodError: no method matching eachline(::CxxWrap.CxxWrapCore.CxxPtr{LCIO.LCReader})

[barche]

OK, other workaround: move the open function from line 59 to before the @wrapmodule call.

Taking into account @rgcv 's comment, it seems that adding the LCIO. actually makes open a method of Base.open, which is certainly not what we want here, and the isdefined here was meant to avoid that. So I need a way to detect if a function was imported or defined in a module to fix this.

[jstrube]

Yeah, for now moving the open method is the easiest solution. I'll try that, otherwise I could rename the method on the C++ side. Could you just change the if statement to also check for `!is defined(Base, function)`?

…

On Mon, Apr 13, 2020, 3:10 AM Bart Janssens ***@***.***> wrote: OK, other workaround: move the open function from line 59 to before the @wrapmodule call. Taking into account @rgcv <https://github.com/rgcv> 's comment, it seems that adding the LCIO. actually makes open a method of Base.open, which is certainly not what we want here, and the isdefined here <https://github.com/JuliaInterop/CxxWrap.jl/blob/master/src/CxxWrap.jl#L453> was meant to avoid that. So I need a way to detect if a function was imported or defined in a module to fix this. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#226 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACX57WJRWFSFG6G4L6CLXLRMLQKTANCNFSM4MGIRVSQ> .

[jstrube]

So, do I understand correctly that the "accidental" inclusion is limited to Base methods? I've just tried adding to the if statement in CxxWrap, but it looks like now I also need to catch the Base includes that I do want to override... So, for my proposal to work, one would have to go through the module's includes and filter out the ones from Base.
Not sure if that's feasible, so for now I'll move my definition of open above @wrapmodule, but in general it would be good to understand if this could lead a situation where multiple modules are included and the behavior depends on the relative order of the imports.

[barche]

Just doing this for Base is not generic enough, we can check if the module for the function is the same using parentmodule, but if I use that and remove the module prefix from the function, I get the error that open must be imported to add a method.

[barche]

OK, so it seems the problem is related to quantum mechanics (😉). If you look to see if open is defined, it triggers the error. Check the difference between the commented and non-commented isdefined here:

module CxxWrapDummy
    function wrapmodule(m)
        #@show isdefined(m, :open)
        Core.eval(m, :(open(s::AbstractString) = s))
    end
    
    macro wrapmodule()
        return :(wrapmodule($__module__))
    end

    export @wrapmodule
end

module Foo
    using Main.CxxWrapDummy
    @wrapmodule
end

edit: or just simply:

module Foo
    #@show isdefined(Foo,:open)
    open(s::AbstractString) = s
end