Subject: Re: bug#36103: 24.5; Blank spaces around parentheses in history expansion in shell mode

From: Mauro Aranda <[email-masked]>
Date: Sat, 7 Oct 2023 10:39:30 -0300

Here's a draft of my idea.

From 56628875e89b022ab42a2d78f207a4391a75198f Mon Sep 17 00:00:00 2001
From: Mauro Aranda <[email-masked]>
Date: Sun, 1 Oct 2023 09:51:53 -0300
Subject: [PATCH] Give comint derived modes a way to specialize
 comint-arguments

The comint-arguments regexp approach for splitting arguments falls
short, as demonstrated in Bug#36103.

* lisp/comint.el (comint-arguments-function): New variable.
(comint-arguments-default-function): New function.
(comint-arguments): Use it.
(comint-delimiter-argument-list): Adapt docstring.

* lisp/shell.el (shell-arguments): New function.  When treesitter
support is available for the shell program, specialize the way we
look for arguments in input.
(shell-mode): Set it as comint-arguments-function.

* test/lisp/shell-tests.el (shell-test-with-temporary-shell): New
macro.
(shell-test-history-expansion-helper): New function.
(shell-test-history-expansion): New test.
---
 lisp/comint.el           | 43 +++++++++++++++++++++++++++++----------
 lisp/shell.el            | 44 ++++++++++++++++++++++++++++++++++++++++
 test/lisp/shell-tests.el | 35 ++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+), 11 deletions(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index de7cc5b0e86..f0c05dd8e2c 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -202,13 +202,22 @@ comint-delimiter-argument-list
   "List of characters to recognize as separate arguments in input.
 Strings comprising a character in this list will separate the arguments
 surrounding them, and also be regarded as arguments in their own right (unlike
-whitespace).  See `comint-arguments'.
+whitespace).  See `comint-arguments-default-function'.
 Defaults to the empty list.
 
 For shells, a good value is (?\\| ?& ?< ?> ?\\( ?\\) ?;).
 
 This is a good thing to set in mode hooks.")
 
+(defvar-local comint-arguments-function #'comint-arguments-default-function
+  "Function to use to split arguments from an input.
+
+By default, this is `comint-arguments-default-function', which assumes, in
+general, that whitespace separates arguments and treats runs of characters in
+`comint-delimiter-argument-list' as a separate argument.
+
+This is a good thing to set in mode hooks.")
+
 (defcustom comint-input-autoexpand nil
   "If non-nil, expand input command history references on completion.
 This mirrors the optional behavior of tcsh (its autoexpand and histlist).
@@ -1785,17 +1794,14 @@ comint-delim-arg
 	  (setq args (cons (substring arg start pos) args))))
       args)))
 
