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

Is Base.Callable OK to use? Should it be documented? #43491

Open
fingolfin opened this issue Dec 20, 2021 · 6 comments
Open

Is Base.Callable OK to use? Should it be documented? #43491

fingolfin opened this issue Dec 20, 2021 · 6 comments
Labels
docs This change adds or pertains to documentation

Comments

@fingolfin
Copy link
Member

There is no docstring for it, it doesn't seem to be covered by the manual. Docstrings even seem to explicitly cover it, e.g.

"""
    replace(new::Function, A; [count::Integer])

Return a copy of `A` where each value `x` in `A` is replaced by `new(x)`.
...
"""
replace(new::Callable, A; count::Integer=typemax(Int)) =
    _replace!(new, _similar_or_copy(A), A, check_count(count))

Yet it seems like a very useful concept, and I've been using it in my code without even questioning how "official" it is. I then was surprised when a colleague didn't know about it, and now I wonder where I first picked it up ... ;-).

Anyway: is it deliberately hidden, or would a patch documenting it be welcome?

@N5N3
Copy link
Member

N5N3 commented Dec 21, 2021

Well everything could be called in Julia if we give it a definition.
I guess we dont need such type restriction for most cases?

@mikmoore
Copy link
Contributor

Base.Callable = Union{Function,Type} is the definition.

As N5N3 suggested, this is not a common pattern. Since virtually any object can be made callable, there's no useful concept of a callable object at the type level. One could aim for a trait-like concept, but the preferred solution is to let the MethodError occur at the place where the call attempt is made rather than preempt it at the parent function.

In idiomatic Julia, Callable should probably be avoided whenever possible rather than documented or encouraged. Most often, the reason people use type restrictions like ::Callable or ::Function is as "self-documentation" or because untyped arguments make them uncomfortable. In practice, these supertypes are nearly useless. Worse, they throw unnecessary (and sometimes confusing) errors when callable (but not "Callable") objects are passed in their position. If you require documentation of arguments, use the doc string rather than method signature. If you need Callable to disambiguate methods, I would first recommend considering whether the two functionalities really need to exist under the same function. I won't guarantee there aren't cases where Callable might truly be a useful tool, but I've never needed Callable or Function in my own code.

@stevengj stevengj added the docs This change adds or pertains to documentation label Dec 22, 2021
@fingolfin
Copy link
Member Author

So, basically, Base.Callable only exists to disambiguate a few methods like get!(dict, key, default) vs. get!(callable, dict, key) ?

@mikmoore
Copy link
Contributor

mikmoore commented Jan 1, 2022

That matches my understanding. In that particular case one can usually disambiguate via the position of dict:

get!(dict::AbstractDict,key,default)
get!(callable,dict::AbstractDict,key)

However, this would fail in the pathological cases where one desires to use callable::AbstractDict or key::AbstractDict, which would cause ambiguities.

@tkf
Copy link
Member

tkf commented Jan 1, 2022

Note that mergewith was introduced in #34296 specifically to avoid depending on Callable. If you are creating a new functional API, I strongly recommend designing it in such a way that the arity and the function name determine which argument positions receive callable objects. The only reasonable use case of Union{Function,Type} at the interface level is for dealing with old APIs. (There's of course nothing stopping people for using it in internal functions.)

Actually, I've been wanting to add a !!! warn note in the Function (which is already a public API) docstring and also to a short reference from Base.Callable. Technically, Base.Callable is probably not a public API but removing it is practically breaking as many packages and "dark matter" (= non-public) codebase use it. So, adding a docstring to it for clarifying the (un)recommended usage pattern sounds helpful to me.

@JeffBezanson
Copy link
Member

Correct; Callable should not be used anymore.

krynju pushed a commit to JuliaParallel/DTables.jl that referenced this issue Aug 18, 2023
Enabling generation of a DTable via `d = DTable(CSV.File, files)` by
removing the `::Function` type annotation per the suggestion in [this
thread](JuliaLang/julia#43491).

---------

Co-authored-by: Julian Samaroo <[email protected]>
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

No branches or pull requests

6 participants