bug#17247: 24.4.50; end-of-defun bug in elisp

bug#17247: 24.4.50; end-of-defun bug in elisp

[Leo Liu]

1. In a fresh elisp buffer insert the following lines

;;;; start
(tan 2)

(sin 2)
(cos 2)
;;;; end

2. goto end of buffer and execute (end-of-defun -1) a few times

Bug: can not move pass (sin 2)

Leo

bug#17247: 24.4.50; end-of-defun bug in elisp

[Andreas Röhler]

Am 12.04.2014 08:04, schrieb Leo Liu:
>
> 1. In a fresh elisp buffer insert the following lines
>
> ;;;; start
> (tan 2)
>
> (sin 2)
> (cos 2)
> ;;;; end
>
> 2. goto end of buffer and execute (end-of-defun -1) a few times
>
> Bug: can not move pass (sin 2)
>
> Leo
>
>
>
>

May confirm that for 24.3.90.1-pretest

Looks like line 407

         (beginning-of-defun-raw (- arg))

must read

  (beginning-of-defun-raw (abs arg))

because it's already decided at that point going backward, so the arg must be positiv for a "beginning-..."
function.


Andreas

bug#17247: 24.4.50; end-of-defun bug in elisp

[Dmitry Gutov]

Andreas Röhler <andreas.roehler@easy-emacs.de> writes:

> May confirm that for 24.3.90.1-pretest
>
> Looks like line 407
>
>         (beginning-of-defun-raw (- arg))
>
> must read
>
>  (beginning-of-defun-raw (abs arg))
>
> because it's already decided at that point going backward, so the arg must be positiv for a "beginning-..."
> function.

`arg' is negative in that clause, so (abs arg) is the same as (- arg).
Haven't you tried your suggestion?

Anyway, the patch below seems to work fine. Not sure what the purpose of
`end-of-line' was there.


=== modified file 'lisp/emacs-lisp/lisp.el'
--- lisp/emacs-lisp/lisp.el	2014-02-26 02:31:27 +0000
+++ lisp/emacs-lisp/lisp.el	2014-05-20 03:58:27 +0000
@@ -373,7 +373,7 @@
       (push-mark))
   (if (or (null arg) (= arg 0)) (setq arg 1))
   (let ((pos (point))
-        (beg (progn (end-of-line 1) (beginning-of-defun-raw 1) (point))))
+        (beg (progn (beginning-of-defun-raw 1) (point))))
     (funcall end-of-defun-function)
     ;; When comparing point against pos, we want to consider that if
     ;; point was right after the end of the function, it's still

bug#17247: 24.4.50; end-of-defun bug in elisp

[Andreas Röhler]

On 20.05.2014 06:01, Dmitry Gutov wrote:
> Andreas Röhler <andreas.roehler@easy-emacs.de> writes:
>
>> May confirm that for 24.3.90.1-pretest
>>
>> Looks like line 407
>>
>>          (beginning-of-defun-raw (- arg))
>>
>> must read
>>
>>   (beginning-of-defun-raw (abs arg))
>>
>> because it's already decided at that point going backward, so the arg must be positiv for a "beginning-..."
>> function.
>
> `arg' is negative in that clause, so (abs arg) is the same as (- arg).
> Haven't you tried your suggestion?
>

Just had a look into the code, my mistake, sorry.

> Anyway, the patch below seems to work fine.
  Not sure what the purpose of
> `end-of-line' was there.
>

AFAIU the purpose is to make sure the beginning of current defun is fetched, not the previous one, i.e. protect for cases, point is at the beginning of defun.
 From there some doubts... untested :)

>
> === modified file 'lisp/emacs-lisp/lisp.el'
> --- lisp/emacs-lisp/lisp.el	2014-02-26 02:31:27 +0000
> +++ lisp/emacs-lisp/lisp.el	2014-05-20 03:58:27 +0000
> @@ -373,7 +373,7 @@
>         (push-mark))
>     (if (or (null arg) (= arg 0)) (setq arg 1))
>     (let ((pos (point))
> -        (beg (progn (end-of-line 1) (beginning-of-defun-raw 1) (point))))
> +        (beg (progn (beginning-of-defun-raw 1) (point))))
>       (funcall end-of-defun-function)
>       ;; When comparing point against pos, we want to consider that if
>       ;; point was right after the end of the function, it's still
>
>

IMHO that end-of-defun section  is over-engineered, thus bug-sourcing.

For example the common design-logic when taking numeric arguments: with positiv go forward, with negativ backward, resp. negate assumed direction.

This seems broken internally by (funcall end-of-defun-function), which doesn't care for arguments.

