Julia: 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.

[mdcfrancis]

@JeffBezanson

currently f{T}(x::T = 0) = T creates an instance f{Int64}(x::Int64 = 0 ) = Int64, well that would be my expectation since the type of T can only be inferred from the default value.

[StefanKarpinski]

Corrected: people seem pretty unanimous on autoconversion for keyword arguments.

For positional arguments, calling a different method than the one that actually provides the default seems like it would almost always be surprising and kind of wrong. If you actually want that behavior, it seems reasonable to explicitly write out the methods.

For f{T}(x::T = 0) = T, could you lower it as something like this:

f{T}(x::T) = T
f() = convert(TypeVar(:T), 0)

with the appropriate methods to convert to typevars supplied by default. Or does using a typevar like that make no sense?

[JeffBezanson]

You can "extensionalize" typevars by converting e.g T<:X to X. We could also do something like requiring that the type of an optional argument only depend on previous arguments.
However I don't think anything along these lines will be really satisfying, because this is at odds with how arguments normally work. An argument pushes information to an argument slot, not the other way around.

[nalimilan]

I'm not sure conversion is really the most prominent problem here. The fact that you can end up calling a different, more specialized method if it exists is even more surprising. Automatic conversion doesn't change that. It would probably compound the problem, since you would convert the argument in reference to one method, and still call a different one.

It seems to me nobody is really arguing that the "ability" to call a different method is really useful in practice. I understand how it's appealing in terms of simplicity of implementation (and thus of logic to explain it), but that simplicity doesn't seem to be obvious to most users when they actually write those methods.

[JeffBezanson]

I suppose we could handle this with renaming (similar to how keyword args work):

f(x, y=0) = 2

becomes

_hidden_f_(x, y) = 2
f(x) = _hidden_f_(x, 0)
f(x, y) = _hidden_f_(x, y)

Which is basically the same as using invoke, except simpler.

[mdcfrancis]

@nalimilan - are you suggesting that the following would no longer work ?

f( x, y::String = "" ) = 2
f( x, y::Int64 = 1 ) = 1 
f( x, y = 0 ) = 0

f( 1, "hello" ) == 2 
f( 1, 2 ) == 1
f( 1, 1.23) == 0 

[toivoh]

+1 to _hidden_f_. This would make it a lot simpler to reason about functions with default arguments and avoid the leaky abstraction that we have right now.

[nalimilan]

@mdcfrancis Not at all. What I think shouldn't be possible is this:

# Original method
julia> f(x::Int, y::Real=1) = 1
f (generic function with 2 methods)

# Somebody adds a more specialized definition somwhere else
julia> f(x::Int, y::Int) = 2
f (generic function with 3 methods)

julia> f(1)
2

[mdcfrancis]

@nalimilan if one adds

f( x::Int, y::Int = 1 ) = 2 

Is that an error ? I'd assume it should be

[JeffBezanson]

I guess that would just be overwriting some of the definitions, which we sometimes warn about.

[mdcfrancis]

Chuckle - I like the 'sometimes warn' :smile:

[JeffBezanson]

Well, we warn if it's overwritten from a different module, so it's not just random, but I didn't want to go into details :)

[mdcfrancis]

It would be nice to be able to write something like

f( x::Int, final y::Number = 2 ) 

or more generally

f( final x::Number ) 

turning the warning into an error, essentially stating that the function is as specialized as it can be in this path of the type tree.

The following would still be valid as they reference different parts of the tree

f( final x::Number )  = 1
f( final x::String ) = 2

The default then for an optional argument would be a final type. Which would remove the surprising behavior.

Is this something which has been considered ?

[nalimilan]

@mdcfrancis That shouldn't be needed in general: if you claim to support a given abstract type, it doesn't mean there can't be a more efficient method for a subtype. That's more an issue with coordination and documentation of the APIs. If blocking specialized methods from being called was possible, Julia would soon become a war between package authors. Let's simply find a solution for optional positional arguments.

[JeffBezanson]

Yes, it's an interesting idea but I think it can be separated from the issue here. So far renaming seems to be the way to go, if one can tolerate a bit of confusion on the reflection side.

[mdcfrancis]

@nalimilan War is a bit strong :smile: - there should be nothing wrong with the following

f( x::MyType, final x::Number ) 
and
f( x::YourType, final x::Number ) 

@JeffBezanson yes a separate issue which could allow us to turn warnings into errors.

[StefanKarpinski]

@nalimilan's analysis of this seems spot on and renaming does seem like the way to go.

[StefanKarpinski]

Resolved: The solution to _this_ issue is the hidden method name. Whether to do autoconversion or not is a separate issue, which applies to both optional positional and keyword arguments.

[StefanKarpinski]

This is technically a breaking change but is unlikely to break any sane code.

[JeffBezanson]

This is pretty easy to fix unless we care about hiding the extra function in stack traces and the debugger. Situation will be similar to some of the issues around keyword arguments. I'm also a little concerned that the number of extra functions generated will be excessive, but I'm willing to try it and see.

[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 https://github.com/JuliaLang/julia/issues/7357#issuecomment-140404331

[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: https://github.com/JuliaLang/julia/blob/d989d3e79b0712e0ab2ed74cd7c3929c39ac7911/base/strings/types.jl#L29-L33
(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.