From 2e82c356d52def33f12cfd9718a46be538cf3006 Mon Sep 17 00:00:00 2001 From: Jim Porter 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 "#")) +(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 ." @@ -174,7 +226,10 @@ em-cmpl-test/lisp-function-completion See ." (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 \"#