unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#30697: make eval-expression only take `read'-able Lisp
@ 2018-03-04 16:46 Charles A. Roelli
  2018-03-04 17:10 ` Eli Zaretskii
  0 siblings, 1 reply; 8+ messages in thread
From: Charles A. Roelli @ 2018-03-04 16:46 UTC (permalink / raw)
  To: 30697

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)





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

* bug#30697: make eval-expression only take `read'-able Lisp
  2018-03-04 16:46 bug#30697: make eval-expression only take `read'-able Lisp Charles A. Roelli
@ 2018-03-04 17:10 ` Eli Zaretskii
  2018-03-05 18:52   ` Charles A. Roelli
  2019-06-24 18:58   ` bug#30697: make eval-expression only take `read'-able Lisp, " Lars Ingebrigtsen
  0 siblings, 2 replies; 8+ messages in thread
From: Eli Zaretskii @ 2018-03-04 17:10 UTC (permalink / raw)
  To: Charles A. Roelli; +Cc: 30697

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





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

* bug#30697: make eval-expression only take `read'-able Lisp
       [not found] <<m27eqriyat.fsf@aurox.ch>
@ 2018-03-04 22:36 ` Drew Adams
  2018-03-05 19:09   ` Charles A. Roelli
  0 siblings, 1 reply; 8+ messages in thread
From: Drew Adams @ 2018-03-04 22:36 UTC (permalink / raw)
  To: Charles A. Roelli, 30697

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





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

* bug#30697: make eval-expression only take `read'-able Lisp
  2018-03-04 17:10 ` Eli Zaretskii
@ 2018-03-05 18:52   ` Charles A. Roelli
  2019-06-24 18:58   ` bug#30697: make eval-expression only take `read'-able Lisp, " Lars Ingebrigtsen
  1 sibling, 0 replies; 8+ messages in thread
From: Charles A. Roelli @ 2018-03-05 18:52 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 30697

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





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

* bug#30697: make eval-expression only take `read'-able Lisp
  2018-03-04 22:36 ` Drew Adams
@ 2018-03-05 19:09   ` Charles A. Roelli
  0 siblings, 0 replies; 8+ messages in thread
From: Charles A. Roelli @ 2018-03-05 19:09 UTC (permalink / raw)
  To: Drew Adams; +Cc: 30697

> 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.)





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

* bug#30697: make eval-expression only take `read'-able Lisp, bug#30697: make eval-expression only take `read'-able Lisp
  2018-03-04 17:10 ` Eli Zaretskii
  2018-03-05 18:52   ` Charles A. Roelli
@ 2019-06-24 18:58   ` Lars Ingebrigtsen
  2020-08-10 13:17     ` Lars Ingebrigtsen
  1 sibling, 1 reply; 8+ messages in thread
From: Lars Ingebrigtsen @ 2019-06-24 18:58 UTC (permalink / raw)
  To: Charles A. Roelli; +Cc: 30697

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





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

* bug#30697: make eval-expression only take `read'-able Lisp, bug#30697: make eval-expression only take `read'-able Lisp
  2019-06-24 18:58   ` bug#30697: make eval-expression only take `read'-able Lisp, " Lars Ingebrigtsen
@ 2020-08-10 13:17     ` Lars Ingebrigtsen
  2020-08-10 13:20       ` Lars Ingebrigtsen
  0 siblings, 1 reply; 8+ messages in thread
From: Lars Ingebrigtsen @ 2020-08-10 13:17 UTC (permalink / raw)
  To: Charles A. Roelli; +Cc: 30697

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





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

* bug#30697: make eval-expression only take `read'-able Lisp, bug#30697: make eval-expression only take `read'-able Lisp
  2020-08-10 13:17     ` Lars Ingebrigtsen
@ 2020-08-10 13:20       ` Lars Ingebrigtsen
  0 siblings, 0 replies; 8+ messages in thread
From: Lars Ingebrigtsen @ 2020-08-10 13:20 UTC (permalink / raw)
  To: Charles A. Roelli; +Cc: 30697

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





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

end of thread, other threads:[~2020-08-10 13:20 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-04 16:46 bug#30697: make eval-expression only take `read'-able Lisp Charles A. Roelli
2018-03-04 17:10 ` Eli Zaretskii
2018-03-05 18:52   ` Charles A. Roelli
2019-06-24 18:58   ` bug#30697: make eval-expression only take `read'-able Lisp, " Lars Ingebrigtsen
2020-08-10 13:17     ` Lars Ingebrigtsen
2020-08-10 13:20       ` Lars Ingebrigtsen
     [not found] <<m27eqriyat.fsf@aurox.ch>
2018-03-04 22:36 ` Drew Adams
2018-03-05 19:09   ` Charles A. Roelli

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).