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

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

[Enric Rodríguez Carbonell]

Hi,

I would like to report what I think is a bug in the history expansion in 
shell mode.

This is the sequence of steps:

$ emacs -Q

Once in emacs:

M-x shell

Then I type in the command line of the shell window:

$ for file in $(ls); do echo $file; done

Then I type at the prompt:

$ !for

and then run command:

M-x comint-replace-by-expanded-history

which replaces the

$ !for

by

$ for file in $ ( ls ) ; do echo $file ; done

The blank spaces around the parentheses should not be there.
Because of these blank spaces, this command cannot be executed,
as "$ (" should be "$(".

Best regards,

Enric

****************************************************************

In GNU Emacs 24.5.1 (x86_64-pc-linux-gnu, GTK+ Version 3.18.9)
  of 2017-09-20 on lcy01-07, modified by Debian
Windowing system distributor `The X.Org Foundation', version 11.0.11804000
System Description:    Ubuntu 16.04.6 LTS

Configured using:
  `configure --build x86_64-linux-gnu --prefix=/usr
  --sharedstatedir=/var/lib --libexecdir=/usr/lib
  --localstatedir=/var/lib --infodir=/usr/share/info
  --mandir=/usr/share/man --with-pop=yes
  --enable-locallisppath=/etc/emacs24:/etc/emacs:/usr/local/share/emacs/24.5/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.5/site-lisp:/usr/share/emacs/site-lisp
  --build x86_64-linux-gnu --prefix=/usr --sharedstatedir=/var/lib
  --libexecdir=/usr/lib --localstatedir=/var/lib
  --infodir=/usr/share/info --mandir=/usr/share/man --with-pop=yes
  --enable-locallisppath=/etc/emacs24:/etc/emacs:/usr/local/share/emacs/24.5/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.5/site-lisp:/usr/share/emacs/site-lisp
  --with-x=yes --with-x-toolkit=gtk3 --with-toolkit-scroll-bars
  'CFLAGS=-g -O2 -fstack-protector-strong -Wformat
  -Werror=format-security -Wall' 'CPPFLAGS=-Wdate-time
  -D_FORTIFY_SOURCE=2' 'LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro''

Important settings:
   value of $LC_MONETARY: es_ES.UTF-8
   value of $LC_NUMERIC: es_ES.UTF-8
   value of $LC_TIME: es_ES.UTF-8
   value of $LANG: en_US.UTF-8
   value of $XMODIFIERS: @im=ibus
   locale-coding-system: utf-8-unix

Major mode: Shell

Minor modes in effect:
   shell-dirtrack-mode: t
   tooltip-mode: t
   electric-indent-mode: t
   mouse-wheel-mode: t
   tool-bar-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   blink-cursor-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t
   line-number-mode: t
   transient-mark-mode: t

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
History item: 1

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
easymenu mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util help-fns mail-prsvr mail-utils shell pcomplete comint ansi-color
ring time-date tooltip electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel x-win x-dnd tool-bar dnd fontset image regexp-opt
fringe tabulated-list newcomment lisp-mode prog-mode register page
menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core frame cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
case-table epa-hook jka-cmpr-hook help simple abbrev minibuffer nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process dbusbind
gfilenotify dynamic-setting system-font-setting font-render-setting
move-toolbar gtk x-toolkit x multi-tty emacs)

Memory information:
((conses 16 75917 7030)
  (symbols 48 18019 0)
  (miscs 40 45 149)
  (strings 32 11232 4144)
  (string-bytes 1 311767)
  (vectors 16 9670)
  (vector-slots 8 389946 11489)
  (floats 8 63 219)
  (intervals 56 308 19)
  (buffers 960 12)
  (heap 1024 16912 853))
-- 
_______________________________________
Enric Rodriguez Carbonell
Technical University of Catalonia (UPC)
www.cs.upc.edu/~erodri
+34-93-4137815

Office 113 - Omega Building
Department of Computer Science (CS)
Jordi Girona 1, 08034 Barcelona, Spain
_______________________________________

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

[Noam Postavsky]

merge 36103 8027
quit

Enric Rodríguez Carbonell <erodri@cs.upc.edu> writes:

> $ for file in $(ls); do echo $file; done

> $ !for

> M-x comint-replace-by-expanded-history

> $ for file in $ ( ls ) ; do echo $file ; done
>
> The blank spaces around the parentheses should not be there.
> Because of these blank spaces, this command cannot be executed,
> as "$ (" should be "$(".

Yes, the problem is in comint-arguments which attempts to parse a shell
line into separate arguments, using just a bunch of regexps.  This is
doomed to failure I think.  But the alternative would be a full blown
shell parser, so I don't know how feasible it will be to fix this bug.
It's been with us a long time.

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

[Mauro Aranda]

Noam Postavsky <npostavs@gmail.com> writes:

 > Enric Rodríguez Carbonell <erodri@cs.upc.edu> writes:
 >
 >> $ for file in $(ls); do echo $file; done
 >
 >> $ !for
 >
 >> M-x comint-replace-by-expanded-history
 >
 >> $ for file in $ ( ls ) ; do echo $file ; done
 >>
 >> The blank spaces around the parentheses should not be there.
 >> Because of these blank spaces, this command cannot be executed,
 >> as "$ (" should be "$(".
 >
 > Yes, the problem is in comint-arguments which attempts to parse a shell
 > line into separate arguments, using just a bunch of regexps.  This is
 > doomed to failure I think.  But the alternative would be a full blown
 > shell parser, so I don't know how feasible it will be to fix this bug.
 > It's been with us a long time.

I just wanted to point out that since we now have treesitter support for
bash, this bug might be easier to fix, at least partially...

A first idea (trying to be backward-compatible and non-intrusive) is to
define a new variable, comint-arguments-function, turn the current
comint-arguments function into a comint-arguments-default function, and
set comint-arguments-function to that by default.  From now on,
comint-arguments would just call the comint-arguments-function and
return whatever that returns.

Then, shell.el can set comint-arguments-function to a new function,
that, if there is treesitter support for the program being run by shell
(bash or sh, AFAICS), tries to do a better job than comint-arguments as
of now.  If there's no treesitter support, or something fails, then it
can just punt to comint-arguments-default-function.

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

[Mauro Aranda]

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

Here's a draft of my idea.

[-- Attachment #2: 0001-Give-comint-derived-modes-a-way-to-specialize-comint.patch --]
[-- Type: text/x-patch, Size: 9501 bytes --]

From 56628875e89b022ab42a2d78f207a4391a75198f Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
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))
+
 \f
 ;;
 ;; 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