It would be nice to have a single argument isa
for cleaner filtering code.
filter(x -> !isa(x, AbstractFloat), [1, 2.0])
# could become
filter(!isa(AbstractFloat), [1, 2.0])
This would provide some consistency with other functions, such as isequal
, but since isa
is a builtin function, defining other variants doesn't seem trivial.
julia> Core.isa(::Type{T}) where {T} = (x) -> isa(x, T)
ERROR: cannot add methods to a builtin function
Yes, this matches a bunch of our other single-argument versions of binary operators.
Can I Work on this issue?
Sure, go for it! You'll want to make a PR that:
Awesome. Do you have any documentation that i can go through to understand the codebase?
There's a section on Developer Docs in the manual: https://docs.julialang.org/en/v1/devdocs/init/. I realized that isa
is not a generic function which makes this a fairly complex change. Probably not a good first issue after all.
Given the comments in https://github.com/JuliaLang/julia/pull/37240, it's somewhat clear that a generic function isn't acceptable. This leads me to another thought about how to approach this (as well as https://github.com/JuliaLang/julia/issues/36163): do the currying during lowering.
Here's the idea, apologies if I'm saying something dumb as I mostly only understand IR from IRTools.jl, not the real deal.
The idea here is that I explicitly do not want a dispatch that depends on types, I just want Core.isa(T)
to always be transformed into Fix2(isa, T)
. Looking at
julia> f(T) = isa(T)
f (generic function with 1 method)
julia> @code_lowered f(Int)
CodeInfo(
1 ─ %1 = (isa)(T)
└── return %1
)
julia> @code_typed f(Int)
CodeInfo(
1 ─ (isa)(T)::Union{}
└── $(Expr(:unreachable))::Union{}
) => Union{}
I think that if during the name resolution (i.e. once we are sure that isa
here refers to Core.isa
) if we replaced all instances of statements like
%n = isa(T)
with
%n = Fix2(isa, T)
then we'd get a curried isa
that nobody can add methods to and no existing valid code should be affected.
Is this a valid line of attack? I've discussed this with @tkf, but neither of us know enough about lowering to do this ourselves.
I think this should do the trick:
diff --git a/src/julia-syntax.scm b/src/julia-syntax.scm
index 4e75874e8a..a079874321 100644
--- a/src/julia-syntax.scm
+++ b/src/julia-syntax.scm
@@ -2201,6 +2201,9 @@
((and (eq? (identifier-name f) '^) (length= e 4) (integer? (cadddr e)))
(expand-forms
`(call (top literal_pow) ,f ,(caddr e) (call (call (core apply_type) (top Val) ,(cadddr e))))))
+ ((and (eq? f 'isa) (length= e 3))
+ (expand-forms
+ `(call (top Fix2) ,f ,(caddr e))))
(else
(map expand-forms e))))
(map expand-forms e)))
If people agree this is the way to go, I can make a PR.
Isn’t that a syntax level replacement? I think this might be better done during lowering, not parsing but I’m not sure.
This is during lowering, but note that lowering doesn't resolve isa
:
julia> dump(Meta.@lower x isa T)
Expr
head: Symbol thunk
args: Array{Any}((1,))
1: Core.CodeInfo
code: Array{Any}((2,))
1: Expr
head: Symbol call
args: Array{Any}((3,))
1: Symbol isa
2: Symbol x
3: Symbol T
2: Expr
head: Symbol return
args: Array{Any}((1,))
1: Core.SSAValue
id: Int64 1
codelocs: Array{Int32}((2,)) Int32[1, 1]
ssavaluetypes: Int64 2
ssaflags: Array{UInt8}((0,)) UInt8[]
method_for_inference_limit_heuristics: Nothing nothing
linetable: Array{Any}((1,))
1: Core.LineInfoNode
method: Symbol top-level scope
file: Symbol none
line: Int64 0
inlined_at: Int64 0
slotnames: Array{Symbol}((0,))
slotflags: Array{UInt8}((0,)) UInt8[]
slottypes: Nothing nothing
rettype: Any
parent: Nothing nothing
edges: Nothing nothing
min_world: UInt64 0x0000000000000001
max_world: UInt64 0xffffffffffffffff
inferred: Bool false
inlineable: Bool false
propagate_inbounds: Bool false
pure: Bool false
Ah, right. I think what I should have said is maybe 'name resolution' rather than lowering?
I think the problem is that we don't want someone to have a local variable or function named isa
which is not Core.isa
and have the pass effect that.
This cannot possibly be reliably done this way if you still want to keep isa
a function.
You are just going to get inconsistent result if the user write f = isa; f(T)
.
Right, that's why I said
during the name resolution (i.e. once we determine that
isa
refers toCore.isa
)
No that's not enough. You don't have enough information until evaluation time regarding how many arguments there are so you can't do this without having the equivalent of adding a method to isa
. The best you could do would be (equivalent to) special case Base.isa
in inference to assume no one overload the two argument variant.
No that's not enough. You don't have enough information until evaluation time regarding how many arguments there are so you can't do this without having the equivalent of adding a method to
isa
.
Ah, of course. I didn't think about the possibility of things like splatting. Thank you for explaining this.
Not even splatting. The use of the object may not be directly tracable to when it is called. e.g.
r = Ref{Any}(isa); #= 5 days later and a hundred global variable assignments apart =#; r[](....)
Most helpful comment
Can I Work on this issue?