dispatch behavior of optional arguments

[nalimilan]

Since empty arrays [] are of type Vector{None}, it will sometimes happen that people do mistakes like this:

julia> function f1(x::Vector{Int} = [])
           true
       end
f1 (generic function with 3 methods)

julia> function f2(; x::Vector{Int} = [])
           true
       end
f2 (generic function with 1 method)

julia> f1()
ERROR: no method f1(Array{None,1})
 in f1 at none:2

julia> f2()
ERROR: no method __f2#3__(Array{None,1})

At least, in the second case, the function name is a bit weird. In the first case, the line number doesn't correspond to anything interesting. If it's possible, it would be great to print a more explicit message, like "default argument value is of an incorrect type".

[JeffBezanson]

Unfortunately this would be extraordinarily difficult to do, odd as that may sound. Default arguments are exactly equivalent to arguments; the system makes no distinction, so treating default arguments specially is not natural. A check could be done at definition time, but that only works for constant types. For example f{T}(x::T, y::Vector{T} = Int[]) works for some arguments but not others.

[vtjnash]

it can certainly lead to some odd behaviors:

julia> f(::Any) = 1
f (generic function with 1 method)

julia> f(x::Int="HI") = 2
f (generic function with 3 methods)

julia> f()
1

[JeffBezanson]

Yes, that's a nice piece of obfuscated code :)

I think it's ultimately a good thing that default arguments are not a special feature, and just reduce to dispatch. You don't need to learn extra rules about how default arguments behave. In this example, it doesn't matter which definition contains ="HI"; everything works the same either way. Giving an error to force you to put the ="HI" on the first definition is fiddly and artificial.

[nalimilan]

I see, it's indeed good that default arguments are handled like others. Though I disagree with your statement:

In this example, it doesn't matter which definition contains ="HI"; everything works the same either way. Giving an error to force you to put the ="HI" on the first definition is fiddly and artificial.

It may be artificial, but it would really make sense (if easy to implement): there's no point in defining a default argument in a method which only applies to another one (apart to obfuscate code). In general, many checks are artificial: why doesn't + broadcast by default, if not just to make things more predictable? So it would already be nice if you can make basic checks at definition time, to catch a few common mistakes.

There are still the two issues that in the first case a line number appears and does not correspond to anything; and that in the second case the function name is weird.

[StefanKarpinski]

I agree that it's good that default arguments are just a shorthand for defining the appropriate additional methods. But it's pretty hard to view this example as anything but problematic. Currently, we have

f(x::Int="HI") = 2

meaning

f(x::Int) = 2
f() = f("HI")

How about making it mean this instead:

f(x::Int) = 2
f() = f(convert(Int,"HI"))

It's not a compile-time error, but at least it ensures that either the expected method gets called or you get a runtime conversion error, which seems much better than the current behavior. If the default is already of the expected type, then the conversion compiles down to a no-op. If we include type-checking and linting into base, enabled via julia -w or something like that, then people will automatically get a warning in the case that no conversion exists for the given default argument type and value.

[JeffBezanson]

I can imagine a case like

# defined in a library
foo(x::Int, y::Int) = ...

You might want to extend this function outside the library as follows

foo(x::Int, y::Float64=1) = ...

Note that it's perfectly valid to write

foo(x::Int, y::Float64) = ...
foo(x::Int) = foo(x, 1)

The default argument form is just a rewrite of that, so why should it be disallowed?

The convert idea is pretty good, but I'm afraid it will create confusion about whether conversion is involved in finding matching definitions, which right now it never is. typeassert would be better, but both that and convert fall apart a bit with abstract types:

foo(x::Real = 1) = ...

If foo(x::Int) is defined later, it will be called instead of this definition. To fully address this, we could use invoke (which also checks for type mismatches of course). But I can't shake the feeling that it's odd to stuff more behaviors into this really simple feature.

[nalimilan]

The default argument form is just a rewrite of that, so why should it be disallowed?

Because approximately nobody is going to understand how such a declaration is going to work? :-)

But I can't shake the feeling that it's odd to stuff more behaviors into this really simple feature.

This is not "stuffing more behaviors", on the contrary this is limiting the complexity of the feature to make it more "simple".

I don't think calling convert is a good idea, it makes things even more complex and might hide mistakes (like writing 1 instead of 1.0). People should write default values in the correct type directly. Just asserting the type should be enough.

[StefanKarpinski]

I don't think the convert business is as bad as the current behavior. If we can ensure that convert(T,x) where isa(x,T) is just returns x for both concrete and abstract types, then it should be fine. Problems can then only arise if you supply a default argument of the wrong type and the conversion doesn't exist. That seems perfectly reasonable to me. Perhaps it could explicitly expand to something like this:

f(x::Int) = 2
f() = let x = "HI"
  isa(x,Int) ? x : convert(Int,x)
