Revert to v0 behaviour for string coercion (#223)

As in v0, strings are once again coerced to token nodes (instead of string nodes).

Effective fixes for coerced strings:
-  character escaping handled automatically
-  string "a\nb\nc" is no longer interpreted as a string appearing on multiple lines.

Bring some clarity to rewrite-clj.node/string-node's usage and
expectations via its docstring.

Alternative to PRs #216 and #217.

Fixes #214

Author: lread

CHANGELOG.adoc

@@ -16,6 +16,22 @@ For a list of breaking changes see link:#v1-breaking[breaking changes].
 
 === Unreleased
 
+* Clojure string to rewrite-clj node coercion fixes
+https://github.com/clj-commons/rewrite-clj/issues/214[#214]
+(@borkdude/@lread collab)
+** `"a\n\b\r\nc"` is now preserved as is.
+It was being coerced to:
++
+[source,clojure]
+----
+"a
+b
+c"
+----
+** escaped characters now coerce correctly.
+Previously we were only handling `\"`.
+Strings like `"\\s+"` now handled and preserved.
+
 === v1.1.46
 
 * added new `rewrite-clj.zip` functions `of-string*` and `of-file*`, these are versions of `of-string` and `of-file` that do no auto-navigation

src/rewrite_clj/node.cljc

@@ -657,7 +657,23 @@
 (defn string-node
   "Create node representing a string value where `lines` can be a sequence of strings or a single string.
 
-  When `lines` is a sequence, the resulting node will `tag` will be `:multi-line`, otherwise `:token`.
+  When `lines` is a sequence, the resulting node `tag` will be `:multi-line`, otherwise `:token`.
+
+  `:multi-line` refers to a single string in your source that appears over multiple lines, for example:
+
+  ```Clojure
+  (def s \"foo
+            bar
+              baz\")
+  ```
+
+  It does not apply to a string that appears on a single line that includes escaped newlines, for example:
+
+  ```Clojure
+  (def s \"foo\\nbar\\n\\baz\")
+  ```
+
+  Naive examples (see example on escaping below):
 
   ```Clojure
   (require '[rewrite-clj.node :as n])
@@ -669,7 +685,31 @@
   (-> (n/string-node [\"line1\" \"\" \"line3\"])
        n/string)
   ;; => \"\\\"line1\\n\\nline3\\\"\"
-  ```"
+  ```
+
+  This function was originally written to serve the rewrite-clj parser.
+  Escaping and wrapping expectations are non-obvious.
+  - characters within strings are assumed to be escaped
+  - but the string should not wrapped with `\\\"`
+
+  Here's an example of conforming to these expectations for a string that has escape sequences.
+  (Best to view this on cljdoc, docstring string escaping is confusing).
+
+  ```Clojure
+  (require '[clojure.string :as string])
+
+  (defn pr-str-unwrapped [s]
+    (apply str (-> s pr-str next butlast)))
+
+  (-> \"hey \\\" man\"
+      pr-str-unwrapped
+      n/string-node
+      n/string)
+  ;; => \"\\\"hey \\\\\\\" man\\\"\"
+  ```
+
+  To construct strings appearing on a single line, consider [[token-node]].
+  It will handle escaping for you."
   [lines] (rewrite-clj.node.stringz/string-node lines))
 
 ;; DO NOT EDIT FILE, automatically imported from: rewrite-clj.node.quote
@@ -768,7 +808,12 @@
 
    (-> (n/token-node 42) n/string)
    ;; => \"42\"
-   ```"
+
+   (-> (n/token-node \"astring\") n/string)
+   ;; => \"\\\"astring\\\"\"
+   ```
+
+   To construct strings appearing over multiple lines, see [[string-node]]."
   ([value] (rewrite-clj.node.token/token-node value))
   ([value string-value] (rewrite-clj.node.token/token-node value string-value)))
 

src/rewrite_clj/node/coercer.cljc

