bug#30697: make eval-expression only take `read'-able Lisp

bug#30697: make eval-expression only take `read'-able Lisp

[Charles A. Roelli]

Here is a change to make eval-expression issue a warning in the
minibuffer when the user enters an unreadable expression.  Currently,
we just error out, so the user has to restart the command and go back
in the history to get back what he typed.

With the change applied, if you type

M-: ( RET

from emacs -q, you'll see,

Eval: ( [End of file during parsing]

which gives you a chance to fix the error right away.

As another example, if you type an extra paren, as in

M-: ( ) ) RET

you'll see,

Eval: ()) [Trailing garbage following expression]

and point is left at the extra paren.


I've also added documentation for read--expression.  The change
follows:

diff --git a/lisp/simple.el b/lisp/simple.el
index 60a0028..7387554 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1517,6 +1517,10 @@ eval-expression-minibuffer-setup-hook
   "Hook run by `eval-expression' when entering the minibuffer.")
 
 (defun read--expression (prompt &optional initial-contents)
+  "Read an Emacs Lisp expression from the minibuffer.
+
+PROMPT and optional argument INITIAL-CONTENTS do the same as in
+function `read-from-minibuffer'."
   (let ((minibuffer-completing-symbol t))
     (minibuffer-with-setup-hook
         (lambda ()
@@ -1526,11 +1530,52 @@ read--expression
           (eldoc-mode 1)
           (add-hook 'completion-at-point-functions
                     #'elisp-completion-at-point nil t)
+          (local-set-key "\r" 'read--expression-try-read)
+          (local-set-key "\n" 'read--expression-try-read)
           (run-hooks 'eval-expression-minibuffer-setup-hook))
       (read-from-minibuffer prompt initial-contents
                             read-expression-map t
                             'read-expression-history))))
 
+(defun read--expression-try-read ()
+  "Try to read an Emacs Lisp expression in the minibuffer.
+
+Exit the minibuffer if successful, else report the error to the
+user and move point to the location of the error.  If point is
+not already at the location of the error, push a mark before
+moving point."
+  (interactive)
+  (unless (> (minibuffer-depth) 0)
+    (error "Minibuffer must be active"))
+  (if (let* ((contents (minibuffer-contents))
+             (error-point nil))
+        (with-temp-buffer
+          (condition-case err
+              (progn
+                (insert contents)
+                (goto-char (point-min))
+                ;; `read' will signal errors like "End of file during
+                ;; parsing" and "Invalid read syntax".
+                (read (current-buffer))
+                ;; Since `read' does not signal the "Trailing garbage
+                ;; following expression" error, we check for trailing
+                ;; garbage ourselves.
+                (or (progn
+                      ;; This check is similar to what `string_to_object'
+                      ;; does in minibuf.c.
+                      (skip-chars-forward " \t\n")
+                      (= (point) (point-max)))
+                    (error "Trailing garbage following expression")))
+            (error
+             (setq error-point (+ (length (minibuffer-prompt)) (point)))
+             (with-current-buffer (window-buffer (minibuffer-window))
+               (unless (= (point) error-point)
+                 (push-mark))
+               (goto-char error-point)
+               (minibuffer-message (error-message-string err)))
+             nil))))
+      (exit-minibuffer)))
+
 (defun eval-expression-get-print-arguments (prefix-argument)
   "Get arguments for commands that print an expression result.
 Returns a list (INSERT-VALUE NO-TRUNCATE CHAR-PRINT-LIMIT)

bug#30697: make eval-expression only take `read'-able Lisp

[Eli Zaretskii]

> Date: Sun, 04 Mar 2018 17:46:18 +0100
> From: charles@aurox.ch (Charles A. Roelli)
> 
> Here is a change to make eval-expression issue a warning in the
> minibuffer when the user enters an unreadable expression.  Currently,
> we just error out, so the user has to restart the command and go back
> in the history to get back what he typed.
> 
> With the change applied, if you type
> 
> M-: ( RET
> 
> from emacs -q, you'll see,
> 
> Eval: ( [End of file during parsing]
> 
> which gives you a chance to fix the error right away.
> 
> As another example, if you type an extra paren, as in
> 
> M-: ( ) ) RET
> 
> you'll see,
> 
> Eval: ()) [Trailing garbage following expression]
> 
> and point is left at the extra paren.

You are changing the behavior of read--expression, which has a few
callers in Emacs.  Did you verify that those callers won't break due
to this change?

This should also have a NEWS entry, and perhaps the manual needs some
changes, please take a look.

Thanks.

bug#30697: make eval-expression only take `read'-able Lisp

[Drew Adams]

> Here is a change to make eval-expression issue a warning in the
> minibuffer when the user enters an unreadable expression.  Currently,
> we just error out, so the user has to restart the command and go back
> in the history to get back what he typed.

I don't see why this is better than what happens now.

We currently have `eval-expression-debug-on-error', which helps
you understand the read or eval error.  And the function is not
necessarily called interactively ("When called interactively...").

What problem is the proposed change trying to solve?

bug#30697: make eval-expression only take `read'-able Lisp

[Charles A. Roelli]

> Date: Sun, 04 Mar 2018 19:10:55 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: 30697@debbugs.gnu.org
> 
> You are changing the behavior of read--expression, which has a few
> callers in Emacs.  Did you verify that those callers won't break due
> to this change?

I've now tested them, and they work fine.  `eval-minibuffer' (which is
used for (interactive "X")) turns out to be one of the callers of
`read--expression', so I think the documentation of that interactive
spec will have to be updated too.

I also did small tests with ido-mode/icomplete-mode enabled, which
also do not seem to be affected by this change.
 
> This should also have a NEWS entry, and perhaps the manual needs some
> changes, please take a look.

Thanks, I will work on that.

bug#30697: make eval-expression only take `read'-able Lisp

[Charles A. Roelli]

> Date: Sun, 4 Mar 2018 14:36:57 -0800 (PST)
> From: Drew Adams <drew.adams@oracle.com>
> Content-Type: text/plain; charset=us-ascii
> 
> > Here is a change to make eval-expression issue a warning in the
> > minibuffer when the user enters an unreadable expression.  Currently,
> > we just error out, so the user has to restart the command and go back
> > in the history to get back what he typed.
> 
> I don't see why this is better than what happens now.
> 
> We currently have `eval-expression-debug-on-error', which helps
> you understand the read or eval error.  And the function is not
> necessarily called interactively ("When called interactively...").
> 
> What problem is the proposed change trying to solve?

It's too easy to evaluate a meaningless expression, which is never
intended.  Instead, we can point the user to the position of the error
so that it can be easily fixed, unlike the current behavior, where the
user is left guessing.

(I don't think eval-expression-debug-on-error catches read errors --
if it did, it would show a *Backtrace* buffer when an unreadable
expression is entered.  But I didn't look very deeply, so correct that
if it's wrong.)

bug#30697: make eval-expression only take `read'-able Lisp, bug#30697: make eval-expression only take `read'-able Lisp

[Lars Ingebrigtsen]

charles@aurox.ch (Charles A. Roelli) writes:

> Here is a change to make eval-expression issue a warning in the
> minibuffer when the user enters an unreadable expression.  Currently,
> we just error out, so the user has to restart the command and go back
> in the history to get back what he typed.

I think this sounds like a good change -- I often type invalid stuff
into M-: and then have to M-: M-p, so this is a usability win.

>> You are changing the behavior of read--expression, which has a few
>> callers in Emacs.  Did you verify that those callers won't break due
>> to this change?
>
> I've now tested them, and they work fine.  `eval-minibuffer' (which is
> used for (interactive "X")) turns out to be one of the callers of
> `read--expression', so I think the documentation of that interactive
> spec will have to be updated too.
>
> I also did small tests with ido-mode/icomplete-mode enabled, which
> also do not seem to be affected by this change.
>
>> This should also have a NEWS entry, and perhaps the manual needs some
>> changes, please take a look.
>
> Thanks, I will work on that.

Did you finish up the change, Charles?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#30697: make eval-expression only take `read'-able Lisp, bug#30697: make eval-expression only take `read'-able Lisp

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>>> This should also have a NEWS entry, and perhaps the manual needs some
>>> changes, please take a look.
>>
>> Thanks, I will work on that.
>
> Did you finish up the change, Charles?

This was a year ago, so I did added the NEWS entry.  I grepped through
the manuals for eval-expression to see whether the previous behaviour
was mentioned there (and would therefore need changing), but I couldn't
find anything, so I didn't change anything.

So this is now pushed to Emacs 28, and I think it's a really good and
natural change.  Thanks, Charles.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#30697: make eval-expression only take `read'-able Lisp, bug#30697: make eval-expression only take `read'-able Lisp

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> This was a year ago, so I did added the NEWS entry.  I grepped through
> the manuals for eval-expression to see whether the previous behaviour
> was mentioned there (and would therefore need changing), but I couldn't
> find anything, so I didn't change anything.

(I also ran a "make check", because there's a bunch of tests around this
area, and it didn't introduce any regressions.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no