end

[StefanKarpinski]

I would be ok with the type assert as well but it does seem a little finicky.

[JeffBezanson]

The issue with abstract types is that a later definition can be more specific, and "intercept" your default argument. There are really 4 options here:

1. Default values must be of the corresponding argument's type.
2. Default values are implicitly converted to the argument's type.
3. The definition containing the default values is always called when trailing arguments are missing, via invoke.
4. When an argument is left out, the default value is supplied instead, and method dispatch happens as normal. (What we do now.)

This is one of those "easy" vs. "simple" things. I'm not sure which of these is easiest, but I think (4) is objectively simplest.

I believe the current rewrite that default arguments do is obvious, and is what everybody would write in the absence of the feature. The alternatives involve generating more code, forcing you to avoid the feature if you don't want the extra code. More stuff happening behind your back is not simpler.

[JeffBezanson]

By the way, I think my (3) above is highly defensible. However I don't like it that much. If you write f(x::Real = 1), and somebody later implements a better version specialized for Int, it's great that the specialized version is called for f() in the future, with no further changes.

[nalimilan]

Yeah, that's why simply asserting sounds like a better solution to me.

[JeffBezanson]

The good thing about asserting is that it's "what we do now but with more errors". So there are no silent behavior changes.

[vtjnash]

The bad part about simply asserting, is that it is "what we do now but with more errors". And it still doesn't necessarily call the function that it "looks" connected to.

I'm actually in favor of the current behavior, and just fixing the backtraces for inline functions, so it is more clear what is happening.

[StefanKarpinski]

Nullables make a strong case for implicit conversion since you may well want to do this sort of thing:

f(x::Int, y::Nullable{Int}=nothing) = ...

and be able to call the function as f(1, 2) instead of having to write f(1, Nullable(2)).

[stevengj]

+1 for conversion+assertion. It makes sense to me that f(x, y::Int = z) = ... should be equivalent to

function f(x)
   y::Int = z
   return f(x, y)
end
f(x, y::Int) = ...

which calls convert(Int, z)::Int.

Note that we should also do the same conversion+assertion for keyword arguments, for the same reasons. Currently, foo(x; y::Int=3.7) = x + y gives a rather obscure error if you call foo(3).

[mdcfrancis]

I find it surprising that a default value is not currently coerced to the type of the argument it is bound to. The idea that a person can define a more 'refined' version of the method later and dispatch sends me off to the new version is quite unexpected. If the user wants this behavior they could specify a type further up the tree.

f(x,y::Number = 1 ) = ...  # Will dispatch based y 
f(x,y::Int64 = 1 ) = ...       # Will always call this method if y is omitted 

I don't think that helps with the type instability of Nullable though.

f(x,y::Number = "foo" ) 

Should be an error

[StefanKarpinski]

It seems pretty unanimous at this point that conversion to the declared argument type is the right thing to do for both optional positional arguments and keyword arguments, although @JeffBezanson has been conspicuously silent on the matter.

[JeffBezanson]

I guess my main question is how to lower f{T}(x::T = 0) = T.

I wouldn't call this unanimous --- both @nalimilan and @vtjnash were not in favor of converting (earlier in this thread).

[JeffBezanson]

There is an asymmetry: we dispatch on the types of positional arguments, but not on the types of keyword arguments. That makes it much more natural to convert keyword arguments than positional arguments.

[nalimilan]

To clarify: I'm all for automatic conversion of keyword arguments, but I'm more reserved about optional positional arguments.

Automatic conversion to Nullable so that you can call f(x::Int, y::Nullable{Int} = nothing) as f(1, 2) is terribly appealing to me, but the inconsistency with mandatory positional arguments is disturbing. This would effectively mean that we have three different sorts of arguments, each with a specific behaviour. Is there any chance keyword arguments could be made as fast as positional ones instead, so that the former would be used when a Nullable argument is needed?

[quinnj]

I also can't think of time when I've really thought I needed auto-conversion for positional arguments. It's always popped up in cases of keyword arguments only.

[StefanKarpinski]

The total number of methods remains the same, so it's hard to imagine it being that bad.

[JeffBezanson]

There is one extra method (and function) per definition. See #7357 (comment)

[StefanKarpinski]

Is this realistically still happening in 0.6?

[StefanKarpinski]

@JeffBezanson did some experiments with this and found that the rename approach is too expensive since it increases the number of methods generated by a very large amount (+10%). There may be other ways to do it (e.g. using invoke), but this isn't happening in 0.6.

[JeffBezanson]

My current thinking is that the simplicity of the existing approach is a good thing, and we should just keep it.

[vtjnash]

Actually, I'm offering to play around with some of the changes to make this possible, and delay deciding on which way to go with this. So the plan is to make it possible to pass a Method to invoke, and then explore whether using invoke to the specific Method is better (or not) compared to the current implementation of doing full re-dispatch.

[JeffBezanson]

True, we haven't tried the invoke approach to this, and I think that might be reasonable.

[rfourquet]

Just wanted to support the current approach, as most people here expressed a preference for an alternative. I understand it can be perceived as surprising, but IMHO this should just request a warning in the docs. I can't help but finding the current behavior as correct:

"""
    f(x::Number=0)

Computes f(x).
"""
f(x::Number=0) = generic_slow_algorithm(x)
f(x::Int) = fast_algorithm(x)

The first definition is the one specifying the API, the most general one (hence it makes some sense to specify the default there). If the second one is defined, now or later, possibly in another file, it means generally (always?) that this specialization should be better for Int, so I don't see why it shouldn't be called for the default argument. Doing otherwise is even a lie to the user: calling f(0) will be fast, but calling f() will be slow, when the doc and the signature pretend they should be equivalent.

I still need to see a use case for the proposed change (with invoke or _hidden_f), and when such a use case happens, it's simple enough to enable manually. On the other hand, it's not difficult to find use cases for the current behavior, e.g.

rand!(A::AbstractArray, rng::AbstractRNG=GLOBAL_RNG) = ...
rand!(A::Array{Float64}, rng::MersenneTwister) = ...

# alternative with proposed change
rand!(A::AbstractArray, rng::AbstractRNG) = ...
# NOTE: don't set GLOBAL_RNG as default in the above method, wouldn't work well for Array{Float64} :(
rand!(A::AbstractArray) = rand!(A, GLOBAL_RNG)
rand!(A::Array{Float64}, rng::MersenneTwister) = ...

The worst part is if the improved method (the last one) is added later in time, then one must review all existing methods with defaults to check that the correct implementation is selected.

To conclude, in the case presented above here for example, if the "somebody" refers to the author of another package "somewhere else", then this somebody is commiting type piracy, so according to current Julia "good practices", this should not happen.

[StefanKarpinski]

It's arguable that in cases where the default value is of the given argument type the current behavior is acceptable, but in cases where the default is not of the given type are really clearly wrong.

[stevengj]

Yes, foo(x::Number=0) seems fine to me, even it it ends up dispatching to foo(x::Int), but foo(x::Number="string") seems like a coding error even if foo(::String) exists.

[StefanKarpinski]

The foo(x::Number="string") example is definitely bad, but I'm actually more worried about subtler cases like foo(x::UInt=0) which will not do what it seems like it should at all, but on the face of it doesn't look completely wrong.

[vtjnash]

For a motivating example of default argument behavior, consider:

julia/base/strings/types.jl

Lines 29 to 33 in d989d3e

	SubString(s::T, i::Int, j::Int) where {T<:AbstractString} = SubString{T}(s, i, j)
	SubString(s::SubString, i::Int, j::Int) = SubString(s.string, s.offset+i, s.offset+j)
	SubString(s::AbstractString, i::Integer, j::Integer) = SubString(s, Int(i), Int(j))
	SubString(s::AbstractString, i::Integer) = SubString(s, i, endof(s))
	SubString{T}(s::T) where {T<:AbstractString} = SubString(s, 1, endof(s))

(which is missing some coverage of correct methods). I would be nicer to be able to write those with default keywords (and more complete coverage of the set of possible method dispatch signatures), and expect that dispatch will work out as desired:

 SubString{T}(s::AbstractString, i::Int, j ::Int) where {T} = SubString{T}(s, i, j)
 SubString{T}(s::SubString, i::Int, j::Int) where {T} = SubString{T}(s.string, s.offset + i, s.offset + j) 
 SubString{T}(s::T, i::Int, j::Int) where {T <: SubString} = SubString{T}(s, i, j) 
 SubString{T}(s::AbstractString, i::Integer=start(s), j::Integer=endof(s)) where {T} = SubString{T}(s, Int(i), Int(j)) 

 SubString(s::AbstractString, i::Int, j::Int) = SubString{typeof(s)}(s, i, j)
 SubString(s::SubString, i::Int, j::Int) = typeof(S)(s.string, s.offset + i, s.offset + j) 
 SubString(s::AbstractString, i::Integer=start(s), j::Integer=endof(s)) = SubString(s, Int(i), Int(j)) 

[StefanKarpinski]

I suspect we should just punt on this for 1.0 unless someone (@vtjnash maybe) has some evidence that we can do something about this without a performance hit.

[JeffBezanson]

@vtjnash 's example argues for keeping the current behavior, which I'm also in favor of. "Obviously wrong" default arg values like f(x::Int = "") are maybe the main concern here, but that strikes me as more of a linting thing.

[oscardssmith]

Closing since at this point, we aren't going to change behavior.