@@ -8,7 +8,6 @@
        if one interface is derived from the other, the more derived is used,
        else which one is used is unspecified"
   (:require
-   [clojure.string :as string]
    #?@(:clj
        [[rewrite-clj.node.comment]
         [rewrite-clj.node.fn]
@@ -22,12 +21,13 @@
         [rewrite-clj.node.reader-macro :refer [reader-macro-node var-node]]
         [rewrite-clj.node.regex :refer [regex-node pattern-string-for-regex]]
         [rewrite-clj.node.seq :refer [vector-node list-node set-node map-node]]
-        [rewrite-clj.node.stringz :refer [string-node]]
+        [rewrite-clj.node.stringz]
         [rewrite-clj.node.token :refer [token-node]]
         [rewrite-clj.node.uneval]
         [rewrite-clj.node.whitespace :as ws]]
        :cljs
-       [[rewrite-clj.node.comment :refer [CommentNode]]
+       [[clojure.string :as string]
+        [rewrite-clj.node.comment :refer [CommentNode]]
         [rewrite-clj.node.fn :refer [FnNode]]
         [rewrite-clj.node.forms :refer [FormsNode]]
         [rewrite-clj.node.integer :refer [IntNode]]
@@ -39,7 +39,7 @@
         [rewrite-clj.node.reader-macro :refer [ReaderNode ReaderMacroNode DerefNode reader-macro-node var-node]]
         [rewrite-clj.node.regex :refer [RegexNode regex-node pattern-string-for-regex]]
         [rewrite-clj.node.seq :refer [SeqNode vector-node list-node set-node map-node]]
-        [rewrite-clj.node.stringz :refer [StringNode string-node]]
+        [rewrite-clj.node.stringz :refer [StringNode]]
         [rewrite-clj.node.token :refer [TokenNode SymbolNode token-node]]
         [rewrite-clj.node.uneval :refer [UnevalNode]]
         [rewrite-clj.node.whitespace :refer [WhitespaceNode CommaNode NewlineNode] :as ws]]))
@@ -90,20 +90,6 @@
 
 ;; ## Helpers
 
