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

Add warn about usage of applicable and hasmethod #51260

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ronisbr
Copy link
Member

@ronisbr ronisbr commented Sep 10, 2023

This commit adds a warning about the functions applicable and hasmethod. A user reading the current documentation can think that if those functions return true, we can call the method without errors. However, it is not always the case and should be clearly stated.

Closes #51258

This commit adds a warning about the functions `applicable` and
`hasmethod`. A user reading the current documentation can think that if
those functions return `true`, we can call the method without errors.
However, it is not always the case and should be clearly stated.

Closes JuliaLang#51258
@Seelengrab Seelengrab added the docs This change adds or pertains to documentation label Sep 10, 2023
@Seelengrab
Copy link
Contributor

LGTM!

@KristofferC
Copy link
Member

KristofferC commented Sep 10, 2023

This feels a bit overly verbose to me at least. I don't really understand how you can infer from the current docstring that the function would not throw any exception. Would you assume that because sqrt(-1) throws an exception that applicable(sqrt, -1) returns false?

Maybe the original docstring could be slightly elaborated on what applicable means (that there is a method defined for the given arguments).

will provide the expected return

I don't really understand what this means.

@ronisbr
Copy link
Member Author

ronisbr commented Sep 10, 2023

This feels a bit overly verbose to me at least.

It is a docstring. IMHO, too much documentation is better than too little, which is the case.

I don't really understand how you can infer from the current docstring that the function would not throw any exception. Would you assume that because sqrt(-1) throws an exception that applicable(sqrt, -1) returns false?

One new user might assume that if applicable(sqrt, -1) returns true, then sqrt(-1) would provide 0 + 1im. It seems reasonable enough since it is what MATLAB/Octave/Mathematica/Maple does.

Please, do not assume that everyone will have a deep understanding about methods, types, and multiple dispatch. Julia is getting more popular everyday. I heard many times from newcomers that Julia feels too complicated. In fact, this usage of applicable has been around in PrettyTables.jl without problems for at least 5 years, and will suddenly break in 1.11. I think we can spare five or six lines to make it really clear that there are corner cases, can't we?

I don't really understand what this means.

From my perspective, it is the expected result that the user calling applicable might assume. For example, I would really think that applicable(round, v) == true means that round can round v to the nearest integer.

However, if it is not clear enough, do you have a suggestion?

@mcabbott
Copy link
Contributor

Re the example, while deliberate fallback methods with an error are rare (and I think discouraged), many functions will try to iterate anything, and fail:

julia> applicable(sum, nothing)
true

julia> sum(nothing)
ERROR: MethodError: no method matching iterate(::Nothing)
Stacktrace:
 [1] _foldl_impl(op::Base.BottomRF{typeof(Base.add_sum)}, init::Base._InitialValue, itr::Nothing)
   @ Base ./reduce.jl:56
...
 [5] mapreduce(f::Function, op::Function, itr::Nothing; kw::@Kwargs{})
   @ Base ./reduce.jl:307
 [6] sum(f::Function, a::Nothing; kw::@Kwargs{})
   @ Base ./reduce.jl:535
 [7] sum(a::Nothing; kw::@Kwargs{})
   @ Base ./reduce.jl:564

Perhaps this idea (accepting ::Any & calling iterate), and the sqrt(-1) idea (failure for reasons impossible to predict from the type) can be compactly summarised?

@ronisbr
Copy link
Member Author

ronisbr commented Sep 10, 2023

Perfect, your example is another important scenario why we really should describe much better what applicable is doing here for people who are not very familiar with julia. It just has a minor difference: in your scenario, we failed in iterate, while in the scenario that originated this PR, we failed in the same function: round. For someone new to julia, this behavior can be deceiving. Like, "julia is telling me that I can apply round to v, but then round is failing, saying that the type of v is not supported."

I add that fallback example because it seems somewhat common in API definitions:

https://github.com/JuliaData/Tables.jl/blob/dac28d9f233c297ab8d22ce9cab439ac3e97899d/src/Tables.jl#L169

https://github.com/JuliaData/Tables.jl/blob/dac28d9f233c297ab8d22ce9cab439ac3e97899d/src/Tables.jl#L636

can be compactly summarised?

What I proposed is the most compact text I can do today sorry :) At least I showed it to a student who had just started learning Julia, and he understood what was happening. I am all ears for suggestions on how to improve this warning.

@ronisbr ronisbr closed this Sep 10, 2023
@ronisbr ronisbr reopened this Sep 10, 2023
@Seelengrab
Copy link
Contributor

Re the example, while deliberate fallback methods with an error are rare (and I think discouraged)

I'm using that mechanism in RequiredInterfaces.jl to give good error messages when some interface is not completely implemented, which is IMO much better than a generic MethodError some place down the stack:

julia> using RequiredInterfaces

julia> abstract type MyInterface end

julia> @required MyInterface begin
           foo(::MyInterface)
       end
foo (generic function with 1 method)

julia> struct MyImplementor <: MyInterface end

julia> foo(MyImplementor())
ERROR: NotImplementedError: The called method is part of a fallback definition for the `MyInterface` interface.
Please implement `foo(::MyInterface)` for your type `T <: MyInterface`.
Stacktrace:
 [1] foo(::MyImplementor)
   @ Main ./none:0
 [2] top-level scope
   @ REPL[7]:1

This would be first I'm hearing about this being discouraged.

@brenhinkeller
Copy link
Contributor

brenhinkeller commented Sep 14, 2023

Re the example, while deliberate fallback methods with an error are rare (and I think discouraged)

This would be first I'm hearing about this being discouraged.

It's been called an antipattern in the past, IIRC https://www.oxinabox.net/2020/04/19/Julia-Antipatterns.html#notimplemented-exceptions

In any case, LGTM

@Seelengrab
Copy link
Contributor

Just because one blogpost says one particular version of this is bad, doesn't mean the entire concept is bad. A MethodError doesn't communicate "this should be implemented, but someone didn't", it merely communicates "this is not implemented", with no indication of whether or not it should be. In RequiredInterfaces.jl, the entire signature is printed, so the criticism in the blogpost doesn't apply.

If you want to discuss this, I'd be happy to take it to the issues of RequiredInterfaces.jl - sorry for highjacking the PR!

@aplavin
Copy link
Contributor

aplavin commented Sep 17, 2023

Actual usecases for applicable and hasmethod are extremely rare, and certainly not for programming beginners. Can these docs be moved to a kind of "advanced" section?

@KristofferC
Copy link
Member

Please, do not assume that everyone will have a deep understanding about methods, types, and multiple dispatch. ... I think we can spare five or six lines to make it really clear that there are corner cases, can't we?

I am of the belief that if there is confusion stemming from a particular piece of documentation, it is better to improve that documentation so the confusion doesn't arise in the first place, instead of leaving it unchanged and adding sizeable sections for everyone's interpretation of the confusing docs.

The core issue here seems to be confusion about what the word "applicable" means in Julia. So to fix this, the correct way would in my mind be to add a few words to define that word.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

applicable and hasmethod is returning wrong information for the function round in Julia master
6 participants