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

Provide a better error for similar methods with Dict #17968

Merged
merged 1 commit into from
Aug 23, 2016

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Aug 11, 2016

Fixes the following error:

julia> d = Dict(:a=>"a")
Dict{Symbol,String} with 1 entry:
  :a => "a"

julia> map(p->p.second, d) == ["a"]
ERROR: MethodError: no method matching similar(::Dict{Symbol,String}, ::Type{String})
Closest candidates are:
  similar(::SubArray{T,N,P,I,L}, ::Type{T}, ::Tuple{Vararg{Int64,N}}) at subarray.jl:52
  similar{T}(::Array{T,1}, ::Type{T}) at array.jl:122
  similar{T}(::Array{T,2}, ::Type{T}) at array.jl:123
  ...
 in _collect(::Dict{Symbol,String}, ::Base.Generator{Dict{Symbol,String},##1#2}, ::Base.EltypeUnknown, ::Base.HasLength) at ./array.jl:318
 in map(::Function, ::Dict{Symbol,String}) at ./abstractarray.jl:1673

Noticed from the PlotlyJS tests.

@timholy timholy added backport pending 0.5 kind:bugfix This change fixes an existing bug labels Aug 11, 2016
d = Dict(:a=>"a")
@test @inferred(map(identity, d)) == d
@test @inferred(map(p->p.first=>p.second[1], d)) == Dict(:a=>'a')
@test @inferred(map(p->p.second, d)) == Any["a"]
Copy link
Contributor

@tkelman tkelman Aug 11, 2016

Choose a reason for hiding this comment

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

isn't this counter to fc3cbc9 / the closing of #17714 ?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Yes. I would be against sometimes mapping a Dict to a Dict, and sometimes to an array. Also I believe the collect machinery assumes the container type doesn't depend on the element type --- as it runs, it might pick a new element type but keeps the container type.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Sounds good. Should this be a deprecation or an outright error? In other words, would you be OK with moving the array-creating version to deprecated.jl?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

In theory it could be a deprecation, but the warning should not depend on
type inference, which might be more trouble than it's worth.
On Aug 11, 2016 2:28 PM, "Tim Holy" notifications@github.com wrote:

In test/dict.jl
#17968 (comment):

@@ -388,6 +388,11 @@ d = Dict('a'=>1, 'b'=>1, 'c'=> 3)
@test [d[k] for k in keys(d)] == [d[k] for k in eachindex(d)] ==
[v for (k, v) in d] == [d[x[1]] for (i, x) in enumerate(d)]

+# generators, similar
+d = Dict(:a=>"a")
+@test @inferred(map(identity, d)) == d
+@test @inferred(map(p->p.first=>p.second[1], d)) == Dict(:a=>'a')
+@test @inferred(map(p->p.second, d)) == Any["a"]

Sounds good. Should this be a deprecation or an outright error? In other
words, would you be OK with moving the array-creating version to
deprecated.jl?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/JuliaLang/julia/pull/17968/files/3cfc74d47ddf7ffc9e9db32e3057ce648438fe1c#r74476669,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAtcbAYxqgPJaOFl02SP9tqPgBnZe1_Eks5qe2nPgaJpZM4JiQcr
.

@timholy
Copy link
Sponsor Member Author

timholy commented Aug 11, 2016

Hmm, this is amusing:

julia> d = Dict(:a=>"abc", :b=>"def")
Dict{Symbol,String} with 2 entries:
  :a => "abc"
  :b => "def"

julia> ret = [p for p in d]
2-element Array{Pair{Symbol,String},1}:
 :a=>"abc"
 :b=>"def"

julia> ret = [p->p[2] for p in d]
2-element Array{##4#6,1}:
 #4
 #4

julia> ret = [v for (k,v) in d]
2-element Array{String,1}:
 "abc"
 "def"

julia> ret = [v->v[1] for (k,v) in d]
2-element Array{##10#12,1}:
 #10
 #10

EDIT: this is on master, not this PR.

@timholy timholy changed the title Fix missing similar methods for Dict Provide a better error for similar methods with Dict Aug 13, 2016
@timholy
Copy link
Sponsor Member Author

timholy commented Aug 13, 2016

Better?

@JeffBezanson
Copy link
Sponsor Member

Yes, this is better. Whether you get the error depends on inference though, so it might be a bit jarring.

@timholy
Copy link
Sponsor Member Author

timholy commented Aug 17, 2016

Isn't that true of the current MethodError too?

@tkelman
Copy link
Contributor

tkelman commented Aug 20, 2016

What's the status here, is this good to merge?

@JeffBezanson
Copy link
Sponsor Member

Yes, we might as well do this.

@JeffBezanson JeffBezanson merged commit 2390055 into master Aug 23, 2016
This was referenced Aug 25, 2016
@tkelman tkelman deleted the teh/similar_for branch August 29, 2016 09:42
tkelman pushed a commit that referenced this pull request Aug 29, 2016
…non-Pair return type

(cherry picked from commit eb95919)
ref #17968
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants