It's too easy to accidentally sidestep our convert(::Type{T}, ::T) no-op fallback

[jrevels]

The following ambiguity error seems wonky to me:

julia> type Foo{T<:Real}
           x::T
       end

# should result in the same behavior as the default inner constructor
julia> Base.convert{T}(::Type{Foo{T}}, f::Foo) = Foo{T}(T(f.x))

# have to define this, since the above method is more specific than Base's default convert no-op
julia> Base.convert{T}(::Type{Foo{T}}, f::Foo{T}) = f

julia> f = Foo{Int}(1)
Foo{Int64}(1)

julia> convert(Foo{Int}, f)
ERROR: MethodError: convert(::Type{Foo{Int64}}, ::Foo{Int64}) is ambiguous. Candidates:
  convert{T}(::Type{Foo{T}}, f::Foo{T}) at REPL[2]:1
  convert{T}(::Type{Foo{T}}, f::Foo) at REPL[1]:1
 in eval(::Module, ::Any) at ./boot.jl:234
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46

Additionally, this kind of behavior can result in silent copies (or worse) whenever implicit conversions occur:

julia> type Baz{T<:Real}
           x::Vector{T}
       end

# Imagine I had some convert method in this case which did something
# relatively expensive (e.g. make a copy). Obviously, this specific implementation 
# is dumb, but it's just an example.
julia> Base.convert{T}(::Type{Baz{T}}, b::Baz) = Baz{T}(convert(Vector{T}, copy(b.x)))

julia> Base.convert{T}(::Type{Baz{T}}, b::Baz{T}) = b

julia> type Wrap{B}
           b::B
       end

julia> b = Baz([1,2,3])
Baz{Int64}([1,2,3])

# implicit conversion caused a copy, such that b !== w.b
julia> w = Wrap(b)
Wrap{Baz{Int64}}(Baz{Int64}([1,2,3]))

julia> w.b.x[1] = 100
100

julia> w
Wrap{Baz{Int64}}(Baz{Int64}([100,2,3]))

julia> b
Baz{Int64}([1,2,3])

Note that the above examples don't have this behavior if the T parameter isn't restricted to T<:Real in the type definition.

Now, these examples were silly, but they show that it's currently very easy to screw this up and get weird behavior and performance problems as a result. convert unexpectedly copying data is a problem that I've seen in real code - it showed up in our own Tuple code a while ago, and I've just spent a couple of hours chasing a bug this behavior caused in a prototype package I'm working on.

I'd argue that the root of this problem is that our current no-op convert fallback (convert{T}(::Type{T}, x::T) = x) is too easy to accidentally sidestep. To fix this, I propose that all new type definitions also add the following method to Base.convert:

Base.convert{T<:MyType}(::Type{T}, x::T) = x

It still wouldn't help in messier cases like #11767, but AFAIK, it'd at least be more specific than our current fallback.

[JeffBezanson]

I would say this is a dup of #6383. As you observe, if the T<:Real restriction is taken into account the problem basically goes away.

[jrevels]

Do you think the action item here (automatically defining Base.convert{T<:MyType}(::Type{T}, x::T) = x as part of the definition of MyType) is reasonable, or should we wait on a future patch to dispatch fix this (i.e. by fixing #6383)?

If it's the former, I can try taking a stab at it when I get some time. If it's the latter, this issue can closed in favor of #6383.

[KristofferC]

As another data point, this was the problem here: eschnett/SIMD.jl#12 (solved by eschnett/SIMD.jl#6). If an experience julia coder like Eric can make this mistake, many others surely do.

[JeffBezanson]

I think we should fix #6383, and not add an extra convert definition for every type.

[jrevels]

Actually, after thinking about it a bit more, I believe this issue might be slightly different than #6383. Even after #6383 is fixed, the following behavior would remain the same, yes?

julia> type Foo{T}
           x::T
       end

julia> function Base.convert{T}(::Type{Foo{T}}, f::Foo)
           println("made new instance")
           return Foo{T}(T(f.x))
       end

julia> convert(Foo{Int}, Foo{Int}(1))
made new instance
Foo{Int64}(made new instance
1)

In this issue, I'm arguing that the above behavior should change; that convert(Foo{Int}, Foo{Int}(1)) should still be a no-op, and not dispatch to a new convert method, because a more specific no-op convertmethod should already exist by default.

Though maybe I'm misunderstanding, and fixing #6383 will result in the no-op behavior I'm arguing for. If that's the case, feel free to close this.

[JeffBezanson]

There is no way to guarantee that converting to the same type is a no-op, or even that converting to a certain type actually returns something of the requested type (though that could potentially be provided by some kind of whole-function type declaration).

[jrevels]

There is no way to guarantee

Nor do I suggest that a guarantee is required here. Rather, I'm suggesting that the current no-op convert method's lack of specificity makes it easier than it should be to accidentally introduce performance (and even semantic) bugs.

Many users who overload convert aren't likely to realize that they should also define Base.convert{T<:MyType}(::Type{T}, x::T) = x if they want things like setindex! and setfield! to not create new instances of their type on every call. Why not simply define it for them in the first place?

[TotalVerb]

There are a lot of types (in particular, one per generic function), so that would create a lot of methods.

[vtjnash]

as the discussion above noted, close as dup of #6383