GNU bug report logs - #52003
Unexpected advising behavior due to recursive implementation

Previous Next

Package: emacs;

Reported by: Daniel Sausner <daniel.sausner <at> posteo.de>

Date: Sat, 20 Nov 2021 17:05:03 UTC

Severity: normal

Done: Mattias Engdegård <mattiase <at> acm.org>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 52003 in the body.
You can then email your comments to 52003 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#52003; Package emacs. (Sat, 20 Nov 2021 17:05:03 GMT) Full text and rfc822 format available.

Acknowledgement sent to Daniel Sausner <daniel.sausner <at> posteo.de>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Sat, 20 Nov 2021 17:05:03 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Daniel Sausner <daniel.sausner <at> posteo.de>
To: bug-gnu-emacs <at> gnu.org
Subject: Unexpected advising behavior due to recursive implementation
Date: Sat, 20 Nov 2021 16:13:32 +0000
Hi,

I stumbled [1] on an issue that seems to affect several functions [2] in 
lisp/emacs-lisp/lisp.el. For the sake of brevity I'll sketch it only for 
forward-sexp but the problematic code is effectively duplicated and was 
introduced with a commit [3] about one year ago.

Here's the problem: Since the commit any advice on the function 
forward-sexp will effectively be called twice before the actual code 
runs in interactive mode. In non-interactive mode everthing is as 
expected however.

The reason is the introduction of an error message if no 
forward/backward sexp is found. This is implemented in a way that the 
functions calls itself immediately again and scans for errors:

(defun forward-sexp (&optional arg interactive)
  "..."
  (interactive "^p\nd")
  (if interactive
      (condition-case _
          (forward-sexp arg nil)              <-- Recursion
        (scan-error (user-error (if (> arg 0)
                                    "No next sexp"
                                  "No previous sexp"))))
    (or arg (setq arg 1))
    (if forward-sexp-function
        (funcall forward-sexp-function arg)
      (goto-char (or (scan-sexps (point) arg) (buffer-end arg)))
      (if (< arg 0) (backward-prefix-chars)))))

In my (very) humble opinion that method of error catching was an 
unfortunate choice in that regard, that it makes the advising very 
counter-intuitive.

I'm far from a lisp expert but my feeling is that the condition-case 
should only wrap the calls where things can actually go wrong.

If there is interest, I'd be happy to provide a patch :-)

Best regards,
Daniel


[1] https://github.com/emacs-evil/evil/issues/1541

[2] On a first glimpse at least: forward-sexp, forward-list, down-list, 
kill-sexp in that particular file.

[3] Commit:

df0f32f04850e7ed106a225addfa82f1b5b91f45
Author:     Mattias Engdegård <mattiase <at> acm.org>
AuthorDate: Fri Sep 18 12:49:33 2020 +0200
Commit:     Mattias Engdegård <mattiase <at> acm.org>
CommitDate: Wed Sep 23 16:31:18 2020 +0200

Don't signal scan-error when moving by sexp interactively





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#52003; Package emacs. (Sat, 20 Nov 2021 19:17:02 GMT) Full text and rfc822 format available.

Message #8 received at 52003 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Daniel Sausner <daniel.sausner <at> posteo.de>
Cc: 52003 <at> debbugs.gnu.org
Subject: bug#52003: Unexpected advising behavior due to recursive
 implementation
Date: Sat, 20 Nov 2021 20:16:43 +0100
Thanks for the report, and yes, it's true that the way interactive use is managed makes advice hacks more interesting. Do remember that you are always on your own when using advice; Emacs cannot reasonably promise any compatibility on that level.

That said, it would be straightforward to straighten out the control flow by extracting the bulk of the code to a new (internal) function which is called with or without `condition-case`. It would be slightly slower since it entails an extra function call in the non-interactive case, and forward-sexp and its ilk are workhorses in many language modes. It may not matter much, of course.

> I'm far from a lisp expert but my feeling is that the condition-case 
> should only wrap the calls where things can actually go wrong.

Oh, but in this case they can. Noninteractive calls expect the scan-errors; interactive use does not. It is also possible for a a forward-sexp-function to raise scan-error.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#52003; Package emacs. (Sun, 21 Nov 2021 11:09:01 GMT) Full text and rfc822 format available.

Message #11 received at 52003 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Daniel Sausner <daniel.sausner <at> posteo.de>
Cc: 52003 <at> debbugs.gnu.org
Subject: Re: bug#52003: Unexpected advising behavior due to recursive
 implementation
Date: Sun, 21 Nov 2021 12:08:39 +0100
21 nov. 2021 kl. 10.32 skrev Daniel Sausner <daniel.sausner <at> posteo.de>:

> I see. I somehow assumed that functions at that level would aim to be compliant to the max.

Yes but what would that mean? The best we can do is to promise that a function F, when called in a manner consistent with the documentation, behaves accordingly. We cannot guarantee the absence of calls to F, can we?

But unless I'm mistaken, that's what you are unhappy about: `forward-sexp` may call itself when you call it. A lot of other code calls that function as part of their implementation. Don't they cause trouble, or is it just the recursive call?