While later on with (cond ((> arg 0)... it takes places as to expect, but has to deal with the effects by previous funcall.

bug#17247: 24.4.50; end-of-defun bug in elisp

[Dmitry Gutov]

On 20.05.2014 10:59, Andreas Röhler wrote:

> AFAIU the purpose is to make sure the beginning of current defun is
> fetched, not the previous one, i.e. protect for cases, point is at the
> beginning of defun.

Guess so. So with the proposed change, the point at the beginning of a 
defun doesn't "belong" to it anymore, but is considered "between defuns" 
instead. Seems to work fine, at least with the given Elisp example and a 
couple similar ones, with small differences.

> IMHO that end-of-defun section  is over-engineered, thus bug-sourcing.
>
> For example the common design-logic when taking numeric arguments: with
> positiv go forward, with negativ backward, resp. negate assumed direction.
>
> This seems broken internally by (funcall end-of-defun-function), which
> doesn't care for arguments.

The additional logic seems to be there to differentiate between two 
cases: we are inside a defun (beginning-of-defun followed by 
end-of-defun will move point forward), we are between defuns 
(beginning-of-defun followed by end-of-defun will move point backward), 
and handle them appropriately.

bug#17247: 24.4.50; end-of-defun bug in elisp

[Andreas Röhler]

On 20.05.2014 14:12, Dmitry Gutov wrote:
> On 20.05.2014 10:59, Andreas Röhler wrote:
>
[  ... ]
> The additional logic seems to be there to differentiate between two cases: we are inside a defun (beginning-of-defun followed by end-of-defun will move point forward), we
> are between defuns (beginning-of-defun followed by end-of-defun will move point backward), and handle them appropriately.
>

Maybe, but is this sane? IMO these commands shoulds always move in one direction.
Why not behave like forward/backward-word for example?

Andreas

bug#17247: 24.4.50; end-of-defun bug in elisp

[Dmitry Gutov]

On 20.05.2014 18:59, Andreas Röhler wrote:

> Maybe, but is this sane? IMO these commands shoulds always move in one
> direction.
> Why not behave like forward/backward-word for example?

Check out the docstring of `end-of-defun-function'. It has a very 
specific calling conditions.

`forward-word' doesn't really need to know where the current word began 
(if we're within a word), whereas `end-of-defun-function' often does 
need to know that. So knowing that it's only ever called from a 
beginning of defun (or beginning of buffer, I guess) can simplify its 
implementation.

bug#17247: 24.4.50; end-of-defun bug in elisp

[Stefan Monnier]

> This seems broken internally by (funcall end-of-defun-function), which
> doesn't care for arguments.

Of course it doesn't.  It just jumps from the beginning of a defun to
its end.  There can be no other direction and it can't be repeated since
after the first call, there's no reason to think we're at the beginning
of another defun.


        Stefan

bug#17247: 24.4.50; end-of-defun bug in elisp

[Andreas Röhler]

On 20.05.2014 16:18, Stefan Monnier wrote:
>> This seems broken internally by (funcall end-of-defun-function), which
>> doesn't care for arguments.
>
> Of course it doesn't.  It just jumps from the beginning of a defun to
> its end.  There can be no other direction and it can't be repeated since
> after the first call, there's no reason to think we're at the beginning
> of another defun.
>
>
>          Stefan
>


Docstring says:

"If variable `end-of-defun-function' is non-nil, its value
is called as a function to find the defun's end."

That's okay. However, if a move-action is taken, it should count WRT repeat-arguments.

Also would consider it as an alternative, excluding the forms run otherwise.

Sadly the common way is followed afterwards nonetheless.
That's an ill-design already mentioned WRT forward-paragraph and others.

While always enjoying Emacs,

Andreas

bug#17247: 24.4.50; end-of-defun bug in elisp

[Stefan Monnier]

> Anyway, the patch below seems to work fine. Not sure what the purpose of
> `end-of-line' was there.

As Andreas mentioned, the end-of-line is there to make sure
we "stay put" in case `pos' is already at a beginning of defun.


        Stefan

bug#17247: 24.4.50; end-of-defun bug in elisp

[Dmitry Gutov]

On 20.05.2014 17:20, Stefan Monnier wrote:
>> Anyway, the patch below seems to work fine. Not sure what the purpose of
>> `end-of-line' was there.
>
> As Andreas mentioned, the end-of-line is there to make sure
> we "stay put" in case `pos' is already at a beginning of defun.

Is the current bug invalid, then?

If we "stay put" in the current defun (which starts right after point), 
then when passed an argument of -1, shouldn't `end-of-defun' go to the 
end of the previous defun, which happens to be the same position?

bug#17247: 24.4.50; end-of-defun bug in elisp

[Stefan Monnier]

>> As Andreas mentioned, the end-of-line is there to make sure
>> we "stay put" in case `pos' is already at a beginning of defun.
> Is the current bug invalid, then?
> If we "stay put" in the current defun (which starts right after point), then
> when passed an argument of -1, shouldn't `end-of-defun' go to the end of the
> previous defun, which happens to be the same position?

In that case, we're both at "beginning of defun" and at "end of defun",
so we have conflicting requirements.  But I think it's clear that
(end-of-defun -1) should move backward if possible, so it's more
important to consider that the position is "at end of defun" than "at
beginning of defun".

IOW, yes, we have a bug.
I installed a brute force patch for it,


        Stefan

bug#17247: 24.4.50; end-of-defun bug in elisp

[Dmitry Gutov]

Version: 24.4

On 20.05.2014 21:56, Stefan Monnier wrote:

> IOW, yes, we have a bug.
> I installed a brute force patch for it,

Thanks, looks fixed. Closing.