-(defun comint-arguments (string nth mth)
+(defun comint-arguments-default-function (string nth mth)
   "Return from STRING the NTH to MTH arguments.
-NTH and/or MTH can be nil, which means the last argument.
-NTH and MTH can be negative to count from the end; -1 means
-the last argument.
-Returned arguments are separated by single spaces.  We assume
-whitespace separates arguments, except within quotes and except
-for a space or tab that immediately follows a backslash.  Also, a
-run of one or more of a single character in
-`comint-delimiter-argument-list' is a separate argument.
-Argument 0 is the command name."
+
+Used as the default function for `comint-arguments', returns the arguments
+separated by single spaces.  Assumes that whitespace separates arguments,
+except within quotes and except for a space or tab that immediately follows a
+backslash.  Also, a run of one or more of a single character in
+`comint-delimiter-argument-list' is a separate argument."
   ;; The first line handles ordinary characters and backslash-sequences
   ;; (except with w32 msdos-like shells, where backslashes are valid).
   ;; The second matches "-quoted strings.
@@ -1853,6 +1859,21 @@ comint-arguments
               (t          (1- (- mth))))))
       (mapconcat
        (lambda (a) a) (nthcdr n (nreverse (nthcdr m args))) " "))))
+
+(defun comint-arguments (string nth mth)
+    "Return from STRING the NTH to MTH arguments.
+
+NTH and/or MTH can be nil, which means the last argument.
+NTH and MTH can be negative to count from the end; -1 means
+the last argument.
+
+Argument 0 is the command name.
+
+Calls the `comint-arguments-function' with STRING, NTH and MTH as arguments
+and returns whatever that function returns, which should be the NTH to MTH
+arguments from STRING."
+  (funcall comint-arguments-function string nth mth))
+
 
 ;;
 ;; Input processing stuff
diff --git a/lisp/shell.el b/lisp/shell.el
index 48978fecbdd..103f39bc455 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -100,6 +100,11 @@
 (eval-when-compile (require 'files-x)) ;with-connection-local-variables
 (require 'subr-x)
 (eval-when-compile (require 'cl-lib))
+;; Used for better history expansion with event/word designators.
+(declare-function treesit-node-text "treesit.el")
+(declare-function treesit-node-on "treesit.el")
+(declare-function treesit-node-children "treesit.el")
+(declare-function treesit-ready-p "treesit.el")
 
 ;;; Customization and Buffer Variables
 
@@ -606,6 +611,44 @@ shell-completion-vars
 
 (defvar sh-shell-file)
 
+(defun shell-arguments (string nth mth)
+  "Return from STRING the NTH to MTH arguments, separated by whitespace.
+
+Used as `comint-arguments-function'.  When there's no tree-sitter support
+for the shell being used, falls back to `comint-arguments-default-function'."
+  (cond ((and (member shell--start-prog '("bash" "sh"))
+              (progn (require 'treesit)
+                     (and (treesit-available-p)
+                          (treesit-ready-p 'bash)))
+              (condition-case nil
+                  (with-temp-buffer
+                    (insert string)
+                    (let ((inhibit-message t))
+                      (bash-ts-mode))
+                    (let* ((ts-node (treesit-node-on (point-min) (point-max)))
+                           (args
+                            (mapcar #'treesit-node-text
+                                    ;; We don't want to return ")" for
+                                    ;; a STRING like $( cat file ).
+                                    ;; So treat everything that's not a
+                                    ;; command node as a single node.
+                                    (if (string= (treesit-node-type ts-node)
+                                                 "command")
+                                        (treesit-node-children ts-node)
+                                      (list ts-node))))
+                           (count (length args))
+                           (n (cond
+                               ((null nth) (1- count))
+                               ((>= nth 0) nth)
+                               (t (+ count nth))))
+                           (m (cond
+                               ((null mth) count)
+                               ((>= mth 0) (1+ mth))
+                               (t (1+ (- count mth))))))
+                      (mapconcat #'identity (seq-subseq args n m) " ")))
+                (error nil))))
+        (t (comint-arguments-default-function string nth mth))))
+
 (define-derived-mode shell-mode comint-mode "Shell"
   "Major mode for interacting with an inferior shell.
 \\<shell-mode-map>
@@ -684,6 +727,7 @@ shell-mode
   (setq-local shell-dirstack nil)
   (setq-local shell-last-dir nil)
   (setq-local comint-get-old-input #'shell-get-old-input)
+  (setq-local comint-arguments-function #'shell-arguments)
   ;; People expect Shell mode to keep the last line of output at
   ;; window bottom.
   (setq-local scroll-conservatively 101)
diff --git a/test/lisp/shell-tests.el b/test/lisp/shell-tests.el
index ddddfdb2e0f..c88b18194d3 100644
--- a/test/lisp/shell-tests.el
+++ b/test/lisp/shell-tests.el
@@ -25,6 +25,7 @@
 
 (require 'shell)
 (require 'ert)
+(require 'treesit)
 
 (ert-deftest shell-tests-unquote-1 ()
   "Test problem found by Filipp Gunbin in emacs-devel."
@@ -95,4 +96,38 @@ shell-directory-tracker-cd
       (should (not (equal start-dir list-buffers-directory)))
       (should (string-prefix-p list-buffers-directory start-dir)))))
 
+(defmacro shell-test-with-temporary-shell (shell &rest body)
+  "Run a temporary SHELL and return the result of evaluating BODY."
+  (declare (indent defun))
+  `(let ((explicit-shell-file-name ,shell))
+     (shell)
+     (unwind-protect
+         (progn ,@body)
+       (let ((shell-kill-buffer-on-exit t))
+         (comint-send-eof)))))
+
+(defun shell-test-history-expansion-helper (cmd designator)
+  "Add CMD to comint history, expand DESIGNATOR and return its expansion."
+  (shell-test-with-temporary-shell "bash"
+    (comint-add-to-input-history cmd)
+    (end-of-buffer)
+    (let ((opoint (point)))
+      (insert designator)
+      (completion-at-point)
+      (prog1 (buffer-substring opoint (point))
+        (delete-region opoint (point))))))
+
+(ert-deftest shell-test-history-expansion ()
+  "Test that history expansion with designators works."
+  (skip-unless (and (treesit-ready-p 'bash)
+                    (executable-find "bash")))
+  (let ((cmd "cat <( date )"))
+    ;; The following three tests come from Bug5007.
+    ;; TODO: More tests with different designators.
+    (should (string= (shell-test-history-expansion-helper cmd "!!") cmd))
+    (should (string= (shell-test-history-expansion-helper cmd "!:$") "<( date )"))
+    (should (string= (shell-test-history-expansion-helper
+                      (substring cmd 4) "!:$")
+                     (substring cmd 4)))))
+
 ;;; shell-tests.el ends here
-- 
2.34.1