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

type inference problem with captured type in closures #23618

Open
sbromberger opened this issue Sep 7, 2017 · 13 comments · May be fixed by #40985
Open

type inference problem with captured type in closures #23618

sbromberger opened this issue Sep 7, 2017 · 13 comments · May be fixed by #40985

Comments

@sbromberger
Copy link
Contributor

sbromberger commented Sep 7, 2017

function foo(a::AbstractVector)
    T = eltype(a)
    b = Vector{T}()
    c = [Set{T}() for x in a]
    return length(c)
end

a = rand(1:10, 200);

@code_warntype shows an inference failure in b and c.

Changing to c = Set{T}[Set{T}() for x in a] fixes c's inference issue.

@yuyichao yuyichao changed the title type inference problem with [...] constructor type inference problem with captured type in closures Sep 7, 2017
@yuyichao
Copy link
Contributor

yuyichao commented Sep 7, 2017

b is just #23280
c is real and seems to be caused by closure captured type only being specialized on T::DataType.

@ChrisRackauckas
Copy link
Member

Is this the same as #15276 ?

@yuyichao
Copy link
Contributor

yuyichao commented Sep 7, 2017

Depending on what do you call the scope of #15276. Both of these are related to closures but the sources are quite different.

All cases in #15276 are about the frontend not able to tell if a variable can be mutated in the closure/while the closure is executing so it creates a Core.Box for it (the original title was more specific about this). In this case, the lowering correctly determined that the variable doesn't need a Box but it fails to generate code the Type{T} and instead generated/inferred code for DataType.

@sbromberger
Copy link
Contributor Author

sbromberger commented Sep 7, 2017

x-ref https://discourse.julialang.org/t/performance-issue-with-use-of-eltype/5764. That is, there is a secondary issue that performance is slow even with explicit typing due to type instability in the inner comprehension loop.

@tkf
Copy link
Member

tkf commented Dec 16, 2019

A MWE is:

julia> let f = Int
           typeof(x -> f(x))
       end
var"#51#52"{DataType}

This comes up in with types:

julia> let f = Int  identity
           @inferred f(1)
       end
ERROR: return type Int64 does not match inferred return type Any
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] top-level scope at REPL[71]:2

A simple workaround is:

julia> begin
       prefer_singleton_callable(::Type{T}) where T = SingletonCallable{T}()
       prefer_singleton_callable(f) = f
       struct SingletonCallable{T} end
       (::SingletonCallable{T})(x) where T = T(x)
       end;

julia> let f = prefer_singleton_callable(Int)  identity
           @inferred f(1)
       end
1

Can this be done during lowering?

@schlichtanders
Copy link

I just stumbled upon this issue and am surprised that it is that old.
Is there a reason that this hasn't been prioritized yet?

@JeffBezanson
Copy link
Sponsor Member

If something seems important but has been sitting around for a long time, it's usually a safe bet that it's difficult.

@timholy
Copy link
Sponsor Member

timholy commented May 11, 2022

One of the few certain fixes is to allow lowering to call type inference. Currently lowering is written in lisp and is firmly "upstream" of type inference---we can't run type inference until code is presented in lowered form.

If there isn't a simpler solution that would be truly reliable, I am not sure I can think of a bug fix that would require more re-engineering than this one. Care to tackle it, @schlichtanders? 😄

@KristofferC
Copy link
Sponsor Member

Seems this issue is almost fixed in #40985, just need a bit more.

@timholy
Copy link
Sponsor Member

timholy commented May 11, 2022

Yeah, I guess this one is a bit easier than #15276, which is the one that really annoys me.

@schlichtanders
Copy link

One of the few certain fixes is to allow lowering to call type inference. Currently lowering is written in lisp and is firmly "upstream" of type inference---we can't run type inference until code is presented in lowered form.

If there isn't a simpler solution that would be truly reliable, I am not sure I can think of a bug fix that would require more re-engineering than this one

Very impressive indeed. Thanks for sharing this summary. I tried to optimize my package ExtensibleEffects.jl when I stumbled upon this, so I am more on the user side of things and won't have enough time and resources to improve Core Julia here ;-)

@mschauer
Copy link
Contributor

The minimum working example doesn't trigger this anymore

julia> versioninfo()
Julia Version 1.10.0-beta1

julia> let f = Int ∘ identity
                  @inferred f(1)
              end
1

but the initial issue seems to persist:

julia> @code_warntype foo(a)
MethodInstance for foo(::Vector{Int64})
  from foo(a::AbstractVector) @ Main REPL[152]:1
Arguments
  #self#::Core.Const(foo)
  a::Vector{Int64}
Locals
  #563::var"#563#564"{DataType}
  c::Vector # <------
  b::Vector{Int64}
  T::Type{Int64}
Body::Int64

@KristofferC
Copy link
Sponsor Member

Still seems to be an issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants