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

overly coarse type inferred for nested comprehensions #7810

Closed
mbauman opened this Issue Aug 1, 2014 · 3 comments

Comments

Projects
None yet
3 participants
@mbauman
Member

mbauman commented Aug 1, 2014

Perhaps this is a little ridiculous, but I have a use-case for nested comprehensions in a library I'm creating. It seems as though type inference cannot follow the types through the nested pair. Here's a slimmed down example:

julia> type Foo{T<:AbstractVector}
         v::T
       end
       bar() = [Foo([(a,b) for a in 1:2]) for b in 3:4]
bar (generic function with 1 method)

julia> @code_typed bar()
    … # Allocate the output array -- correctly typed
    #s122 = top(ccall)(:jl_alloc_array_1d,$(Expr(:call1, :(top(apply_type)), :Array, Foo{Array{(Int64,Int64),1}}, 1))::Type{Array{Foo{Array{(Int64,Int64),1}},1}},$(Expr(:call1, :(top(tuple)), :Any, :Int))::(Type{Any},Type{Int64}),Array{Foo{Array{(Int64,Int64),1}},1},0,#s118::Int64,0)::Array{Foo{Array{(Int64,Int64),1}},1}
    … # Loop through each element and setindex! — #s122 lost its type!
    top(setindex!)(#s122,#s119::Foo{Array{(Int64,Int64),1}},#s121::Int64)
    … # Return var is totally untyped
    return #s122

It seems like everything up until the setindex! call is typed correctly. I'm completely ignorant of how the lowering and typing process works, but it seems like there's no reason for #s122 to lose its typing. Which makes me think that this is simply an unsupported case within lowering step… and maybe an easy fix?

If I simplify the inner comprehension and lose the custom type, the type inference improves but it's still not entirely correct:

julia> baz() = [[(a,b) for a in 1:2] for b in 3:4]
         @code_typed baz()
    … # Allocate the output array -- correctly typed
    #s122 = top(ccall)(:jl_alloc_array_1d,$(Expr(:call1, :(top(apply_type)), :Array, Array{(Int64,Int64),1}, 1))::Type{Array{Array{(Int64,Int64),1},1}},$(Expr(:call1, :(top(tuple)), :Any, :Int))::(Type{Any},Type{Int64}),Array{Array{(Int64,Int64),1},1},0,#s118::Int64,0)::Array{Array{(Int64,Int64),1},1}
    … # Loop through each element and setindex! -- why did this became a Union type?
    top(setindex!)(#s122::Union(Array{None,1},Array{Array{(Int64,Int64),1},1}),#s119::Array{(Int64,Int64),1},#s121::Int64)::Array{Array{(Int64,Int64),1},1}
    … # Returns the Union type
    return #s122::Union(Array{None,1},Array{Array{(Int64,Int64),1},1})

Could this be supported? This seems to be somewhat similar to issue #947, but I'm not sure how/if that was fixed. I can pre-allocate the arrays and do the typing myself, but it'd be a whole lot simpler just to punt to comprehensions.

@JeffBezanson JeffBezanson changed the title from Should inference support nested comprehensions? to overly coarse type inferred for nested comprehensions Aug 1, 2014

@JeffBezanson JeffBezanson added parallel and removed parallel labels Aug 1, 2014

@JeffBezanson

This comment has been minimized.

Member

JeffBezanson commented Aug 1, 2014

My usual terminology harping: the types are correct, but not as precise as we'd like.

@carlobaldassi

This comment has been minimized.

Member

carlobaldassi commented Aug 1, 2014

If you're sure of the type in advance, you can always use typed comprehensions (which were introduced after issue #947 was closed):

bar() = Foo{Vector{(Int,Int)}}[Foo([(a,b) for a in 1:2]) for b in 3:4]

This does not look particularly pretty in this case, but it will force the element type of the resulting Vector to be Foo{Vector{(Int,Int)}}, without relying on type inference (except for the inner comprehension, which could indeed be typed as well if needed in mode complex cases).

julia> @code_typed bar()
        # ....
        return #s120::Array{Foo{Array{(Int64,Int64),1}},1}
    end::Array{Foo{Array{(Int64,Int64),1}},1}))))
@mbauman

This comment has been minimized.

Member

mbauman commented Aug 3, 2014

You rock. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment