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

Factor MethodList representation by optional arguments #44434

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Liozou
Copy link
Member

@Liozou Liozou commented Mar 3, 2022

When defining a function with both optional and keyword arguments, the representation of the resulting methods do not show the keyword arguments, except for the last one. For example, there is no foo(a; kwarg) method appearing in the following list, although calling foo(3; kwarg=6) is valid:

julia> foo(a, b=3; kwarg) = nothing
foo (generic function with 2 methods)

julia> methods(foo)
# 2 methods for generic function "foo" from Main:
 [1] foo(a)
     @ REPL[1]:1
 [2] foo(a, b; kwarg)
     @ REPL[1]:1

I suppose this comes from the actual implementation of method dispatch with keyword arguments, with the kwsorter and stuff, and there are good reasons why those methods are defined like that. Without changing any of these internals thus, this PR changes the representation of the MethodList created when calling methods(foo), in order to show a "factored method" closer to the definition:

julia> methods(foo)
# 2 methods for generic function "foo" from Main:
 [1:2] foo(a, [b]; kwarg)
     @ REPL[1]:1

This is done by converting the MethodList to the new FactoredMethodList type upon calling show, so the MethodList itself is untouched. You can still access the previous representation by looking at the actual list of methods:

julia> methods(foo).ms
[1] foo(a) @ Main REPL[1]:1
[2] foo(a, b; kwarg) @ Main REPL[1]:1

The detection of "factored method" definitions works by comparing the locations of the method definitions, as well as their argument names and types. It's not perfect since you could define methods at the same position through macros or @eval, but I don't think it's too bad since there is no loss of information anyway, all previously represented methods will still be represented (in a more compact way possibly).

This is also ported to REPL completions, which was the initial reason for this PR (see #43536):

julia> iterate(trues(6), #TAB
iterate(B::BitArray, [i::Int64]) @ Base bitarray.jl:361
iterate(A::AbstractArray, state) @ Base abstractarray.jl:1217

@Liozou
Copy link
Member Author

Liozou commented Mar 12, 2022

CI failure looks unrelated. Just a few more examples to illustrate:

Multiple optional arguments

julia> foo(a, b=12, c=""; kwarg) = nothing;

julia> methods(foo)
# 3 methods for generic function "foo" from Main:
 [1:3] foo(a, [b, c]; kwarg)
     @ REPL[1]:1

Non-consecutive factored methods

julia> bar(x, y::Integer) = 2;

julia> bar(a, b=5) = 1;

julia> bar( #TAB
bar(x, y::Integer) @ Main REPL[3]:1
bar(a, [b]) @ Main REPL[4]:1

julia> methods(bar)
# 3 methods for generic function "bar" from Main:
 [1,3] bar(a, [b])
     @ REPL[4]:1
   [2] bar(x, y::Integer)
     @ REPL[3]:1

Note that the order of methods in the MethodList is untouched compared to before.

Optional arguments and varargs

julia> qux(a, b=12, args...) = b;

julia> methods(qux)
# 2 methods for generic function "qux" from Main:
 [1:2] qux(a, [b], args...)
     @ REPL[6]:1

@vtjnash
Copy link
Member

vtjnash commented Mar 15, 2022

I suppose one of the questions for this is whether we want to be accurate or simplified (like here). The actual lowering uses kwargs, and accepts any list of them, so ... would be more precisely accurate:

julia> foo1(a) = foo(a, 3)
julia> foo1(a, b) = nothing
julia> foo1(a; kwargs...) = foo(a, 3; kwargs...)
julia> foo1(a, b; kwarg) = nothing

julia> foo(a, b=3; kwarg) = nothing
foo (generic function with 2 methods)

julia> methods(foo1)
# 2 methods for generic function "foo":
[1] foo1(a; ...) in Main at REPL[1]:1
[2] foo1(a, b; kwarg) in Main at REPL[1]:1

@Liozou
Copy link
Member Author

Liozou commented Mar 15, 2022

So the question is between hiding the internal methods created when lowering the definition of foo (the foo1 methods you show), as I'm currently doing, or exposing them, is that right?

I think I still prefer not to show those internal methods. Even though there may be an internal method for foo1(a; kwargs...) for instance, any call to foo without kwarg or with a different keyword argument will result in either a MethodError or a UndefKeywordError anyway, so from the perspective of the end-user, it's easier to forget about it. I also had this end-user in mind when thinking of REPL-completion: it's impossible to know that kwarg specifically should be proposed as a keyword argument to complete foo(a; k#TAB from the more accurate representation since any keyword argument is actually accepted for that call (and later rejected if invalid).

However, I understand that this representation may also be queried by devs who would prefer to have the most accurate representation. At the same time, I would expect them to know how to extract the desired methods "by hand" from methods(methods(foo).mt.kwsorter) if need be. So I'm still leaning towards the simplified version.

@frankwswang
Copy link

The current representation can lead to inaccurate exception info in some cases like this:

julia> f(a, b=1; c=1) = a+b+c
f (generic function with 2 methods)

julia> f(1, d=2)
ERROR: MethodError: no method matching f(::Int64, ::Int64; d=2) # Should be f(::Int64; d=2)
Closest candidates are:
  f(::Any, ::Any; c) at REPL[2]:1 got unsupported keyword argument "d"
  f(::Any) at REPL[2]:1 got unsupported keyword argument "d"
Stacktrace:
 [1] kwerr(::NamedTuple{(:d,), Tuple{Int64}}, ::Function, ::Int64, ::Int64)
   @ Base .\error.jl:163
 [2] top-level scope
   @ REPL[3]:1

@Liozou
Copy link
Member Author

Liozou commented Jul 4, 2022

Rebased on top of #45069. I incidentally expanded support of that PR for the MIME"text/html" target to avoid having too much code duplication but I can revert that part if need be. Remaining CI failures look unrelated.

I still feel that this is a prerequisite for #43536 since many expected kwarg completions will fail otherwise, so any feedback would be appreciated.
#43536 has been merged since, but I still believe that this PR nicely completes it by making completion more precise.

@HiperMaximus
Copy link

Up

@Liozou Liozou force-pushed the factoredmethodlists branch 4 times, most recently from cfa5c0d to 821dd43 Compare January 2, 2023 14:24
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

Successfully merging this pull request may close these issues.

4 participants