> Well, I'd say that in that case the non-interactive performance outweighs this particular corner case of advising these core functions, which I came to understand is a hot iron.

The cost of that extra function call is probably going to be lost in the noise. I'm not looking for excuses to avoid work here but would like to know what problem rearranging the code would actually solve. 

> Therefore I'd say this bug report can be closed from my point of view. I guess I still have to use advising as I currently see no other feasible way out, but I can make it sensitive to `interactive` as well.

What are you trying to do? Can't you define a mode-specific forward-sexp-function?





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#52003; Package emacs. (Sun, 21 Nov 2021 17:18:01 GMT) Full text and rfc822 format available.

Message #14 received at 52003 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Sausner <daniel.sausner <at> posteo.de>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 52003 <at> debbugs.gnu.org
Subject: Re: bug#52003: Unexpected advising behavior due to recursive
 implementation
Date: Sun, 21 Nov 2021 09:32:43 +0000
> Do remember that you are always on your own when using advice; Emacs 
> cannot reasonably promise any compatibility on that level.
I see. I somehow assumed that functions at that level would aim to be 
compliant to the max.
> That said, it would be straightforward to straighten out the control flow by extracting the bulk of the code to a new (internal) function which is called with or without `condition-case`. It would be slightly slower since it entails an extra function call in the non-interactive case, and forward-sexp and its ilk are workhorses in many language modes. It may not matter much, of course.
Well, I'd say that in that case the non-interactive performance 
outweighs this particular corner case of advising these core functions, 
which I came to understand is a hot iron.

Therefore I'd say this bug report can be closed from my point of view. I 
guess I still have to use advising as I currently see no other feasible 
way out, but I can make it sensitive to `interactive` as well.

Thanks for the helpful input!




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#52003; Package emacs. (Sun, 21 Nov 2021 17:30:02 GMT) Full text and rfc822 format available.

Message #17 received at 52003 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Sausner <daniel.sausner <at> posteo.de>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 52003 <at> debbugs.gnu.org
Subject: Re: bug#52003: Unexpected advising behavior due to recursive
 implementation
Date: Sun, 21 Nov 2021 17:29:36 +0000
> Yes but what would that mean? The best we can do is to promise that a function F, when called in a manner consistent with the documentation, behaves accordingly. We cannot guarantee the absence of calls to F, can we?
>
> But unless I'm mistaken, that's what you are unhappy about: `forward-sexp` may call itself when you call it. A lot of other code calls that function as part of their implementation. Don't they cause trouble, or is it just the recursive call?
Well, my initial concern was the (new) recursive call, which adds 
another layer of complexity for advising. I now see too that an advice 
on such a deep rooted function is kind of madness anyway. In fact I 
would need to make a distinction between the interactive modes both ways.

> What are you trying to do? Can't you define a mode-specific forward-sexp-function?
The problem I'm trying to solve is, that the cursor in evil normal state 
is not between chars but _on_ a char. Moving to the end of a sexp in 
lisp I would expect the cursor to be on the closing paren instead of 
behind it. There was already an advice planted on 
`elisp--precedent-sexp` to achieve this effect for `eval-last-sexp`. It 
basically only moves the point one char forward if in normal mode before 
eval-last-sexp, hence the sexp including the paren on which the cursor 
rests will be evaluated instead of the thing before the cursor/paren.

I wanted to transport this behaviour to the motion-sexp commands and 
initially I was naive enough to think that this is a low hanging fruit, 
because I could take the same advice function and add it to 
backward/forward-sexp.

In essence I would like to move the visible cursor by a single char in 
one or the other direction before and after one or more 
`forward-sexp`-based commands are executed. But I'm not sure anymore if 
this is really worth the effort :-)




Reply sent to Mattias Engdegård <mattiase <at> acm.org>:
You have taken responsibility. (Mon, 22 Nov 2021 09:22:02 GMT) Full text and rfc822 format available.

Notification sent to Daniel Sausner <daniel.sausner <at> posteo.de>:
bug acknowledged by developer. (Mon, 22 Nov 2021 09:22:02 GMT) Full text and rfc822 format available.

Message #22 received at 52003-done <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Daniel Sausner <daniel.sausner <at> posteo.de>
Cc: 52003-done <at> debbugs.gnu.org
Subject: Re: bug#52003: Unexpected advising behavior due to recursive
 implementation
Date: Mon, 22 Nov 2021 10:20:57 +0100
21 nov. 2021 kl. 18.29 skrev Daniel Sausner <daniel.sausner <at> posteo.de>:

> The problem I'm trying to solve is, that the cursor in evil normal state is not between chars but _on_ a char. Moving to the end of a sexp in lisp I would expect the cursor to be on the closing paren instead of behind it.

> In essence I would like to move the visible cursor by a single char in one or the other direction before and after one or more `forward-sexp`-based commands are executed. But I'm not sure anymore if this is really worth the effort :-)

Thanks for the explanation. Sounds fiddly. Best of luck! I'm closing this bug then.





bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Mon, 20 Dec 2021 12:24:04 GMT) Full text and rfc822 format available.

This bug report was last modified 2 years and 89 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.