not always easy to be brief but clear 🙂

@lee btw, i encountered a problem processing clojure's core.clj:

(require
   '[rewrite-clj.zip :as rz]
   :reload-all)

  (def source-str
    (slurp "../clojure/src/clj/clojure/core.clj"))

  (def root-zloc
    (rz/of-string ;;(subs source-str 0 26232) ; no problem
                  (subs source-str 0 26981) ; exception below
                  {:track-position? true}))

  ;; ExceptionInfo unsupported operation for uneval-node ...
  (def strings
    (loop [zloc root-zloc
           results []]
      (if (rz/end? zloc)
        results
        (let [sexpr (rz/sexpr zloc)]
          (recur (rz/next zloc)
                 (if (string? sexpr) 
                   (conj results zloc)
                   results))))))

;equals-based
#_(defn =
  "Equality. Returns true if x equals y, false if not. Same as Java
  x.equals(y) except it also works for nil. Boxed numbers must have
  same type. Clojure's immutable data structures define equals() (and
  thus =) as a value, not an identity, comparison."
  {:inline (fn [x y] `(. clojure.lang.Util equals ~x ~y))
   :inline-arities #{2}
   :added "1.0"}
  ([x] true)
  ([x y] (clojure.lang.Util/equals x y))
  ([x y & more]
   (if (= x y)
     (if (next more)
       (recur y (first more) (next more))
       (= y (first more)))
     false)))

I think that makes sense, you can’t turn an uneval into a sexpr:

$ clj
Clojure 1.10.0
user=> (require '[rewrite-clj.parser :as p])
nil
user=> (p/parse-string "#_foo")
<uneval: "#_foo">
user=> (require '[rewrite-clj.node :as n])
nil
user=> (n/sexpr (p/parse-string "#_foo"))
Execution error (UnsupportedOperationException) at rewrite_clj.node.uneval.UnevalNode/sexpr (uneval.clj:6).
null

or it should return nil maybe, but then you can’t distinguish between a token that represents nil

$ clj
Clojure 1.10.0
user=> (require '[rewrite-clj.node :as n])
nil
user=> (require '[rewrite-clj.parser :as p])
nil
user=> (n/sexpr (p/parse-string "nil"))
nil

same for comments:

user=> (n/sexpr (p/parse-string ";; hello"))
Execution error (UnsupportedOperationException) at rewrite_clj.node.comment.CommentNode/sexpr (comment.clj:6).
null

Thinking about rewrite-clj sexpr feature is on my todo list. I guess it might be convenient but comes with limitation that should be documented. I’m thinking that I should probably remove internal uses of sexpr because of these limitations. I’ll also have to think about cljs vs clj differences and how sexpr handles them - like ratio is only available in clj, differences in max integers, no char in cljs etc.

My current thinking is rewrite-clj sexpr should be used cautiously if at all. What do you folks think?

yeah, I try to avoid calling sexpr in clj-kondo as much as I can

although I already filter out every uneval and comment before

I guess if you are quite certain of what you are trying to sexpr you’ll probably be ok, but if you are sexpr-ing an unknown then maybe stay away from sexpr.

yeah, exactly

I have also made a few predicates like symbol-token? so I don’t need to sexpr to check if it’s a symbol

caveat is that there might be metadata on anything in clojure

I wonder if it would have made better sense if the metadata was a child instead of a parent. it certainly maybe would have made my life easier, but I haven’t pondered the consequences of that

interesting, we should probably eventually bring your predicates into rewrite-clj. Also interesting thought on metadata, would make it easier to parse the meat, right?

right, for example: I expect the first node after defn to be a symbol, but in rewrite-clj it might be a metadata node with a symbol in it

I would probably make metadata a field on the defrecord of every node or something

but that might not work for rewriting (which I’m not concerned with) to the original expressions, including spaces, etc

hmmm... yeah I see your point. It is worth thinking about more.

I really like rewrite-clj btw. but I might need some clone for tuning towards clj-kondo for more performance… but not now, it’s already very fast

what I basically do for nodes that might be metadata, is rip out the contents and store the metadata node as proper metadata on the node

cool, it is very nice to have heavy users of rewrite-clj here like you and @sogaiu. Your feedback and ideas are greatly appreciated!

after I finish up a cljs ticket, I’ll get back on my rewrite-clj todo list and work toward the alpha release.

@lee do you have commit rights to rewrite-clj?

I’ll make use of your #qa channel @borkdude

no, but I do have commit rights to clj-commons/rewrite-cljs. But... that’s not the ideal spot for my work. I think ideally there would be a rewrite-clj under clj-commons but have not managed to get an answer on this idea from the author of rewrite-clj yet.

I’d love to see a proper fix for namespaced maps in rewrite-clj and also support for reading ##Inf

I patched it in clj-kondo to make it work

I do have a pre-alpha playground https://github.com/lread/rewrite-cljs-playground/tree/cljc-spike-2

but you might want to wait until alpha. I know that @sogaiu is dipping his toe into my playground as he has already provided some great feedback.

my alpha goal is to bring rewrite-cljs features up to rewrite-clj while not breaking rewrite-clj/rewrite-cljs apis - but I did include a version of my rewrite-clj namespace PR (which I still want to tweak to not use sexpr)

I think a fundamental mistake right now in rewrite-clj is that parsing namespaced maps depends on the namespace state from which you are calling rewrite-clj: https://github.com/xsc/rewrite-clj/issues/54#issuecomment-494445992

ah... good reminder, thanks. If memory serves, I think the namespace state requirement is to support sexpr? But surely annoying if you aren’t using sexpr.

I’ll add another todo for this issue, thanks.

clj -Sdeps '{:deps {rewrite-clj {:git/url "" :sha "69ef791b949eac1e3cebf6cec154bcda0a109edc"}}}'
Cloning: 
Error building classpath. Destination path "rewrite-cljs-playground" already exists and is not an empty directory

not sure what this is

this worked: ~/.gitlibs $ rm -rf _repos/github.com/lread

so in summary:

$ clj -Sdeps '{:deps {rewrite-clj {:git/url "" :sha "69ef791b949eac1e3cebf6cec154bcda0a109edc"}}}'
Clojure 1.10.0
user=> (require '[rewrite-clj.parser :as p])
nil
user=> (def x (p/parse-string "#::a {:a #::a {:b 1}}"))
Syntax error (AssertionError) compiling at (REPL:1:8).
Assert failed: :namespaced-map could not resolve namespace ::a

I’m not even calling sexpr

yeah, I see your point and it is a very good one.

FWIW, what I’m doing right now to fix it:

(p/parse-string "#::a {:a #::a {:b 1}}")
#clj_kondo.impl.node.seq.NamespacedMapNode{:ns <token: ::a>, :aliased? true, :children [<map: {:a #::a{:b 1}}>]}

I just store the raw node of the ns and the children as is

thanks @borkdude , I’ll add this to my notes

sorry for being a little bit pushy maybe 😉

I don’t find you pushy at all. I find you both considerate and helpful!

the patch is defined in these three files: https://github.com/borkdude/clj-kondo/blob/master/src/clj_kondo/impl/node/seq.clj https://github.com/borkdude/clj-kondo/blob/master/src/clj_kondo/impl/parser/namespaced_map.clj https://github.com/borkdude/clj-kondo/blob/master/src/clj_kondo/impl/rewrite_clj_patch.clj and here’s a test: https://github.com/borkdude/clj-kondo/blob/master/test/clj_kondo/impl/parser_test.clj#L10 as you can see the roundtrip of parsing and sexpr-ing isn’t identical, but that’s easy to fix using the aliased? boolean I’m also storing

for the purposes of linting that wasn’t a problem for me

Thanks, I’ll be sure to take a close look