Skip to content
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.

Sign up for GitHub

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

Close #305: Qualify @ ~ ~@ sexpr's under clojure.core #306

Merged
merged 4 commits into from
Sep 8, 2024
Merged

Close #305: Qualify @ ~ ~@ sexpr's under clojure.core #306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
37b75f0
Qualify @ ~ ~@ sexpr's under clojure.core
frenchy64 Aug 14, 2024
b0de1ac
test & ci: bump sci-test for new tools.reader vars
lread Sep 7, 2024
6d9bc6d
Merge remote-tracking branch 'upstream/main' into issue-305-sexpr-dif…
lread Sep 7, 2024
a6a17f1
lib test: patch zprint to disable new failing test
lread Sep 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion deps.edn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@

:sci-test {:extra-paths ["target/generated/sci-test/src"]
:extra-deps {lread/sci-test {:git/url "https://github.com/lread/sci-test.git"
:sha "5adc46b18efa8661c9c86bb79c0876265cf6612a"}}}
:sha "27318786f077d491c9cbc0c9d99e5781d9a9ef83"}}}

:native-test {:extra-paths ["target/generated/graal"]}

Expand Down
12 changes: 9 additions & 3 deletions doc/01-user-guide.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1062,7 +1062,7 @@ a|<unsupported operation>

2+a|*Deref* `:deref`
a|`@form`
a|`(deref form)`
a|`(clojure.core/deref form)`

2+a|*Metadata* `:meta`
a|`^{:a 1 :b 2} [1 2 3]`
Expand Down Expand Up @@ -1098,7 +1098,11 @@ a|`(quote symbol)`

2+a|*Syntax unquote* `:unquote`
a|`~symbol`
a|`(unquote symbol)`
a|`(clojure.core/unquote symbol)`

2+a|*Unquote splicing* `:unquote-spliciing`
a|`~@symbol`
a|`(clojure.core/unquote-splicing symbol)`

2+a|*Tagged literal* `:reader-macro`
a|`#foo/bar [1 2 3]`
Expand Down Expand Up @@ -1181,7 +1185,7 @@ a|`'#::my-ns-alias{my-symbol 1}`

==== Rewrite-clj Default Auto-Resolve Handling

When calling `sepxr` on an auto-resolved keyword or symbol node, rewrite-clj will resolve:
When calling `sexpr` on an auto-resolved keyword or symbol node, rewrite-clj will resolve:

* the current namespace to `?\_current-ns_?`
* namespaced alias `x` to `??\_x_??`
Expand All @@ -1202,6 +1206,8 @@ To illustrate:
;; => #:??_my-alias_??{s1 3, :b 2, :a 1}
----

Currently, symbols under syntax quote are never resolved.

