Falling back to Base.convert during construction does not work for custom types with a single field

[helgee]

When I run the following code

type A
    v::Float64
end

type B
    v::Float64
end

Base.convert(::Type{A}, b::B) = A(b.v/2)
b = B(2)
A(b)

it fails with this error message because the implicit constructor is called.

ERROR: MethodError: `convert` has no method matching convert(::Type{Float64}, ::A)
This may have arisen from a call to the constructor Float64(...),
since type constructors fall back to convert methods.
Closest candidates are:
  call{T}(::Type{T}, ::Any)
  convert(::Type{Float64}, ::Int8)
  convert(::Type{Float64}, ::Int16)
  ...
 in call at none:2

I expected this to work since the manual states:

defining Base.convert(::Type{T}, args...) = ... automatically defines a constructor T(args...) = ...."

The following code on the other hand works:

type A
    v::Float64
end

Base.convert(::Type{Float64}, a::A) = a.v/12
a = A(2)
Float64(a)

It seems that the new constructor is only generated for built-in types and not for custom types.

EDIT: Typo.

[yuyichao]

Maybe related #15044 ?

[cstjean]

It fails on Julia 0.4.3.

[JeffBezanson]

The wording in the manual is not ideal. Defining convert does not create a constructor; it only adds a method to convert. Then there is a fallback that invokes convert if a type doesn't have a matching constructor. If for a given type convert and "construct" mean the same thing, you can define convert inside the type block to suppress generation of default constructors.

[helgee]

Why does the type constructor not fall back to the convert method that I have defined, then?

[JeffBezanson]

Because of the default constructors:

julia> type A
           v::Float64
       end

julia> methods(A)
3-element Array{Any,1}:
 A(v::Float64) at none:2          
 A(v) at none:2                   
 (::Type{T}){T}(arg) at int.jl:419

[helgee]

I apologize if I am being dense. But if this is the intended behavior I do not understand the reasoning behind it. Especially since the following example works.

type C
    v::Float64
    w::Float64
end

type D
    v::Float64
    w::Float64
end

Base.convert(::Type{C}, d::D) = C(d.v/2, d.w/2)
d = D(2, 2)
C(d)

[swt30]

This was surprising to me too. I would have assumed that the default constructor for A in the first example was A(v::Float64) and that it would coexist with A(b::B) from convert. But @JeffBezanson has indicated that convert will only be called as a fallback. Looking at the output from methods, it seems that the more general case A(v) is also defined for us. This appears to do a conversion or promotion on v, which enables us to write A(x) where x is not a Float64. Because a general single-argument method is already defined, the fallback isn't used.

In contrast, the second case with types C and D works because there is no general case for the single-argument C(x), only for C(v,w). Therefore the fallback method gets called as expected.

[swt30]

Just to clarify on that second case, the following will give the same error:

type C
    v::Float64
    w::Float64
end

type D
    v::Float64
    w::Float64
end

Base.convert(::Type{D}, c1::C, c2::C) = D(c1.v, c2.w)

c1 = C(2, 2)
c2 = C(3, 3)
D(c1, c2)

[helgee]

@swt30 Thanks for the explanation. I finally got it.

I have opened a PR for clarifying the documentation. This corner case seems very unfortunate to me, though. "You can also extend Base.convert unless the number of arguments matches the number of fields" might be hard to sell to a newcomer.

[cstjean]

@helgee I agree that it's not pretty, but maybe it's better to frame it the way Jeff did? "If you want to use convert as your only constructor, put it in the type definition so that Julia does not automatically create a constructor."

type B
    v::Float64
end

type A
    v::Float64
    Base.convert(::Type{A}, b::B) = new(b.v/2)
end

b = B(2)
A(b)

[helgee]

@cstjean In my real-world code the whole thing is not an issue because the number of arguments and the number of fields are not the same. I was just stumped by it during testing. In cases were conversion and construction are not the same it is also easy to circumvent the problem by doing this:

type A
    v::Float64
end

type B
    v::Float64
end

Base.convert(::Type{A}, b::B) = A(b.v/2)
# Explicit constructor
A(b::B) = convert(A, b)

b = B(2)
A(b)

I am just bothered by it because it seems like a serious WAT?! to me and the manual was actively misleading.

[nalimilan]

I agree this is terribly confusing. Would it be possible to stop defining the default A(x) = ... constructor, and define convert(::Type{A}, x) = ... instead?

[swt30]

@helgee, to further complicate things, I'm not sure that

A(b::B) = convert(A, b)

is exactly right either. It's what I do, but the manual seems to suggest I had a vague idea that something I read suggested defining a call method on the type instead:

Base.call(::Type{A}, b::B) = convert(A, b)

or I guess on master something like

(::Type{A})(b::B) = convert(A, b)

I don't know what the advantage of this is as compared to writing A(b) = ... directly.

[JeffBezanson]

A(x) = ... and (::Type{A})(x) = ... are identical.

We didn't always generate default constructors for Any arguments. However if we took them away, the conflict would only occur for a specific set of argument types (the declared field types), which might be even more confusing.

A case can be made that types should have to individually opt-in to the fall-back-to-convert behavior. That way the default would be to keep convert and construct totally separate functions. It also fits better with the new functions design. Of course this would be a significant breaking change.

[nalimilan]

Another strategy would be to get rid of the distinction between convert and constructors. But I guess there are downsides to that...

[JeffBezanson]

I don't think that's workable; for example List(lst) would wrap its argument in another list, while convert(List, lst) would be the identity function when lst is already a List.

[JeffreySarnoff]

I find JeffB's note "...the default would be to keep convert and construct totally separate functions [and that would fit] better with the new functions design." compelling as it better smooths future Julia. And having the sort of disclarity discussed above complicate proper use of whatever is to serve as a protocol or as refinable interface specifier is best avoided.