-(defn- split-to-lines
-  "Slightly different than string/split-lines in that:
-   - escape inline double quotes (to emulate the clojure reader)
-   - includes all lines even if empty
-   - behaves the same on clj and cljs"
-  [s]
-  (loop [s (string/escape s {\" "\\\""})
-         lines []]
-    (if-let [m (first (re-find #"(\r\n|\r|\n)" s))]
-      (let [eol-ndx (string/index-of s m)]
-        (recur (subs s (+ eol-ndx (count m)))
-               (conj lines (subs s 0 eol-ndx))))
-      (conj lines s))))
-
 (defn node-with-meta
   [n value]
   (if #?(:clj (instance? clojure.lang.IMeta value)
@@ -149,8 +135,13 @@
 
 (extend-protocol NodeCoerceable
   #?(:clj java.lang.String :cljs string)
+  ;; You might we should be coercing to a string-node here.
+  ;; We did this initially for rewrite-clj v1 but found it made more sense to revert to v0 behaviour.
+  ;; The string-node was created to serve the parser and expects the strings to be escaped in a particular way.
+  ;; The token-node has no such expectations and is therefore, currently, what we use for coercing strings.
+  ;; If this makes future internal changes awkward we'll revisit.
   (coerce [v]
-    (string-node (split-to-lines v))))
+    (token-node v)))
 
 #?(:clj
    (extend-protocol NodeCoerceable

src/rewrite_clj/node/string.clj

@@ -11,7 +11,23 @@
 (defn string-node
   "Create node representing a string value where `lines` can be a sequence of strings or a single string.
 
-  When `lines` is a sequence, the resulting node will `tag` will be `:multi-line`, otherwise `:token`.
+  When `lines` is a sequence, the resulting node `tag` will be `:multi-line`, otherwise `:token`.
+
+  `:multi-line` refers to a single string in your source that appears over multiple lines, for example:
+
+  ```Clojure
+  (def s \"foo
+            bar
+              baz\")
+  ```
+
+  It does not apply to a string that appears on a single line that includes escaped newlines, for example:
+
+  ```Clojure
+  (def s \"foo\\nbar\\n\\baz\")
+  ```
+
+  Naive examples (see example on escaping below):
 
   ```Clojure
   (require '[rewrite-clj.node :as n])
@@ -23,5 +39,29 @@
   (-> (n/string-node [\"line1\" \"\" \"line3\"])
        n/string)
   ;; => \"\\\"line1\\n\\nline3\\\"\"
-  ```"
+  ```
+
+  This function was originally written to serve the rewrite-clj parser.
+  Escaping and wrapping expectations are non-obvious.
+  - characters within strings are assumed to be escaped
+  - but the string should not wrapped with `\\\"`
+
+  Here's an example of conforming to these expectations for a string that has escape sequences.
+  (Best to view this on cljdoc, docstring string escaping is confusing).
+
+  ```Clojure
+  (require '[clojure.string :as string])
+
+  (defn pr-str-unwrapped [s]
+    (apply str (-> s pr-str next butlast)))
+
+  (-> \"hey \\\" man\"
+      pr-str-unwrapped
+      n/string-node
+      n/string)
+  ;; => \"\\\"hey \\\\\\\" man\\\"\"
+  ```
+
+  To construct strings appearing on a single line, consider [[token-node]].
+  It will handle escaping for you."
   [lines] (rewrite-clj.node.stringz/string-node lines))

src/rewrite_clj/node/stringz.cljc

@@ -42,7 +42,23 @@
 (defn string-node
   "Create node representing a string value where `lines` can be a sequence of strings or a single string.
 
-  When `lines` is a sequence, the resulting node will `tag` will be `:multi-line`, otherwise `:token`.
+  When `lines` is a sequence, the resulting node `tag` will be `:multi-line`, otherwise `:token`.
+
+  `:multi-line` refers to a single string in your source that appears over multiple lines, for example:
+
+  ```Clojure
+  (def s \"foo
+            bar
+              baz\")
+  ```
+
+  It does not apply to a string that appears on a single line that includes escaped newlines, for example:
+
+  ```Clojure
+  (def s \"foo\\nbar\\n\\baz\")
+  ```
+
+  Naive examples (see example on escaping below):
 
   ```Clojure
   (require '[rewrite-clj.node :as n])
@@ -54,7 +70,31 @@
   (-> (n/string-node [\"line1\" \"\" \"line3\"])
        n/string)
   ;; => \"\\\"line1\\n\\nline3\\\"\"
-  ```"
+  ```
+
+  This function was originally written to serve the rewrite-clj parser.
+  Escaping and wrapping expectations are non-obvious.
+  - characters within strings are assumed to be escaped
+  - but the string should not wrapped with `\\\"`
+
+  Here's an example of conforming to these expectations for a string that has escape sequences.
+  (Best to view this on cljdoc, docstring string escaping is confusing).
+
+  ```Clojure
+  (require '[clojure.string :as string])
+
+  (defn pr-str-unwrapped [s]
+    (apply str (-> s pr-str next butlast)))
+
+  (-> \"hey \\\" man\"
+      pr-str-unwrapped
+      n/string-node
+      n/string)
+  ;; => \"\\\"hey \\\\\\\" man\\\"\"
+  ```
+
+  To construct strings appearing on a single line, consider [[token-node]].
+  It will handle escaping for you."
   [lines]
   (if (string? lines)
     (->StringNode [lines])

src/rewrite_clj/node/token.cljc

@@ -79,7 +79,12 @@
 
    (-> (n/token-node 42) n/string)
    ;; => \"42\"
-   ```"
+
+   (-> (n/token-node \"astring\") n/string)
+   ;; => \"\\\"astring\\\"\"
+   ```
+
+   To construct strings appearing over multiple lines, see [[string-node]]."
   ([value]
    (token-node value (pr-str value)))
   ([value string-value]

test/rewrite_clj/node/coercer_test.cljc

@@ -41,14 +41,15 @@
       ::1.5.1                :token      :keyword
       :namespace/keyword     :token      :keyword
 
-      ""                     :token      :string
-      "hello, over there!"   :token      :string
-      "multi\nline"          :multi-line :string
-      " "                    :token      :string
-      "\n"                   :multi-line :string
-      "\n\n"                 :multi-line :string
-      ","                    :token      :string
-      "inner\"quote"         :token      :string
+      ""                     :token      :token
+      "hello, over there!"   :token      :token
+      "multi\nline"          :token      :token
+      " "                    :token      :token
+      "\n"                   :token      :token
+      "\n\n"                 :token      :token
+      ","                    :token      :token
+      "inner\"quote"         :token      :token
+      "\\s+"                 :token      :token
 
       ;; seqs
       []                     :vector     :seq
@@ -63,10 +64,10 @@
 
       ;; date
       #inst "2014-11-26T00:05:23" :token :token))
-  (testing "multi-line string newline variants are normalized"
+  (testing "multi-line string newline variants are preserved"
     (let [s "hey\nyou\rover\r\nthere"
           n (node/coerce s)]
-      (is (= "hey\nyou\nover\nthere" (node/sexpr n)))))
+      (is (= s (node/sexpr n)))))
   (testing "coerce string roundtrip"
     (is (= "\"hey \\\" man\"" (-> "hey \" man" node/coerce node/string)))))