Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

incorrect dispatch with invoke on methods with default arguments #9938

Closed
skariel opened this issue Jan 27, 2015 · 5 comments
Closed

incorrect dispatch with invoke on methods with default arguments #9938

skariel opened this issue Jan 27, 2015 · 5 comments
Labels
breaking This change will break code needs decision A decision on this change is needed

Comments

@skariel
Copy link
Contributor

skariel commented Jan 27, 2015

see image below, this works correctly if no keyword arguments are used. This is causing the current test failures of #9575

screenshot from 2015-01-27 14 53 57

@simonster
Copy link
Member

This makes some sort of sense based on the implementation of default arguments. f(i::Number, o::Int64=1) = println("Number") is eventually lowered to:

f(i::Number, o::Int64) = println("Number")
f(i::Number) = f(i, 1)

which is why f above has 4 methods, not 2. So invoke calls f(::Number), but then f(::Number) dispatches to f(::Int64, ::Int64) instead of f(::Number, ::Int64). I think there has been some discussion of this in another issue but I can't find it. We could potentially lower to:

f(i::Number, o::Int64) = println("Number")
f(i::Number) = invoke(f, (Number, Int64), i)

although we'd need #9642 for this to perform well, and it would be a breaking change, since a different method would be called for:

f(i::Int64, o::Int64) = println("Int64")
f(i::Number, o::Int64=1) = println("Number")
f(1)

For #9575 it seems like the tests that use invoke could be changed to pass the default arguments so that the appropriate method gets dispatched.

@vtjnash vtjnash added the bug Indicates an unexpected problem or unintended behavior label Jan 27, 2015
@vtjnash
Copy link
Member

vtjnash commented Jan 27, 2015

i believe @simonster's solution is correct. might need to search back through the issues to make sure there wasn't specific concerns with it. i think the previous concern was that it would change the behavior of:

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

which is valid under the current rules (where f(1) would return 1)

@vtjnash vtjnash added bug Indicates an unexpected problem or unintended behavior needs decision A decision on this change is needed breaking This change will break code and removed bug Indicates an unexpected problem or unintended behavior labels Jan 27, 2015
@skariel
Copy link
Contributor Author

skariel commented Jan 27, 2015

@simonster good point, I changed the test it works now.

@JeffBezanson
Copy link
Member

More or less a duplicate of #7357. In that issue the solution of lowering default arguments to use invoke is mentioned.

@JeffBezanson
Copy link
Member

Closing as dup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests

4 participants