Skip to content

Fix #77: tagged literal issue #79

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix #77: tagged literal issue #79

wants to merge 2 commits into from

Conversation

borkdude
Copy link
Contributor

No description provided.

Unverified

The signing certificate or its chain could not be verified.
@borkdude borkdude requested a review from danielcompton as a code owner March 20, 2025 11:41
@yuhan0
Copy link

yuhan0 commented Mar 20, 2025

Other functions are still broken which rely on a seq of MapEntry's :

(keys #ordered/map ([:a 1])) 
;; => Error printing return value (ClassCastException) at clojure.lang.APersistentMap$KeySeq/first (APersistentMap.java:171).
;;    class clojure.lang.PersistentVector cannot be cast to class java.util.Map$Entry (clojure.lang.PersistentVector is in unnamed module of loader 'app'; java.util.Map$Entry is in module java.base of loader 'bootstrap')

I guess we could further patch the seq implementation

  (seq [this]
    (->> order 
         (keep identity)
         (map #(MapEntry. (nth % 0) (nth % 1)))))

but that seems inefficient and hides the fact that the data structure invariants are being violated, which may break other parts of the code.

@yuhan0
Copy link

yuhan0 commented Mar 20, 2025

Namely given m : OrderedMap,

  • (.-order m) should be a sequence of MapEntry
  • (.-backing-map m) should be a map from Object to MapEntry

These are violated in the case of an OrderedMap constructed from a data literal, which have PersistentVector in place of MapEntry in both those fields

(def yes (ordered-map [[:a 1]]))
(let [m #ordered/map ([:a 1])]
  (def no m))

(-> yes .-order first type) ;; => clojure.lang.MapEntry
(-> no .-order first type) ;; => clojure.lang.PersistentVector

(-> yes .-backing-map (get :a) type) ;; => clojure.lang.MapEntry
(-> no .-backing-map (get :a) type) ;; => clojure.lang.PersistentVector

Unverified

The signing certificate or its chain could not be verified.
@borkdude
Copy link
Contributor Author

I pushed additional "fixes"

@yuhan0
Copy link

yuhan0 commented Mar 20, 2025

Yeah, I guess that would work.. feels slightly icky though, and the (no longer accurate) ^MapEntry tags should probably be removed.

@borkdude
Copy link
Contributor Author

There's actually no reason that MapEntries should have to be used internally, as long as they are produced when called. But not sure if it's worth all this trouble.

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.

None yet

2 participants