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

Portal confuses values that are equal but of different types #218

Open
mrkam2 opened this issue Apr 5, 2024 · 6 comments
Open

Portal confuses values that are equal but of different types #218

mrkam2 opened this issue Apr 5, 2024 · 6 comments

Comments

@mrkam2
Copy link
Contributor

mrkam2 commented Apr 5, 2024

I was able to reproduce the problem I reported in this slack discussion using plain clojure types.

(tap> {:a [[1]]})
(tap> {:a ['(1)]})

Here one object contains a vector [1] and another contains a list (1). However, the portal shows both as vectors.
image

If I clear it and tap them in the opposite order, both will be shown as lists:
image

This time I wasn't able to observe diferrent values by accessing portal selection, but tap-list clearly shows two different types:

@#'portal.runtime/tap-list ; => #<Atom@aeae4fd: ({:a [(1)]} {:a [[1]]})>

When types are important this becomes extremely confusing since you're used to trust what you see in portal.

@djblue
Copy link
Owner

djblue commented Apr 13, 2024

Thanks for the bug report, should be fixed in 0.55.0.

@djblue djblue closed this as completed Apr 13, 2024
@mrkam2
Copy link
Contributor Author

mrkam2 commented Apr 16, 2024

Thanks for the bug report, should be fixed in 0.55.0.

Awesome, thanks a lot for a quick fix!

@mrkam2
Copy link
Contributor Author

mrkam2 commented Apr 16, 2024

Unfortunately I just found that this didn't address my original problem.

@mrkam2
Copy link
Contributor Author

mrkam2 commented Apr 16, 2024

So if I'm reading the fix correctly, it takes hashcode and meta into account but it doesn't take type into account. I'm pretty sure adding type to the hash would address my problem. As a workaround I could also force my custom objects to generate a hashcode that's different from the one that regular Clojure collections produce.

In general, I think we should expect key clashes when storing data in a cache. So when two or more different objects share the same key, we should still be able to distinguish them in the cache.

Thoughts?

@mrkam2
Copy link
Contributor Author

mrkam2 commented Apr 16, 2024

Redefining hashCode function didn't work. hash function in Clojure ignores different values as can be seen here:

; Here is what I see after tapping two of my objects:
(map #(-> % :metrics first hash) @@#'portal.runtime/tap-list)
(-724974908 -724974908) ; identical values here

(map #(-> % :metrics first type) @@#'portal.runtime/tap-list)
(clojure.lang.PersistentArrayMap cash.Metric) ; different types here

(map #(-> % :metrics first .hashCode) @@#'portal.runtime/tap-list)
(1655286045 1655286051) ; different values here

@djblue
Copy link
Owner

djblue commented Apr 16, 2024

With the example provided above:

(tap> {:a [[1]]})
(tap> {:a ['(1)]})

I get:

image

The inner list / vector types are now correctly sent to the client.

Is the other example you are trying out something I can do locally?

The solution involves adding the additional hash-like number computed by:

(defn- hash+ [x]
  (cond
    (map? x)
    (reduce-kv
     (fn [out k v]
       (+ out (hash+ k) (hash+ v)))
     (+ 1 (if (sorted? x) 1 0) (hash+ (meta x)))
     x)

    (coll? x)
    (reduce
     (fn [out v]
       (+ out (hash+ v)))
     (+ (cond
          (list? x)   3
          (set? x)    (if (sorted? x) 4 5)
          (vector? x) 6
          :else       7)
        (hash+ (meta x)))
     x)

    :else
    (cond-> (hash x)
      (can-meta? x)
      (+ (hash+ (meta x))))))

Which assigns different values to different shapes based on various Clojure type predicates. This will easily produce collisions which is why the value is still part of the cache key. The idea being that there is a minimal chance of both hash and hash+ producing a collision at the same time.

here are the current tests.

@djblue djblue reopened this Apr 16, 2024
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

No branches or pull requests

2 participants