bug#50470: 27.1; 'company-mode' 'eshell'

[Jim Porter <jporterbugs@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

On 3/19/2023 6:34 PM, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> I definitely don't want to force preloading that module.
> But maybe that var could have a meaning that's independent
> from completion, thus justifying to move it out of the completion
> extension module?

Well, luckily(?) it turns out my patch wasn't quite right anyway, so I 
completely rewrote it. (In particular, it didn't correctly generate a 
top-level stub if there was a subcommand nested somewhere *inside* an 
argument.)

With this change, we now have a more-general way of preventing commands 
that can cause side effects: 'eshell-allow-commands'. We can let-bind 
that to nil, and then any commands within an argument will signal an error.

Then we just need to disable globbing via a different method (using the 
patch I originally posted), and all is well for this bug.

I also added a couple preliminary patches to fix some semi-related 
issues I discovered while working on this. These could probably go in a 
separate bug, but I'm lazy. ;) The real meat of this change is patch 0003.

[-- Attachment #2: 0001-Fix-an-edge-case-in-how-eshell-do-eval-handles-let-b.patch --]
[-- Type: text/plain, Size: 998 bytes --]

From 499dd578d8072e56d1268797d5407d021c4f1d93 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 20 Mar 2023 17:24:28 -0700
Subject: [PATCH 1/3] ; Fix an edge case in how 'eshell-do-eval' handles 'let'
 bodies

* lisp/eshell/esh-cmd.el (ehell-do-eval): Use 'car-safe'; the object
in question might not be a cons cell.
---
 lisp/eshell/esh-cmd.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 93f2616020c..e0651b76249 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -1168,7 +1168,7 @@ eshell-do-eval
 	(setcar (cdr args) (eshell-do-eval (cadr args) synchronous-p))
 	(eval form))
        ((eq (car form) 'let)
-        (when (not (eq (car (cadr args)) 'eshell-do-eval))
+        (unless (eq (car-safe (cadr args)) 'eshell-do-eval)
           (eshell-manipulate "evaluating let args"
             (dolist (letarg (car args))
               (when (and (listp letarg)
-- 
2.25.1


[-- Attachment #3: 0002-Simplify-parsing-subcommands-slightly.patch --]
[-- Type: text/plain, Size: 6951 bytes --]

From 1b6c988ed697de9c30690aa69c3fc5a5f305a342 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 20 Mar 2023 17:25:24 -0700
Subject: [PATCH 2/3] Simplify parsing subcommands slightly

This mainly reduces some overly-deep indentation, but also fixes some
minor issues with the "$<subcmd>" form: it unnecessarily added " >
TEMP" (we already set this later via 'eshell-create-handles'), and it
didn't properly unescape inner double quotes.

* lisp/eshell/esh-cmd.el (eshell-parse-subcommand-argument): Simplify.

* lisp/eshell/esh-var.el (eshell-parse-variable-ref): Simplify and
fix edge cases in "$<subcmd>".
---
 lisp/eshell/esh-cmd.el | 14 +++----
 lisp/eshell/esh-var.el | 95 +++++++++++++++++++++---------------------
 2 files changed, 55 insertions(+), 54 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index e0651b76249..1a458290dfe 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -675,13 +675,13 @@ eshell-parse-subcommand-argument
 	   (or (= (point-max) (1+ (point)))
 	       (not (eq (char-after (1+ (point))) ?\}))))
       (let ((end (eshell-find-delimiter ?\{ ?\})))
-	(if (not end)
-            (throw 'eshell-incomplete "{")
-	  (when (eshell-arg-delimiter (1+ end))
-	    (prog1
-		`(eshell-as-subcommand
-                  ,(eshell-parse-command (cons (1+ (point)) end)))
-	      (goto-char (1+ end))))))))
+        (unless end
+          (throw 'eshell-incomplete "{"))
+        (when (eshell-arg-delimiter (1+ end))
+          (prog1
+              `(eshell-as-subcommand
+                ,(eshell-parse-command (cons (1+ (point)) end)))
+            (goto-char (1+ end)))))))
 
 (defun eshell-parse-lisp-argument ()
   "Parse a Lisp expression which is specified as an argument."
diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index 5d6299af564..7dcaff1e24f 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -507,55 +507,56 @@ eshell-parse-variable-ref
   (cond
    ((eq (char-after) ?{)
     (let ((end (eshell-find-delimiter ?\{ ?\})))
-      (if (not end)
-          (throw 'eshell-incomplete "${")
-        (forward-char)
-        (prog1
-            `(eshell-apply-indices
-              (eshell-convert
-               (eshell-command-to-value
-                (eshell-as-subcommand
-                 ,(let ((subcmd (or (eshell-unescape-inner-double-quote end)
-                                    (cons (point) end)))
-                        (eshell-current-quoted nil))
-                    (eshell-parse-command subcmd))))
-               ;; If this is a simple double-quoted form like
-               ;; "${COMMAND}" (i.e. no indices after the subcommand
-               ;; and no `#' modifier before), ensure we convert to a
-               ;; single string.  This avoids unnecessary work
-               ;; (e.g. splitting the output by lines) when it would
-               ;; just be joined back together afterwards.
-               ,(when (and (not modifier-p) eshell-current-quoted)
-                  '(not indices)))
-              indices ,eshell-current-quoted)
-          (goto-char (1+ end))))))
+      (unless end
+        (throw 'eshell-incomplete "${"))
+      (forward-char)
+      (prog1
+          `(eshell-apply-indices
+            (eshell-convert
+             (eshell-command-to-value
+              (eshell-as-subcommand
+               ,(let ((subcmd (or (eshell-unescape-inner-double-quote end)
+                                  (cons (point) end)))
+                      (eshell-current-quoted nil))
+                  (eshell-parse-command subcmd))))
+             ;; If this is a simple double-quoted form like
+             ;; "${COMMAND}" (i.e. no indices after the subcommand and
+             ;; no `#' modifier before), ensure we convert to a single
+             ;; string.  This avoids unnecessary work (e.g. splitting
+             ;; the output by lines) when it would just be joined back
+             ;; together afterwards.
+             ,(when (and (not modifier-p) eshell-current-quoted)
+                '(not indices)))
+            indices ,eshell-current-quoted)
+        (goto-char (1+ end)))))
    ((eq (char-after) ?\<)
     (let ((end (eshell-find-delimiter ?\< ?\>)))
-      (if (not end)
-          (throw 'eshell-incomplete "$<")
-        (let* ((temp (make-temp-file temporary-file-directory))
-               (cmd (concat (buffer-substring (1+ (point)) end)
-                            " > " temp)))
-          (prog1
-              `(let ((eshell-current-handles
-                      (eshell-create-handles ,temp 'overwrite)))
-                 (progn
-                   (eshell-as-subcommand
-                    ,(let ((eshell-current-quoted nil))
-                       (eshell-parse-command cmd)))
-                   (ignore
-                    (nconc eshell-this-command-hook
-                           ;; Quote this lambda; it will be evaluated
-                           ;; by `eshell-do-eval', which requires very
-                           ;; particular forms in order to work
-                           ;; properly.  See bug#54190.
-                           (list (function
-                                  (lambda ()
-                                    (delete-file ,temp)
-                                    (when-let ((buffer (get-file-buffer ,temp)))
-                                      (kill-buffer buffer)))))))
-                   (eshell-apply-indices ,temp indices ,eshell-current-quoted)))
-            (goto-char (1+ end)))))))
+      (unless end
+        (throw 'eshell-incomplete "$<"))
+      (forward-char)
+      (let* ((temp (make-temp-file temporary-file-directory))
+             (subcmd (or (eshell-unescape-inner-double-quote end)
+                         (cons (point) end))))
+        (prog1
+            `(let ((eshell-current-handles
+                    (eshell-create-handles ,temp 'overwrite)))
+               (progn
+                 (eshell-as-subcommand
+                  ,(let ((eshell-current-quoted nil))
+                     (eshell-parse-command subcmd)))
+                 (ignore
+                  (nconc eshell-this-command-hook
+                         ;; Quote this lambda; it will be evaluated by
+                         ;; `eshell-do-eval', which requires very
+                         ;; particular forms in order to work
+                         ;; properly.  See bug#54190.
+                         (list (function
+                                (lambda ()
+                                  (delete-file ,temp)
+                                  (when-let ((buffer (get-file-buffer ,temp)))
+                                    (kill-buffer buffer)))))))
+                 (eshell-apply-indices ,temp indices ,eshell-current-quoted)))
+          (goto-char (1+ end))))))
    ((eq (char-after) ?\()
     (condition-case nil
         `(eshell-apply-indices
-- 
2.25.1


[-- Attachment #4: 0003-Avoid-parsing-some-Eshell-forms-when-performing-comp.patch --]
[-- Type: text/plain, Size: 10686 bytes --]

From 0a06c1e41bcc119da29f46e2f9e1e85da06dc5b1 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 20 Mar 2023 17:25:54 -0700
Subject: [PATCH 3/3] 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/file-completion/glob, em-cmpl-test/command-completion)
(em-cmpl-test/subcommand-completion): New tests.
(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 | 59 +++++++++++++++++++++++++++++-
 3 files changed, 113 insertions(+), 21 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 @@ eshell--pcomplete-insert-tab
 
 (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 @@ eshell-complete-parse-arguments
       (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 @@ eshell-last-async-procs
 
 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 @@ eshell/which
 (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 @@ eshell-last-output-end
 
 (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..1f7712e23d1 100644
--- a/test/lisp/eshell/em-cmpl-tests.el
+++ b/test/lisp/eshell/em-cmpl-tests.el
@@ -123,6 +123,36 @@ em-cmpl-test/parse-arguments/variable/splice
               (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/file-completion/unique ()
   "Test completion of file names when there's a unique result."
   (with-temp-eshell
@@ -150,6 +180,15 @@ em-cmpl-test/file-completion/non-unique
          (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."
@@ -159,6 +198,21 @@ em-cmpl-test/file-completion/after-list
      (should (equal (eshell-insert-and-complete "echo (list 1 2) fi")
                     "echo (list 1 2) 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\".
 See <lisp/eshell/esh-cmd.el>."
@@ -174,7 +228,10 @@ em-cmpl-test/lisp-function-completion
 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\".
-- 
2.25.1