Subject: bug#60845: closed (Re: bug#60845: 30.0.50; [PATCH] Add tests for Eshell interactive completion (and fix a bug in it))

From: [email-masked] (GNU bug Tracking System)
Date: Tue, 10 Oct 2023 20:09:02 +0000

Your bug report

#60845: 30.0.50; [PATCH] Add tests for Eshell interactive completion (and fix a bug in it)

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
60845: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=60845
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#60845: 30.0.50; [PATCH] Add tests for Eshell interactive completion (and fix a bug in it)
From: Jim Porter <[email-masked]>
To: Stefan Kangas <[email-masked]>
Cc: [email-masked], Daniel Mendler <[email-masked]>, Gregory Heytings <[email-masked]>, [email-masked], Stefan Monnier <[email-masked]>
Date: Tue, 10 Oct 2023 13:07:50 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

On 9/5/2023 6:37 PM, Jim Porter wrote:
> Attached is a patch to revert the Emacs 29 workarounds. I *believe* I've 
> fixed this on the Eshell side by always providing Pcomplete with the 
> arguments in their string form. Could everyone try the patch out to make 
> sure things still work?
> 
> In particular, see the cases in the following bugs: bug#60464, 
> bug#60021, and bug#59956.

Since there have been no further comments in the last month, I've now 
merged my patch to the master branch as 239db5d5162. If anyone sees any 
issues resulting from this, feel free to back it out, file a bug, and/or 
let me know directly.

Subject: 30.0.50; [PATCH] Add tests for Eshell interactive completion (and fix a bug in it)
From: Jim Porter <[email-masked]>
To: [email-masked]
Date: Sun, 15 Jan 2023 17:50:10 -0800
List-Id: <debbugs-submit.debbugs.gnu.org>

This is a followup from bug#60464 and friends. Over there, we discussed 
problems with Pcomplete when used from Eshell, namely that Eshell 
sometimes gives Pcomplete non-string arguments. I mentioned that we 
should probably have automated tests for the Eshell side so that we can 
make further improvements to Pcomplete without causing regressions, so 
here are some tests.

I also fixed an edge case in 'eshell-complete-parse-arguments' where it 
wasn't correctly handling the new variable-splicing syntax in Eshell. 
That's patch 0002.

Of course, these tests are just a start, and there are probably lots of 
others that we could add.

From bdef37f6cc101820f060c718f4016a29206c51d1 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Sun, 15 Jan 2023 16:44:17 -0800
Subject: [PATCH 1/2] Add regression tests for Eshell completions

* lisp/eshell/esh-cmd.el (eshell-complete-lisp-symbols): Fix
docstring.

