Add unwrap function to allow wrapper types to avoid over-overloading convert

[quinnj]

Would allow packages like CategoricalArrays and DataValues to avoid having to overload convert(::Type{Any}, x) with their own definitions (due to wanting generic convert(::Type{T}, x::MyType) definitions). Just putting this up as a POC to get feedback.

Ref: JuliaData/CategoricalArrays.jl#177, queryverse/DataValues.jl#51

[JeffBezanson]

MethodError if y === x?

[quinnj]

Yeah, I wasn't quite sure on how this should interact (replace?) convert(Any, x), so I just simulated convert to Any

[JeffBezanson]

But convert(T, x) is supposed to return something of type T. Returning x works for Any but not all T.

[bkamins]

Just a report of possible consequence of not having this (this is on Windows, Julia 1.1, on Linux and OSx the problem does not seem to be that big - which is surprising):

$ julia -e "@time using Missings, CategoricalArrays"
  0.712632 seconds (2.05 M allocations: 137.079 MiB, 2.84% gc time)

$ julia -e "@time using CategoricalArrays, Missings"
  2.155997 seconds (5.49 M allocations: 299.823 MiB, 4.72% gc time)

CC @nalimilan

[bkamins]

The same timings for Linux:

$ julia-1.1.1/bin/julia -e "@time using Missings, CategoricalArrays"
  0.615162 seconds (2.02 M allocations: 136.352 MiB, 2.88% gc time)
$ julia-1.1.1/bin/julia -e "@time using CategoricalArrays, Missings"
  1.811056 seconds (5.19 M allocations: 286.239 MiB, 5.31% gc time)

[quinnj]

Alright, added the MethodError, docs for Base.unwrap, and some tests (showing usefulness)

[nalimilan]

What do "lower" and "Base-defined" mean exactly?

[quinnj]

Base-defined really means that you convert/transform your wrapper type to a value with a type that already has a convert(T, x) definition, which for the most part, will be Base/stdlib-defined types.

[nalimilan]

OK. That doesn't sound very explicit to me, and if that's only try "for the most part" it shouldn't be presented as a general property. Why not just say "unwrap"?

[nalimilan]

Also test that it works when the destination type differs from typeof(unwrap(wrapperstruct))? E.g. with Int and Float64.

[quinnj]

@JeffBezanson, this is ready for another review when you have a moment.

[davidanthoff]

Do we really need this? I just retested queryverse/DataValues.jl#52, which simply removes that offending convert method, and things work on julia 1.1 and 1.2 (but break on 1.0). Maybe that is good enough, I could put that method behind a VERSION check?

I'm not keen of thinking of DataValue as a wrapper, I'd much prefer to simply think of it as a type that supports the values from some other type plus one extra value, but not pin down any internal way to represent that or a mental idea of a wrapper. Would the idea here be that unwrap is a more generic API than just to solve this recompile issue, i.e. some way to introduce a generic idea of wrapper types into julia? If so, at least for DataValue, I think I would like to think through what that actually means, implies before I would commit to it. If the goal here is only to solve the recompile issue, though, then unwrap seems too generic of a name to me that seems to carry quite a lot of implications of generic API design with it.

[quinnj]

No, this isn't meant as a generic "wrapper" API or anything; unwrap isn't exported from Base and is just meant for this convert scenario (and could be renamed to something else if something like a "wrapper" API became a thing). I'll look into the PR that removes the method a little more; I'm interested to see how it works and what we'd need to do in CategoricalArrays similarly.

[davidanthoff]

and could be renamed to something else if something like a "wrapper" API became a thing

In that case, why not use a less "good" name now and preserve unwrap for some future use in Base? Yes, one could rename, but that would trigger churn in downstream packages that we could easily avoid by picking some other name now.

[quinnj]

I don't think we're going with this approach, as it's a bit of a hack anyway. I'm beginning to believe that might not be reasonable for CategoricalValue to expect to act like "any" value it wraps; I think moving to an explicit wrap/unwrap API would alleviate a lot of its problems.