bug#13994: End of buffer error for forward-sexp

bug#13994: End of buffer error for forward-sexp

[Aaron S. Hawley]

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

I would like C-M-f (`forward-sexp') to signal an error when reaching
the beginning or end of a buffer.  When running a keyboard macro that
contains this command, it should reach an error condition of "End of
buffer".  If wish to run C-0 C-x e on a file full of S-expressions and
have the macro end when an error is reached.  Currently, my macro puts
Emacs in an infinite loop.

I've attached a patch that adds these error conditions.  I also need
to correct my email address in the ChangeLog.

What I wrote isn't every strict.  I only catch the condition when the
function is started from beginning or end of buffer.  If the user
gives an argument for more S-expressions than there are between the
beginning of the buffer there is no error triggered.  This was a
compromise but also a simpler patch to contribute.  I've added the
unit tests I wrote to implement.   I hope they're helpful.

I'm not sure why this was never the case in the first place, nor do I
know what the consequence of fixing it is.  I'm not sure if the lack
of an error is necessary in the 112 libraries where this function is
used in Emacs (and probably many times more in libraries outside of
Emacs!).  I'm hoping that after the release Emacs 24.3, now is a good
time to install this in trunk and find out.

Thanks,
/a

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2013-03-18 19:44:15 +0000
+++ lisp/ChangeLog	2013-03-18 20:37:01 +0000
@@ -1,3 +1,5 @@
+2013-03-18  Aaron S. Hawley  <aaron.s.hawley@gmail.com>
+
+        * emacs-lisp/lisp.el (forward-sexp): Signal an error when end of
+        buffer or beginning of buffer reached.
+
@@ -1974,7 +1979,7 @@
 	(js--multi-line-declaration-indentation): New function.
 	(js--proper-indentation): Use it.

-2013-01-11  Aaron S. Hawley  <Aaron.Hawley@vtinfo.com>
+2013-01-11  Aaron S. Hawley  <aaron.s.hawley@gmail.com>

 	* calc/calc.el (calc-highlight-selections-with-faces)
 	 (calc-dispatch):

=== modified file 'lisp/emacs-lisp/lisp.el'
--- lisp/emacs-lisp/lisp.el	2013-01-01 09:11:05 +0000
+++ lisp/emacs-lisp/lisp.el	2013-03-18 19:38:50 +0000
@@ -58,6 +58,10 @@
   (or arg (setq arg 1))
   (if forward-sexp-function
       (funcall forward-sexp-function arg)
+    (when (and (> arg 0) (eobp))
+      (signal 'end-of-buffer nil))
+    (when (and (< arg 0) (bobp))
+      (signal 'beginning-of-buffer nil))
     (goto-char (or (scan-sexps (point) arg) (buffer-end arg)))
     (if (< arg 0) (backward-prefix-chars))))


=== added file 'test/automated/sexp-tests.el'
--- test/automated/sexp-tests.el	1970-01-01 00:00:00 +0000
+++ test/automated/sexp-tests.el	2013-03-18 21:51:52 +0000
@@ -0,0 +1,98 @@
+;;; sexp-tests.el --- Test S-expression support in Emacs
+
+;; Copyright (C) 2013  Aaron S. Hawley
+
+;; Author: Aaron S. Hawley <aaron.s.hawley@gmail.com>
+;; Keywords: internal
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Testing of `forward-sexp'.
+
+;;; Code:
+
+(require 'ert)
+
+(ert-deftest sexp-forward-1 ()
+  "Test basics of \\[forward-sexp]."
+  (with-temp-buffer
+    (insert "()")
+    (goto-char (point-min))
+    (should (null
+             (forward-sexp 1)))))
+
+(ert-deftest sexp-backward-1 ()
+  "Test basics of \\[backward-sexp]."
+  (with-temp-buffer
+    (insert "()")
+    (should (null
+             (forward-sexp -1)))))
+
+(ert-deftest sexp-forward-1-error-eobp ()
+  "Test error when \\[forward-sexp] at `eobp'."
+  (with-temp-buffer
+    (insert "()")
+    (should-error
+     (forward-sexp 1))))
+
+(ert-deftest sexp-backward-1-error-eobp ()
+  "Test error when \\[backward-sexp] at `bobp'."
+  (with-temp-buffer
+    (insert "()")
+    (goto-char (point-min))
+    (should-error
+     (forward-sexp -1))))
+
+(ert-deftest sexp-forward-2-eobp-no-error ()
+  "Test lack of error when \\[forward-sexp] beyond `eobp'."
+  (with-temp-buffer
+    (insert "()")
+    (goto-char (point-min))
+    (should (null
+     (forward-sexp 2)))
+    (should (eobp))))
+
+(ert-deftest sexp-backward-2-bobp-no-error ()
+  "Test lack of error when \\[backward-sexp] beyond `bobp'."
+  (with-temp-buffer
+    (insert "()")
+    (should (null
+     (forward-sexp -2)))
+    (should (bobp))))
+
+(ert-deftest sexp-forward-2-eobp-subsequent-error ()
+  "Test error when \\[forward-sexp] when started from `eobp'."
+  (with-temp-buffer
+    (insert "()")
+    (goto-char (point-min))
+    (should (null
+     (forward-sexp 2)))
+    (should (eobp))
+    (should-error
+     (forward-sexp 1))))
+
+(ert-deftest sexp-backward-2-bobp-subsequent-error ()
+  "Test error when \\[backward-sexp] when started from `bobp'."
+  (with-temp-buffer
+    (insert "()")
+    (should (null
+     (forward-sexp -2)))
+    (should (bobp))
+    (should-error
+     (forward-sexp -1))))
+
+(provide 'sexp-tests)
+;;; sexp-tests.el ends here

-- 
In general, we reserve the right to have a poor
memory--the computer, however, is supposed to
remember!  Poor computer.  -- Guy Lewis Steele Jr.

[-- Attachment #2: lisp.el.diff --]
[-- Type: application/octet-stream, Size: 4107 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2013-03-18 19:44:15 +0000
+++ lisp/ChangeLog	2013-03-18 20:37:01 +0000
@@ -1,3 +1,5 @@
+2013-03-18  Aaron S. Hawley  <aaron.s.hawley@gmail.com>
+
+        * emacs-lisp/lisp.el (forward-sexp): Signal an error when end of
+        buffer or beginning of buffer reached.
+
@@ -1974,7 +1979,7 @@
 	(js--multi-line-declaration-indentation): New function.
 	(js--proper-indentation): Use it.
 
-2013-01-11  Aaron S. Hawley  <Aaron.Hawley@vtinfo.com>
+2013-01-11  Aaron S. Hawley  <aaron.s.hawley@gmail.com>
 
 	* calc/calc.el (calc-highlight-selections-with-faces)
 	 (calc-dispatch):

=== modified file 'lisp/emacs-lisp/lisp.el'
--- lisp/emacs-lisp/lisp.el	2013-01-01 09:11:05 +0000
+++ lisp/emacs-lisp/lisp.el	2013-03-18 19:38:50 +0000
@@ -58,6 +58,10 @@
   (or arg (setq arg 1))
   (if forward-sexp-function
       (funcall forward-sexp-function arg)
+    (when (and (> arg 0) (eobp))
+      (signal 'end-of-buffer nil))
+    (when (and (< arg 0) (bobp))
+      (signal 'beginning-of-buffer nil))
     (goto-char (or (scan-sexps (point) arg) (buffer-end arg)))
     (if (< arg 0) (backward-prefix-chars))))
 

=== added file 'test/automated/sexp-tests.el'
--- test/automated/sexp-tests.el	1970-01-01 00:00:00 +0000
+++ test/automated/sexp-tests.el	2013-03-18 21:51:52 +0000
@@ -0,0 +1,98 @@
+;;; sexp-tests.el --- Test S-expression support in Emacs
+
+;; Copyright (C) 2013  Aaron S. Hawley
+
+;; Author: Aaron S. Hawley <aaron.s.hawley@gmail.com>
+;; Keywords: internal
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Testing of `forward-sexp'.
+
+;;; Code:
+
+(require 'ert)
+
+(ert-deftest sexp-forward-1 ()
+  "Test basics of \\[forward-sexp]."
+  (with-temp-buffer
+    (insert "()")
+    (goto-char (point-min))
+    (should (null
+             (forward-sexp 1)))))
+
+(ert-deftest sexp-backward-1 ()
+  "Test basics of \\[backward-sexp]."
+  (with-temp-buffer
+    (insert "()")
+    (should (null
+             (forward-sexp -1)))))
+
+(ert-deftest sexp-forward-1-error-eobp ()
+  "Test error when \\[forward-sexp] at `eobp'."
+  (with-temp-buffer
+    (insert "()")
+    (should-error
+     (forward-sexp 1))))
+
+(ert-deftest sexp-backward-1-error-eobp ()
+  "Test error when \\[backward-sexp] at `bobp'."
+  (with-temp-buffer
+    (insert "()")
+    (goto-char (point-min))
+    (should-error
+     (forward-sexp -1))))
+
+(ert-deftest sexp-forward-2-eobp-no-error ()
+  "Test lack of error when \\[forward-sexp] beyond `eobp'."
+  (with-temp-buffer
+    (insert "()")
+    (goto-char (point-min))
+    (should (null
+     (forward-sexp 2)))
+    (should (eobp))))
+
+(ert-deftest sexp-backward-2-bobp-no-error ()
+  "Test lack of error when \\[backward-sexp] beyond `bobp'."
+  (with-temp-buffer
+    (insert "()")
+    (should (null
+     (forward-sexp -2)))
+    (should (bobp))))
+
+(ert-deftest sexp-forward-2-eobp-subsequent-error ()
+  "Test error when \\[forward-sexp] when started from `eobp'."
+  (with-temp-buffer
+    (insert "()")
+    (goto-char (point-min))
+    (should (null
+     (forward-sexp 2)))
+    (should (eobp))
+    (should-error
+     (forward-sexp 1))))
+
+(ert-deftest sexp-backward-2-bobp-subsequent-error ()
+  "Test error when \\[backward-sexp] when started from `bobp'."
+  (with-temp-buffer
+    (insert "()")
+    (should (null
+     (forward-sexp -2)))
+    (should (bobp))
+    (should-error
+     (forward-sexp -1))))
+
+(provide 'sexp-tests)
+;;; sexp-tests.el ends here

bug#13994: End of buffer error for forward-sexp

[Stefan Monnier]

> I would like C-M-f (`forward-sexp') to signal an error when reaching
> the beginning or end of a buffer.

That would make sense, indeed, but I'm afraid there's a fair bit of code
out there that needs the current behavior.


        Stefan

bug#13994: End of buffer error for forward-sexp

[Aaron S. Hawley]

>> I would like C-M-f (`forward-sexp') to signal an error when reaching
>> the beginning or end of a buffer.
>
> That would make sense, indeed, but I'm afraid there's a fair bit of code
> out there that needs the current behavior.

I by no means use all of Emacs, but haven't run into an issue yet with
my patch.  I use Emacs with t-d-o-e on.

Do you have an example of code that you've seen "needs the current
behavior" at the beginning or end of the buffer.  I'd be happy to
crusade and study this further and root those out.

So far, I've looked at these functions in Emacs to see if they still work.

end-of-sexp
beginning-of-sexp
thing-at-point-bounds-of-list-at-point
sort-numeric-fields
transpose-sexps
blink-matching-open
diary-list-sexp-entries
diary-mark-sexp-entries
comint-extract-string

bug#13994: End of buffer error for forward-sexp

[Drew Adams]

> I by no means use all of Emacs, but haven't run into an issue yet with
> my patch.  I use Emacs with t-d-o-e on.

Pretty small sample.

> Do you have an example of code that you've seen "needs the current
> behavior" at the beginning or end of the buffer....
> 
> So far, I've looked at these functions in Emacs to see if 
> they still work.
> 
> end-of-sexp
> beginning-of-sexp
> thing-at-point-bounds-of-list-at-point
> sort-numeric-fields
> transpose-sexps
> blink-matching-open
> diary-list-sexp-entries
> diary-mark-sexp-entries
> comint-extract-string

Pretty small sample.  There must be thousands of uses of `forward-sexp' in
existing Emacs-Lisp code out there.  How many of them depend (intentionally or
not) on `forward-sexp' returning nil for premature `eobp'?  Who knows.

Now everytime some code does (when (looking-at "\"") (forward-sexp 1)) or
whatever it will need to be fixed to wrap it in `ignore-errors' or otherwise
treat the `eobp' case?

Doesn't sound like a sound approach at this point.
Might have been a reasonable suggestion 30 years ago.

What's the point?  If you have some code that needs to raise an error when
`forward-sexp' reaches eob, that's easy enough to do, no?

Not to mention that the code for `forward-sexp' explicitly takes point to eob
when `scan-sexps' returns nil.  IOW, that behavior is presumably by design.

And not to mention that it calls `forward-sexp-function', if non-nil, to do
everything, in which case (depending on where you would place your call to
`error') you might be changing the meaning/behavior for its code as well, if it
reaches eob.

bug#13994: End of buffer error for forward-sexp

[Aaron S. Hawley]

> Now everytime some code does (when (looking-at "\"") (forward-sexp 1)) or
> whatever it will need to be fixed to wrap it in `ignore-errors' or otherwise
> treat the `eobp' case?

What I'm finding in my small sample is that most code already has
ignore-errors or is finding the end or beginning of a buffer with a
different Emacs primitive.  In other words, no code changes!

> What's the point?  If you have some code that needs to raise an error when
> `forward-sexp' reaches eob, that's easy enough to do, no?

I wouldn't have sent a patch if I didn't think it should be the default.

> Not to mention that the code for `forward-sexp' explicitly takes point to eob
> when `scan-sexps' returns nil.  IOW, that behavior is presumably by design.

I don't believe my patch is changing that behavior.

> And not to mention that it calls `forward-sexp-function', if non-nil, to do
> everything, in which case (depending on where you would place your call to
> `error') you might be changing the meaning/behavior for its code as well, if it
> reaches eob.

I'm pretty sure my patch doesn't take anything away from the user's
ability to entirely redefine forward-sexp by setting
forward-sexp-function.

bug#13994: End of buffer error for forward-sexp

[Stefan Monnier]

>>> I would like C-M-f (`forward-sexp') to signal an error when reaching
>>> the beginning or end of a buffer.
>> That would make sense, indeed, but I'm afraid there's a fair bit of code
>> out there that needs the current behavior.
> I by no means use all of Emacs, but haven't run into an issue yet with
> my patch.  I use Emacs with t-d-o-e on.

Thoughts in random order:
- the gain is not very large, so the pain needs to be very low.
- forward-sexp is used at many places in many different circumstances,
  so it's difficult to find risky cases.
- I do agree that it is very likely that many/most uses of forward-sexp
  wouldn't suffer.
- Even if few problematic cases are out there (or out here in Emacs
  itself), it may take a very long time (read: not before an actual
  release) to find some of them.
- your patch only affects behavior at BOB/EOB, whereas I think it would
  make more sense to do the same for "before the first non-whitespace"
  and "after the last non-whitespace".  And comments should be considered
  whitespace in this respect (at least when parse-sexp-ignore-comments
  is non-nil).  Of course, fixing this might introduce more
  problematic cases.


        Stefan

bug#13994: End of buffer error for forward-sexp

[Juri Linkov]

> I would like C-M-f (`forward-sexp') to signal an error when reaching
> the beginning or end of a buffer.

If you want only `C-M-f' to signal an error, then you could create
a new command with a name like `forward-sexp-command' that signals
an error and bind it to `C-M-f'.  That's what e.g `scroll-up-command'
does to control error-signaling only for the command bound to `C-v'
without affecting the lower-level function `scroll-up'.

bug#13994: End of buffer error for forward-sexp

[Andreas Röhler]

Am 18.03.2013 22:54, schrieb Aaron S. Hawley:
> I would like C-M-f (`forward-sexp') to signal an error when reaching
> the beginning or end of a buffer.  When running a keyboard macro that
> contains this command, it should reach an error condition of "End of
> buffer".  If wish to run C-0 C-x e on a file full of S-expressions and
> have the macro end when an error is reached.  Currently, my macro puts
> Emacs in an infinite loop.
>
> I've attached a patch that adds these error conditions.  I also need
> to correct my email address in the ChangeLog.
>
> What I wrote isn't every strict.  I only catch the condition when the
> function is started from beginning or end of buffer.  If the user
> gives an argument for more S-expressions than there are between the
> beginning of the buffer there is no error triggered.  This was a
> compromise but also a simpler patch to contribute.  I've added the
> unit tests I wrote to implement.   I hope they're helpful.
>
> I'm not sure why this was never the case in the first place, nor do I
> know what the consequence of fixing it is.  I'm not sure if the lack
> of an error is necessary in the 112 libraries where this function is
> used in Emacs (and probably many times more in libraries outside of
> Emacs!).  I'm hoping that after the release Emacs 24.3, now is a good
> time to install this in trunk and find out.
>
> Thanks,
> /a
>
> === modified file 'lisp/ChangeLog'
> --- lisp/ChangeLog	2013-03-18 19:44:15 +0000
> +++ lisp/ChangeLog	2013-03-18 20:37:01 +0000
> @@ -1,3 +1,5 @@
> +2013-03-18  Aaron S. Hawley  <aaron.s.hawley@gmail.com>
> +
> +        * emacs-lisp/lisp.el (forward-sexp): Signal an error when end of
> +        buffer or beginning of buffer reached.
> +
> @@ -1974,7 +1979,7 @@
>   	(js--multi-line-declaration-indentation): New function.
>   	(js--proper-indentation): Use it.
>
> -2013-01-11  Aaron S. Hawley  <Aaron.Hawley@vtinfo.com>
> +2013-01-11  Aaron S. Hawley  <aaron.s.hawley@gmail.com>
>
>   	* calc/calc.el (calc-highlight-selections-with-faces)
>   	 (calc-dispatch):
>
> === modified file 'lisp/emacs-lisp/lisp.el'
> --- lisp/emacs-lisp/lisp.el	2013-01-01 09:11:05 +0000
> +++ lisp/emacs-lisp/lisp.el	2013-03-18 19:38:50 +0000
> @@ -58,6 +58,10 @@
>     (or arg (setq arg 1))
>     (if forward-sexp-function
>         (funcall forward-sexp-function arg)
> +    (when (and (> arg 0) (eobp))
> +      (signal 'end-of-buffer nil))
> +    (when (and (< arg 0) (bobp))
> +      (signal 'beginning-of-buffer nil))
>       (goto-char (or (scan-sexps (point) arg) (buffer-end arg)))
>       (if (< arg 0) (backward-prefix-chars))))
>
>
> === added file 'test/automated/sexp-tests.el'
> --- test/automated/sexp-tests.el	1970-01-01 00:00:00 +0000
> +++ test/automated/sexp-tests.el	2013-03-18 21:51:52 +0000
> @@ -0,0 +1,98 @@
> +;;; sexp-tests.el --- Test S-expression support in Emacs
> +
> +;; Copyright (C) 2013  Aaron S. Hawley
> +
> +;; Author: Aaron S. Hawley <aaron.s.hawley@gmail.com>
> +;; Keywords: internal
> +
> +;; This program is free software; you can redistribute it and/or modify
> +;; it under the terms of the GNU General Public License as published by
> +;; the Free Software Foundation, either version 3 of the License, or
> +;; (at your option) any later version.
> +
> +;; This program is distributed in the hope that it will be useful,
> +;; but WITHOUT ANY WARRANTY; without even the implied warranty of
> +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +;; GNU General Public License for more details.
> +
> +;; You should have received a copy of the GNU General Public License
> +;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +;;; Commentary:
> +
> +;; Testing of `forward-sexp'.
> +
> +;;; Code:
> +
> +(require 'ert)
> +
> +(ert-deftest sexp-forward-1 ()
> +  "Test basics of \\[forward-sexp]."
> +  (with-temp-buffer
> +    (insert "()")
> +    (goto-char (point-min))
> +    (should (null
> +             (forward-sexp 1)))))
> +
> +(ert-deftest sexp-backward-1 ()
> +  "Test basics of \\[backward-sexp]."
> +  (with-temp-buffer
> +    (insert "()")
> +    (should (null
> +             (forward-sexp -1)))))
> +
> +(ert-deftest sexp-forward-1-error-eobp ()
> +  "Test error when \\[forward-sexp] at `eobp'."
> +  (with-temp-buffer
> +    (insert "()")
> +    (should-error
> +     (forward-sexp 1))))
> +
> +(ert-deftest sexp-backward-1-error-eobp ()
> +  "Test error when \\[backward-sexp] at `bobp'."
> +  (with-temp-buffer
> +    (insert "()")
> +    (goto-char (point-min))
> +    (should-error
> +     (forward-sexp -1))))
> +
> +(ert-deftest sexp-forward-2-eobp-no-error ()
> +  "Test lack of error when \\[forward-sexp] beyond `eobp'."
> +  (with-temp-buffer
> +    (insert "()")
> +    (goto-char (point-min))
> +    (should (null
> +     (forward-sexp 2)))
> +    (should (eobp))))
> +
> +(ert-deftest sexp-backward-2-bobp-no-error ()
> +  "Test lack of error when \\[backward-sexp] beyond `bobp'."
> +  (with-temp-buffer
> +    (insert "()")
> +    (should (null
> +     (forward-sexp -2)))
> +    (should (bobp))))
> +
> +(ert-deftest sexp-forward-2-eobp-subsequent-error ()
> +  "Test error when \\[forward-sexp] when started from `eobp'."
> +  (with-temp-buffer
> +    (insert "()")
> +    (goto-char (point-min))
> +    (should (null
> +     (forward-sexp 2)))
> +    (should (eobp))
> +    (should-error
> +     (forward-sexp 1))))
> +
> +(ert-deftest sexp-backward-2-bobp-subsequent-error ()
> +  "Test error when \\[backward-sexp] when started from `bobp'."
> +  (with-temp-buffer
> +    (insert "()")
> +    (should (null
> +     (forward-sexp -2)))
> +    (should (bobp))
> +    (should-error
> +     (forward-sexp -1))))
> +
> +(provide 'sexp-tests)
> +;;; sexp-tests.el ends here
>

IIRC that was the default at XEmacs. Very annoying, as beginning or EOB are hitted quit often. Than the debugger starts.

For now, what about writing

(if (forward-sexp)
     DO-SOMEthing
   (message "%s" "nil"))

Best regards,

Andreas