make isa a generic function

[MasonProtter]

Currently, isa is a builtin function which causes various annoyances. For instance,

julia> @which 1 isa Int
ERROR: ArgumentError: argument is not a generic function
Stacktrace:
 [1] which(::Any, ::Any) at ./reflection.jl:1149
 [2] top-level scope at REPL[14]:1

and it also makes it so that we can't have a curried method

julia> Core.isa(T) = x -> x isa T
ERROR: cannot add methods to a builtin function
Stacktrace:
 [1] top-level scope at REPL[17]:1

This PR simply makes generic function Base.isa(x, T) = Core.isa(x, T) and adds a curried method isa(T) = Fix2(isa, T) so that we can make functions like isastring = isa(String).

I think this is a nice, minor convenience that costs us little to add.

Fixes #32018

[MasonProtter]

Hm, I'm not sure what's going on with these build errors. It built fine for me locally...

[KristofferC]

Might be a difference in running with asserts enabled or not.

[quinnj]

One point of hesitation for me here is that this would allow people to overload Base.isa, which seems to get into "violation of language fundamentals" territory. I bring it up because I've sketched out some ideas before around an Interfaces.jl package and wondered about possibly overloading isa for interface types (so that it could do method definition checks, for example), but I wasn't sure if it would "break" the language too much in weird ways.

[JeffBezanson]

See my comment #36759 (comment). isa is very important to the compiler so I'm worried that this will have a high cost. While I admit ===(x) is useful, I've hardly ever wanted isa(T); extra complexity, compile time, etc. is not worth it IMO.

[JeffBezanson]

this would allow people to overload Base.isa

Disallowing further overloading is table stakes for me not to veto this change.

[MasonProtter]

See my comment #36759 (comment). isa is very important to the compiler so I'm worried that this will have a high cost. While I admit ===(x) is useful, I've hardly ever wanted isa(T); extra complexity, compile time, etc. is not worth it IMO.

Ah, sorry I looked around for issues / PRs but was having trouble finding relevant search results, I didn't realize this was previously discussed. Should I close this PR as a duplicate?

I will say that I have found myself in situations with packages like SymbolicUtils.jl where we wanted predicate functions isa(T), and I think my plans for a testing library will also involve an interface where I want users to be able to provide me predicates like isa(T). Of course. I can always just make local functions that then dispatch to isa, but I was hoping for a shared solution.

The counterpoints raised are quite understandable though and I see the potential downsides.

Here's one potential way to make this a win-win: what if I went through all the usage of isa in Base and switched them to Core.isa? That way, nobody can pirate Base.isa and break internals, but we can still expose a generic function to users. Package developers who are sufficently scared of overloading would also be free to use Core.isa when appropriate.

Disallowing further overloading is table stakes for me not to veto this change.

That would indeed be a great feature, but one that is beyond my ken 😅

[KristofferC]

Here's one potential way to make this a win-win: what if I went through all the usage of isa in Base and switched them to Core.isa? That way, nobody can pirate Base.isa and break internals, but we can still expose a generic function to users.

isa is frequently used in package code and often with the expectation that the compiler can properly optimize it (so the a Union block turns into a concrete one). Will that still be the case?

[MasonProtter]

isa is frequently used in package code and often with the expectation that the compiler can properly optimize it (so the a Union block turns into a concrete one). Will that still be the case?

It'd be the same object as far as the compiler is concerned, right?

---

Edit: I realize now you're talking about if people used Base.isa rather than Core.isa. Good question, I don't know. I wouldn't have expected that the single layer of indirection would matter in package code but I'm far from an expert.

[timholy]

isa is frequently used in package code and often with the expectation that the compiler can properly optimize it (so the a Union block turns into a concrete one). Will that still be the case?

Not if there are enough isa methods, because the first argument is almost always not of known type. So isa will infer as returning Any, and currently isa is one of the main tools we use to claw back good inferrability when handling inherently non-inferrable code like that which deals with Expr.

When the result is used in an if or a && then it might not be a catastrophe because those are essentially also intrinsics, but any place where you use Boolean logic you'll lose inference.

I'm against this change, too.

[KristofferC]

I wouldn't have expected that the single layer of indirection would matter in package code but I'm far from an expert.

But isn't the point exactly that it can now be overloaded so in the presence of non-precise type information, you (the compiler) have no idea what Base.isa does anymore.

[MasonProtter]

In such a case where that matters, couldn't we just recommend the package author use Core.isa?

I guess at this point though if we're just going to end up using Core.isa everywhere that performance matters, I can certainly see the argument that this isn't a worthwhile change and there should just be a separate function that does what I want.

It's just unfortunate that Base claims the most obvious name for such a thing.

[MasonProtter]

No idea how robust it is or how easily it can be undone by a malicious user, but I've used Jameson's trick to stop people from being able to add methods to Base.isa as well as a test case.

[MasonProtter]

Okay, so I haven't yet figured out how to get Core to not export isa so for this I need to qualify with Base., but here's how it currently looks / works:

   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.6.0-DEV.742 (2020-08-27)
 _/ |\__'_|_|_|\__'_|  |  generic-isa/01e15fe6a1 (fork: 4 commits, 0 days)
|__/                   |

julia> Base.isa(1, Int)
true

julia> Base.isa(Int)(1)
true

julia> Base.isa(x, y) = "boo!"
ERROR: cannot add methods to a builtin function
Stacktrace:
 [1] top-level scope
   @ REPL[3]:1