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

WIP: remove fallback hash method. fixes #12198 #24354

Closed
wants to merge 1 commit into from

Conversation

JeffBezanson
Copy link
Member

Here's my initial experiment towards removing this method. Overall I think it's a good change; it caught a few cases of sketchy hashing in Base. Haven't tested thoroughly yet; let's see what CI thinks.

I guess we might want to add a deprecation for this.

The most common issues I found were (1) using types as keys, and (2) checking for the presence of random objects in dictionaries that will never be found. With types, === and == differ, and you can only sensibly hash them by object id. With the second problem, you try e.g. CrazyThing in set, and the answer is false, but you get a method error that CrazyThing can't be hashed. There have been discussions in the past about giving errors when trying to look up objects that couldn't possibly match anything of the key type. Something like that could maybe solve this, but I'm not sure what to do. You might just need to be careful when looking up something whose type is totally unknown.

@JeffBezanson JeffBezanson added the breaking This change will break code label Oct 26, 2017
@JeffBezanson
Copy link
Member Author

Sigh, Documenter has a few dictionaries with keys that don't have == or hash methods. It might be time to add a TypedObjectIdDict{K,V} that wraps an ObjectIdDict with type checks. And since those names are getting a bit long, perhaps a rename to TypedIdDict and IdDict. Or we could add type parameters to IdDict and make ObjectIdDict an alias for IdDict{Any,Any}.

@StefanKarpinski
Copy link
Member

I like the idea of IdDict{K,V} and const ObjectIdDict = IdDict{Any,Any}, although at that point I wonder if we shouldn't just call it IdDict{Any,Any}. But that could be considered separately.

@JeffBezanson JeffBezanson changed the title remove fallback hash method. fixes #12198 WIP: remove fallback hash method. fixes #12198 Nov 2, 2017
@mauro3
Copy link
Contributor

mauro3 commented Nov 22, 2017

Maybe call it EgalDict?

@StefanKarpinski
Copy link
Member

Can we just add parameters to ObjectIdDict since not specifying the parameters is almost equivalent to having Any parameters?

@mauro3
Copy link
Contributor

mauro3 commented Nov 23, 2017

ObjectIdDict is implemented in C and is pretty simple, compared to other Dicts. I suspect that is the reason Jeff suggested to make the typed version by wrapping ObjectIdDict + type checks. I once implemented one in #7348, maybe that could be re-activated.

@vtjnash
Copy link
Member

vtjnash commented Dec 5, 2017

I also remember once doing something like that in https://github.com/JuliaLang/julia/compare/jn/iddict before too. I think Jeff wasn't convinced it was worthwhile to change the memory layout, since the cases in which you know what you wanted to store, but can't hash it (translation: you want to add type parameters, but the object could be anything), are rare or non-existent.

@mauro3
Copy link
Contributor

mauro3 commented Dec 13, 2017

Is this going in for 1.0? I think this would be great and save us many discourse-questions. Let me know if I should do anything on #24932. I can also try to update this PR in combination with #24932, if that would help.

@StefanKarpinski
Copy link
Member

Ideally, yes, but unfortunately (for us, not him), @JeffBezanson is on vacation until early next week. Someone may need to pick this up for him, or we may need a few extra days to get this in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants