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

Make return type of map inferrable with heterogeneous arrays #42046

Merged
merged 2 commits into from
Sep 1, 2021
Merged

Conversation

nalimilan
Copy link
Member

Inference is not able to detect the element type automatically,
but we can do it manually since we know promote_typejoin is used for widening.
This is similar to the approach used for broadcast at #30485.

@nalimilan nalimilan requested a review from a team as a code owner August 29, 2021 14:27
Inference is not able to detect the element type automatically,
but we can do it manually since we know promote_typejoin is used for widening.
This is similar to the approach used for `broadcast` at #30485.
base/array.jl Outdated
Comment on lines 781 to 782
ElType = promote_typejoin_union(et)
ElType′ = ElType <: Type ? Type : ElType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this should be part of the et computation at the top of the function (so that the total function here is more type-stable), no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I did the same for broadcast (see here) so that would make sense, even if that changes the return type in corner cases. I've just pushed a commit to do this.

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Aug 30, 2021
@DilumAluthge DilumAluthge removed the request for review from a team August 31, 2021 16:34
@nalimilan
Copy link
Member Author

I'm told FreeBSD failure is unrelated. Merging.

@nalimilan nalimilan merged commit 49e3aec into master Sep 1, 2021
@nalimilan nalimilan deleted the nl/map branch September 1, 2021 08:20
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Sep 6, 2021
KristofferC pushed a commit that referenced this pull request Sep 15, 2021
Inference is not able to detect the element type automatically,
but we can do it manually since we know promote_typejoin is used for widening.
This is similar to the approach used for `broadcast` at #30485.

(cherry picked from commit 49e3aec)
@KristofferC
Copy link
Member

Backporting since it is a bit tangled up with #42172

KristofferC pushed a commit that referenced this pull request Sep 15, 2021
Inference is not able to detect the element type automatically,
but we can do it manually since we know promote_typejoin is used for widening.
This is similar to the approach used for `broadcast` at #30485.

(cherry picked from commit 49e3aec)
@MrVPlusOne
Copy link

Hi,

I just noticed that the newly introduced type assertion in collect(::Generator) breaks my existing code. A minimal example:

map(enumerate(1:4)) do (i, _) 
    a = (x=i, y=(i==1 ? 1 : "a"))
    (a, 3)
end

Throws the following error

ERROR: LoadError: TypeError: in typeassert, expected AbstractVector{<:Tuple{NamedTuple{(:x, :y), _A} where _A<:Tuple{Int64, Union{Int64, String}}, Int64}}, got a value of type Vector{Tuple{NamedTuple{(:x, :y)}, Int64}}
Stacktrace:
 [1] collect(itr::Base.Generator{Base.Iterators.Enumerate{UnitRange{Int64}}, Main.SEDL.var"#632#633"})
   @ Base /Applications/Julia-1.7.app/Contents/Resources/julia/share/julia/base/array.jl:734
 [2] map(f::Function, A::Base.Iterators.Enumerate{UnitRange{Int64}})
   @ Base ./abstractarray.jl:2878
 [3] top-level scope
   @ ~/.julia/dev/SEDL/scripts/hovercraft_example.jl:69
in expression starting at ...

I just upgraded to 1.7.0-rc3 from rc1 this morning and immediately ran into this. Looks like the computed type annotation is more precise but is somehow not compatible with the actual returned collection type.

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
…ng#42046)

Inference is not able to detect the element type automatically,
but we can do it manually since we know promote_typejoin is used for widening.
This is similar to the approach used for `broadcast` at JuliaLang#30485.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
…ng#42046)

Inference is not able to detect the element type automatically,
but we can do it manually since we know promote_typejoin is used for widening.
This is similar to the approach used for `broadcast` at JuliaLang#30485.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants