bug#24427: 25.1.50; end-of-defun jumps too far

bug#24427: 25.1.50; end-of-defun jumps too far

[Marcin Borkowski]

Hi Emacs bug team,

consider an Emacs Lisp buffer like this:

--8<---------------cut here---------------start------------->8---
(defun a ())

(defun b ())

(defun c ())
--8<---------------cut here---------------end--------------->8---

Place the point anywhere on the first line and press C-M-e
(`end-of-defun') twice.  Now place the point back on the first line and
press C-u 2 C-M-e.  The result is (and should be) the same as before.

Now consider this buffer:

--8<---------------cut here---------------start------------->8---
(defun a ())
(defun b ())
(defun c ())
--8<---------------cut here---------------end--------------->8---

Repeat the same steps as above.  After pressing C-M-e twice (starting on
the first line), the point lands right before "(defun c ())".  After
pressing C-u 2 C-M-e, it lands _after_ that defun.  I am quite sure it
should be in the same place as after C-M-e C-M-e.

I use GNU Emacs 25.1.50.6 (commit b12fac8).  I tested this on emacs -Q.

Best,
Marcin Borkowski




In GNU Emacs 25.1.50.6 (x86_64-unknown-linux-gnu, GTK+ Version 3.20.6)
 of 2016-08-03 built on jane
Repository revision: 0f690c4a0e4e98a929b5439f6076539bbea59242
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY ACL GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 LIBSYSTEMD

Important settings:
  value of $LC_COLLATE: pl_PL.UTF-8
  value of $LC_CTYPE: pl_PL.UTF-8
  value of $LC_MESSAGES: pl_PL.UTF-8
  value of $LANG: pl_PL.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  diff-auto-refine-mode: t
  global-git-commit-mode: t
  async-bytecomp-package-mode: t
  beeminder-org-integration-mode: t
  TeX-PDF-mode: t
  pdf-occur-global-minor-mode: t
  yas-global-mode: t
  yas-minor-mode: t
  savehist-mode: t
  global-flycheck-mode: t
  projectile-global-mode: t
  projectile-mode: t
  ivy-mode: t
  show-paren-mode: t
  shell-dirtrack-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-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
  line-number-mode: t
  transient-mark-mode: t

Memory information:
((conses 16 1884431 142870)
 (symbols 48 85829 0)
 (miscs 40 9808 8727)
 (strings 32 324415 120559)
 (string-bytes 1 12120978)
 (vectors 16 127614)
 (vector-slots 8 2717980 77714)
 (floats 8 31935 16669)
 (intervals 56 171322 1289)
 (buffers 976 132)
 (heap 1024 236107 10150))

-- 
Marcin Borkowski

bug#24427: 25.1.50; end-of-defun jumps too far

[Robert Cochran]

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

Appears to be an off-by-one error when calling `beginning-of-defun-raw`
during the second part of moving forward. This patch `1+`s the `(- arg)`
to make both methods equivalent.

Please do not hesitate to mention anything I may have missed.

-----


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix off-by-one error in end-of-defun --]
[-- Type: text/x-patch, Size: 1065 bytes --]

From 0f4facc6ea2bfe01ba3b8c7e3a0e99210d1bb1e1 Mon Sep 17 00:00:00 2001
From: Robert Cochran <robert-git@cochranmail.com>
Date: Tue, 13 Sep 2016 13:17:32 -0700
Subject: [PATCH] Fix off-by-one error when going forward in end-of-defun

end-of-defun (C-M-e) goes forward one too many defuns when given a
prefix argument. Fix this so that doing 'C-M-e' foo times and using 'C-u
foo C-M-e' do the same thing.

* lisp/emacs-lisp/lisp.el (end-of-defun): Fix off-by-one error when
going forward.
---
 lisp/emacs-lisp/lisp.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index ea7cce6..bf03c44 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -452,7 +452,7 @@ end-of-defun
         ;; We started from after the end of the previous function.
         (goto-char pos))
       (unless (zerop arg)
-        (beginning-of-defun-raw (- arg))
+        (beginning-of-defun-raw (1+ (- arg)))
         (funcall end-of-defun-function)))
      ((< arg 0)
       ;; Moving backward.
-- 
2.7.4


[-- Attachment #3: Type: text/plain, Size: 102 bytes --]

-----

HTH,
-- 
~Robert Cochran

GPG Fingerprint - E778 2DD4 FEA6 6A68 6F26  AD2D E5C3 EB36 4886 8871

bug#24427: 25.1.50; end-of-defun jumps too far

[Robert Cochran]

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

Oops, didn't double-space the sentence ending (again!). Fixed in this
copy.

-----


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix off-by-one error in end-of-defun --]
[-- Type: text/x-patch, Size: 1066 bytes --]

From bffa381cd9847b22b3589076b9dabeb572577580 Mon Sep 17 00:00:00 2001
From: Robert Cochran <robert-git@cochranmail.com>
Date: Tue, 13 Sep 2016 13:17:32 -0700
Subject: [PATCH] Fix off-by-one error when going forward in end-of-defun

end-of-defun (C-M-e) goes forward one too many defuns when given a
prefix argument.  Fix this so that doing 'C-M-e' foo times and using
'C-u foo C-M-e' do the same thing.

* lisp/emacs-lisp/lisp.el (end-of-defun): Fix off-by-one error when
going forward.
---
 lisp/emacs-lisp/lisp.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index ea7cce6..bf03c44 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -452,7 +452,7 @@ end-of-defun
         ;; We started from after the end of the previous function.
         (goto-char pos))
       (unless (zerop arg)
-        (beginning-of-defun-raw (- arg))
+        (beginning-of-defun-raw (1+ (- arg)))
         (funcall end-of-defun-function)))
      ((< arg 0)
       ;; Moving backward.
-- 
2.7.4


[-- Attachment #3: Type: text/plain, Size: 102 bytes --]

-----

HTH,
-- 
~Robert Cochran

GPG Fingerprint - E778 2DD4 FEA6 6A68 6F26  AD2D E5C3 EB36 4886 8871

bug#24427: 25.1.50; end-of-defun jumps too far

[Robert Cochran]

Ping!

Has anyone had the opportunity to look over this?

TIA,
-- 
~Robert Cochran

GPG Fingerprint - E778 2DD4 FEA6 6A68 6F26  AD2D E5C3 EB36 4886 8871

bug#24427: 25.1.50; end-of-defun jumps too far

[Marcin Borkowski]

On 2016-09-20, at 20:31, Robert Cochran <robert-emacs@cochranmail.com> wrote:

> Ping!
>
> Has anyone had the opportunity to look over this?

Hi,

I'm extremely busy right now, but I looked into it and it seemed to
break some other tests of mine.  I haven't yet found the problem (or
even confirmed it), but I expect to have some time for it in the next
few days.

Anyway, thanks for looking into it in the first place!

> TIA,

Stay tuned and best regards,

-- 
Marcin Borkowski

bug#24427: 25.1.50; end-of-defun jumps too far

[Marcin Borkowski]

On 2016-09-21, at 21:59, Marcin Borkowski <mbork@mbork.pl> wrote:

> On 2016-09-20, at 20:31, Robert Cochran <robert-emacs@cochranmail.com> wrote:
>
>> Ping!
>>
>> Has anyone had the opportunity to look over this?
>
> Hi,
>
> I'm extremely busy right now, but I looked into it and it seemed to
> break some other tests of mine.  I haven't yet found the problem (or
> even confirmed it), but I expect to have some time for it in the next
> few days.

OK, so here's the problem I found when running my personal tests for my
`mark-defun'.

Consider this Elisp buffer:

--8<---------------cut here---------------start------------->8---
;; Comment header

(defun func-1 (arg)
  "docstring"
  body)
-!-
;; Comment before a defun
(defun func-2 (arg)
  "docstring"
  body)

(defun func-3 (arg)
  "docstring"
  body)
(defun func-4 (arg)
  "docstring"
  body)

;; end
--8<---------------cut here---------------end--------------->8---

where -!- means the point location.  Now press C-u 2 C-M-e, and you
moved by one defun instead of two.

Best,

-- 
Marcin Borkowski

bug#24427: 25.1.50; end-of-defun jumps too far

[Robert Cochran]

Marcin Borkowski <mbork@mbork.pl> writes:

> OK, so here's the problem I found when running my personal tests for my
> `mark-defun'.
>
> Consider this Elisp buffer:
>
> ;; Comment header
>
> (defun func-1 (arg)
>   "docstring"
>   body)
> -!-
> ;; Comment before a defun
> (defun func-2 (arg)
>   "docstring"
>   body)
>
> (defun func-3 (arg)
>   "docstring"
>   body)
> (defun func-4 (arg)
>   "docstring"
>   body)
>
> ;; end
>
> where -!- means the point location.  Now press C-u 2 C-M-e, and you
> moved by one defun instead of two.

This particular problem is the result of this bit in the body of
`end-of-defun`:

#+BEGIN_SRC emacs-lisp
(if (> (point) pos)
    ;; We already moved forward by one because we started from
    ;; within a function.
    (setq arg (1- arg))
  ;; We started from after the end of the previous function.
  (goto-char pos))
#+END_SRC

When the whitespace is skipped after doing the initial position
calculations, point, which ends up either on or after the

> ;; Comment before a defun

line, is indeed after `pos`, a recording of point before doing any
movement. The assumption that we were in a function body, as stated in
the comment, doesn't hold. So the definition count is erroneously
decreased.

Nothing has come to mind for a method to fix it without breaking other
things. Perhaps the solution is obvious for someone else? Suggestions
would be nice if you have them.

(As an aside, to vent a little, it's rather frustrating that both pre-
and post-patch do what you regard as TRT in different
circumstances. Especially so because pre-patch is only doing TRT as a
result of what I would say is two bugs canceling each other
out. Obviously not your fault, but still frustrating.)

Thanks,
-- 
~Robert Cochran

GPG Fingerprint - E778 2DD4 FEA6 6A68 6F26  AD2D E5C3 EB36 4886 8871

bug#24427: 25.1.50; end-of-defun jumps too far

[Noam Postavsky]

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

Robert Cochran <robert-emacs@cochranmail.com> writes:

> Nothing has come to mind for a method to fix it without breaking other
> things. Perhaps the solution is obvious for someone else? Suggestions
> would be nice if you have them.

Not entirely sure if this is correct, but the patch below seems to fix
it for me, without breaking any of the previously mentioned scenarios.
I find the fact that end-of-defun goes to the line following the closing
paren a bit dubious, though I guess since it's been that way so long, we
can hardly change it now.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 2580 bytes --]

From 111ea54ce469d7e601c0e7c2b48d1748021e71a6 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 17 Jun 2018 13:29:37 -0400
Subject: [PATCH v1] Fix (end-of-defun N) for N >= 2 (Bug#24427)

* lisp/emacs-lisp/lisp.el (end-of-defun): Only skip to next line when
after end of defun when ARG is 1 or less.
* test/lisp/emacs-lisp/lisp-tests.el (end-of-defun-twice): New test.
---
 lisp/emacs-lisp/lisp.el            |  3 ++-
 test/lisp/emacs-lisp/lisp-tests.el | 55 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index 5a89923f8f..67e7b3ef7b 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -481,7 +481,8 @@ end-of-defun
 		  (if (looking-at "\\s<\\|\n")
 		      (forward-line 1))))))
     (funcall end-of-defun-function)
-    (funcall skip)
+    (when (<= arg 1)
+      (funcall skip))
     (cond
      ((> arg 0)
       ;; Moving forward.
diff --git a/test/lisp/emacs-lisp/lisp-tests.el b/test/lisp/emacs-lisp/lisp-tests.el
index 07eddb74d5..3c87879c3f 100644
--- a/test/lisp/emacs-lisp/lisp-tests.el
+++ b/test/lisp/emacs-lisp/lisp-tests.el
@@ -367,6 +367,61 @@ elisp-tests-with-temp-buffer
 "
     "Test buffer for `mark-defun'."))
 
+;;; end-of-defun
+
+(ert-deftest end-of-defun-twice ()
+  "Test behavior of prefix arg for `end-of-defun' (Bug#24427).
+Calling `end-of-defun' twice should be the same as a prefix arg
+of two."
+  (setq last-command nil)
+  (cl-flet ((eod2 (lambda ()
+                    (goto-char (point-min))
+                    (end-of-defun)
+                    (end-of-defun)
+                    (let ((pt-eod2 (point)))
+                      (goto-char (point-min))
+                      (end-of-defun 2)
+                      (should (= (point) pt-eod2))))))
+    (with-temp-buffer
+      (insert "\
+\(defun a ())
+
+\(defun b ())
+
+\(defun c ())")
+      (eod2))
+    (with-temp-buffer
+      (insert "\
+\(defun a ())
+\(defun b ())
+\(defun c ())")
+      (eod2)))
+  (elisp-tests-with-temp-buffer ";; Comment header
+
+\(defun func-1 (arg)
+  \"docstring\"
+  body)
+=!p1=
+;; Comment before a defun
+\(defun func-2 (arg)
+  \"docstring\"
+  body)
+
+\(defun func-3 (arg)
+  \"docstring\"
+  body)
+=!p2=(defun func-4 (arg)
+  \"docstring\"
+  body)
+
+;; end
+"
+    (goto-char p1)
+    (end-of-defun 2)
+    (should (= (point) p2))))
+
+;;; mark-defun
+
 (ert-deftest mark-defun-no-arg-region-inactive ()
   "Test `mark-defun' with no prefix argument and inactive
 region."
-- 
2.11.0

bug#24427: 25.1.50; end-of-defun jumps too far

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> Not entirely sure if this is correct, but the patch below seems to fix
> it for me, without breaking any of the previously mentioned scenarios.
> I find the fact that end-of-defun goes to the line following the closing
> paren a bit dubious, though I guess since it's been that way so long, we
> can hardly change it now.

[...]

> * lisp/emacs-lisp/lisp.el (end-of-defun): Only skip to next line when
> after end of defun when ARG is 1 or less.
> * test/lisp/emacs-lisp/lisp-tests.el (end-of-defun-twice): New test.

There was no followup on this.  I tested the patch, and it seems to work
for me.  (There's no test failures in "make check" either.)

So I just went ahead and pushed the fix to Emacs 28.

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