* test/lisp/eshell/em-cmpl-tests.el: New file.
---
 lisp/eshell/esh-cmd.el            |   2 +-
 test/lisp/eshell/em-cmpl-tests.el | 147 ++++++++++++++++++++++++++++++
 2 files changed, 148 insertions(+), 1 deletion(-)
 create mode 100644 test/lisp/eshell/em-cmpl-tests.el

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 99c3d7f627d..b5f1d60ff18 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -343,7 +343,7 @@ eshell-cmd-initialize
 	      #'eshell-complete-lisp-symbols nil t)))
 
 (defun eshell-complete-lisp-symbols ()
-  "If there is a user reference, complete it."
+  "If there is a Lisp symbol, complete it."
   (let ((arg (pcomplete-actual-arg)))
     (when (string-match (concat "\\`" eshell-lisp-regexp) arg)
       (setq pcomplete-stub (substring arg (match-end 0))
diff --git a/test/lisp/eshell/em-cmpl-tests.el b/test/lisp/eshell/em-cmpl-tests.el
new file mode 100644
index 00000000000..837af9d1061
--- /dev/null
+++ b/test/lisp/eshell/em-cmpl-tests.el
@@ -0,0 +1,147 @@
+;;; em-cmpl-tests.el --- em-cmpl test suite  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2023 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Tests for Eshell's interactive completion.
+
+;;; Code:
+
+(require 'ert)
+(require 'eshell)
+(require 'em-cmpl)
+(require 'em-dirs)
+(require 'em-hist)
+(require 'em-tramp)
+(require 'em-unix)
+
+(require 'eshell-tests-helpers
+         (expand-file-name "eshell-tests-helpers"
+                           (file-name-directory (or load-file-name
+                                                    default-directory))))
+
+(defvar eshell-test-value nil)
+
+(defun eshell-insert-and-complete (input)
+  "Insert INPUT and invoke completion, returning the result."
+  (insert input)
+  (completion-at-point)
+  (eshell-get-old-input))
+
+;;; Tests:
+
+(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 (equal (car (eshell-complete-parse-arguments))
+                    `("cat"))))))
+
+(ert-deftest em-cmpl-test/parse-arguments/variable ()
+  "Test parsing arguments with a variable interpolation."
+  (with-temp-eshell
+   (let ((eshell-test-value '("foo" "bar")))
+     (insert "echo $eshell-test-value")
+     (should (equal (car (eshell-complete-parse-arguments))
+                    `("echo" ("foo" "bar")))))))
+
+(ert-deftest em-cmpl-test/file-completion/unique ()
+  "Test completion of file names when there's a unique result."
+  (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 fi")
+                    "echo file.txt ")))))
+
+(ert-deftest em-cmpl-test/file-completion/non-unique ()
+  "Test completion of file names when there are multiple results."
+  (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")
+                    "echo file."))
+     ;; Now try completing again.
+     (let ((minibuffer-message-timeout 0)
+           (inhibit-message t))
+       (completion-at-point))
+     ;; FIXME: We can't use `current-message' here.
+     (with-current-buffer (messages-buffer)
+       (save-excursion
+         (goto-char (point-max))
+         (forward-line -1)
+         (should (looking-at "Complete, but not unique")))))))
+
+(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 ")))))
+
+(ert-deftest em-cmpl-test/lisp-symbol-completion ()
+  "Test completion of Lisp forms like \"#'symbol\" and \"`symbol\".
+See <lisp/eshell/esh-cmd.el>."
+  (with-temp-eshell
+   (should (equal (eshell-insert-and-complete "echo #'system-nam")
+                  "echo #'system-name ")))
+  (with-temp-eshell
+   (should (equal (eshell-insert-and-complete "echo `system-nam")
+                  "echo `system-name "))))
+
+(ert-deftest em-cmpl-test/lisp-function-completion ()
+  "Test completion of Lisp forms like \"(func)\".
+See <lisp/eshell/esh-cmd.el>."
+  (with-temp-eshell
+   (should (equal (eshell-insert-and-complete "echo (eshell/ech")
+                  "echo (eshell/echo"))))
+
+(ert-deftest em-cmpl-test/variable-ref-completion ()
+  "Test completion of variable references like \"$var\".
+See <lisp/eshell/esh-var.el>."
+  (with-temp-eshell
+   (should (equal (eshell-insert-and-complete "echo $system-nam")
+                  "echo $system-name "))))
+
+(ert-deftest em-cmpl-test/variable-assign-completion ()
+  "Test completion of variable assignments like \"var=value\".
+See <lisp/eshell/esh-var.el>."
+  (with-temp-eshell
+   (ert-with-temp-directory default-directory
+     (write-region nil nil (expand-file-name "file.txt"))
+     (should (equal (eshell-insert-and-complete "VAR=f")
+                    "VAR=file.txt ")))))
+
+(ert-deftest em-cmpl-test/user-ref-completion ()
+  "Test completeion of user references like \"~user\".
+See <lisp/eshell/em-dirs.el>."
+  (unwind-protect
+      (with-temp-eshell
+       (cl-letf (((symbol-function 'eshell-read-user-names)
+                  (lambda () (setq eshell-user-names '((1234 . "user"))))))
+         ;; FIXME: Should this really add a space at the end?
+         (should (equal (eshell-insert-and-complete "echo ~us")
+                        "echo ~user/ "))))
+    ;; Clear the cached user names we set above.
+    (setq eshell-user-names nil)))
+
+;;; em-cmpl-tests.el ends here
-- 
2.25.1

From 39fe1f15f0804e608891f158afc49e806f969e98 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Sun, 15 Jan 2023 16:44:23 -0800
Subject: [PATCH 2/2] Properly parse Eshell variable splices for interactive
 completion

Previously, the code simply ignored the splice operator, which usually
worked, but isn't actually correct.

* lisp/eshell/em-cmpl.el (eshell-complete-eval-argument-form): New
function.
(eshell-complete-parse-arguments): Properly parse variable splices.

* test/lisp/eshell/em-cmpl-tests.el
(em-cmpl-test/parse-arguments/variable-splice): New test.
---
 lisp/eshell/em-cmpl.el            | 56 ++++++++++++++++++++-----------
 test/lisp/eshell/em-cmpl-tests.el |  8 +++++
 2 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/lisp/eshell/em-cmpl.el b/lisp/eshell/em-cmpl.el
index 4206ad048fa..a67fe53ed46 100644
--- a/lisp/eshell/em-cmpl.el
+++ b/lisp/eshell/em-cmpl.el
@@ -306,6 +306,12 @@ eshell--pcomplete-insert-tab
     (insert-and-inherit "\t")
     (throw 'pcompleted t)))
 
+(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)))
+
 (defun eshell-complete-parse-arguments ()
   "Parse the command line arguments for `pcomplete-argument'."
   (when (and eshell-no-completion-during-jobs
@@ -344,11 +350,6 @@ eshell-complete-parse-arguments
     (cl-assert (= (length args) (length posns)))
     (let ((a args) (i 0) new-start)
       (while a
-        ;; Remove any top-level `eshell-splice-args' sigils.  These
-        ;; are meant to be rewritten and can't actually be called.
-        (when (and (consp (car a))
-                   (eq (caar a) 'eshell-splice-args))
-          (setcar a (cadar a)))
         ;; If there's an unreplaced `eshell-operator' sigil, consider
         ;; the token after it the new start of our arguments.
         (when (and (consp (car a))
@@ -364,23 +365,38 @@ eshell-complete-parse-arguments
 	       (not (eq (char-before (1- end)) ?\\)))
       (nconc args (list ""))
       (nconc posns (list (point))))
+    ;; Evaluate and expand Eshell forms.
+    (let (evaled-args evaled-posns)
+      (cl-mapc
+       (lambda (arg posn)
+         (pcase arg
+           (`(eshell-splice-args ,val)
+            (dolist (subarg (eshell-complete-eval-argument-form val))
+              (push subarg evaled-args)
+              (push posn evaled-posns)))
+           ((pred listp)
+            (push (eshell-complete-eval-argument-form arg) evaled-args)
+            (push posn evaled-posns))
+           (_
+            (push arg evaled-args)
+            (push posn evaled-posns))))
+       args posns)
+      (setq args (nreverse evaled-args)
+            posns (nreverse evaled-posns)))
+    ;; Convert arguments to forms that Pcomplete can understand.
     (cons (mapcar
            (lambda (arg)
-             (let ((val
-                    (if (listp arg)
-                        (let ((result
-                               (eshell-do-eval
-                                (list 'eshell-commands arg) t)))
-                          (cl-assert (eq (car result) 'quote))
-                          (cadr result))
-                      arg)))
-               (cond ((numberp val)
-                      (setq val (number-to-string val)))
-                     ;; expand .../ etc that only eshell understands to
-                     ;; standard ../../
-                     ((and (stringp val)) (string-match "\\.\\.\\.+/" val)
-                      (setq val (eshell-expand-multiple-dots val))))
-               (or val "")))
+             (cond
+              ((numberp arg)
+               (number-to-string arg))
+              ;; Expand ".../" etc that only Eshell understands to the
+              ;; standard "../../".
+              ((and (stringp arg) (string-match "\\.\\.\\.+/" arg))
+               (eshell-expand-multiple-dots arg))
+              ((null arg)
+               "")
+              (t
+               arg)))
 	   args)
 	  posns)))
 
diff --git a/test/lisp/eshell/em-cmpl-tests.el b/test/lisp/eshell/em-cmpl-tests.el
index 837af9d1061..2460f58c25c 100644
--- a/test/lisp/eshell/em-cmpl-tests.el
+++ b/test/lisp/eshell/em-cmpl-tests.el
@@ -62,6 +62,14 @@ em-cmpl-test/parse-arguments/variable
      (should (equal (car (eshell-complete-parse-arguments))
                     `("echo" ("foo" "bar")))))))
 
+(ert-deftest em-cmpl-test/parse-arguments/variable-splice ()
+  "Test parsing arguments with a spliced variable interpolation."
+  (with-temp-eshell
+   (let ((eshell-test-value '("foo" "bar")))
+     (insert "echo $@eshell-test-value")
+     (should (equal (car (eshell-complete-parse-arguments))
+                    `("echo" "foo" "bar"))))))
+
 (ert-deftest em-cmpl-test/file-completion/unique ()
   "Test completion of file names when there's a unique result."
   (with-temp-eshell
-- 
2.25.1