type piracy in convert causes ambiguity errors

[simeonschaub]

This definition of convert is type piracy, since LMDB doesn't own the Ref type:

LMDB.jl/src/common.jl

Line 12 in 6212c61

convert(::Type{T}, mdb_val_ref::Ref{MDB_val}) where {T} = _convert(T, mdb_val_ref[])

Among the usual problems with type piracy, this also causes ambiguities for convert(Any, ::Ref{MDB_val}). Caught in the PkgEval run for JuliaLang/julia#43671, see: https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/3656ddf_vs_1db8b8f/LMDB.primary.log

I'd recommend just removing this definition.

[meggart]

This definition of convert is type piracy, since LMDB doesn't own the Ref type

Really? This is only called when a Ref{MDBVal} is involved so I don't really see how this should modify the behavior of existing programs. I will look into fixing this once your PR got merged, so it gets easier to test this.

[simeonschaub]

This is a bit of a grey area, but defining methods on functions you don't own on types you don't own, even if you own one of the type parameter can still lead to unexpected behavior.

The reproducer for this is quite easy, you just call convert(Any, Ref{MDBVal}()), which should fail on current julia releases as well.

[wildart]

I aggree with @simeonschaub. All these _convert methods should really be convert in that case it's clear what kind of conversion is lacking, if any.

[meggart]

In principle that would be fine. However, wouldn't then the ambiguity come from this method (if we rename it convert)

LMDB.jl/src/common.jl

Line 20 in 6212c61

function _convert(::Type{T}, mdb_val::MDB_val) where {T}

which still has an unbound first argument, so that convert(Any, MDBVal) would be ambigous? I personally would suggest two options to solving this:

1. Not extending convert at all but rather renaming the function to something like mbd_unpack, over which we have full control, This would be breaking, but ok IMO, since the package was just revived and should not have a large userbase in its current version.
2. Just fixing the ambiguity by defining convert(::Type{Any}, v::Ref{MDBVal}) = v

Probably @simeonschaub will suggest to implement the first option, although my lazy self go rather go for 2)

[simeonschaub]

It's your package, so I won't tell you how it's supposed to work. You might be interested in the discussion in invenia/Intervals.jl#144 regarding approach 2 though.

[meggart]

Ok, thanks for the link. I will check their proposed solution in more detail and will probably go for 1) then.