master cde38f0df3f 3/3: Avoid parsing some Eshell forms when performing completion

[Jim Porter]

branch: master
commit cde38f0df3fa3540ce411a48d95da1c2f1be1b60
Author: Jim Porter <jporterbugs@gmail.com>
Commit: Jim Porter <jporterbugs@gmail.com>

    Avoid parsing some Eshell forms when performing completion
    
    During completion, we want to evaluate most Eshell forms
    (e.g. variable references), but skip others (e.g. globbing,
    subcommands).  For globbing, we want to pass the literal glob to
    Pcomplete so it can use the glob for selecting completion candidates.
    For subcommands (including Lisp forms), we especially want to avoid
    evaluation, since they can produce arbitary side effects!  (Bug#50470)
    
    * lisp/eshell/esh-cmd.el (eshell-allow-commands): New variable...
    (eshell-commands-forbidden): New error...
    (eshell-named-command, eshell-lisp-command): ... use them.
    
    * lisp/eshell/em-cmpl.el (eshell-complete--eval-argument-form):
    Disallow command forms and handle errors ourselves.
    (eshell-complete-parse-arguments): Don't parse glob characters.
    
    * test/lisp/eshell/em-cmpl-tests.el
    (em-cmpl-test/parse-arguments/unevaluated-subcommand)
    (em-cmpl-test/parse-arguments/unevaluated-lisp-form)
    (em-cmpl-test/parse-arguments/unevaluated-inner-subcommand)
    (em-cmpl-test/file-completion/glob, em-cmpl-test/command-completion)
    (em-cmpl-test/subcommand-completion): New tests.
    (em-cmpl-test/parse-arguments/pipeline): Remove superfluous
    let-binding.
    (em-cmpl-test/file-completion/after-list): Use a list variable rather
    than a subexpression; the latter is no longer evaluated during
    completion.
    (em-cmpl-test/lisp-function-completion): Check "$(func)" syntax.
---
 lisp/eshell/em-cmpl.el            | 60 ++++++++++++++++++---------
 lisp/eshell/esh-cmd.el            | 15 +++++++
 test/lisp/eshell/em-cmpl-tests.el | 86 ++++++++++++++++++++++++++++++++++-----
 3 files changed, 131 insertions(+), 30 deletions(-)

diff --git a/lisp/eshell/em-cmpl.el b/lisp/eshell/em-cmpl.el
index b65652019d4..732bbb3f1fa 100644
--- a/lisp/eshell/em-cmpl.el
+++ b/lisp/eshell/em-cmpl.el
@@ -306,9 +306,24 @@ to writing a completion function."
 
 (defun eshell-complete--eval-argument-form (arg)
   "Evaluate a single Eshell argument form ARG for the purposes of completion."
-  (let ((result (eshell-do-eval `(eshell-commands ,arg) t)))
-    (cl-assert (eq (car result) 'quote))
-    (cadr result)))
+  (condition-case err
+      (let* (;; Don't allow running commands; they could have
+             ;; arbitrary side effects, which we don't want when we're
+             ;; just performing completions!
+             (eshell-allow-commands)
+             ;; Handle errors ourselves so that we can properly catch
+             ;; `eshell-commands-forbidden'.
+             (eshell-handle-errors)
+             (result (eshell-do-eval `(eshell-commands ,arg) t)))
+        (cl-assert (eq (car result) 'quote))
+        (cadr result))
+    (eshell-commands-forbidden
+     (propertize "\0" 'eshell-argument-stub
+                 (intern (format "%s-command" (cadr err)))))
+    (error
+     (lwarn 'eshell :error
+            "Failed to evaluate argument form during completion: %S" arg)
+     (propertize "\0" 'eshell-argument-stub 'error))))
 
 (defun eshell-complete-parse-arguments ()
   "Parse the command line arguments for `pcomplete-argument'."
@@ -325,23 +340,28 @@ to writing a completion function."
       (if (= begin end)
          (end-of-line))
       (setq end (point-marker)))
-    (if (setq delim
-             (catch 'eshell-incomplete
-               (ignore
-                (setq args (eshell-parse-arguments begin end)))))
-        (cond ((member (car delim) '("{" "${" "$<"))
-              (setq begin (1+ (cadr delim))
-                    args (eshell-parse-arguments begin end)))
-              ((member (car delim) '("$'" "$\"" "#<"))
-               ;; Add the (incomplete) argument to our arguments, and
-               ;; note its position.
-               (setq args (append (nth 2 delim) (list (car delim)))
-                     incomplete-arg t)
-               (push (- (nth 1 delim) 2) posns))
-              ((member (car delim) '("(" "$("))
-              (throw 'pcompleted (elisp-completion-at-point)))
-             (t
-              (eshell--pcomplete-insert-tab))))
+    ;; Don't expand globs when parsing arguments; we want to pass any
+    ;; globs to Pcomplete unaltered.
+    (declare-function eshell-parse-glob-chars "em-glob" ())
+    (let ((eshell-parse-argument-hook (remq #'eshell-parse-glob-chars
+                                            eshell-parse-argument-hook)))
+      (if (setq delim
+               (catch 'eshell-incomplete
+                 (ignore
+                  (setq args (eshell-parse-arguments begin end)))))
+          (cond ((member (car delim) '("{" "${" "$<"))
+                (setq begin (1+ (cadr delim))
+                      args (eshell-parse-arguments begin end)))
+                ((member (car delim) '("$'" "$\"" "#<"))
+                 ;; Add the (incomplete) argument to our arguments, and
+                 ;; note its position.
+                 (setq args (append (nth 2 delim) (list (car delim)))
+                       incomplete-arg t)
+                 (push (- (nth 1 delim) 2) posns))
+                ((member (car delim) '("(" "$("))
+                (throw 'pcompleted (elisp-completion-at-point)))
+               (t
+                (eshell--pcomplete-insert-tab)))))
     (when (get-text-property (1- end) 'comment)
       (eshell--pcomplete-insert-tab))
     (let ((pos (1- end)))
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 1a458290dfe..d5237ee1f04 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -293,6 +293,17 @@ CDR are the same process.
 
 When the process in the CDR completes, resume command evaluation.")
 
+(defvar eshell-allow-commands t
+  "If non-nil, allow evaluating command forms (including Lisp forms).
+If you want to forbid command forms, you can let-bind this to a
+non-nil value before calling `eshell-do-eval'.  Then, any command
+forms will signal `eshell-commands-forbidden'.  This is useful
+if, for example, you want to evaluate simple expressions like
+variable expansions, but not fully-evaluate the command.  See
+also `eshell-complete-parse-arguments'.")
+
+(define-error 'eshell-commands-forbidden "Commands forbidden")
+
 ;;; Functions:
 
 (defsubst eshell-interactive-process-p ()
@@ -1328,6 +1339,8 @@ have been replaced by constants."
 (defun eshell-named-command (command &optional args)
   "Insert output from a plain COMMAND, using ARGS.
 COMMAND may result in an alias being executed, or a plain command."
+  (unless eshell-allow-commands
+    (signal 'eshell-commands-forbidden '(named)))
   (setq eshell-last-arguments args
        eshell-last-command-name (eshell-stringify command))
   (run-hook-with-args 'eshell-prepare-command-hook)
@@ -1465,6 +1478,8 @@ via `eshell-errorn'."
 
 (defun eshell-lisp-command (object &optional args)
   "Insert Lisp OBJECT, using ARGS if a function."
+  (unless eshell-allow-commands
+    (signal 'eshell-commands-forbidden '(lisp)))
   (catch 'eshell-external               ; deferred to an external command
     (setq eshell-last-command-status 0
           eshell-last-arguments args)
diff --git a/test/lisp/eshell/em-cmpl-tests.el 
b/test/lisp/eshell/em-cmpl-tests.el
index ea907f1945d..29a41625d5e 100644
--- a/test/lisp/eshell/em-cmpl-tests.el
+++ b/test/lisp/eshell/em-cmpl-tests.el
@@ -69,11 +69,10 @@ ACTUAL and EXPECTED should both be lists of strings."
 (ert-deftest em-cmpl-test/parse-arguments/pipeline ()
   "Test that parsing arguments for completion discards earlier commands."
   (with-temp-eshell
-   (let ((eshell-test-value '("foo" "bar")))
-     (insert "echo hi | cat")
-     (should (eshell-arguments-equal
-              (car (eshell-complete-parse-arguments))
-              '("cat"))))))
+   (insert "echo hi | cat")
+   (should (eshell-arguments-equal
+            (car (eshell-complete-parse-arguments))
+            '("cat")))))
 
 (ert-deftest em-cmpl-test/parse-arguments/multiple-dots ()
   "Test parsing arguments with multiple dots like \".../\"."
@@ -123,6 +122,45 @@ ACTUAL and EXPECTED should both be lists of strings."
               (car (eshell-complete-parse-arguments))
               '("echo" "foo" "bar"))))))
 
+(ert-deftest em-cmpl-test/parse-arguments/unevaluated-subcommand ()
+  "Test that subcommands return a stub when parsing for completion."
+  (with-temp-eshell
+   (insert "echo {echo hi}")
+   (should (eshell-arguments-equal
+            (car (eshell-complete-parse-arguments))
+            `("echo" ,(propertize
+                       "\0" 'eshell-argument-stub 'named-command)))))
+  (with-temp-eshell
+   (insert "echo ${echo hi}")
+   (should (eshell-arguments-equal
+            (car (eshell-complete-parse-arguments))
+            `("echo" ,(propertize
+                       "\0" 'eshell-argument-stub 'named-command))))))
+
+(ert-deftest em-cmpl-test/parse-arguments/unevaluated-lisp-form ()
+  "Test that Lisp forms return a stub when parsing for completion."
+  (with-temp-eshell
+   (insert "echo (concat \"hi\")")
+   (should (eshell-arguments-equal
+            (car (eshell-complete-parse-arguments))
+            `("echo" ,(propertize
+                       "\0" 'eshell-argument-stub 'lisp-command)))))
+  (with-temp-eshell
+   (insert "echo $(concat \"hi\")")
+   (should (eshell-arguments-equal
+            (car (eshell-complete-parse-arguments))
+            `("echo" ,(propertize
+                       "\0" 'eshell-argument-stub 'lisp-command))))))
+
+(ert-deftest em-cmpl-test/parse-arguments/unevaluated-inner-subcommand ()
+  "Test that nested subcommands return a stub when parsing for completion."
+  (with-temp-eshell
+   (insert "echo $exec-path[${echo 0}]")
+   (should (eshell-arguments-equal
+            (car (eshell-complete-parse-arguments))
+            `("echo" ,(propertize
+                       "\0" 'eshell-argument-stub 'named-command))))))
+
 (ert-deftest em-cmpl-test/file-completion/unique ()
   "Test completion of file names when there's a unique result."
   (with-temp-eshell
@@ -150,14 +188,39 @@ ACTUAL and EXPECTED should both be lists of strings."
          (forward-line -1)
          (should (looking-at "Complete, but not unique")))))))
 
+(ert-deftest em-cmpl-test/file-completion/glob ()
+  "Test completion of file names using a glob."
+  (with-temp-eshell
+   (ert-with-temp-directory default-directory
+     (write-region nil nil (expand-file-name "file.txt"))
+     (write-region nil nil (expand-file-name "file.el"))
+     (should (equal (eshell-insert-and-complete "echo fi*.el")
+                    "echo file.el ")))))
+
 (ert-deftest em-cmpl-test/file-completion/after-list ()
   "Test completion of file names after previous list arguments.
 See bug#59956."
   (with-temp-eshell
-   (ert-with-temp-directory default-directory
-     (write-region nil nil (expand-file-name "file.txt"))
-     (should (equal (eshell-insert-and-complete "echo (list 1 2) fi")
-                    "echo (list 1 2) file.txt ")))))
+   (let ((eshell-test-value '("foo" "bar")))
+     (ert-with-temp-directory default-directory
+       (write-region nil nil (expand-file-name "file.txt"))
+       (should (equal (eshell-insert-and-complete "echo $eshell-test-value fi")
+                      "echo $eshell-test-value file.txt "))))))
+
+(ert-deftest em-cmpl-test/command-completion ()
+  "Test completion of command names like \"command\"."
+  (with-temp-eshell
+   (should (equal (eshell-insert-and-complete "listif")
+                  "listify "))))
+
+(ert-deftest em-cmpl-test/subcommand-completion ()
+  "Test completion of command names like \"{command}\"."
+  (with-temp-eshell
+   (should (equal (eshell-insert-and-complete "{ listif")
+                  "{ listify ")))
+  (with-temp-eshell
+   (should (equal (eshell-insert-and-complete "echo ${ listif")
+                  "echo ${ listify "))))
 
 (ert-deftest em-cmpl-test/lisp-symbol-completion ()
   "Test completion of Lisp forms like \"#'symbol\" and \"`symbol\".
@@ -174,7 +237,10 @@ See <lisp/eshell/esh-cmd.el>."
 See <lisp/eshell/esh-cmd.el>."
   (with-temp-eshell
    (should (equal (eshell-insert-and-complete "echo (eshell/ech")
-                  "echo (eshell/echo"))))
+                  "echo (eshell/echo")))
+  (with-temp-eshell
+   (should (equal (eshell-insert-and-complete "echo $(eshell/ech")
+                  "echo $(eshell/echo"))))
 
 (ert-deftest em-cmpl-test/special-ref-completion/type ()
   "Test completion of the start of special references like \"#<buffer\".