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

Convert key to Dict key type before hashing #4166

Closed
wants to merge 1 commit into from

Conversation

simonster
Copy link
Member

After fc05d74:

julia> a = Dict{Int16, Any}()
Dict{Int16,Any}()

julia> a[1] = 1
1

julia> a[1]
ERROR: key not found: 1
 in getindex at dict.jl:486

This is because setindex!() calls convert() on the key but getindex() does not. This PR calls convert() in ht_keyindex(), which fixes this. I wrapped this in try/catch so that get()/getkey() won't throw when convert(::KeyType, ::DictKeyType) fails or doesn't exist.

@JeffBezanson
Copy link
Sponsor Member

The try/catch definitely smells odd here.
One issue is that 65537 will also look up the key int16(1), which we probably don't want. Maybe this means lossy integer conversions have to throw errors, but that would be a pretty big change at this point.

@simonster
Copy link
Member Author

I agree that the try/catch is awkward, but I don't know a way around it. In getindex() we could just let convert() throw, but throwing in get()/getkey() if convert() fails due to InexactError etc. seems bad.

That lossy integer conversions don't throw is a bit of a problem for Dicts even without this PR:

julia> a = Dict{Int16, Any}()
Dict{Int16,Any}()

julia> a[65537] = 2
2

julia> a[int16(1)]
2

@JeffBezanson
Copy link
Sponsor Member

Some other options:

  • don't call convert on keys when assigning keys either
  • make isequal true for integers of different types

@johnmyleswhite
Copy link
Member

My preference would be to not call convert in setindex!.

@StefanKarpinski
Copy link
Sponsor Member

We rely pretty heavily on the convert call. Personally, I think that throwing errors on integer conversion is the right approach – the fact that we don't has slightly bothered me for a while now. But it would be a very big change.

@staticfloat
Copy link
Sponsor Member

Why shouldn't we just throw errors when the convert doesn't exist? That seems like the right approach here; you're trying to fit a square peg into a round hole.

@simonster
Copy link
Member Author

Throwing on a MethodError seems acceptable, but throwing on exceptions in convert would mean that get could work or throw for different values of the same type, which I find somewhat objectionable.

@simonster
Copy link
Member Author

Maybe we could rethrow anything that's not an InexactError? Still kind of smells.

@StefanKarpinski
Copy link
Sponsor Member

To me this is a slightly clearer example of the root problem:

julia> d = Dict{Float64,Any}()
Dict{Float64,Any}()

julia> d[1] = "foo"
"foo"

julia> d
[1.0=>"foo"]

julia> d[1]
ERROR: key not found: 1
 in getindex at dict.jl:486

This patch feels really wrong. I think we need a different solution, but there's definitely a problem here.

@StefanKarpinski
Copy link
Sponsor Member

Jeff and I decided that the right thing to do here is to allow setindex! to convert but then check that the resulting value is isequal to the original key and throw an error if it isn't. Intuitively, this means that you can convert keys as long as the conversion doesn't do anything drastic.

@JeffBezanson
Copy link
Sponsor Member

The great thing about this is it makes Dicts generically do the right thing consistent with however convert and isequal work. For example if we change isequal to be true for equal integers of different types, everything would go through.

@simonster simonster deleted the convert-key2 branch August 29, 2013 05:32
@stevengj
Copy link
Member

For numeric key types, at least, it seems like the right least-surprise thing to do would be to allow both setindex! and getindex to convert but to check that the resulting value is == to the original (or that they are both NaN, which would need special handling), not isequal, as a result of #3385.

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.

6 participants