clj-commons / rewrite-clj

Rewrite Clojure code and edn
https://cljdoc.org/d/rewrite-clj
MIT License
590 stars 55 forks source link

Some reader macros `z/sexpr` to different forms than `read-string` #305

Closed frenchy64 closed 4 days ago

frenchy64 commented 1 month ago

Version clj -Sforce -Sdeps '{:deps {rewrite-clj/rewrite-clj {:mvn/version "1.1.47"}}}'

Platform Operating System: macOS 14.5 Clojure version: 1.11.4 JDK vendor and version: openjdk version "21.0.1" 2023-10-17 LTS

Symptom sexpr returns a different value than using read-string for certain reader macros.

Reproduction

clj -Sforce -Sdeps '{:deps {rewrite-clj/rewrite-clj {:mvn/version "1.1.47"}}}'
Clojure 1.11.4
user=> (require '[rewrite-clj.zip :as z])
nil
user=> (-> "~a" read-string)
(clojure.core/unquote a)
user=> (-> "~a" z/of-string z/sexpr)
(unquote a)
user=> (-> "@a" read-string)
(clojure.core/deref a)
user=> (-> "@a" z/of-string z/sexpr)
(deref a)
user=> (-> "~@a" read-string)
(clojure.core/unquote-splicing a)
user=> (-> "~@a" z/of-string z/sexpr)
(unquote-splicing a)

Actual behavior Reader macros ~, @, ~@ expand to unquote, deref, and unquote-splicing.

Expected behavior Reader macros ~, @, ~@ expand to clojure.core/unquote, clojure.core/deref, and clojure.core/unquote-splicing.

Diagnosis The symbols should be updated in the source code to be qualified in clojure.core.

Action I can submit a PR if there's interest.

lread commented 1 month ago

Thanks for the issue, @frenchy64! This seems like a reasonable tweak to me. But first, so I can better understand:

  1. What issues are you bumping into without having the qualifying clojure.core?
  2. What problem are we solving by adding the clojure.core?
frenchy64 commented 1 month ago

What issues are you bumping into without having the qualifying clojure.core?

I'm not using the library in practice, but the main effect is that they evaluate to different forms.

user=> (-> "'`~a" read-string eval)
a
user=> (-> "'`~a" z/of-string z/sexpr eval)
(quote (unquote a))
user=> (-> "'`[~@[]]" read-string eval)
(clojure.core/apply clojure.core/vector (clojure.core/seq (clojure.core/concat [])))
user=> (-> "'`[~@[]]" z/of-string z/sexpr eval)
(quote [(unquote-splicing [])])

What problem are we solving by adding the clojure.core?

This one stated in the user guide: Within reason, Clojure’s read-string and rewrite-clj’s sexpr functions should return equivalent Clojure forms.

frenchy64 commented 4 weeks ago

I found two more inconsistencies. I will open separate issues for them, I think the #= is the most urgent (security related). Here's edamame's approach.

clj -Sforce -Sdeps '{:deps {rewrite-clj/rewrite-clj {:mvn/version "1.1.47"}}}'
Clojure 1.8.0
user=> (require '[rewrite-clj.zip :as z])
nil
user=> (-> "`a" read-string)
(quote user/a)
user=> (-> "`a" z/of-string z/sexpr)
(quote a)
user=> (-> "#=(inc 1)" z/of-string z/sexpr)
(eval (quote (inc 1)))
user=> (binding [*read-eval* true] (-> "#=(inc 1)" read-string))
2
user=> (binding [*read-eval* false] (-> "#=(inc 1)" read-string))
RuntimeException EvalReader not allowed when *read-eval* is false.  clojure.lang.Util.runtimeException (Util.java:221)
frenchy64 commented 4 weeks ago

PR for this issue is ready for review https://github.com/clj-commons/rewrite-clj/pull/306