Using isdefined to check where a function is defined

Hi all,

In CxxWrap, there is a check to determine if a function declaration should be prepended with the module, for example to extend functions in Base, using isdefined. However, when creating functions that have the same name as a function in Base without the intention to add methods to Base, this triggers a bug. In simplified form, this is what happens, the objective being to create a new function open in Foo that does not add a method to Base.open.

This is fine:

module Foo
    open(s::AbstractString) = s
end

@show Foo.open("test")

This fails with ERROR: error in method definition: function Base.open must be explicitly imported to be extended (which is what we don’t want to do):

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

Questions:

  1. Is it allowed to have a function that is named the same as a function in Base, but not extending it?
  2. If the answer to 1 is “yes” (which I expect), then is there a way to call isdefined (and later on parentmodule) or define the function so that this error is not triggered?

Yes, there’s nothing special about Base.

Just define it?

Right. That works, except if you define it in an included file. See also this related question Trying not to overload Base.open - #4 by Sukera, where @Sukera demonstrated how you can trigger this by “just defining it”.
This is the case here: CxxWrap builds the list of names that are defined on the C++ side and imports them into the current module. That’s what triggers this error.
The code for importing the list of names is here: https://github.com/JuliaInterop/CxxWrap.jl/blob/master/src/CxxWrap.jl#L451-L457
The isdefined guard was put in place to fix a bug:
set_override_module within add_type<Parametric...>().apply<...> · Issue #199 · JuliaInterop/CxxWrap.jl · GitHub
The question is if there’s a way to import the names from the C++ module without triggering the error.
See also the discussion here: Change in name resolution leads to clash with Base.open · Issue #226 · JuliaInterop/CxxWrap.jl · GitHub

It’s not very clear to me what’s the requirement here. What’s the difference between what you need and importing from a normal module?

1 Like

In another word,

  1. There’s nothing special about included file.
  2. The error you get in Trying not to overload Base.open - #2 by Sukera is entirely expected and there’s no way around it. You should not change the behavior based on if something has been used or not to avoid the error. This error has to be fixed by the user. Trying to be smart here is bad.
1 Like

Right. There’s nothing special about the included file.
As far as I understand the problem is that isdefined(mymodule, open) returns true, even if I never explicitly import Base: open in mymodule. In that case, the code in CxxWrap returns the name of the function and exports it to the module, but it gets attached to Base. This is bad, because I didn’t import Base: open. So, julia sees that I called Base.open and thinks I want to override it. And complains. But I don’t want to override Base.open. I want the name to be exported as mymodule.open. The question is how to achieve that in complicated cases with different includes and without triggering issue 226 in CxxWrap.

One way to achieve this is to define a method mymodule.open before importing the name from CxxWrap. In that case, it finds the name mymodule.open and just adds another method mymodule.open, and not Base.open. But that’s not a very general solution.

No. IIUC, you are talking about the case where you have open(...) = ..., this will never create anything attached to Base without an explicit import.

No, julia complains because what you want to do (using both Base.open and mymodule.open as open) in your module is impossible. The solution is to stop doing that and it’s strictly a user decision to make (i.e. you have to decide on which one to use). If you want Base.open as open, you have to not try to define any additional open (CxxWrap or not). If you want mymodule.open as open, you have to not try to use Base.open as open and fully qualify any use instead (you can obviously define aliases like const base_open = Base.open though this particular one doesn’t save any typing…)

No, that won’t solve any problems (other than any “broken”/“too smart” tricks CxxWrap might be doing). Defining such a method will just cause the very same error if Base.open was used as open before hand or causing any later use of Base.open as open to resolve to the wrong function. The error should not exist (again, absense of any “smart” tricks from CxxWrap) anyway in the absense of such use (of open).

1 Like

Please take a look at the second example in the OP. The very call of isdefined causes the Base.open error without Base.open being imported or called directly anywhere.
That is what causes the problem.
I’m out of my depth on the other items, so I cannot comment on those.

Right, what I said about “just define it” means not using isdefined. Also, the error doesn’t contradict what I was saying. The isdefined call is a use. Combining that with a overriding function definition will give you an error.

And the mention of import is not related to the use. In fact, what I said is that an explicitly import will cause it to “attach” to Base which will then NOT throw an error on defintion, whether or not that’s the intention of the code.

That’s exactly what I meant by

And just to be clear. I’m not talking about how you can use CxxWrap. I’ve never used it and I don’t know how it is intended to work. Instead, I’m talking about how it has to work and what kind of semantics it should not aim to provide.

You might have misunderstood the code snippet I posted in the other thread - I was not defining a new addition to Base.open in there, merely using the definition exported from Base. Since that brings the Base.open into scope, defining your open afterwards in the module leads to a name clash which has to be resolved somehow. In your case, defining the new open as LCIO.open would probably solve that issue.

Well, I think I wasn’t the only one who was surprised that isdefined(Foo, open) as in the second example resolves to Base.open. But, there you go, I learned something.
I think the only remaining question is this: Importing every name from the C++ library as Foo.name into Julia worked fine, except for the problem that was reported in Change in name resolution leads to clash with Base.open · Issue #226 · JuliaInterop/CxxWrap.jl · GitHub. If isdefined is the wrong way to fix this, what’s the right way?

I agree that may not be very obvious. And FWIW, whether that is a use or not doesn’t really matter here since relying on that information to do things differently here is almost certainly wrong.

As I said, there’s nothing that should be done automatically here. It’s fully on the user (I.e. you, not CxxWrap) to determine what you want. AFAICT, this should be no different from wanting to define a open function in your module in Julia, it’ll always have to override Base.open in your module and you can’t use Base.open as open in your module.

In another word, if you want to name your function open you have to make your own decision of what open is in your module,

  1. Base function
    Then don’t define your function in this module. Define it in another module and use it fully qualified in this module.
  2. Your function
    Then don’t use open as if it is the Base one.
  3. Both (I.e. extending base)
    Then explicitly import Base.open before using or defining open in your module.

Again, after you make the decision, nothing should be done from cxxwrap side. It isn’t in a position to make that decision for you… Trying to do so is what I meant by “too smart”.

2 Likes

Of course, what cxxwrap need is the tool to allow you to implement any of the above as you will in a easy way. I don’t know whether it does (it shouldn’t be very hard… that’s the just define it I mentioned in the first reply) since I’ve never used it but that’s kinda out of the scope of this thread. In any case though, cxxwrap shouldn’t need isdefined to provide such a functionality.

1 Like

Thanks for all the replies. The choice between extending and defining a new function is indeed with the user, but since all functions are generated, the code has to decide between AModule.foo() and just foo() when generating the function definition expression.

Luckily, the miracle called “sleep” brought a solution that doesn’t rely on isdefined, checking instead if the method module and the module that the function is generated in are the same, and leaving off the module name in that case.

I still think it’s weird that isdefined here triggers a “use” of open, especially since it is passed only the symbol :open and not the function itself, but for me it doesn’t really matter anymore, since my use-case disappeared :wink:

1 Like

Yes, that’s what you are doing if you were writing Julia code.

1 Like