bug#45226: Avoid redundant calls in narrow-to-defun

bug#45226: Avoid redundant calls in narrow-to-defun

[Tomas Nordin]

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

Hello Emacs

Working with bug 40563 I had reason "demystify" the narrow-to-defun
function and I think one can see a redundant check and call to
beginning-of-defun there. Here is a walk-through of the beginning of the
narrow-to-defun function to try and explain what I mean.

    ...

    (let ((opoint (point))
	  beg end)
      ;; Try first in this order for the sake of languages with nested
      ;; functions where several can end at the same place as with
      ;; the offside rule, e.g. Python.

      ;; Finding the start of the function is a bit problematic since
      ;; `beginning-of-defun' when we are on the first character of
      ;; the function might go to the previous function.
      ;;
      ;; Therefore we first move one character forward and then call
      ;; `beginning-of-defun'.  However now we must check that we did
      ;; not move into the next function.
      (let ((here (point)))
        (unless (eolp)
          (forward-char))

forward-char to allow narrowing with point on the entry of a defun
(allow the beginning-of-defun function to find the beginning of /this/
defun).

        (beginning-of-defun)
        (when (< (point) here)

This test pass in all cases but the case when point was on the entry of a
defun at calling narrow-to-defun.

          (goto-char here)
          (beginning-of-defun)))

And then beginning-of-defun function is called again, with point on
'here', from where we started.

      (setq beg (point))

beg is now possibly the result from the second call to
beginning-of-defun.

In the case of lisp defuns (defuns starting at beginning-of-line) this
is no problem, only redundant I would claim (the test for (< (point)
here) and the additional call to beginning-of-defun). With a language
with the mentioned "offside rule", like Python, it might be a problem.

If the intention was to narrow a nested function with point on the the
first character of the defun, the following happens, (| is point)

class A():

    |def a1(self):
        pass

The forward-char call allow beginning-of-defun function to find the
beginning of a1. If not elsewhere, the beginning-of-defun function in
lisp.el finalize with beginning-of-line, so we get

class A():

|    def a1(self):
        pass

and that means (< (point) here) and beginning-of-defun is called again,
this time from the beginning of def ('here') and no forward-char
support, ending up at the beginning of class A. Other funny things can
happen but I stop there.

It is not a problem with lisp because a defun start at
beginning-of-line.

In any case, I cannot see what problem the forward-char could cause, and
the protection for it. The nested let with the forward-char service was
introduced with

commit 050cc68b402f5998193a6026d0eeeecb9d2cb9c4
Author: Lennart Borgman <lennart.borgman@gmail.com>
Date:   Wed Apr 11 04:12:20 2012 +0200

    `narrow-to-defun' fixup

    * emacs-lisp/lisp.el (narrow-to-defun): `beginning-of-defun' goes
    to previous function when point is on the first character of a
    function. Take care of that in `narrow-to-defun'.

    Fixes: debbugs:6157

and in the discussion about it (bug 6157) I cannot see an example of
where it would be a problem. The comment is saying we must check we
didn't end up in the next function, but that's not what is checked in
that 'when' test as far as I understand.

With the attached patch I suggest a partial revert of the above commit,
keeping only the forward-char service. I have been experimenting with it
a lot and see no problem. I made 'make check' and see no failures FWIW.
As a bonus, in combination with a patch I submitted with bug#40563,
python nested defuns can be narrowed.

What do you think, maybe I am missing something. Or do you agree the
check and the additional call is redundant?

GNU Emacs 28.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.5,
cairo version 1.16.0) of 2020-12-12

lisp.el(c) commit 8ace7700b9

Best regards
--
Tomas


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

diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index 124900168c..83c20933b3 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -659,15 +659,10 @@ narrow-to-defun
       ;; the function might go to the previous function.
       ;;
       ;; Therefore we first move one character forward and then call
-      ;; `beginning-of-defun'.  However now we must check that we did
-      ;; not move into the next function.
-      (let ((here (point)))
-        (unless (eolp)
-	  (forward-char))
-        (beginning-of-defun)
-        (when (< (point) here)
-          (goto-char here)
-          (beginning-of-defun)))
+      ;; `beginning-of-defun'.
+      (unless (eolp)
+	(forward-char))
+      (beginning-of-defun)
       (setq beg (point))
       (end-of-defun)
       (setq end (point))

bug#45226: Avoid redundant calls in narrow-to-defun

[Lars Ingebrigtsen]

Tomas Nordin <tomasn@posteo.net> writes:

>     `narrow-to-defun' fixup
>
>     * emacs-lisp/lisp.el (narrow-to-defun): `beginning-of-defun' goes
>     to previous function when point is on the first character of a
>     function. Take care of that in `narrow-to-defun'.
>

[...]

> With the attached patch I suggest a partial revert of the above commit,
> keeping only the forward-char service. I have been experimenting with it
> a lot and see no problem. I made 'make check' and see no failures FWIW.
> As a bonus, in combination with a patch I submitted with bug#40563,
> python nested defuns can be narrowed.
>
> What do you think, maybe I am missing something. Or do you agree the
> check and the additional call is redundant?

I think the use case is if you have a language where you can define
several function definitions on the same line.  Like something like:

(defun foo ())(defun bar ())(defun zot ())

Unfortunately the original bug report didn't have a case for
reproduction, so I don't know what the language in question where the
problem may have been in.  (There should have been a test case, of
course.)

(And in Emacs Lisp, this doesn't work anyway)

So I'd prefer not to revert the change (it's a defensive change), unless
we know what it's not really doing anything useful.  So I'm closing this
bug report.

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