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

[Jim Porter <jporterbugs@gmail.com>]

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

On 3/19/2023 11:39 AM, Jim Porter wrote:
> Ok, here we are.
Here's an updated patch based on some off-list comments from Stefan. 
Most of them are just small doc/naming tweaks, but a couple are worth 
mentioning here, I think:

On 3/19/2023 12:15 PM, Stefan Monnier wrote:
 >> -  (when (memq (char-after) eshell-glob-chars-list)
 >> +  (when (and (not (bound-and-true-p eshell-parse-for-completion-p))
 >
 > Can we (cheaply) arrange so that the var is always defined at this
 > point (same for the other uses further down in the patch)?
 > Maybe by moving the `defvar` elsewhere (e.g. next to
 > `eshell-parse-argument-hook`)?

It's a bit ugly, but I'm trying to follow the conventions in Eshell: 
since completion is an optional extension module for Eshell, other 
modules jump through hoops like this to allow the module to be not-loaded.

Another way to do this (arguably more Eshell-y) would be:

   (when (and (eshell-using-module 'eshell-cmpl)
              eshell-parsing-for-completion)

But that seemed a little overly-verbose for this...

 >> +              (if (bound-and-true-p eshell-parse-for-completion-p)
 >> +                  "(unevaluated subcommand)"
 >
 > Any reason we don't return the actual string that we're trying to
 > parse instead (i.e. here, the subcommand)?

I wanted something where we could be pretty sure that Pcomplete wouldn't 
treat it specially, since it should be "opaque" to Pcomplete. I changed 
this to be a propertized string with just the NUL character:

   (propertize "\0" 'eshell-argument-stub TYPE)

That should be pretty unlikely to trigger anything in Pcomplete. 
(Arguably, Pcomplete should have some way of marking an argument as "not 
real", but I'm not sure anything outside of Eshell would need that...)

[-- Attachment #2: 0001-Avoid-parsing-some-Eshell-forms-when-performing-comp.patch --]
[-- Type: text/plain, Size: 15718 bytes --]

From 2e82c356d52def33f12cfd9718a46be538cf3006 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 18 Mar 2023 15:39:57 -0700
Subject: [PATCH] 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, Lisp forms).  For globbing, we want to pass the literal
glob to Pcomplete so it can use the glob for selecting completion
candidates.  For subcommands and Lisp forms in particular, we
especially want to avoid evaluation, since they can produce arbitary
side effects!  (Bug#50470)

* lisp/eshell/esh-arg.el (eshell-argument-stub): New function.

* lisp/eshell/em-cmpl.el (eshell-parsing-for-completion): New
variable...
(eshell-complete-parse-arguments): ... let-bind it to 't'.

* lisp/eshell/em-glob.el (eshell-parse-glob-chars):
* lisp/eshell/esh-var.el (eshell-parse-variable-ref):
* lisp/eshell/esh-cmd.el (eshell-parse-subcommand-argument)
(eshell-parse-lisp-argument): Check 'eshell-parsing-for-completion'.
(eshell-do-eval): Use 'car-safe' when checking the body of a 'let'
form.

* 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            | 21 ++++++++-
 lisp/eshell/em-glob.el            |  3 +-
 lisp/eshell/esh-arg.el            |  8 ++++
 lisp/eshell/esh-cmd.el            | 27 ++++++-----
 lisp/eshell/esh-var.el            | 78 +++++++++++++++++--------------
 test/lisp/eshell/em-cmpl-tests.el | 57 +++++++++++++++++++++-
 6 files changed, 143 insertions(+), 51 deletions(-)

diff --git a/lisp/eshell/em-cmpl.el b/lisp/eshell/em-cmpl.el
index b65652019d4..14781fb0ee8 100644
--- a/lisp/eshell/em-cmpl.el
+++ b/lisp/eshell/em-cmpl.el
@@ -85,6 +85,24 @@ eshell-cmpl
   :tag "Argument completion"
   :group 'eshell-module))
 
+;;; Internal variables:
+
+;; FIXME: Instead of using a dynamic variable for this, it might be
+;; better to pass `eshell-parse-argument-hook' functions a parsing
+;; context.  This could also be useful for restructuring parsing more
+;; generally, e.g. to fix bug#59752.
+(defvar eshell-parsing-for-completion nil
+  "This is bound to t when calling `eshell-parse-arguments' for completion.
+Functions added to `eshell-parse-argument-hook' should consult this to
+adjust their behavior when parsing a command for completion, if
+necessary.
+
+When parsing for completion, we need to ensure that the resulting
+Lisp form has no side effects, and returns quickly.  For example,
+this means that Eshell subcommands should return some stub value
+when this is set so that the completion code doesn't try to
+invoke the subcommand (see `eshell-parse-subcommand-argument').")
+
 ;;; User Variables:
 
 (defcustom eshell-cmpl-load-hook nil
@@ -328,7 +346,8 @@ eshell-complete-parse-arguments
     (if (setq delim
 	      (catch 'eshell-incomplete
 		(ignore
-		 (setq args (eshell-parse-arguments begin end)))))
+                 (setq args (let ((eshell-parsing-for-completion t))
+                              (eshell-parse-arguments begin end))))))
         (cond ((member (car delim) '("{" "${" "$<"))
 	       (setq begin (1+ (cadr delim))
 		     args (eshell-parse-arguments begin end)))
diff --git a/lisp/eshell/em-glob.el b/lisp/eshell/em-glob.el
index 8a2ba13b2ad..d6372bc30a6 100644
--- a/lisp/eshell/em-glob.el
+++ b/lisp/eshell/em-glob.el
@@ -162,7 +162,8 @@ eshell-parse-glob-chars
 The character is not advanced for ordinary globbing characters, so
 that other function may have a chance to override the globbing
 interpretation."
-  (when (memq (char-after) eshell-glob-chars-list)
+  (when (and (not (bound-and-true-p eshell-parsing-for-completion))
+             (memq (char-after) eshell-glob-chars-list))
     (if (not (memq (char-after) '(?\( ?\[)))
 	(ignore (eshell-add-glob-modifier))
       (let ((here (point)))
diff --git a/lisp/eshell/esh-arg.el b/lisp/eshell/esh-arg.el
index aa1e8f77ea5..e29a57c9f46 100644
--- a/lisp/eshell/esh-arg.el
+++ b/lisp/eshell/esh-arg.el
@@ -189,6 +189,14 @@ eshell-insert-buffer-name
   (interactive "BName of buffer: ")
   (insert-and-inherit "#<buffer " buffer-name ">"))
 
+(defsubst eshell-argument-stub (type)
+  "Return an argument stub for TYPE.
+This is just a string containing the NUL character, with the
+`eshell-argument-stub' property set to TYPE.  This is useful for
+marking that an argument wasn't fully parsed (e.g. when
+`eshell-parsing-for-completion' is non-nil)."
+  (propertize "\0" 'eshell-argument-stub type))
+
 (defsubst eshell-escape-arg (string)
   "Return STRING with the `escaped' property on it."
   (if (stringp string)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 93f2616020c..12843abc777 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -675,13 +675,15 @@ 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
+        (unless end
+          (throw 'eshell-incomplete "{"))
+        (when (eshell-arg-delimiter (1+ end))
+          (prog1
+              (if (bound-and-true-p eshell-parsing-for-completion)
+                  (eshell-argument-stub 'subcommand)
 		`(eshell-as-subcommand
-                  ,(eshell-parse-command (cons (1+ (point)) end)))
-	      (goto-char (1+ end))))))))
+                  ,(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."
@@ -689,14 +691,15 @@ eshell-parse-lisp-argument
 	   (not eshell-current-quoted)
 	   (looking-at eshell-lisp-regexp))
       (let* ((here (point))
-	     (obj
+             (lisp-form
 	      (condition-case nil
 		  (read (current-buffer))
-		(end-of-file
-                 (throw 'eshell-incomplete "(")))))
+                (end-of-file (throw 'eshell-incomplete "(")))))
 	(if (eshell-arg-delimiter)
-	    `(eshell-command-to-value
-              (eshell-lisp-command (quote ,obj)))
+            (if (bound-and-true-p eshell-parsing-for-completion)
+                (eshell-argument-stub 'lisp)
+              `(eshell-command-to-value
+                (eshell-lisp-command ',lisp-form)))
 	  (ignore (goto-char here))))))
 
 (defun eshell-separate-commands (terms separator &optional
@@ -1168,7 +1171,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)
diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index 5d6299af564..0c3381839f4 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -507,10 +507,12 @@ eshell-parse-variable-ref
   (cond
    ((eq (char-after) ?{)
     (let ((end (eshell-find-delimiter ?\{ ?\})))
-      (if (not end)
-          (throw 'eshell-incomplete "${")
-        (forward-char)
-        (prog1
+      (unless end
+        (throw 'eshell-incomplete "${"))
+      (forward-char)
+      (prog1
+          (if (bound-and-true-p eshell-parsing-for-completion)
+              (eshell-argument-stub 'subcommand)
             `(eshell-apply-indices
               (eshell-convert
                (eshell-command-to-value
@@ -527,45 +529,49 @@ eshell-parse-variable-ref
                ;; just be joined back together afterwards.
                ,(when (and (not modifier-p) eshell-current-quoted)
                   '(not indices)))
-              indices ,eshell-current-quoted)
-          (goto-char (1+ end))))))
+              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
+      (unless end
+        (throw 'eshell-incomplete "$<"))
+      (let* ((temp (make-temp-file temporary-file-directory))
+             (cmd (concat (buffer-substring (1+ (point)) end)
+                          " > " temp)))
+        (prog1
+            (if (bound-and-true-p eshell-parsing-for-completion)
+                (eshell-argument-stub 'subcommand)
               `(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)))))))
+                 (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))))))
    ((eq (char-after) ?\()
-    (condition-case nil
+    (let ((lisp-form
+           (condition-case nil
+               (read (or (eshell-unescape-inner-double-quote (point-max))
+                         (current-buffer)))
+             (end-of-file (throw 'eshell-incomplete "$(")))))
+      (if (bound-and-true-p eshell-parsing-for-completion)
+          (eshell-argument-stub 'lisp)
         `(eshell-apply-indices
           (eshell-command-to-value
-           (eshell-lisp-command
-            ',(read (or (eshell-unescape-inner-double-quote (point-max))
-                        (current-buffer)))))
-          indices ,eshell-current-quoted)
-      (end-of-file
-       (throw 'eshell-incomplete "$("))))
+           (eshell-lisp-command ',lisp-form))
+          indices ,eshell-current-quoted))))
    ((looking-at (rx-to-string
                  `(or "'" ,(if eshell-current-quoted "\\\"" "\""))))
     (eshell-with-temp-command
diff --git a/test/lisp/eshell/em-cmpl-tests.el b/test/lisp/eshell/em-cmpl-tests.el
index ea907f1945d..fea33669f08 100644
--- a/test/lisp/eshell/em-cmpl-tests.el
+++ b/test/lisp/eshell/em-cmpl-tests.el
@@ -123,6 +123,34 @@ 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" ,(eshell-argument-stub 'subcommand)))))
+  (with-temp-eshell
+   (insert "echo ${echo hi}")
+   (should (eshell-arguments-equal
+            (car (eshell-complete-parse-arguments))
+            `("echo" ,(propertize (eshell-argument-stub 'subcommand)
+                                  'escaped t))))))
+
+(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" ,(eshell-argument-stub 'lisp)))))
+  (with-temp-eshell
+   (insert "echo $(concat \"hi\")")
+   (should (eshell-arguments-equal
+            (car (eshell-complete-parse-arguments))
+            `("echo" ,(propertize (eshell-argument-stub 'lisp)
+                                  'escaped t))))))
+
 (ert-deftest em-cmpl-test/file-completion/unique ()
   "Test completion of file names when there's a unique result."
   (with-temp-eshell
@@ -150,6 +178,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 +196,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 +226,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