[#custom-auto-resolve]
==== Custom Auto-Resolve Handling

Expand Down
5 changes: 5 additions & 0 deletions doc/design/namespaced-elements.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,11 @@ The following rewrite-clj nodes throw an exception for `sexpr` which is sensible
- whitespace
- uneval, which is rewrite-clj's term for `#_`

The following nodes have different `sexpr` representations than if you called `read-string`:

- read eval
- syntax quote

=== Sexpr support for namespaced elements in rewrite-clj v0 and rewrite-cljs
_Auto-resolved_ keywords have been around https://groups.google.com/g/clojure/c/i770QaIFiF0/m/v63cZgrlBwAJ[since at least Clojure 1.0, which was released in May 2009].
https://github.com/clojure/clojure/blob/master/changes.md#12-support-for-working-with-maps-with-qualified-keys[Namespaced maps were introduced in Clojure 1.9, released in December 2017].
Expand Down
13 changes: 11 additions & 2 deletions script/test_libs.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,16 @@
(string/replace #"\[lein-zprint \"1.2.4\"\]"
"[lein-zprint \"1.2.4.1\"]")
(->> (spit p))))
(show-patch-diff lib))
;; zprint 1.2.9 has a single failing test for https://github.com/clj-commons/rewrite-clj/pull/306
;; Have raised this with over at zprint https://github.com/kkinnear/zprint/issues/333
;; and have agreement that it is a zprint issue.
;; Disable the failing test which starts on line 2538
(status/line :detail "Patching for failing test in v1.2.9")
(let [p (str (fs/file home-dir "test/zprint/guide_test.cljc"))
lines (-> p slurp string/split-lines)
new-lines (update lines 2537 #(str "#_" %))]
(->> (string/join "\n" new-lines)
(spit p))))

(defn- zprint-prep [{:keys [home-dir]}]
(status/line :detail "=> Building uberjar for uberjar tests")
Expand Down Expand Up @@ -449,7 +458,7 @@
:test-cmds ["lein test"]}
{:name "zprint"
:version "1.2.9"
:note "1) planck cljs tests disabled for now: https://github.com/planck-repl/planck/issues/1088"
:note "1) planck cljs tests disabled for now: https://github.com/planck-repl/planck/issues/1088 2) failing v1.2.9 test disabled"
:platforms [:clj :cljs]
:github-release {:repo "kkinnear/zprint"}
:patch-fn zprint-patch
Expand Down
4 changes: 2 additions & 2 deletions src/rewrite_clj/node/quote.cljc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@
[children]
(if (sequential? children)
(->node
:unquote "~" 'unquote
:unquote "~" 'clojure.core/unquote
children)
(recur [children])))

Expand All @@ -135,6 +135,6 @@
[children]
(if (sequential? children)
(->node
:unquote-splicing "~@" 'unquote-splicing
:unquote-splicing "~@" 'clojure.core/unquote-splicing
children)
(recur [children])))
2 changes: 1 addition & 1 deletion src/rewrite_clj/node/reader_macro.cljc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
(node-type [_node] :deref)
(printable-only?[_node] false)
(sexpr* [_node opts]
(list* 'deref (node/sexprs children opts)))
(list* 'clojure.core/deref (node/sexprs children opts)))
(length [_node]
(inc (node/sum-lengths children)))
(string [_node]
Expand Down
65 changes: 38 additions & 27 deletions test/rewrite_clj/parser_test.cljc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
(ns ^{:doc "Tests for EDN parser."
:author "Yannick Scherer"}
rewrite-clj.parser-test
(:refer-clojure :exclude [read-string])
(:require [clojure.string :as string]
[clojure.test :refer [deftest is]]
[clojure.tools.reader.edn :refer [read-string]]
[clojure.test :refer [deftest is testing]]
[clojure.tools.reader :as rdr]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to use clojure.tools.reader instead of clojure.tools.reader.edn for your tests to be valid?

Copy link
Contributor Author

@frenchy64 frenchy64 Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial motivation to tweak this was to automatically test this statement in the docs: "Within reason, Clojure’s read-string and rewrite-clj’s sexpr functions should return equivalent Clojure forms".

So that meant using clojure.core/read-string instead of clojure.tools.reader.edn/read-string. This is because:

  1. I interpreted "Clojure’s read-string" to mean clojure.core/read-string
  2. Things like '@~ are not valid edn, so the tests would not make sense.

Since CLJS doesn't have read-string, I replaced c.t.n with c.t.n.edn for reason number 2.

The main downside is *read-eval* can be dangerous, so I tried to be as conservative as possible in setting that.

I think perhaps the test in t-parsing-seqs could also test against clojure.core/read-string in the :default case for reason number 1.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, thanks for explaining, @frenchy64.

The thing is sci tests are currently failing.
Sci tests are a bit involved, would you like me to have a look-see?

[rewrite-clj.node :as node]
[rewrite-clj.parser :as p])
#?(:clj (:import [clojure.lang ExceptionInfo]
Expand Down Expand Up @@ -130,29 +129,40 @@
:default (is (Double/isNaN e)))))

(deftest t-parsing-reader-prefixed-data
(doseq [[s t ws sexpr]
[["@sym" :deref [] '(deref sym)]
["@ sym" :deref [:whitespace] '(deref sym)]
["'sym" :quote [] '(quote sym)]
["' sym" :quote [:whitespace] '(quote sym)]
["`sym" :syntax-quote [] '(quote sym)]
["` sym" :syntax-quote [:whitespace] '(quote sym)]
["~sym" :unquote [] '(unquote sym)]
["~ sym" :unquote [:whitespace] '(unquote sym)]
["~@sym" :unquote-splicing [] '(unquote-splicing sym)]
["~@ sym" :unquote-splicing [:whitespace] '(unquote-splicing sym)]
["#=sym" :eval [] '(eval 'sym)]
["#= sym" :eval [:whitespace] '(eval 'sym)]
["#'sym" :var [] '(var sym)]
["#'\nsym" :var [:newline] '(var sym)]]]
(let [n (p/parse-string s)
children (node/children n)
c (map node/tag children)]
(is (= t (node/tag n)))
(is (= :token (last c)))
(is (= sexpr (node/sexpr n)))
(is (= 'sym (node/sexpr (last children))))
(is (= ws (vec (butlast c)))))))
(doseq [[ s t ws sexpr ltag lcld]
[["@sym" :deref [] '@sym :token 'sym]
["@ sym" :deref [:whitespace] '@sym :token 'sym]
["'sym" :quote [] ''sym :token 'sym]
["' sym" :quote [:whitespace] ''sym :token 'sym]
["`sym" :syntax-quote [] ''sym :token 'sym]
["` sym" :syntax-quote [:whitespace] ''sym :token 'sym]
["~sym" :unquote [] '~sym :token 'sym]
["~ sym" :unquote [:whitespace] '~sym :token 'sym]
["~@sym" :unquote-splicing [] '~@sym :token 'sym]
["~@ sym" :unquote-splicing [:whitespace] '~@sym :token 'sym]
["~ @sym" :unquote [:whitespace] '~ @sym :deref '@sym]
["#=sym" :eval [] '(eval 'sym) :token 'sym]
["#= sym" :eval [:whitespace] '(eval 'sym) :token 'sym]
["#'sym" :var [] '#'sym :token 'sym]
["#'\nsym" :var [:newline] '#'sym :token 'sym]]]
(testing (pr-str s)
(let [n (p/parse-string s)
children (node/children n)
c (map node/tag children)]
(is (= t (node/tag n)) "tag")
(is (= ltag (last c)) "ltag")
(is (= sexpr (node/sexpr n)) "sexpr")
(is (= s (node/string n)) "string")
;; ` and #= return different sexpr's than via clojure.core/read-string
(when-not (#{:syntax-quote :eval} t)
(is (= sexpr
#?(:cljs (rdr/read-string s)
:default (binding [rdr/*read-eval* false] (rdr/read-string s)))
#?@(:cljs []
:default [(binding [*read-eval* false] (read-string s))]))
"read-string"))
(is (= lcld (node/sexpr (last children))) "lcld")
(is (= ws (vec (butlast c))) "ws")))))

(deftest t-eval
(let [n (p/parse-string "#=(+ 1 2)")]
Expand Down Expand Up @@ -223,7 +233,8 @@
fq (frequencies (map node/tag children))]
(is (= t (node/tag n)))
(is (= (string/trim s) (node/string n)))
(is (= (node/sexpr n) (read-string s)))
(is (= (node/sexpr n) #?(:cljs (rdr/read-string s)
:default (binding [rdr/*read-eval* false] (rdr/read-string s)))))
(is (= w (:whitespace fq 0)))
(is (= c (:token fq 0))))))

Expand Down