Clojurians Log v2

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! :simple_smile:

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

ah, right. good point. thanks!

also a good point.

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

it appears i use it a fair bit, but likely not carefully enough. i need to go back and review in detail 🙂

possibly use from w/in a try/catch might be ok?

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.

on a possibly related note, i have a sort of parser for defn -- when i wrote it, i used the following as a specification of it:

; from defn docs

;; single arity
;; (defn name doc-string? attr-map? [params*] prepost-map? body)

;; multi-arity
;; (defn name doc-string? attr-map? ([params*] prepost-map? body)+ attr-map?)

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. :simple_smile:

clj -Sdeps '{:deps {rewrite-clj {:git/url "<https://github.com/lread/rewrite-cljs-playground>" :sha "69ef791b949eac1e3cebf6cec154bcda0a109edc"}}}'
Cloning: <https://github.com/lread/rewrite-cljs-playground>
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 "<https://github.com/lread/rewrite-cljs-playground>" :sha "69ef791b949eac1e3cebf6cec154bcda0a109edc"}}}'
Clojure 1.10.0
user=&gt; (require '[rewrite-clj.parser :as p])
nil
user=&gt; (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. :simple_smile:

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

probably depends on your use case, what would you do in the catch?

I think this is another place the docs could give better guidance

just avoid having the program die?

oh I see, a no-op catch? Probably not ideal, but might be appropriate depending in what you are trying to achieve?

may be i should just revisit how i've been using it in detail -- at least in some cases checking for the node's tag before deciding whether to call sexpr should be doable.

i think i've been using sexpr without any(!) caution. would appreciate hearing more about how to use it appropriately. do you think it's mostly a matter of checking a zipper's tag before using sexpr?

i guess if that's so, it might be helpful to know exactly what supports sexpr and what doesn't. based on recent conversations and looking at the source, it looks like this includes: comment, some reader nodes, uneval, whitespace, comma, newline.

I might be wrong, but am currently thinking that sexpr should not be used in general cases.

yeah maybe, please let me know how this goes.

i went over where i'm using sexpr and iiuc am using it mostly in situations where i could check the tag before use. i'm leaning toward changing things to check before use.

it's a bit long, but here's a typical set of uses:

(defn study-defn-to-tail
  [zloc]
  (let [{:keys [zloc] :as m}
        {:root-zloc zloc
         :zloc (rz/down zloc)}
        {:keys [zloc] :as m}
        (merge m
               (if (some #{'defn 'defn-}
                         [(rz/sexpr zloc)])
                 {:zloc (rz/right zloc)}
                 (throw (#?(:cljs js/Error. :default Exception.)
                         (str "Expected defn or defn-, but got: "
                              (rz/sexpr zloc)))))
               {:form-type zloc})
        {:keys [zloc] :as m}
        (merge m
               (if (symbol? (rz/sexpr zloc))
                 {:zloc (rz/right zloc)}
                 (throw (#?(:cljs js/Error. :default Exception.)
                         (str "Expected symbol, but got: "
                              (rz/sexpr zloc)))))
               {:fn-name zloc})
        {:keys [zloc] :as m}
        (merge m
               (if (string? (rz/sexpr zloc))
                 {:found-docstring true
                  :zloc (rz/right zloc)}
                 {:found-docstring false})
               {:docstring zloc})
        {:keys [zloc] :as m}
        (merge m
               (if (map? (rz/sexpr zloc))
                 {:found-meta true
                  :zloc (rz/right zloc)}
                 {:found-meta false})
               {:meta zloc})]
    m))

there are 6 uses there.

two of them could have used rz/string and it looks like in the other 4 cases, the tag could be checked, iiuc

Cool, I’m interested to learn about any other cases where you feel you still need to use sepxr - its good for me to understand various uses of rewrite-clj.

if i turn any more up, intend to bug you about them 🙂

btw, i noticed there are predicates like: map?, list?, etc. for some things, but i don't see string? and symbol? (and some others). does it seem practical / worth having other things like those of the latter?

am starting to wonder if it's really sensible for sexpr to be a part of the Node protocol...if one were to start from scratch, would it not be more sensible for there to be a separate protocol for things that support sexpr? i don't feel i have enough of a good understanding of protocols to have a decent opinion on this though 🙂

yeah me neither, not yet anyway! :simple_smile:

i think borkdude mentioned he added some predicates in clj-kondo

so yeah, maybe more would be handy