org-macro: Placeholders in (eval ...) macros are always strings

* lisp/org-macro.el (org-macro-initialize-templates): Update
  templates.
(org-macro-expand): Ensure placeholders in "eval" macros are strings.

* testing/lisp/test-org-macro.el (test-org/macro-replace-all): Update
  tests.

etc/ORG-NEWS

@@ -58,6 +58,29 @@ With the new template expansion mechanism (see
 [[*~org-insert-structure-template~]]), the variable changed its data type.
 See docstring for details.
 
+*** Placeholders in =(eval ...)= macros are always strings
+
+Within =(eval ...)= macros, =$1=-like placeholders are always replaced
+with a string.  As a consequence, they must not be enclosed within
+quotes. As an illustration, consider the following, now valid,
+examples:
+
+#+begin_example
+  ,#+macro: join (eval (concat $1 $2))
+  ,#+macro: sum (eval (+ (string-to-number $1) (string-to-number $2)))
+
+  {{{join(a,b)}}} => ab
+  {{{sum(1,2)}}}  => 3
+#+end_example
+
+However, there is no change in non-eval macros:
+
+#+begin_example
+  ,#+macro: disp argument: $1
+
+  {{{disp(text)}}} => argument: text
+#+end_example
+
 *** =align= STARTUP value no longer narrow table columns
 
 Columns narrowing (or shrinking) is now dynamic. See [[*Dynamically

lisp/org-macro.el

@@ -151,20 +151,20 @@ function installs the following ones: \"property\",
 		   (if (and (consp date)
 			    (not (cdr date))
 			    (eq (org-element-type (car date)) 'timestamp))
-		       (format "(eval (if (org-string-nw-p \"$1\") %s %S))"
-			       (format "(org-timestamp-format '%S \"$1\")"
+		       (format "(eval (if (org-string-nw-p $1) %s %S))"
+			       (format "(org-timestamp-format '%S $1)"
 				       (org-element-copy (car date)))
 			       value)
 		     value)))
 	   (cons "email" (org-macro--find-keyword-value "EMAIL"))
-	   (cons "keyword" "(eval (org-macro--find-keyword-value \"$1\"))")
+	   (cons "keyword" "(eval (org-macro--find-keyword-value $1))")
 	   (cons "results" "$1")
 	   (cons "title" (org-macro--find-keyword-value "TITLE"))))
     ;; Install "property", "time" macros.
     (mapc update-templates
 	  (list (cons "property"
 		      "(eval (save-excursion
-        (let ((l \"$2\"))
+        (let ((l $2))
           (when (org-string-nw-p l)
             (condition-case _
                 (let ((org-link-search-must-match-exact-headline t))
@@ -172,8 +172,8 @@ function installs the following ones: \"property\",
               (error
                (error \"Macro property failed: cannot find location %s\"
                       l)))))
-        (org-entry-get nil \"$1\" 'selective)))")
-		(cons "time" "(eval (format-time-string \"$1\"))")))
+        (org-entry-get nil $1 'selective)))")
+		(cons "time" "(eval (format-time-string $1))")))
     ;; Install "input-file", "modification-time" macros.
     (let ((visited-file (buffer-file-name (buffer-base-buffer))))
       (when (and visited-file (file-exists-p visited-file))
@@ -181,8 +181,8 @@ function installs the following ones: \"property\",
 	      (list (cons "input-file" (file-name-nondirectory visited-file))
 		    (cons "modification-time"
 			  (format "(eval
-\(format-time-string \"$1\"
-                     (or (and (org-string-nw-p \"$2\")
+\(format-time-string $1
+                     (or (and (org-string-nw-p $2)
                               (org-macro--vc-modified-time %s))
                      '%s)))"
 				  (prin1-to-string visited-file)
@@ -191,7 +191,7 @@ function installs the following ones: \"property\",
     ;; Initialize and install "n" macro.
     (org-macro--counter-initialize)
     (funcall update-templates
-	     (cons "n" "(eval (org-macro--counter-increment \"$1\" \"$2\"))"))
+	     (cons "n" "(eval (org-macro--counter-increment $1 $2))"))
     (setq org-macro-templates templates)))
 
 (defun org-macro-expand (macro templates)
@@ -204,18 +204,22 @@ default value.  Return nil if no template was found."
 	 ;; Macro names are case-insensitive.
 	 (cdr (assoc-string (org-element-property :key macro) templates t))))
     (when template
-      (let ((value (replace-regexp-in-string
-                    "\\$[0-9]+"
-                    (lambda (arg)
-                      (or (nth (1- (string-to-number (substring arg 1)))
-                               (org-element-property :args macro))
-                          ;; No argument: remove place-holder.
-                          ""))
-                    template nil 'literal)))
-        ;; VALUE starts with "(eval": it is a s-exp, `eval' it.
-        (when (string-match "\\`(eval\\>" value)
-          (setq value (eval (read value))))
-        ;; Return string.
+      (let* ((eval? (string-match-p "\\`(eval\\>" template))
+	     (value
+	      (replace-regexp-in-string
+	       "\\$[0-9]+"
+	       (lambda (m)
+		 (let ((arg (or (nth (1- (string-to-number (substring m 1)))
+				     (org-element-property :args macro))
+				;; No argument: remove place-holder.
+				"")))
+		   ;; `eval' implies arguments are strings.
+		   (if eval? (format "%S" arg) arg)))
+	       template nil 'literal)))
+        (when eval?
+          (setq value (eval (condition-case nil (read value)
+			      (error (debug))))))
+        ;; Force return value to be a string.
         (format "%s" (or value ""))))))
 
 (defun org-macro-replace-all (templates &optional keywords)

testing/lisp/test-org-macro.el

@@ -29,9 +29,9 @@
    (equal
     "#+MACRO: A B\n1 B 3"
     (org-test-with-temp-text "#+MACRO: A B\n1 {{{A}}} 3"
-      (progn (org-macro-initialize-templates)
-             (org-macro-replace-all org-macro-templates)
-             (buffer-string)))))
+      (org-macro-initialize-templates)
+      (org-macro-replace-all org-macro-templates)
+      (buffer-string))))
   ;; Macro with arguments.
   (should
    (equal
@@ -43,20 +43,22 @@
   ;; Macro with "eval".
   (should
    (equal
-    "#+MACRO: add (eval (+ $1 $2))\n3"
-    (org-test-with-temp-text "#+MACRO: add (eval (+ $1 $2))\n{{{add(1,2)}}}"
-      (progn (org-macro-initialize-templates)
-             (org-macro-replace-all org-macro-templates)
-             (buffer-string)))))
+    "3"
+    (org-test-with-temp-text
+	"#+MACRO: add (eval (+ (string-to-number $1) (string-to-number $2)))
+<point>{{{add(1,2)}}}"
+      (org-macro-initialize-templates)
+      (org-macro-replace-all org-macro-templates)
+      (buffer-substring-no-properties (point) (line-end-position)))))
   ;; Nested macros.
   (should
    (equal
     "#+MACRO: in inner\n#+MACRO: out {{{in}}} outer\ninner outer"
     (org-test-with-temp-text
         "#+MACRO: in inner\n#+MACRO: out {{{in}}} outer\n{{{out}}}"
-      (progn (org-macro-initialize-templates)
-             (org-macro-replace-all org-macro-templates)
-             (buffer-string)))))
+      (org-macro-initialize-templates)
+      (org-macro-replace-all org-macro-templates)
+      (buffer-string))))
   ;; Error out when macro expansion is circular.
   (should-error
    (org-test-with-temp-text