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

hasmethod returns wrong answer when using positional and keyword arguments #39613

Closed
nielsls opened this issue Feb 11, 2021 · 2 comments
Closed

Comments

@nielsls
Copy link

nielsls commented Feb 11, 2021

There have been some recent fuss about hasmethod (#9498, #30808) - but I believe this to be a new issue.

julia> f(a=1;b=2) = 42
f (generic function with 2 methods)

julia> methods(f)
# 2 methods for generic function "f":
[1] f() in Main at REPL[1]:1
[2] f(a; b) in Main at REPL[1]:1

julia> hasmethod(f, (), (:b,))  # should return true !?
false

even though the following of course works

julia> f(;b=2)
42

The method list returned by methods may be "correct" - but hasmethod(f, (), (:b,)) returns false, since the implementation of hasmethod will:

  1. match the empty args tuple () against the first method in the method list,
  2. conclude the first method does not take kwargs and thus return false

To be completely fair, hasmethod actually does exactly what its docstring says - however it seems very strange if the above would be the intended behaviour of hasmethod.

@goretkin
Copy link
Contributor

goretkin commented Feb 11, 2021

(just a note: applicable doesn't support kwargs yet (#36611) but the behavior between the two should match.)

To me methods is giving an unexpected answer:

julia> ff(a, b=2; c) = nothing
ff (generic function with 2 methods)

julia> methods(ff)
# 2 methods for generic function "ff":
[1] ff(a) in Main at REPL[16]:1
[2] ff(a, b; c) in Main at REPL[16]:1

There is no method for ff with just one positional argument. All methods require a c keyword argument.

Similarly

julia> h(a, b=2) = "a: $a b: $b"
h (generic function with 2 methods)

julia> methods(h)
# 2 methods for generic function "h":
[1] h(a) in Main at REPL[5]:1
[2] h(a, b) in Main at REPL[5]:1

julia> g(a, b=2; c=3) = "a: $a b: $b c: $c"
g (generic function with 2 methods)

julia> methods(g)
# 2 methods for generic function "g":
[1] g(a) in Main at REPL[7]:1
[2] g(a, b; c) in Main at REPL[7]:1

methods of g are just like those of h, except they take an optional c keyword argument.

There's a bunch of logic here to craft the right function call to Core.kwfunc version of a function:

function gen_call_with_extracted_types(__module__, fcn, ex0, kws=Expr[])

I wonder if that should be leveraged by hasmethod and applicable, thereby sidestepping methods altogether.

Regarding the possible issue with methods:
When optional positional arguments, and keyword argument method definitions get lowered to multiple method definitions (see below). It seems like methods is trying to undo some of that lowering, but it is doing so unsuccessfully.

julia> struct Foo{T} # just for help in following along with the multiple method definitions
       _
       end

julia> Meta.@lower ff(a::Foo{:A}, b::Foo{:B}=2; kwarg_named_c) = nothing
:($(Expr(:thunk, CodeInfo(
    @ none within `top-level scope'
1 ─       $(Expr(:thunk, CodeInfo(
    @ none within `top-level scope'
1 ─     return $(Expr(:method, :ff))
)))
$(Expr(:thunk, CodeInfo(
    @ none within `top-level scope'
1 ─     return $(Expr(:method, Symbol("#ff#10")))
)))
$(Expr(:method, :ff))
$(Expr(:method, Symbol("#ff#10")))
│   %5  = Core.typeof(var"#ff#10")
│   %6  = Core.Typeof(ff)
│   %7  = Core.apply_type(Foo, :A)
│   %8  = Core.apply_type(Foo, :B)
│   %9  = Core.svec(%5, Core.Any, %6, %7, %8)
│   %10 = Core.svec()
│   %11 = Core.svec(%9, %10, $(QuoteNode(:(#= REPL[34]:1 =#))))
$(Expr(:method, Symbol("#ff#10"), :(%11), CodeInfo(
    @ REPL[34]:1 within `none'
1 ─     $(Expr(:meta, :nkw, 1))
└──     return nothing
)))
$(Expr(:method, :ff))
│   %14 = Core.Typeof(ff)
│   %15 = Core.apply_type(Foo, :A)
│   %16 = Core.svec(%14, %15)
│   %17 = Core.svec()
│   %18 = Core.svec(%16, %17, $(QuoteNode(:(#= REPL[34]:1 =#))))
$(Expr(:method, :ff, :(%18), CodeInfo(
    @ REPL[34]:1 within `none'
1 ─ %1 = (#self#)(a, 2)
└──      return %1
)))
$(Expr(:method, :ff))
│   %21 = Core.Typeof(ff)
│   %22 = Core.apply_type(Foo, :A)
│   %23 = Core.apply_type(Foo, :B)
│   %24 = Core.svec(%21, %22, %23)
│   %25 = Core.svec()
│   %26 = Core.svec(%24, %25, $(QuoteNode(:(#= REPL[34]:1 =#))))
$(Expr(:method, :ff, :(%26), CodeInfo(
    @ REPL[34]:1 within `none'
1 ─ %1 = Core.UndefKeywordError(:kwarg_named_c)
│        kwarg_named_c = Core.throw(%1)
│   %3 = var"#ff#10"(kwarg_named_c, #self#, a, b)
└──      return %3
)))
$(Expr(:method, :ff))
│   %29 = Core.Typeof(ff)
│   %30 = Core.kwftype(%29)
│   %31 = Core.Typeof(ff)
│   %32 = Core.apply_type(Foo, :A)
│   %33 = Core.svec(%30, Core.Any, %31, %32)
│   %34 = Core.svec()
│   %35 = Core.svec(%33, %34, $(QuoteNode(:(#= REPL[34]:1 =#))))
$(Expr(:method, :ff, :(%35), CodeInfo(
    @ REPL[34]:1 within `none'
1 ─ %1 = (#s25)(@_2, @_3, a, 2)
└──      return %1
)))
$(Expr(:method, :ff))
│   %38 = Core.Typeof(ff)
│   %39 = Core.kwftype(%38)
│   %40 = Core.Typeof(ff)
│   %41 = Core.apply_type(Foo, :A)
│   %42 = Core.apply_type(Foo, :B)
│   %43 = Core.svec(%39, Core.Any, %40, %41, %42)
│   %44 = Core.svec()
│   %45 = Core.svec(%43, %44, $(QuoteNode(:(#= REPL[34]:1 =#))))
$(Expr(:method, :ff, :(%45), CodeInfo(
    @ REPL[34]:1 within `none'
1 ─ %1  = Base.haskey(@_2, :kwarg_named_c)
└──       goto #3 if not %1
2 ─       @_7 = Base.getindex(@_2, :kwarg_named_c)
└──       goto #4
3 ─ %5  = Core.UndefKeywordError(:kwarg_named_c)
└──       @_7 = Core.throw(%5)
4 ┄       kwarg_named_c = @_7
│   %8  = Core.tuple(:kwarg_named_c)
│   %9  = Core.apply_type(Core.NamedTuple, %8)
│   %10 = Base.structdiff(@_2, %9)
│   %11 = Base.pairs(%10)
│   %12 = Base.isempty(%11)
└──       goto #6 if not %12
5 ─       goto #7
6 ─       Base.kwerr(@_2, @_3, a, b)
7 ┄ %16 = var"#ff#10"(kwarg_named_c, @_3, a, b)
└──       return %16
)))
│   %47 = ff
│   %48 = Core.ifelse(false, false, %47)
└──       return %48
))))

@goretkin
Copy link
Contributor

goretkin commented Feb 11, 2021

A comment on the discrepancy between methods and ___ : #37936 (comment)

Edit: actually I think that that has more to do with splatting (...).

vtjnash added a commit that referenced this issue Feb 9, 2023
Since these tfuncs are pretty simple for inference, but hard for anyone
else, we define these tfuncs here rather than letting package authors
re-implement it poorly to create duplicates like static_hasmethod.

Fix #39613 while we are here
vtjnash added a commit that referenced this issue Feb 10, 2023
Since these tfuncs are pretty simple for inference, but hard for anyone
else, we define these tfuncs here rather than letting package authors
re-implement it poorly to create duplicates like static_hasmethod.

Fix #39613 while we are here
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants