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

Making JSObject hashable #158

Closed
mhavu opened this issue Feb 2, 2022 · 5 comments · Fixed by #162
Closed

Making JSObject hashable #158

mhavu opened this issue Feb 2, 2022 · 5 comments · Fixed by #162
Labels
enhancement New feature or request

Comments

@mhavu
Copy link

mhavu commented Feb 2, 2022

Is there anything that would prevent using the object id for hashing in JSObject? Since hash values in Swift change between sessions anyway, I can't immediately see why using id would be a bad idea. All that would be needed to make JSObject conform to Hashable is to add:

public func hash(into hasher: inout Hasher) {
    hasher.combine(id)
}
@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Feb 2, 2022

Yeah, that could work, but we'd need to make id public to allow you adding this conformance in your code. I think you can just use ObjectIdentifier from stdlib in the meantime. Off the top of my head I can't come up with a scenario where one could have different JSObject instances that share the same id, but I may be wrong.

@swiftwasm/jskit-team WDYT about this approach? Does ObjectIdentifier seem safe enough to be used on JSObject for a custom Hashable conformance? Or should we just implement Hashable using id as the OP suggests?

@MaxDesiatov MaxDesiatov added the enhancement New feature or request label Feb 2, 2022
@mhavu
Copy link
Author

mhavu commented Feb 2, 2022

Thanks for the tip! This seems to work just fine for now:

extension JSObject : Hashable {
    public func hash(into hasher: inout Hasher) {
        hasher.combine(ObjectIdentifier(self))
    }
}

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Feb 2, 2022

Yeah, it may work in trivial scenarios, but until I'm 100% sure there's no way one can have multiple different JSObject instances with the same id, I hesitate to recommend that as a reliable solution. And maybe same can be said of id, but identity in JavaScript is defined much more loosely if at all.

When we do have that certainty, I guess it makes sense to add some Hashable conformance to our codebase, so that you don't have to keep this implementation in your code.

What exactly are you trying to achieve with it? Do you just want a dictionary or a set of JSObject keys/elements, or something more advanced than that?

@mhavu
Copy link
Author

mhavu commented Feb 2, 2022

I just need a dictionary with JSObject keys at the moment.

@j-f1
Copy link
Member

j-f1 commented Feb 3, 2022

I think using id makes sense for hashing. SwiftRuntimeHeap.retain (on the JS side) checks whether the given object is already on the heap and reuses that heap entry (including the id) if possible. Therefore, JSObject.global.Array will return two different JSObject instances when called twice, but they will both have the same id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants