unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#50470: 27.1; 'company-mode' 'eshell'
@ 2021-09-08  6:23 Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-09-08 16:00 ` bug#50470: eshell Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
                   ` (2 more replies)
  0 siblings, 3 replies; 41+ messages in thread
From: Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-09-08  6:23 UTC (permalink / raw)
  To: 50470


Hello,
when company-mode is activate:
(use-package company
  :ensure t
  :init
  (global-company-mode t)
  )
there are some weird things:
- when you type something, if you enter a single quote a blank space is added.
ex: $ ls Images/'
- and it's impossible to use a wildcad (*) if you want for example copy all
.txt file from a directory. And the C-q not working, I have dot do M-x
quoted-insert to insert '*'.
With the 26.1 version of emacs there was a workarround that worked (deactivate
completion at point) but in this actual version that remove all autocompletion
in eshell.



In GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0)
 of 2021-03-28, modified by Debian built on x86-conova-01
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

Recent messages:
Making completion list...
You can run the command ‘dired’ with C-x d
Mark saved where search started
Mark set
Making completion list... [3 times]
Quit [2 times]
Making completion list...
Complete, but not unique
Quit
Making completion list...

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 --enable-libsystemd --with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/27.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/27.1/site-lisp:/usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils --build x86_64-linux-gnu
 --prefix=/usr --sharedstatedir=/var/lib --libexecdir=/usr/lib
 --localstatedir=/var/lib --infodir=/usr/share/info --mandir=/usr/share/man
 --enable-libsystemd --with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/27.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/27.1/site-lisp:/usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils --with-cairo --with-x=yes
 --with-x-toolkit=gtk3 --with-toolkit-scroll-bars 'CFLAGS=-g -O2
 -ffile-prefix-map=/build/emacs-LlFm6W/emacs-27.1+1=. -fstack-protector-strong
 -Wformat -Werror=format-security -Wall' 'CPPFLAGS=-Wdate-time
 -D_FORTIFY_SOURCE=2' LDFLAGS=-Wl,-z,relro'

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY INOTIFY
ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD JSON PDUMPER
LCMS2 GMP

Important settings:
  value of $LANG: fr_FR.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Eshell

Minor modes in effect:
  pyvenv-mode: t
  shell-dirtrack-mode: t
  show-paren-mode: t
  global-flycheck-mode: t
  global-company-mode: t
  company-mode: t
  xclip-mode: t
  global-subword-mode: t
  subword-mode: t
  display-time-mode: t
  display-battery-mode: t
  electric-pair-mode: t
  server-mode: t
  tooltip-mode: t
  global-eldoc-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
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/tex-site hides /usr/share/emacs/site-lisp/tex-site
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/auctex hides /usr/share/emacs/site-lisp/auctex
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/preview hides /usr/share/auctex/preview
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/font-latex hides /usr/share/auctex/font-latex
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/tex-buf hides /usr/share/auctex/tex-buf
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/tex hides /usr/share/auctex/tex
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/tex-style hides /usr/share/auctex/tex-style
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/bib-cite hides /usr/share/auctex/bib-cite
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/multi-prompt hides /usr/share/auctex/multi-prompt
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/context hides /usr/share/auctex/context
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/context-nl hides /usr/share/auctex/context-nl
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/tex-mik hides /usr/share/auctex/tex-mik
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/tex-bar hides /usr/share/auctex/tex-bar
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/tex-font hides /usr/share/auctex/tex-font
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/latex hides /usr/share/auctex/latex
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/tex-fold hides /usr/share/auctex/tex-fold
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/latex-flymake hides /usr/share/auctex/latex-flymake
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/tex-jp hides /usr/share/auctex/tex-jp
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/plain-tex hides /usr/share/auctex/plain-tex
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/tex-ispell hides /usr/share/auctex/tex-ispell
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/texmathp hides /usr/share/auctex/texmathp
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/toolbar-x hides /usr/share/auctex/toolbar-x
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/context-en hides /usr/share/auctex/context-en
/home/mutenroshii/.emacs.d/elpa/auctex-13.0.14/tex-info hides /usr/share/auctex/tex-info

Features:
(shadow mailalias emacsbug sendmail misearch multi-isearch dired-aux em-unix
em-script em-prompt em-ls em-hist em-pred em-glob em-dirs esh-var em-cmpl
em-basic em-banner em-alias esh-mode flyspell ispell .emacs programmation
ac-geiser geiser-guile info-look geiser geiser-repl geiser-image geiser-company
geiser-doc geiser-menu geiser-edit geiser-completion geiser-autodoc geiser-eval
geiser-connection tq geiser-syntax scheme geiser-log geiser-popup view
py-autopep8 yasnippet highlight-indentation flymake-proc flymake warnings elpy
elpy-rpc pyvenv elpy-shell elpy-profile elpy-django s elpy-refactor diff-mode
easy-mmode python tramp-sh tramp tramp-loaddefs trampver tramp-integration
tramp-compat shell pcomplete parse-time iso8601 ls-lisp grep files-x blacken
jedi jedi-core python-environment epc ctable concurrent deferred auto-complete
popup virtualenv ada-mode align ada-skel wisi-skel ada-process
wisi-process-parse ada-indent-user-options ada-core wisi-prj wisi wisi-fringe
wisi-parse-common semantic/lex semantic/fw mode-local uniquify-files find-file
compile skeleton paren flycheck find-func rx dash elfeed-show elfeed-search
bookmark pp shr svg dom elfeed-csv elfeed elfeed-curl elfeed-log elfeed-db
elfeed-lib thingatpt avl-tree url-queue xml-query xml em-term eshell esh-cmd
esh-ext esh-opt esh-proc esh-io esh-arg esh-module esh-groups esh-util
company-oddmuse company-keywords company-etags etags fileloop generator xref
project company-gtags company-dabbrev-code company-dabbrev company-files
company-clang company-capf company-cmake company-semantic company-template
company-bbdb company pcase ido cus-edit cus-start cus-load wid-edit xclip linum
cap-words superword subword time battery sr-speedbar speedbar sb-image ezimage
dframe elec-pair multi-term advice term disp-table comint ansi-color ehelp
edmacro kmacro akira-theme exec-path-from-shell cl-extra use-package-ensure
use-package-core server mm-archive message dired dired-loaddefs format-spec
rfc822 mml mml-sec epa derived gnus-util rmail rmail-loaddefs
text-property-search time-date mailabbrev gmm-utils mailheader mm-decode
mm-bodies mm-encode mail-utils gnutls network-stream url-http mail-parse
rfc2231 rfc2047 rfc2045 mm-util ietf-drums mail-prsvr url-gw nsm rmc puny
url-cache url-auth url url-proxy url-privacy url-expand url-methods url-history
url-cookie url-domsuf url-util mailcap epg epg-config finder-inf preview-latex
tex-site geiser-impl help-fns radix-tree help-mode geiser-custom geiser-base
ring info package easymenu browse-url url-handlers url-parse auth-source cl-seq
eieio eieio-core cl-macs eieio-loaddefs password-cache json subr-x map url-vars
seq byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib tooltip eldoc
electric uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode prog-mode
register page tab-bar menu-bar rfn-eshadow isearch timer select scroll-bar
mouse jit-lock font-lock syntax facemenu font-core term/tty-colors frame
minibuffer cl-generic cham georgian utf-8-lang misc-lang vietnamese tibetan
thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek romanian slovak
czech european ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray cl-preloaded
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 threads dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo move-toolbar gtk
x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 497748 27141)
 (symbols 48 32295 1)
 (strings 32 157981 6846)
 (string-bytes 1 4388026)
 (vectors 16 49196)
 (vector-slots 8 578680 43664)
 (floats 8 193 258)
 (intervals 56 551 184)
 (buffers 1000 13))


-- 
Christophe BOLLARD





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: eshell
  2021-09-08  6:23 bug#50470: 27.1; 'company-mode' 'eshell' Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-09-08 16:00 ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-09-08 16:07 ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-09-09  1:57 ` bug#50470: 27.1; 'company-mode' 'eshell' Dmitry Gutov
  2 siblings, 0 replies; 41+ messages in thread
From: Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-09-08 16:00 UTC (permalink / raw)
  To: 50470

Hello,
in fact when I run emacs -Q there is the '*' issue.
It's impossible to type the wikdcard character after a directory's name, so it

-- 
Christophe BOLLARD





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: eshell
  2021-09-08  6:23 bug#50470: 27.1; 'company-mode' 'eshell' Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-09-08 16:00 ` bug#50470: eshell Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-09-08 16:07 ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-09-09  1:57 ` bug#50470: 27.1; 'company-mode' 'eshell' Dmitry Gutov
  2 siblings, 0 replies; 41+ messages in thread
From: Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-09-08 16:07 UTC (permalink / raw)
  To: 50470

Sorry I made a mistake with the last message.
I said, even while running emacs with the -Q argument, it's impossible to type
the wildcard character (following a directory's name)


-- 
Christophe BOLLARD





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2021-09-08  6:23 bug#50470: 27.1; 'company-mode' 'eshell' Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-09-08 16:00 ` bug#50470: eshell Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-09-08 16:07 ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-09-09  1:57 ` Dmitry Gutov
  2021-09-09  5:48   ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-12-05 22:06   ` Dmitry Gutov
  2 siblings, 2 replies; 41+ messages in thread
From: Dmitry Gutov @ 2021-09-09  1:57 UTC (permalink / raw)
  To: Christophe, 50470

Hi!

On 08.09.2021 09:23, Christophe via Bug reports for GNU Emacs, the Swiss 
army knife of text editors wrote:
> Hello,
> when company-mode is activate:
> (use-package company
>    :ensure t
>    :init
>    (global-company-mode t)
>    )
> there are some weird things:
> - when you type something, if you enter a single quote a blank space is added.
> ex: $ ls Images/'

This is unfortunately an old problem. I though there was already a bug 
report for it, but couldn't find it. Hope someone will find time to dig 
in through the leaky abstraction of c-a-p-f -> pcomplete.

> - and it's impossible to use a wildcad (*) if you want for example copy all
> .txt file from a directory. And the C-q not working, I have dot do M-x
> quoted-insert to insert '*'.

That sounds like bug#18951. But it was fixed, though. Maybe some minor 
variation of it?

> With the 26.1 version of emacs there was a workarround that worked (deactivate
> completion at point) but in this actual version that remove all autocompletion
> in eshell.






^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2021-09-09  1:57 ` bug#50470: 27.1; 'company-mode' 'eshell' Dmitry Gutov
@ 2021-09-09  5:48   ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-09-09 12:06     ` Dmitry Gutov
  2021-12-05 22:06   ` Dmitry Gutov
  1 sibling, 1 reply; 41+ messages in thread
From: Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-09-09  5:48 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 50470

Maybe the debian version was frozen befor the fix then.

-- 
Christophe BOLLARD





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2021-09-09  5:48   ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-09-09 12:06     ` Dmitry Gutov
  2021-09-09 13:09       ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 41+ messages in thread
From: Dmitry Gutov @ 2021-09-09 12:06 UTC (permalink / raw)
  To: Christophe; +Cc: 50470

On 09.09.2021 08:48, Christophe wrote:
> Maybe the debian version was frozen befor the fix then.

That's very unlikely. And the fix was in 26.1 even, not 27.

You can jump to your sources and see whether the commit 
(https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=5d744e032fee9ce60446a3cc0cf7c2e681ace465) 
it applied anyway.





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2021-09-09 12:06     ` Dmitry Gutov
@ 2021-09-09 13:09       ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-09-09 23:30         ` Dmitry Gutov
  0 siblings, 1 reply; 41+ messages in thread
From: Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-09-09 13:09 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 50470

Exact. So I don't know why I cannot use the wildcards without that is replace
by pcomplete.
I'll going on searching a solution, I can't give up eshell like this.

-- 
Christophe BOLLARD





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2021-09-09 13:09       ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-09-09 23:30         ` Dmitry Gutov
  2021-09-10  5:11           ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 41+ messages in thread
From: Dmitry Gutov @ 2021-09-09 23:30 UTC (permalink / raw)
  To: Christophe; +Cc: 50470

On 09.09.2021 16:09, Christophe wrote:
> Exact. So I don't know why I cannot use the wildcards without that is replace
> by pcomplete.
> I'll going on searching a solution, I can't give up eshell like this.

Until this is fixed, you can disable pcomplete <-> capf intergration in 
eshell like this:

(defun my/disable-pcomplete-capf ()
   (remove-hook 'completion-at-point-functions
                #'pcomplete-completions-at-point
                t))

(add-hook 'eshell-mode-hook #'my/disable-pcomplete-capf)





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2021-09-09 23:30         ` Dmitry Gutov
@ 2021-09-10  5:11           ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 0 replies; 41+ messages in thread
From: Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-09-10  5:11 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 50470

I used something like this in the 26.1 (I've found in a site stackoverflow or stackexchange):

(defun my-eshell-remove-pcomplete ()
  "Remove anoying tab insert when no completion found by company."
  (remove-hook 'completion-at-point-functions #'pcomplete-completions-at-point t))

(add-hook 'eshell-mode-hook #'my-eshell-remove-pcomplete)

And like yours, actually if I use this I have no more autocompletion in eshell.
(Except fore the path if that start by ./)

So I have to add this now:

(add-hook 'eshell-mode-hook
  (lambda () 
    (define-key eshell-mode-map (kbd "<tab>")
      (lambda () (interactive) (pcomplete-std-complete)))))

But that's less efficient.
(except for the wildcards and the blank spaces that's appears after single and
double quotes)

Thanks for the time spent on this, at least I have no more blank
spaces/wildcards issues and I can use the tab for the autocompletion.

-- 
Christophe BOLLARD





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2021-09-09  1:57 ` bug#50470: 27.1; 'company-mode' 'eshell' Dmitry Gutov
  2021-09-09  5:48   ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-12-05 22:06   ` Dmitry Gutov
  2021-12-10 10:50     ` jakanakaevangeli
  2022-01-23  3:23     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 2 replies; 41+ messages in thread
From: Dmitry Gutov @ 2021-12-05 22:06 UTC (permalink / raw)
  To: Christophe, 50470, Stefan Monnier

On 09.09.2021 04:57, Dmitry Gutov wrote:
> Hi!
> 
> On 08.09.2021 09:23, Christophe via Bug reports for GNU Emacs, the Swiss 
> army knife of text editors wrote:
>> Hello,
>> when company-mode is activate:
>> (use-package company
>>    :ensure t
>>    :init
>>    (global-company-mode t)
>>    )
>> there are some weird things:
>> - when you type something, if you enter a single quote a blank space 
>> is added.
>> ex: $ ls Images/'
> 
> This is unfortunately an old problem. I though there was already a bug 
> report for it, but couldn't find it. Hope someone will find time to dig 
> in through the leaky abstraction of c-a-p-f -> pcomplete.

Some new details, from 
https://github.com/company-mode/company-mode/discussions/1276:

When this happens (the user types a quote character and the tab 
character is inserted), there is a message in the echo area:

   Completion function pcomplete-completions-at-point uses a deprecated 
calling convention

I'm going to add Stefan to Cc in case maybe he had a quick fix in mind, 
since I saw him reply to a similar-ish bug report about pcomplete 
integration.





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2021-12-05 22:06   ` Dmitry Gutov
@ 2021-12-10 10:50     ` jakanakaevangeli
  2021-12-10 13:10       ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-01-23  3:23     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 1 reply; 41+ messages in thread
From: jakanakaevangeli @ 2021-12-10 10:50 UTC (permalink / raw)
  To: Dmitry Gutov, Christophe, 50470, Stefan Monnier

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 09.09.2021 04:57, Dmitry Gutov wrote:
>> Hi!
>> 
>> On 08.09.2021 09:23, Christophe via Bug reports for GNU Emacs, the Swiss 
>> army knife of text editors wrote:
>>> Hello,
>>> when company-mode is activate:
>>> (use-package company
>>>    :ensure t
>>>    :init
>>>    (global-company-mode t)
>>>    )
>>> there are some weird things:
>>> - when you type something, if you enter a single quote a blank space 
>>> is added.
>>> ex: $ ls Images/'
>> 
>> This is unfortunately an old problem. I though there was already a bug 
>> report for it, but couldn't find it. Hope someone will find time to dig 
>> in through the leaky abstraction of c-a-p-f -> pcomplete.
>
> Some new details, from 
> https://github.com/company-mode/company-mode/discussions/1276:
>
> When this happens (the user types a quote character and the tab 
> character is inserted),

In my package capf-autosuggest, I run completion-at-point-functions
somewhat like this:

    (let (;; `pcomplete-completions-at-point' may illegally use
          ;; `completion-in-region' itself instead of returning a collection.
          ;; Let's try to outsmart it.
          (completion-in-region-function
           (lambda (start end collection predicate)
             (throw 'illegal-comp-in-region
                    (list start end collection :predicate predicate))))
          ;; Prevent `pcomplete-completions-at-point' from inserting a TAB
          (buffer-read-only t))
      ;; `ielm-complete-filename' may illegaly move point
      (save-excursion
        (condition-case nil
            (catch 'illegal-comp-in-region
              (run-hook-wrapped 'completion-at-point-functions ...))
          (buffer-read-only nil))))

This way, old style capf functions are prevented from inserting a TAB or
moving point.





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2021-12-10 10:50     ` jakanakaevangeli
@ 2021-12-10 13:10       ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-12-13  2:45         ` Dmitry Gutov
  0 siblings, 1 reply; 41+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-12-10 13:10 UTC (permalink / raw)
  To: jakanakaevangeli; +Cc: Christophe, 50470, Dmitry Gutov

> In my package capf-autosuggest, I run completion-at-point-functions
> somewhat like this:
>
>     (let (;; `pcomplete-completions-at-point' may illegally use
>           ;; `completion-in-region' itself instead of returning a collection.
>           ;; Let's try to outsmart it.
>           (completion-in-region-function
>            (lambda (start end collection predicate)
>              (throw 'illegal-comp-in-region
>                     (list start end collection :predicate predicate))))
>           ;; Prevent `pcomplete-completions-at-point' from inserting a TAB
>           (buffer-read-only t))
>       ;; `ielm-complete-filename' may illegaly move point
>       (save-excursion
>         (condition-case nil
>             (catch 'illegal-comp-in-region
>               (run-hook-wrapped 'completion-at-point-functions ...))
>           (buffer-read-only nil))))
>
> This way, old style capf functions are prevented from inserting a TAB or
> moving point.

Hmm... capf itself tries to "solve" that problem in the following way:

    (defvar completion--capf-misbehave-funs nil
      "List of functions found on `completion-at-point-functions' that misbehave.
    These are functions that neither return completion data nor a completion
    function but instead perform completion right away.")
    (defvar completion--capf-safe-funs nil
      "List of well-behaved functions found on `completion-at-point-functions'.
    These are functions which return proper completion data rather than
    a completion function or god knows what else.")
    
    (defun completion--capf-wrapper (fun which)
      ;; FIXME: The safe/misbehave handling assumes that a given function will
      ;; always return the same kind of data, but this breaks down with functions
      ;; like comint-completion-at-point or mh-letter-completion-at-point, which
      ;; could be sometimes safe and sometimes misbehaving (and sometimes neither).
      (if (pcase which
            ('all t)
            ('safe (member fun completion--capf-safe-funs))
            ('optimist (not (member fun completion--capf-misbehave-funs))))
          (let ((res (funcall fun)))
            (cond
             ((and (consp res) (not (functionp res)))
              (unless (member fun completion--capf-safe-funs)
                (push fun completion--capf-safe-funs))
              (and (eq 'no (plist-get (nthcdr 3 res) :exclusive))
                   ;; FIXME: Here we'd need to decide whether there are
                   ;; valid completions against the current text.  But this depends
                   ;; on the actual completion UI (e.g. with the default completion
                   ;; it depends on completion-style) ;-(
                   ;; We approximate this result by checking whether prefix
                   ;; completion might work, which means that non-prefix completion
                   ;; will not work (or not right) for completion functions that
                   ;; are non-exclusive.
                   (null (try-completion (buffer-substring-no-properties
                                          (car res) (point))
                                         (nth 2 res)
                                         (plist-get (nthcdr 3 res) :predicate)))
                   (setq res nil)))
             ((not (or (listp res) (functionp res)))
              (unless (member fun completion--capf-misbehave-funs)
                (message
                 "Completion function %S uses a deprecated calling convention" fun)
                (push fun completion--capf-misbehave-funs))))
            (if res (cons fun res)))))

    (defun completion-at-point ()
      "Perform completion on the text around point.
    The completion method is determined by `completion-at-point-functions'."
      (interactive)
      (let ((res (run-hook-wrapped 'completion-at-point-functions
                                   #'completion--capf-wrapper 'all)))
        ...))

Maybe this should be improved/refined?


        Stefan






^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2021-12-10 13:10       ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-12-13  2:45         ` Dmitry Gutov
  2021-12-13  3:14           ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 41+ messages in thread
From: Dmitry Gutov @ 2021-12-13  2:45 UTC (permalink / raw)
  To: Stefan Monnier, jakanakaevangeli; +Cc: Christophe, 50470

On 10.12.2021 16:10, Stefan Monnier wrote:
> Hmm... capf itself tries to "solve" that problem in the following way:

The problem with this approach is that pcomplete-completion-at-point 
behaves properly most of the time, and only "misbehaves" after some 
particular inputs.





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2021-12-13  2:45         ` Dmitry Gutov
@ 2021-12-13  3:14           ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 0 replies; 41+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-12-13  3:14 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Christophe, 50470, jakanakaevangeli

Dmitry Gutov [2021-12-13 05:45:01] wrote:
> On 10.12.2021 16:10, Stefan Monnier wrote:
>> Hmm... capf itself tries to "solve" that problem in the following way:
> The problem with this approach is that pcomplete-completion-at-point behaves
> properly most of the time, and only "misbehaves" after some
> particular inputs.

The quotes around "solve" intended to say that I'm quite aware it's not
a full solution.  But it's also not cast in stone.  So whichever
better/additional workaround you come up with might be welcome in
`minibuffer.el`.


        Stefan






^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2021-12-05 22:06   ` Dmitry Gutov
  2021-12-10 10:50     ` jakanakaevangeli
@ 2022-01-23  3:23     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-01-24  1:50       ` Dmitry Gutov
  1 sibling, 1 reply; 41+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-01-23  3:23 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Christophe, 50470

> Some new details, from
> https://github.com/company-mode/company-mode/discussions/1276:
>
> When this happens (the user types a quote character and the tab character is
> inserted), there is a message in the echo area:
>
>   Completion function pcomplete-completions-at-point uses a deprecated
>   calling convention
>
> I'm going to add Stefan to Cc in case maybe he had a quick fix in mind,
> since I saw him reply to a similar-ish bug report about
> pcomplete integration.

I think I managed to reproduce the problem and get a good backtrace for
the above with:

    src/emacs -Q -l .../company/company-autoloads.el \
              -f eshell -f company-mode \
              --eval '(advice-add `pcomplete-completions-at-point :around (lambda (orig-fun &rest args) (let ((buffer-read-only t) (debug-on-signal t)) (apply orig-fun args))))' \
              --eval '(setq debug-on-error t debug-on-signal nil debug-ignored-errors nil)' \
              -l company.el \
              --eval "(progn (sit-for 1) (insert \"echo '\")      \
                             (company-idle-begin (current-buffer) \
                                                 (selected-window)\
                                                 (buffer-chars-modified-tick)\
                                                 (point)))"

And the 100% untested patch below is a suggestion for how to try and fix
those kinds of bugs.
Can someone try and maybe make it work?


        Stefan


diff --git a/lisp/eshell/em-cmpl.el b/lisp/eshell/em-cmpl.el
index c6a51b1793..bc35c92493 100644
--- a/lisp/eshell/em-cmpl.el
+++ b/lisp/eshell/em-cmpl.el
@@ -311,18 +311,24 @@ eshell-completion-help
       (describe-prefix-bindings)
     (call-interactively 'pcomplete-help)))
 
+(defun eshell--pcomplete-insert-tab ()
+  (if (not pcomplete-allow-modifications)
+      (throw 'pcompleted nil)
+    (insert-and-inherit "\t")
+    (throw 'pcompleted t)))
+
 (defun eshell-complete-parse-arguments ()
   "Parse the command line arguments for `pcomplete-argument'."
   (when (and eshell-no-completion-during-jobs
 	     (eshell-interactive-process))
-    (insert-and-inherit "\t")
-    (throw 'pcompleted t))
+    (eshell--pcomplete-insert-tab))
   (let ((end (point-marker))
 	(begin (save-excursion (eshell-bol) (point)))
 	(posns (list t))
 	args delim)
-    (when (memq this-command '(pcomplete-expand
-			       pcomplete-expand-and-complete))
+    (when (and pcomplete-allow-modifications
+	       (memq this-command '(pcomplete-expand
+			            pcomplete-expand-and-complete)))
       (run-hook-with-args 'eshell-expand-input-functions begin end)
       (if (= begin end)
 	  (end-of-line))
@@ -335,14 +341,11 @@ eshell-complete-parse-arguments
 	       (setq begin (1+ (cadr delim))
 		     args (eshell-parse-arguments begin end)))
 	      ((eq (car delim) ?\()
-	       (eshell-complete-lisp-symbol)
-	       (throw 'pcompleted t))
+	       (throw 'pcompleted (elisp-completion-at-point)))
 	      (t
-	       (insert-and-inherit "\t")
-	       (throw 'pcompleted t))))
+	       (eshell--pcomplete-insert-tab))))
     (when (get-text-property (1- end) 'comment)
-      (insert-and-inherit "\t")
-      (throw 'pcompleted t))
+      (eshell--pcomplete-insert-tab))
     (let ((pos begin))
       (while (< pos end)
 	(if (get-text-property pos 'arg-begin)
diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 289312e0bb..f9c5b00719 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -189,6 +189,16 @@ pcomplete-expand-before-complete
 `pcomplete-parse-arguments-function'."
   :type 'boolean)
 
+(defvar pcomplete-allow-modifications nil
+  "If non-nil, allow effects in `pcomplete-parse-arguments-function'.
+For the `pcomplete' command, it was common for functions in
+`pcomplete-parse-arguments-function' to make modifications to the
+buffer, like expanding variables are such.
+For `completion-at-point-functions', this is not an option any more, so
+this variable is used to tell `pcomplete-parse-arguments-function'
+whether it can do the modifications like it used to, or whether
+it should refrain from doing so.")
+
 (defcustom pcomplete-parse-arguments-function
   #'pcomplete-parse-buffer-arguments
   "A function to call to parse the current line's arguments.
@@ -392,6 +402,9 @@ pcomplete-completions-at-point
   ;; imposing the pcomplete UI over the standard UI.
   (catch 'pcompleted
     (let* ((pcomplete-stub)
+           (buffer-read-only
+            ;; Make sure the function obeys `pcomplete-allow-modifications'.
+            (if pcomplete-allow-modifications buffer-read-only t))
            pcomplete-seen pcomplete-norm-func
            pcomplete-args pcomplete-last pcomplete-index
            (pcomplete-autolist pcomplete-autolist)
@@ -526,6 +539,7 @@ pcomplete
 	  pcomplete-last-completion-raw nil)
     (catch 'pcompleted
       (let* ((pcomplete-stub)
+	     (pcomplete-allow-modifications t)
 	     pcomplete-seen pcomplete-norm-func
 	     pcomplete-args pcomplete-last pcomplete-index
 	     (pcomplete-autolist pcomplete-autolist)






^ permalink raw reply related	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2022-01-23  3:23     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-01-24  1:50       ` Dmitry Gutov
  2022-01-25 23:05         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 41+ messages in thread
From: Dmitry Gutov @ 2022-01-24  1:50 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Christophe, 50470

Hi Stefan,

On 23.01.2022 05:23, Stefan Monnier wrote:
> And the 100% untested patch below is a suggestion for how to try and fix
> those kinds of bugs.
> Can someone try and maybe make it work?

I've tried the patch, and it seems to work already, as well as fix this 
particular scenario. (Thanks!)

Might as well install it, I think.

There is a scenario that is more noticeably broken (yet actually better 
with this patch): a modification of bug#18951. Instead of

   ls *

try

   ls ~/Docu*

...and [on master] the result is that the asterisk is replaced with the 
"common part" of the possible completions automatically. If there is 
nothing to expand with, the asterisk is similarly deleted.

With your patch, we get the "Buffer is read-only" error in *Messages* 
instead, which is probably an improvement. Because it doesn't modify the 
input, nor break Company completions long-term (after the asterisk is 
removed).

The offending functions is pcomplete-parse-arguments. There is some 
complex global state going on there, but the following addition seems to 
fix the problem:

@@ -790,6 +804,9 @@ pcomplete-parse-arguments
  		   (common-stub (car completions))
  		   (c completions)
  		   (len (length common-stub)))
+              (unless pcomplete-allow-modifications
+                (setq pcomplete-stub (buffer-substring begin (point)))
+                (throw 'pcomplete-completions completions))
  	      (while (and c (> len 0))
  		(while (and (> len 0)
  			    (not (string=


Not sure if this new value of pcomplete-stub is always TRT, but it has 
passed a bunch of my experiments successfully.





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2022-01-24  1:50       ` Dmitry Gutov
@ 2022-01-25 23:05         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-06-04 22:29           ` Dmitry Gutov
  0 siblings, 1 reply; 41+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-01-25 23:05 UTC (permalink / raw)
  To: John Wiegley; +Cc: Christophe, 50470, Dmitry Gutov

Hi John,

Could you explain to me some of the code of `pcomplete-parse-arguments`?
I know that was many years ago but hopefully there's still some fond
memories of how you designed it.

To be honest, the only part I sort-of understand are the first
5-6 lines.  Then comes the `(let ((begin (pcomplete-begin 'last)))` and
after that I'm lost: it doesn't look like we're parsing arguments
any more.

E.g. Why/when would pcomplete-stub contain a list rather than a string?


        Stefan


Dmitry Gutov [2022-01-24 03:50:59] wrote:

> Hi Stefan,
>
> On 23.01.2022 05:23, Stefan Monnier wrote:
>> And the 100% untested patch below is a suggestion for how to try and fix
>> those kinds of bugs.
>> Can someone try and maybe make it work?
>
> I've tried the patch, and it seems to work already, as well as fix this
> particular scenario. (Thanks!)
>
> Might as well install it, I think.
>
> There is a scenario that is more noticeably broken (yet actually better with
> this patch): a modification of bug#18951. Instead of
>
>   ls *
>
> try
>
>   ls ~/Docu*
>
> ...and [on master] the result is that the asterisk is replaced with the
>  "common part" of the possible completions automatically. If there is
>  nothing to expand with, the asterisk is similarly deleted.
>
> With your patch, we get the "Buffer is read-only" error in *Messages*
> instead, which is probably an improvement. Because it doesn't modify the
> input, nor break Company completions long-term (after the asterisk is
> removed).
>
> The offending functions is pcomplete-parse-arguments. There is some complex
> global state going on there, but the following addition seems to fix the
> problem:
>
> @@ -790,6 +804,9 @@ pcomplete-parse-arguments
>  		   (common-stub (car completions))
>  		   (c completions)
>  		   (len (length common-stub)))
> +              (unless pcomplete-allow-modifications
> +                (setq pcomplete-stub (buffer-substring begin (point)))
> +                (throw 'pcomplete-completions completions))
>  	      (while (and c (> len 0))
>  		(while (and (> len 0)
>  			    (not (string=
>
>
> Not sure if this new value of pcomplete-stub is always TRT, but it has
> passed a bunch of my experiments successfully.






^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2022-01-25 23:05         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-06-04 22:29           ` Dmitry Gutov
  2022-06-05  0:17             ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 41+ messages in thread
From: Dmitry Gutov @ 2022-06-04 22:29 UTC (permalink / raw)
  To: Stefan Monnier, John Wiegley; +Cc: Christophe, 50470

It seems like this bug was fixed in bug#55204?

Or at least the problems scenarios are not reproducible anymore.

The downside compared to my latest patch is that it actually expanded 
~/Downl* into a bunch of completions (and the current behavior is "no 
completions"), but that seems minor.

It might be considered a regression, though.

On 26.01.2022 01:05, Stefan Monnier wrote:
> Hi John,
> 
> Could you explain to me some of the code of `pcomplete-parse-arguments`?
> I know that was many years ago but hopefully there's still some fond
> memories of how you designed it.
> 
> To be honest, the only part I sort-of understand are the first
> 5-6 lines.  Then comes the `(let ((begin (pcomplete-begin 'last)))` and
> after that I'm lost: it doesn't look like we're parsing arguments
> any more.
> 
> E.g. Why/when would pcomplete-stub contain a list rather than a string?
> 
> 
>          Stefan
> 
> 
> Dmitry Gutov [2022-01-24 03:50:59] wrote:
> 
>> Hi Stefan,
>>
>> On 23.01.2022 05:23, Stefan Monnier wrote:
>>> And the 100% untested patch below is a suggestion for how to try and fix
>>> those kinds of bugs.
>>> Can someone try and maybe make it work?
>>
>> I've tried the patch, and it seems to work already, as well as fix this
>> particular scenario. (Thanks!)
>>
>> Might as well install it, I think.
>>
>> There is a scenario that is more noticeably broken (yet actually better with
>> this patch): a modification of bug#18951. Instead of
>>
>>    ls *
>>
>> try
>>
>>    ls ~/Docu*
>>
>> ...and [on master] the result is that the asterisk is replaced with the
>>   "common part" of the possible completions automatically. If there is
>>   nothing to expand with, the asterisk is similarly deleted.
>>
>> With your patch, we get the "Buffer is read-only" error in *Messages*
>> instead, which is probably an improvement. Because it doesn't modify the
>> input, nor break Company completions long-term (after the asterisk is
>> removed).
>>
>> The offending functions is pcomplete-parse-arguments. There is some complex
>> global state going on there, but the following addition seems to fix the
>> problem:
>>
>> @@ -790,6 +804,9 @@ pcomplete-parse-arguments
>>   		   (common-stub (car completions))
>>   		   (c completions)
>>   		   (len (length common-stub)))
>> +              (unless pcomplete-allow-modifications
>> +                (setq pcomplete-stub (buffer-substring begin (point)))
>> +                (throw 'pcomplete-completions completions))
>>   	      (while (and c (> len 0))
>>   		(while (and (> len 0)
>>   			    (not (string=
>>
>>
>> Not sure if this new value of pcomplete-stub is always TRT, but it has
>> passed a bunch of my experiments successfully.
> 






^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2022-06-04 22:29           ` Dmitry Gutov
@ 2022-06-05  0:17             ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-06-05  0:36               ` Dmitry Gutov
  0 siblings, 1 reply; 41+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-06-05  0:17 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 50470, Christophe, John Wiegley

> The downside compared to my latest patch is that it actually expanded
> ~/Downl* into a bunch of completions (and the current behavior is "no
> completions"), but that seems minor.
>
> It might be considered a regression, though.

AFAIK it is a regression, indeed.  I thought you'd install your patch
on top.  Any reason why you refrained from doing so?


        Stefan






^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2022-06-05  0:17             ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-06-05  0:36               ` Dmitry Gutov
  2022-06-05  0:53                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-06-05 23:52                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 2 replies; 41+ messages in thread
From: Dmitry Gutov @ 2022-06-05  0:36 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 50470, Christophe, John Wiegley

On 05.06.2022 03:17, Stefan Monnier wrote:
>> The downside compared to my latest patch is that it actually expanded
>> ~/Downl* into a bunch of completions (and the current behavior is "no
>> completions"), but that seems minor.
>>
>> It might be considered a regression, though.
> AFAIK it is a regression, indeed.  I thought you'd install your patch
> on top.  Any reason why you refrained from doing so?

Erm, I missed the time when you installed yours: you never wrote about 
it here, and my "Emacs Diffs" folder still has 738 unread messages.

So I figured the fix is due to another bug report and patch. :-/

My patch doesn't help with the 'cd ~/Down*' behavior, though: when I 
worked on it, pressing C-M-i did expand it as expected, it was mostly 
problematic with company-mode (and other similar frontends).

But now C-M-i results in "No match" as well, with or without my patch.





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2022-06-05  0:36               ` Dmitry Gutov
@ 2022-06-05  0:53                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-06-05 23:45                   ` Dmitry Gutov
  2022-06-05 23:52                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 1 reply; 41+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-06-05  0:53 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 50470, Christophe, John Wiegley

> Erm, I missed the time when you installed yours: you never wrote about
> it here,

Indeed, I had lost track of this bug (and couldn't find your patch when
I looked for it, probably for the same reason).

> and my "Emacs Diffs" folder still has 738 unread messages.

And you're still here chatting?
Kids these days!

> My patch doesn't help with the 'cd ~/Down*' behavior, though: when I worked
> on it, pressing C-M-i did expand it as expected, it was mostly problematic
> with company-mode (and other similar frontends).
>
> But now C-M-i results in "No match" as well, with or without my patch.

Hmm... looks like my lack of understanding of this pcomplete code
strikes again.


        Stefan






^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2022-06-05  0:53                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-06-05 23:45                   ` Dmitry Gutov
  2022-06-06  1:34                     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 41+ messages in thread
From: Dmitry Gutov @ 2022-06-05 23:45 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Christophe, 50470, John Wiegley

On 05.06.2022 03:53, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
>> Erm, I missed the time when you installed yours: you never wrote about
>> it here,
> 
> Indeed, I had lost track of this bug (and couldn't find your patch when
> I looked for it, probably for the same reason).
> 
>> and my "Emacs Diffs" folder still has 738 unread messages.
> 
> And you're still here chatting?
> Kids these days!

Aye-aye, cap'n!

>> My patch doesn't help with the 'cd ~/Down*' behavior, though: when I worked
>> on it, pressing C-M-i did expand it as expected, it was mostly problematic
>> with company-mode (and other similar frontends).
>>
>> But now C-M-i results in "No match" as well, with or without my patch.
> 
> Hmm... looks like my lack of understanding of this pcomplete code
> strikes again.

Debugging shows that COMPLETIONS just before the bit of code I added 
gets set to absolute file names. E.g. if I'm typing

   cd ~/Do*

COMPLETIONS is '("/home/dgutov/Documents" "/home/dgutov/Downloads"), 
which fails to match "~/Do" because we do prefix-matching by default.

I suppose whatever code does the expansion here, shouldn't 
dis-abbreviate the file name (or resolve symlinks, etc). I could find 
where that logic resides, though.

Another issue which made investigating this harder, is that eshell-mode 
(and only it) has 'C-M-i' bound to eshell-complete-lisp-symbol.





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2022-06-05  0:36               ` Dmitry Gutov
  2022-06-05  0:53                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-06-05 23:52                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-06-07 22:10                   ` Dmitry Gutov
  1 sibling, 1 reply; 41+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-06-05 23:52 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Christophe, 50470, John Wiegley

> My patch doesn't help with the 'cd ~/Down*' behavior, though: when I worked
> on it, pressing C-M-i did expand it as expected, it was mostly problematic
> with company-mode (and other similar frontends).
>
> But now C-M-i results in "No match" as well, with or without my patch.

AFAICT the "no match" is because we have "~/Down*" as the pattern and
"/home/<foo>/Downloads" as one of the proposed completions, and the
completion style has no idea how the two can match since it doesn't know
we're dealing with files.

In any case, I think your change is going in the right direction, so
I installed a patch which does basically the same.


        Stefan






^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2022-06-05 23:45                   ` Dmitry Gutov
@ 2022-06-06  1:34                     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-06-06  9:07                       ` Dmitry Gutov
  0 siblings, 1 reply; 41+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-06-06  1:34 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Christophe, 50470, John Wiegley

> Debugging shows that COMPLETIONS just before the bit of code I added gets
> set to absolute file names. E.g. if I'm typing

[ I see we got back to this issue at the very same time :-)  ]

>   cd ~/Do*
>
> COMPLETIONS is '("/home/dgutov/Documents" "/home/dgutov/Downloads"), which
> fails to match "~/Do" because we do prefix-matching by default.

Yup.  We could try and handle this with a `completion-table-subvert`
hack like we do in `pcomplete-completions-at-point`.

Still, if you remove the ~/ the behavior is still not great: it seems I get
"Do*" completed to "Documents/ " where the SPC might not be what I want.

Maybe we should return a special completion-table which implements the
`backend` style.


        Stefan






^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2022-06-06  1:34                     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-06-06  9:07                       ` Dmitry Gutov
  2022-06-07 15:52                         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 41+ messages in thread
From: Dmitry Gutov @ 2022-06-06  9:07 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Christophe, 50470, John Wiegley

On 06.06.2022 04:34, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> Still, if you remove the ~/ the behavior is still not great: it seems I get
> "Do*" completed to "Documents/ " where the SPC might not be what I want.

I think that space comes from exit-function (defined at the end of 
pcomplete-completions-at-point).

So it should be orthogonal to the contents of the completion table.

The file names in COMPLETIONS inside pcomplete-parse-arguments come 
without trailing space.





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2022-06-06  9:07                       ` Dmitry Gutov
@ 2022-06-07 15:52                         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-06-07 22:39                           ` Dmitry Gutov
  0 siblings, 1 reply; 41+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-06-07 15:52 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Christophe, 50470, John Wiegley

Dmitry Gutov [2022-06-06 12:07:58] wrote:
> On 06.06.2022 04:34, Stefan Monnier via Bug reports for GNU Emacs, the Swiss
> army knife of text editors wrote:
>> Still, if you remove the ~/ the behavior is still not great: it seems I get
>> "Do*" completed to "Documents/ " where the SPC might not be what I want.
>
> I think that space comes from exit-function (defined at the end of
> pcomplete-completions-at-point).
> So it should be orthogonal to the contents of the completion table.

Right, but when we complete file names in Eshell, the behavior is
better, because the exit-function is different.  I don't think there's
much we can do about it within `pcomplete.el`, tho.


        Stefan






^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2022-06-05 23:52                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-06-07 22:10                   ` Dmitry Gutov
  0 siblings, 0 replies; 41+ messages in thread
From: Dmitry Gutov @ 2022-06-07 22:10 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Christophe, 50470, John Wiegley

On 06.06.2022 02:52, Stefan Monnier wrote:
> In any case, I think your change is going in the right direction, so
> I installed a patch which does basically the same.

I've pushed a fix for that ('pcomplete-completions needs to be a list of 
completions AFAICT).





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2022-06-07 15:52                         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-06-07 22:39                           ` Dmitry Gutov
  2023-03-17  6:26                             ` Jim Porter
  0 siblings, 1 reply; 41+ messages in thread
From: Dmitry Gutov @ 2022-06-07 22:39 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Christophe, 50470, John Wiegley

On 07.06.2022 18:52, Stefan Monnier wrote:
> Dmitry Gutov [2022-06-06 12:07:58] wrote:
>> On 06.06.2022 04:34, Stefan Monnier via Bug reports for GNU Emacs, the Swiss
>> army knife of text editors wrote:
>>> Still, if you remove the ~/ the behavior is still not great: it seems I get
>>> "Do*" completed to "Documents/ " where the SPC might not be what I want.
>> I think that space comes from exit-function (defined at the end of
>> pcomplete-completions-at-point).
>> So it should be orthogonal to the contents of the completion table.
> Right, but when we complete file names in Eshell, the behavior is
> better, because the exit-function is different.  I don't think there's
> much we can do about it within `pcomplete.el`, tho.

I'm sorry, I don't understand.

pcomplete-completions-at-point is the completion function used for 
Eshell, and the exit-function it defines at the end is the one that 
inserts the spaces.

So... which behaviors are you comparing?

Speaking of trying to use completion-table-subvert, it doesn't seem 
obvious which value to use as S2. What we have is a list of strings, and 
the common prefix isn't going to always match the (unexpanded) input.

pcomplete-completions-at-point somehow has pcomplete-stub pointing to 
the necessary value (e.g. "/home/dgutov/Do") in the asterisk-less cases 
(due to some other code path being taken), but not in this specific one.

Conceptually, it seems easier and cleaner to avoid expansion in the 
first place. The patch below does that, though I'm not sure what 
unwanted side-effects it might have ('cd' still works).

In any case, supporting completion with asterisk doesn't seem very 
useful, given that the user might as well omit that char and get the 
same list of completions, and typing asterisk in the middle of a work 
doesn't work. That's where the 'backend' style could help indeed.

diff --git a/lisp/eshell/em-dirs.el b/lisp/eshell/em-dirs.el
index 5396044d8c..fa504bb618 100644
--- a/lisp/eshell/em-dirs.el
+++ b/lisp/eshell/em-dirs.el
@@ -204,8 +204,8 @@ eshell-dirs-initialize
                              'eshell-dirs-substitute-cd)
                        eshell-interpreter-alist)))

-  (add-hook 'eshell-parse-argument-hook
-	    #'eshell-parse-user-reference nil t)
+  ;; (add-hook 'eshell-parse-argument-hook
+  ;;           #'eshell-parse-user-reference nil t)
    (if (eshell-under-windows-p)
        (add-hook 'eshell-parse-argument-hook
  		#'eshell-parse-drive-letter nil t))





^ permalink raw reply related	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2022-06-07 22:39                           ` Dmitry Gutov
@ 2023-03-17  6:26                             ` Jim Porter
  2023-03-18  1:01                               ` Dmitry Gutov
  0 siblings, 1 reply; 41+ messages in thread
From: Jim Porter @ 2023-03-17  6:26 UTC (permalink / raw)
  To: Dmitry Gutov, Stefan Monnier; +Cc: Christophe, 50470, John Wiegley

I've recently been digging through how Eshell and Pcomplete interact, so 
I think I understand what's happening here.

On 6/7/2022 3:39 PM, Dmitry Gutov wrote:
> pcomplete-completions-at-point somehow has pcomplete-stub pointing to 
> the necessary value (e.g. "/home/dgutov/Do") in the asterisk-less cases 
> (due to some other code path being taken), but not in this specific one.

I believe the problem is that when Eshell parses the command line to 
figure out what to give Pcomplete, it expands the globs itself, so 
things get messed up. So we want to prevent glob-expansion before 
passing to Pcomplete.

The below patch does this, but it's probably not the right way to do it. 
  However, it's a simple change, and before I go through the larger 
effort of a proper patch, I want to be sure I'm actually solving the 
right thing.

For some background/explanation of how I'm thinking we should solve 
this: in Emacs 30, while fixing some other completion issues, I added 
'eshell-complete--eval-argument-form' (Emacs 29 does a similar thing, 
but the code is in 'eshell-complete-parse-arguments'). We probably want 
to enhance this function so that it only evaluates Eshell arguments 
forms that we know are ok. For a fun example of why the current behavior 
is wrong, try pressing TAB at the end of this command: "cd ${sleep 5; 
echo Doc}". Yes, it actually *runs* that subcommand before passing it to 
Pcomplete. :/


--------------------------------------------------


diff --git a/lisp/eshell/em-cmpl.el b/lisp/eshell/em-cmpl.el
index b65652019d4..7168f91d774 100644
--- a/lisp/eshell/em-cmpl.el
+++ b/lisp/eshell/em-cmpl.el
@@ -325,6 +325,10 @@ eshell-complete-parse-arguments
        (if (= begin end)
           (end-of-line))
        (setq end (point-marker)))
+    ;; Don't expand globs when parsing arguments; we want to pass any
+    ;; globs to Pcomplete unaltered.
+    (let ((eshell-parse-argument-hook (remq #'eshell-parse-glob-chars
+                                            eshell-parse-argument-hook)))
        (if (setq delim
                 (catch 'eshell-incomplete
                   (ignore
@@ -341,7 +345,7 @@ eshell-complete-parse-arguments
                  ((member (car delim) '("(" "$("))
                  (throw 'pcompleted (elisp-completion-at-point)))
                 (t
-              (eshell--pcomplete-insert-tab))))
+                (eshell--pcomplete-insert-tab)))))
      (when (get-text-property (1- end) 'comment)
        (eshell--pcomplete-insert-tab))
      (let ((pos (1- end)))






^ permalink raw reply related	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2023-03-17  6:26                             ` Jim Porter
@ 2023-03-18  1:01                               ` Dmitry Gutov
  2023-03-18  6:36                                 ` Jim Porter
  0 siblings, 1 reply; 41+ messages in thread
From: Dmitry Gutov @ 2023-03-18  1:01 UTC (permalink / raw)
  To: Jim Porter, Stefan Monnier; +Cc: Christophe, 50470, John Wiegley

On 17/03/2023 08:26, Jim Porter wrote:
> I've recently been digging through how Eshell and Pcomplete interact, so 
> I think I understand what's happening here.

Thanks!

> On 6/7/2022 3:39 PM, Dmitry Gutov wrote:
>> pcomplete-completions-at-point somehow has pcomplete-stub pointing to 
>> the necessary value (e.g. "/home/dgutov/Do") in the asterisk-less 
>> cases (due to some other code path being taken), but not in this 
>> specific one.
> 
> I believe the problem is that when Eshell parses the command line to 
> figure out what to give Pcomplete, it expands the globs itself, so 
> things get messed up. So we want to prevent glob-expansion before 
> passing to Pcomplete.
> 
> The below patch does this, but it's probably not the right way to do it. 
>   However, it's a simple change, and before I go through the larger 
> effort of a proper patch, I want to be sure I'm actually solving the 
> right thing.

I can't comment on the exact right way to implement this, but the patch 
does seem to solve the remaining problem here. Which is completion for 
inputs containing *. The result looks rather nice.





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2023-03-18  1:01                               ` Dmitry Gutov
@ 2023-03-18  6:36                                 ` Jim Porter
  2023-03-19 18:39                                   ` Jim Porter
  0 siblings, 1 reply; 41+ messages in thread
From: Jim Porter @ 2023-03-18  6:36 UTC (permalink / raw)
  To: Dmitry Gutov, Stefan Monnier; +Cc: Christophe, 50470, John Wiegley

On 3/17/2023 6:01 PM, Dmitry Gutov wrote:
> On 17/03/2023 08:26, Jim Porter wrote:
>> The below patch does this, but it's probably not the right way to do 
>> it.   However, it's a simple change, and before I go through the 
>> larger effort of a proper patch, I want to be sure I'm actually 
>> solving the right thing.
> 
> I can't comment on the exact right way to implement this, but the patch 
> does seem to solve the remaining problem here. Which is completion for 
> inputs containing *. The result looks rather nice.

Excellent. I'll get to work on a final patch then. I have a pretty good 
idea of some ways to implement it, but it'll definitely need some 
regression tests to go with it.

Hopefully I can get it finished up over the weekend.





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2023-03-18  6:36                                 ` Jim Porter
@ 2023-03-19 18:39                                   ` Jim Porter
  2023-03-20  0:30                                     ` Jim Porter
  0 siblings, 1 reply; 41+ messages in thread
From: Jim Porter @ 2023-03-19 18:39 UTC (permalink / raw)
  To: Dmitry Gutov, Stefan Monnier; +Cc: Christophe, 50470, John Wiegley

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

On 3/17/2023 11:36 PM, Jim Porter wrote:
> Hopefully I can get it finished up over the weekend.
Ok, here we are. This adds a new defvar called 
'eshell-parse-for-completion-p', which Eshell argument parsers can 
consult to adjust their behavior. In particular, when that variable is 
true, it means a) don't expand globs (let Pcomplete handle that), and b) 
return a stub for subcommands and Lisp function forms (we don't want to 
execute these inadvertently).

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

From 7f1bce1768f0072dd469f9047a59ffd1fde8b4f4 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/em-cmpl.el (eshell-parse-for-completion-p): 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-parse-for-completion-p'.
(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            | 13 +++++-
 lisp/eshell/em-glob.el            |  3 +-
 lisp/eshell/esh-cmd.el            | 27 ++++++-----
 lisp/eshell/esh-var.el            | 78 +++++++++++++++++--------------
 test/lisp/eshell/em-cmpl-tests.el | 56 +++++++++++++++++++++-
 5 files changed, 126 insertions(+), 51 deletions(-)

diff --git a/lisp/eshell/em-cmpl.el b/lisp/eshell/em-cmpl.el
index b65652019d4..5714aeaabfb 100644
--- a/lisp/eshell/em-cmpl.el
+++ b/lisp/eshell/em-cmpl.el
@@ -85,6 +85,16 @@ eshell-cmpl
   :tag "Argument completion"
   :group 'eshell-module))
 
+;;; Internal variables:
+
+(defvar eshell-parse-for-completion-p nil
+  "This is set to t before calling `eshell-parse-arguments' for completion.
+Hooks for `eshell-parse-argument-hook' should consult this to
+adjust their behavior when parsing a command for completion, if
+necessary.  For example, subcommands should return some stub
+value when this is set so that the completion code doesn't try to
+invoke the subcommand.")
+
 ;;; User Variables:
 
 (defcustom eshell-cmpl-load-hook nil
@@ -328,7 +338,8 @@ eshell-complete-parse-arguments
     (if (setq delim
 	      (catch 'eshell-incomplete
 		(ignore
-		 (setq args (eshell-parse-arguments begin end)))))
+                 (setq args (let ((eshell-parse-for-completion-p 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..7517ee57833 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-parse-for-completion-p))
+             (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-cmd.el b/lisp/eshell/esh-cmd.el
index 93f2616020c..9ca3d06fc22 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-parse-for-completion-p)
+                  "(unevaluated 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-parse-for-completion-p)
+                "(unevaluated lisp form)"
+              `(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..b7420f2437b 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-parse-for-completion-p)
+              "(unevaluated 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-parse-for-completion-p)
+                "(unevaluated 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-parse-for-completion-p)
+          "(unevaluated lisp form)"
         `(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..e0976c380cb 100644
--- a/test/lisp/eshell/em-cmpl-tests.el
+++ b/test/lisp/eshell/em-cmpl-tests.el
@@ -123,6 +123,33 @@ 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" "(unevaluated subcommand)"))))
+  (with-temp-eshell
+   (insert "echo ${echo hi}")
+   (should (eshell-arguments-equal
+            (car (eshell-complete-parse-arguments))
+            `("echo" ,(propertize "(unevaluated 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" "(unevaluated lisp form)"))))
+  (with-temp-eshell
+   (insert "echo (concat \"hi\")")
+   (should (eshell-arguments-equal
+            (car (eshell-complete-parse-arguments))
+            '("echo" "(unevaluated lisp form)")))))
+
 (ert-deftest em-cmpl-test/file-completion/unique ()
   "Test completion of file names when there's a unique result."
   (with-temp-eshell
@@ -150,6 +177,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 +195,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 +225,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


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2023-03-19 18:39                                   ` Jim Porter
@ 2023-03-20  0:30                                     ` Jim Porter
  2023-03-20  1:34                                       ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 41+ messages in thread
From: Jim Porter @ 2023-03-20  0:30 UTC (permalink / raw)
  To: Dmitry Gutov, Stefan Monnier; +Cc: Christophe, 50470, John Wiegley

[-- 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


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2023-03-20  0:30                                     ` Jim Porter
@ 2023-03-20  1:34                                       ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2023-03-21  2:30                                         ` Jim Porter
  0 siblings, 1 reply; 41+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-03-20  1:34 UTC (permalink / raw)
  To: Jim Porter; +Cc: Christophe, 50470, John Wiegley, Dmitry Gutov

>> Ok, here we are.
>>> -  (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.

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?

E.g. something like "keep parsing free of side effects"?  This would also
have the benefit of clarifying the actual meaning of this var: defining
a var based on who uses it or how it's used is always a source of
trouble.

> Another way to do this (arguably more Eshell-y) would be:
>
>   (when (and (eshell-using-module 'eshell-cmpl)
>              eshell-parsing-for-completion)

`boundp` is definitely much better.


        Stefan






^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2023-03-20  1:34                                       ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-03-21  2:30                                         ` Jim Porter
  2023-03-28  0:41                                           ` Dmitry Gutov
  0 siblings, 1 reply; 41+ messages in thread
From: Jim Porter @ 2023-03-21  2:30 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Christophe, 50470, John Wiegley, Dmitry Gutov

[-- 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


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2023-03-21  2:30                                         ` Jim Porter
@ 2023-03-28  0:41                                           ` Dmitry Gutov
  2023-03-28  4:06                                             ` Jim Porter
  0 siblings, 1 reply; 41+ messages in thread
From: Dmitry Gutov @ 2023-03-28  0:41 UTC (permalink / raw)
  To: Jim Porter, Stefan Monnier; +Cc: Christophe, 50470, John Wiegley

On 21/03/2023 04:30, Jim Porter wrote:
> 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.

Again, no proper review from me, but I've tried the patches.

Completion looks good, just like with the previous one.

But here's an error I encountered when trying to call a command with 
asterisks without expanding them with completion:

$ ls ~/Documents/Sp*
Wrong type argument: stringp, ("~/Documents/Spain/")

This issue is present in master (without the patches applied), but not 
in emacs-29.

The patch(es) fix a similar error in company-mode completion -- 
hopefully the straight evaluation could be fixed as easily.





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2023-03-28  0:41                                           ` Dmitry Gutov
@ 2023-03-28  4:06                                             ` Jim Porter
  2023-03-28  6:10                                               ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 41+ messages in thread
From: Jim Porter @ 2023-03-28  4:06 UTC (permalink / raw)
  To: Dmitry Gutov, Stefan Monnier; +Cc: Christophe, 50470, John Wiegley

On 3/27/2023 5:41 PM, Dmitry Gutov wrote:
> Again, no proper review from me, but I've tried the patches.

Thanks for taking a look. I'll give it a couple more days in case Stefan 
has any thoughts, and if not, I'll merge the patches.

> Completion looks good, just like with the previous one.
> 
> But here's an error I encountered when trying to call a command with 
> asterisks without expanding them with completion:
> 
> $ ls ~/Documents/Sp*
> Wrong type argument: stringp, ("~/Documents/Spain/")

And thanks for catching this. It was fallout from my fix to bug#28064. 
I've pushed a fix to master as 28a9438169.





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2023-03-28  4:06                                             ` Jim Porter
@ 2023-03-28  6:10                                               ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2023-03-28 17:43                                                 ` Drew Adams
  2023-03-28 19:35                                                 ` Jim Porter
  0 siblings, 2 replies; 41+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-03-28  6:10 UTC (permalink / raw)
  To: Jim Porter; +Cc: Christophe, 50470, John Wiegley, Dmitry Gutov

> Thanks for taking a look. I'll give it a couple more days in case Stefan has
> any thoughts, and if not, I'll merge the patches.

Sorry, my thoughts have left the building, so just go ahead.


        Stefan






^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2023-03-28  6:10                                               ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2023-03-28 17:43                                                 ` Drew Adams
  2023-03-28 19:35                                                 ` Jim Porter
  1 sibling, 0 replies; 41+ messages in thread
From: Drew Adams @ 2023-03-28 17:43 UTC (permalink / raw)
  To: Stefan Monnier, Jim Porter
  Cc: Christophe, 50470@debbugs.gnu.org, John Wiegley, Dmitry Gutov

> Sorry, my thoughts have left the building, so just go ahead.

Think I saw a few of them jumping over a fence next
to University Ave.  They looked to be having fun.





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2023-03-28  6:10                                               ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2023-03-28 17:43                                                 ` Drew Adams
@ 2023-03-28 19:35                                                 ` Jim Porter
  2023-03-28 21:21                                                   ` Dmitry Gutov
  1 sibling, 1 reply; 41+ messages in thread
From: Jim Porter @ 2023-03-28 19:35 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Christophe, 50470, John Wiegley, Dmitry Gutov

On 3/27/2023 11:10 PM, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
>> Thanks for taking a look. I'll give it a couple more days in case Stefan has
>> any thoughts, and if not, I'll merge the patches.
> 
> Sorry, my thoughts have left the building, so just go ahead.

Thanks. Pushed as cde38f0df3f.

Is there anything left to do on this bug now? It seems to me that we 
could close it, but I came into this discussion pretty late, so I don't 
want to jump the gun...





^ permalink raw reply	[flat|nested] 41+ messages in thread

* bug#50470: 27.1; 'company-mode' 'eshell'
  2023-03-28 19:35                                                 ` Jim Porter
@ 2023-03-28 21:21                                                   ` Dmitry Gutov
  0 siblings, 0 replies; 41+ messages in thread
From: Dmitry Gutov @ 2023-03-28 21:21 UTC (permalink / raw)
  To: Jim Porter, Stefan Monnier; +Cc: Christophe, John Wiegley, 50470-done

On 28/03/2023 22:35, Jim Porter wrote:
> On 3/27/2023 11:10 PM, Stefan Monnier via Bug reports for GNU Emacs, the 
> Swiss army knife of text editors wrote:
>>> Thanks for taking a look. I'll give it a couple more days in case 
>>> Stefan has
>>> any thoughts, and if not, I'll merge the patches.
>>
>> Sorry, my thoughts have left the building, so just go ahead.
> 
> Thanks. Pushed as cde38f0df3f.
> 
> Is there anything left to do on this bug now? It seems to me that we 
> could close it, but I came into this discussion pretty late, so I don't 
> want to jump the gun...

Looks like there's not.

Everything that I tried, has worked well. Thanks for the fixes! Closing.





^ permalink raw reply	[flat|nested] 41+ messages in thread

end of thread, other threads:[~2023-03-28 21:21 UTC | newest]

Thread overview: 41+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-09-08  6:23 bug#50470: 27.1; 'company-mode' 'eshell' Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-09-08 16:00 ` bug#50470: eshell Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-09-08 16:07 ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-09-09  1:57 ` bug#50470: 27.1; 'company-mode' 'eshell' Dmitry Gutov
2021-09-09  5:48   ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-09-09 12:06     ` Dmitry Gutov
2021-09-09 13:09       ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-09-09 23:30         ` Dmitry Gutov
2021-09-10  5:11           ` Christophe via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-12-05 22:06   ` Dmitry Gutov
2021-12-10 10:50     ` jakanakaevangeli
2021-12-10 13:10       ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-12-13  2:45         ` Dmitry Gutov
2021-12-13  3:14           ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-01-23  3:23     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-01-24  1:50       ` Dmitry Gutov
2022-01-25 23:05         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-06-04 22:29           ` Dmitry Gutov
2022-06-05  0:17             ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-06-05  0:36               ` Dmitry Gutov
2022-06-05  0:53                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-06-05 23:45                   ` Dmitry Gutov
2022-06-06  1:34                     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-06-06  9:07                       ` Dmitry Gutov
2022-06-07 15:52                         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-06-07 22:39                           ` Dmitry Gutov
2023-03-17  6:26                             ` Jim Porter
2023-03-18  1:01                               ` Dmitry Gutov
2023-03-18  6:36                                 ` Jim Porter
2023-03-19 18:39                                   ` Jim Porter
2023-03-20  0:30                                     ` Jim Porter
2023-03-20  1:34                                       ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-03-21  2:30                                         ` Jim Porter
2023-03-28  0:41                                           ` Dmitry Gutov
2023-03-28  4:06                                             ` Jim Porter
2023-03-28  6:10                                               ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2023-03-28 17:43                                                 ` Drew Adams
2023-03-28 19:35                                                 ` Jim Porter
2023-03-28 21:21                                                   ` Dmitry Gutov
2022-06-05 23:52                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-06-07 22:10                   ` Dmitry Gutov

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).