[vtjnash]

A case can be made that types should have to individually opt-in to the fall-back-to-convert behavior. That way the default would be to keep convert and construct totally separate functions. It also fits better with the new functions design. Of course this would be a significant breaking change.

+1. Furthermore, I believe that if the convert-constructor was generated by default for any bitstype then this would fit better with expectations (and not be a significantly breaking change). This would be more similar to how default constructors are created for other types, and I feel this may be more consistent with the primary purpose and usage of bitstypes – they are pointer-free bitstypes, which seems like it would make them mostly useless as containers – but generally meaningful for conversion. And besides: that should get rid of the last method override warning during bootstrapping.

[JeffBezanson]

Agreed. Let's make convert the "default constructor" for bitstypes, and otherwise remove the fallback.

[StefanKarpinski]

We should do this sooner rather than later and tear the bandaid off.

[JeffBezanson]

If we make convert the default constructor for bitstypes, a minor issue is which module's convert to use. Specifically Core has a separate convert function from Base. However since we no longer replace modules during bootstrap (which is quite nice) it might work to do const convert = Core.convert in Base, merging the functions.

[JeffBezanson]

I started digging into this, and it is a bit tricky. So far the change that seems to be the easiest and work the best is to restrict the existing definition to subtypes of Number:

(::Type{T}){T<:Number}(arg) = convert(T, arg)::T

There is a bit of precedent for defining certain things for Number, e.g. promotion behavior. I imagine this will drastically cut down the number of definitions that will need to be added to adjust to this change.

[JeffBezanson]

Here's some more detail. @vtjnash & I are not sure the bitstype approach makes sense. There are lots of numeric types defined as structs, so it won't buy us much backwards compatibility. It also means definitions like the following exist in Core:

Int(x) = convert(Int, x)

Since that's in Core, it calls Core.convert. Therefore Core.convert needs to be extended to make these definitions really work. Core.Inference would need to do that first, but Base would later do the same thing, overwriting a really large number of convert definitions.

[StefanKarpinski]

We had talked about merging Core.convert and Base.convert – is that not feasible? If so, why?

[JeffBezanson]

See above. There is also Core.Inference.convert, and it wants to add many of the same definitions that Base does.

I also don't know if it makes sense to tie this to bitstypes. Ever since immutable, new number types tend to be defined as immutables with one field, so it's not so useful.

[tkelman]

Could we maybe make a trait to determine when this happens?

[tkelman]

Getting too late in the process to make more large breaking changes, but worth revisiting in the future to see if there's a better option for resolving the confusing behavior.

[ajkeller34]

To chime in regarding a case where T(1) is not the same as convert(T, 1), this is currently the case in Unitful, which is awaiting incorporation into METADATA (it's like a souped-up SIUnits.jl). In discussing how units/quantities should play with ordinary numbers, several people raised concerns about using convert to tack on units or remove them. I reached that conclusion as well, and now convert(typeof(1m), 1) (or the opposite) throws an error.

However, using T(1) allows me to get the additive group generator for either a Real or a Quantity type: (typeof(1m))(1) == 1m, since it is calling a constructor. All Quantity types have just one field, representing the underlying number out in front of the unit. Note that one(typeof(1m)) is of course just 1 without units, since one is supposed to give a multiplicative identity, so I do need some way to be able to "add one" to quantities.

[JeffBezanson]

@vtjnash and I had a good discussion about this recently, and we decided that we might have simply gotten this backwards. The idea is that essentially every type has a constructor, with possibly varying semantics, but convert has more restrictions (it needs to be "safe" in some sense). Therefore new types should feel free to define whatever constructors they want, and convert should be defined separately to pick the "safe" subset of those constructors. This could include definitions like:

convert(::Type{T}, x::Number) where {T<:Number} = T(x)

The current definition (::Type{T})(arg) where {T} = convert(T, arg)::T is problematic in that it does not read as a true statement: it's not the case that any type can be constructed by calling convert, because convert might not be defined (since convert is more restrictive, it's expected to work only in a small subset of cases). In contrast, the above definition of convert for Number is much more plausible: you probably can convert to a type of Number by calling that type's constructor, since such a constructor is at least likely to exist.

I think a major reason we implemented the current approach is that we have tons of convert methods, and we didn't want to go through and change them all to constructors, but we probably should have.

[StefanKarpinski]

Doesn't the idea that convert is a subset of constructors match the arrangement of T(x) falling back to convert(T, x)? That's effectively saying "if there's a conversion, then you can safely use that as a constructor". I feel like I'm missing something here. The biggest issue, imo, is that there's still a lot of lack of clarity around whether one should implement constructor methods or convert methods. When I'm defining new types, I'm unsure quite often, and if I'm unsure...

[JeffBezanson]

That's one of the major reasons we did it this way --- convert satisfies the contract of construct, but not the other way around. But having that definition causes other problems, since all we can do is blindly guess that a type without a constructor might have convert defined, and try to call it. That leads to the fallback constructor showing up in method lists unexpectedly, and tooling keeps having to special-case it. It also leads to confusing behaviors like the one in this issue.

[JeffreySarnoff]

I run into this with every new package I sketch. My approach now is to define the type, some Base.convert pairings convert(::Type{NewType}, x::BaseType); covert(::Type{BaseType}, x::NewType (or more general versions using e.g. where T<:Real) and then define constructors as NewType(x::BaseType) = convert(NewType, x). If this comports with the contract, then perhaps it should be documented as best practice (or better still document that defining Base.convert(NewType, x::KnownType) will autogenerate the constructor NewType(x::KnownType) should that be used). In the event that I have this all backwards, please read from here to top.