Strange `convert` method

[fingolfin]

I've recently had reason to again check for convert(Any, ...) methods and discovered that CxxWrap still exports two:

julia> methods(convert, (Type{Any}, Any))
# 3 methods for generic function "convert":
[1] convert(::Type{Any}, x::CxxWrap.CxxWrapCore.SmartPointer{DerivedT}) where {BaseT, DerivedT<:BaseT} in CxxWrap.CxxWrapCore at /Users/mhorn/.julia/packages/CxxWrap/ptbgM/src/CxxWrap.jl:282
[2] convert(::Type{T1}, p::CxxWrap.CxxWrapCore.SmartPointer{DerivedT}) where {BaseT, T1<:BaseT, DerivedT<:BaseT} in CxxWrap.CxxWrapCore at /Users/mhorn/.julia/packages/CxxWrap/ptbgM/src/CxxWrap.jl:276
[3] convert(::Type{Any}, x) in Base at essentials.jl:217

The first method just returns x, but the second does not. Indeed, its where clause is really weird; it looks as if it meant to enforce a common overtype, but of course Any always is one, so I am not sure what the real idea is.

function Base.convert(::Type{T1}, p::SmartPointer{DerivedT}) where {BaseT,T1 <: BaseT, DerivedT <: BaseT}
  return cxxupcast(T1, p[])[]
end

@barche I'd suggest a fix... except I am not sure what this is meant to do, so I can't really suggest one

[fingolfin]

The next convert method is also weird (note that BaseT, DerivedT basically do nothing and could be eliminated)

function Base.convert(to_type::Type{<:Ref{T1}}, p::T2) where {BaseT,DerivedT, T1 <: BaseT, T2 <: SmartPointer{DerivedT}}
  return to_type(convert(T1,p))
end

[barche]

The first one (returning x) fixes an ambiguity (see issue #301) The second method is used to dereference a smart pointer to a derived type D to its base type, if it is removed this test fails:

https://github.com/JuliaInterop/CxxWrap.jl/blob/36c7f3a5d11b38b9e33bce15e518304d01e6e889/test/inheritance.jl#L58

The last one is also to fix some ambiguity I think.

[fingolfin]

I am not saying these methods are not necessary, but I am saying their where clauses are fishy.

The second method is used to dereference a smart pointer to a derived type D to its base type, if it is removed this test fails:

This reference to a "derived type" and "base type" seems like a red herring, though? Given that this method can be simplified to the following:

function Base.convert(::Type{T1}, p::SmartPointer) where {T1}
  return cxxupcast(T1, p[])[]
end

So then if I have a SmartPointer{Something} value x, and call convert(SmartPointer{Something}, x) by specification it should return x (as it already has the right type).

But what it instead does is to return cxxupcast(SmartPointer{Something}, p[])[]. Is that really what's desired?

[barche]

I think that simplification will indeed work, I need to do some more tests for this. Unfortunately, it still remains a convert-to-any method.

[fingolfin]

I've simplified the methods a bit in PR #322. That way at least when we come across this code again in the future it'll be slightly easier to reason about :-)

[jw3126]

I also have some issues with conversion. I get an error like

ERROR: LoadError: MethodError: convert(::Type{Observable}, ::CxxWrap.StdLib.SharedPtrAllocated{MeshLib.Mesh}) is ambiguous. Candidates:
  convert(::Type{T1}, p::CxxWrap.CxxWrapCore.SmartPointer) where T1 in CxxWrap.CxxWrapCore at /home/jan/.julia/packages/CxxWrap/IdOJ
a/src/CxxWrap.jl:275
  convert(::Type{T}, x) where T<:Observable in Observables at /home/jan/.julia/packages/Observables/jbVpe/src/Observables.jl:128
Possible fix, define
  convert(::Type{T}, ::CxxWrap.CxxWrapCore.SmartPointer) where T<:Observable

It is a tricky issue. Can maybe T1 be restricted in ?

function Base.convert(::Type{T1}, p::SmartPointer) where {T1}
  return cxxupcast(T1, p[])[]
end

I am not familiar with CxxInternals, but AFAICT somewhere code like cxxupcast(::Type{MoreConcrete}, ...) gets generted? Could that be accompanied by Base.convert(::Type{MoreConcrete}, ...) = cxxupcast?

Or other ideas how to solve this?

[barche]

I am not familiar with CxxInternals, but AFAICT somewhere code like cxxupcast(::Type{MoreConcrete}, ...) gets generted? Could that be accompanied by Base.convert(::Type{MoreConcrete}, ...) = cxxupcast?

Yes, the cxxupcast method gets generated from C++. The current system was an attempt to limit the amount of convert methods that get added, since this can easily go into the hundreds when using template types. In what context are you getting this error, exactly?

[jw3126]

The context is I want to add Makie plotting support to a Cxx wrapped type. Makie internally needs to convert stuff to observables and hits this error.

[jw3126]

since this can easily go into the hundreds when using template types.

Ok that sounds excessive, do you know if that causes practical problems? Like degrading package load time or dynamic dispatch time?

One option might be instead of

Base.convert(::Type{MyClass{S1}} ...
Base.convert(::Type{MyClass{S2}} ...
Base.convert(::Type{MyClass{S3}} ...

to generate

Base.convert(::Type{<:MyClass} ...

[barche]

It seems redefining the convert method signature as follows fixes this. No idea why, it feels like a dubious trick. See PR #334

function Base.convert(::Type{T1}, p::SmartPointer{T2}) where {T1, T2 <: T1}

[jw3126]

Oh, interesting. The devil is in the details with these tricky dispatch specificity cases.

[jw3126]

But makes sense, that this cannot clash with Base.convert(::Type{Observable}, ...). The only way it could clash would be

convert(::Type{Observable}, ::SmartPointer{<:Observable})

which probably never ever will happen.

[fingolfin]

This is fixed by #338, which essentially removes automatic unboxing of smart pointers, which include these method this issue is about. Without that patch, loading CxxWrap there kills most of the benefits of the great new feature there, which caches generated native code across Julia invocations. I think it already is bad now (as it causes lots of code invalidations) but the effects in Julia 1.10 (resp. 1.9, as the plan is to backport this feature) is devastating. See this comment for some benchmarking.