borkdude / rewrite-edn

Utility lib on top of rewrite-clj with common operations to update EDN while preserving whitespace and comments.
Eclipse Public License 1.0
85 stars 14 forks source link

`borkdude.rewrite-edn/get-in` gives Unhandled java.lang.ClassCastException #39

Open teodorlu opened 3 months ago

teodorlu commented 3 months ago

Hi :)

During work on a Neil PR, I encountered an exception in rewrite-edn.

Versions for repro

;; rewrite-edn version
borkdude/rewrite-edn {:mvn/version "0.4.8"}

;; java and clojure version
user> *clojure-version*
;; => {:major 1, :minor 11, :incremental 1, :qualifier nil}
user> (System/getProperty "java.version")
;; => "21.0.4"

Repro

  (def edn-nodes (borkdude.rewrite-edn/parse-string "{:deps #:babashka{pods #:git{:url \"https://github.com/babashka/babashka.pods\", :sha \"6ad6045b94bc871c5107bfc75d39643b6c1bc8ba\"}}}"))
  (def nl-path '[:deps babashka/pods])
  (borkdude.rewrite-edn/get-in edn-nodes nl-path)

Expected: return nil or a value.

Actual: I get a crash:

1. Unhandled java.lang.ClassCastException
   class clojure.lang.Symbol cannot be cast to class java.lang.Number
   (clojure.lang.Symbol is in unnamed module of loader 'app'; java.lang.Number
   is in module java.base of loader 'bootstrap')

              Numbers.java:  265  clojure.lang.Numbers/gte
              Numbers.java: 3991  clojure.lang.Numbers/gte
                 impl.cljc:  154  borkdude.rewrite_edn.impl$get/invokeStatic
                 impl.cljc:  119  borkdude.rewrite_edn.impl$get/invoke
                 impl.cljc:  162  borkdude.rewrite_edn.impl$get_in$fn__15166/invoke
     PersistentVector.java:  343  clojure.lang.PersistentVector/reduce
                  core.clj: 6885  clojure.core/reduce
                  core.clj: 6868  clojure.core/reduce
                 impl.cljc:  159  borkdude.rewrite_edn.impl$get_in/invokeStatic
                 impl.cljc:  158  borkdude.rewrite_edn.impl$get_in/invoke
          rewrite_edn.cljc:   36  borkdude.rewrite_edn$get_in/invokeStatic
          rewrite_edn.cljc:   30  borkdude.rewrite_edn$get_in/invoke
          rewrite_edn.cljc:   34  borkdude.rewrite_edn$get_in/invokeStatic
          rewrite_edn.cljc:   30  borkdude.rewrite_edn$get_in/invoke
                      REPL:  321  babashka.neil.dep-upgrade-test/eval18159
teodorlu commented 3 months ago

Repro above as a test:

https://github.com/teodorlu/rewrite-edn/blob/0579b1fa92fceac922d2f0fd4c2976fac82fab9c/test/borkdude/rewrite_edn_test.cljc#L342-L345

teodorlu commented 3 months ago

As far as I can tell, comparing k and (count coll) fails here.

https://github.com/borkdude/rewrite-edn/blob/81de541847438625c5cd10c4a0682cb08e3714e8/src/borkdude/rewrite_edn/impl.cljc#L154-L156

When I run the test, k appears to be 'babashka/pods (namespace qualified symbol), whereas (count coll) is 2 (number).

borkdude commented 3 months ago

I guess this is the branch where the tag of the node isn't a :map so it's assumed to be a vector with a numeric key k. Can you maybe print what the tag is, assuming it's not a vector?

borkdude commented 3 months ago

Oh I guess it's one of those namespaced maps which might not be handled so well yet.

borkdude commented 3 months ago

We can take a look at how antq is dealing with namespaced maps here:

https://github.com/liquidz/antq/blob/daf6321f316ca33386dbbc5787ec2807f3ba3cb1/src/antq/upgrade/clojure.clj#L21