Issue #165 wontfix

Problem with defining advice for `evil-delete'

York Zhao avatarYork Zhao created an issue

I found very weird problem after added advice for `evil-delete'. Here is the .emacs:

(require 'evil)
(evil-mode 1)

(defadvice evil-delete (around around-delete activate)
  ad-do-it)

(require 'surround)
(global-surround-mode 1)

Now add:

(print "Shouldn't have come here.")

to the function `evil-delete', like the following:

(evil-define-operator evil-delete (beg end type register yank-handler)
  "Delete text from BEG to END with TYPE.
Save in REGISTER or in the kill-ring with YANK-HANDLER."
  (interactive "<R><x><y>")
+ (print "Shouldn't have come here.")
  (evil-yank beg end type register yank-handler)
  (if (eq type 'block)
      (evil-apply-on-block #'delete-region beg end)
    (delete-region beg end))
  ;; place cursor on beginning of line
  (when (and (evil-called-interactively-p)
             (eq type 'line))
    (evil-first-non-blank)))

Find some text surrounded by "()", say

(test)

Move the point to the 's' and type "ds(", you will notice that "ds(" is not working as expected, and you will get the message "Shouldn't have come here".

If you remove the advice to `evil-delete' and do this test again everything will work as expected and the message "Shouldn't have come here" wouldn't show up.

Comments (16)

  1. Frank Fischer
    • changed status to open

    Currently I have no idea what do. But I think I can explain the problem.

    The internal operator code depends on a call to evil-called-interactively-p which in turn calls called-interactively-p. This function must return t if the operator evil-delete is called using keystrokes (i.e. interactively). The code that cancels the execution of the operator in case of a surround object relies on this return value.

    The problem when advising the function is that the original body is now called as function in the advice-body. Here's the critical part of (disassemble 'evil-delete) after advising (this is essentially the body of the advice itself):

    0	constant  nil
    1	varbind	  ad-return-value
    2	constant  ad-Orig-evil-delete
    3	varref	  beg
    4	varref	  end
    5	varref	  type
    6	varref	  register
    7	varref	  yank-handler
    8	call	  5
    9	dup	  
    10	varset	  ad-return-value
    11	unbind	  1
    12	return	  
    

    The problem is that the original function definition is executed by calling ad-Orig-evil-delete (line 8: call 5). But when a function is called using such a function call then called-interactively-p returns nil!

    One can easily test this with the following short example

    (defun myfunc () 
      (interactive)
      (message "MYFUNC %s" (called-interactively-p)))
    

    Now calling M-x myfunc RET shows "MYFUNC t" in the message line. With the advice

    (defadvice myfunc (around around-myfunc activate)
      ad-do-it)
    

    calling again M-x myfunc RET shows "MYFUNC nil".

    This difference causes the whole trouble, but I have no idea how to fix it.

  2. York Zhao

    Thanks a lot for the explanation. Knowing the reason of the problem, I had come up with a workaround by:

    (defadvice evil-delete (around york activate)
      ( ...
        (if (called-interactively-p)
            (flet ((called-interactively-p (kind) t)) ad-do-it)
          ad-do-it)))
    

    This works wonderfully for me. However, if I use this exact technique when advicing `evil-change' I got another errer when executing "dw" which I had no idea of why.

  3. Frank Fischer

    Oh, that's easy ;)

    evil-change calls evil-delete -- but this time non-interactively as a subroutine. Thus the call to called-interactively-p should return nil when called from evil-delete and evil-delete is called as a subroutine, but your workaround returns t no matter from where it has been called (the new definition of called-interactively-p should somehow detect whether it is called from within the body of evil-change and should return t (and only then), but I have no idea how to do is).

  4. York Zhao
    evil-change calls evil-delete -- but this time non-interactively as a subroutine. Thus the call to called-interactively-p should return nil when called from evil-delete and evil-delete is called as a subroutine, but your workaround returns t no matter from where it has been called (the new definition of called-interactively-p should somehow detect whether it is called from within the body of evil-change and should return t (and only then), but I have no idea how to do is).
    

    I actually had come up with this explanation, however if that was the case, it should simply doesn't work and not giving any error (correct me if I'm wrong). But I got the error message:

    "if: Wrong number of arguments: (lambda (kind) (block called-interactively-p t)), 0"

  5. Frank Fischer

    Hm, called-interactively-p used to take no arguments in older Emacs versions. The newer version take one argument but because of backward compatibility this argument is optional (even if this is not stated in the function documentation). Evil itself should not call called-interactively-p without an argument (but perhaps there's a bug). It should be better to define the lambda with an optional argument

    (flet ((called-interactively-p (&optional kind) t)) ad-do-it)
    

    Anyhow, it would be interesting to know from which function called-interactively-p has been called in order to know if it's an evil function or something else (what does the backtrace say?).

    Btw, I asked about the problem on the emacs mailing list but haven't got an answer till now.

  6. York Zhao

    Oh, I also had thought about try adding the "optional" keyword in `flet', but I trusted the documentation of `called-interactively'. So, I have learnt the lesson: "Never trust a documentation completely".

    After adding the "optional" keyword everything works, and yes, when `evil-delete' is being called by `evil-change', `called-interactively' still returned t which is not good, but I haven't noticed any problem yet. Please keep me posted when you get any better solution on this.

  7. Frank Fischer

    I've got finally an answer that works: using ad-get-orig-definition

    (defun bar ()
      (interactive)
      (message "BAR: %s" (called-interactively-p 'any)))
    
    (defun foo ()
      (interactive)
      (message "FOO %s" (called-interactively-p 'any))
      (bar))
    
    (defmacro my-ad-do-it (name)
      `(when (called-interactively-p 'any)
    	 (call-interactively (ad-get-orig-definition ',name))))
    
    (defadvice foo (around around-foo activate)
      (my-ad-do-it foo))
    

    But note that this way you cannot override the interactive form in your advice because the original interactive form is reevaluated when using call-interactively (this would require further hacks).

  8. York Zhao

    I have read your thread at "emacs-help" list, nice solution and thank you very much.

    Just one thing want to be cleared, I think this should have been what you meant:

    (defmacro my-ad-do-it (name)
      `(if (called-interactively-p 'any)
           (call-interactively (ad-get-orig-definition ',name))
         ad-do-it))
    

    Am I right?

  9. York Zhao

    When I tested this with "ds(" (advicing `evil-delete'), it worked, but Emacs reports the error:

    Key sequence contains no complete binding
    

    When testing with "dw" the cursor changes to half size, press "w" again deleted the word.

    What's the problem here do you think?

  10. Frank Fischer

    Your first question: of course, you're right. Obviously I only tested the "difficult" case and missed the easy one, sorry );

    Your second question. I think the problem is that this hack executes the original interactive form of evil-delete twice, once when the advice is called and once by call-interactively when my-ad-do-it is used. The problem is that the whole operator-state magic is done in the interactive form of an operator, including the determination of the motion to be used for the operator. Thus executing this interactive form twice confuses evil and in the case of dw causes the motion to be read twice.

    A solution could be to overwrite the outer interactive form of the advice, perhaps like this:

    (defadvice evil-delete (around around-delete (&rest args) activate)
      (interactive)
      (my-ad-do-it evil-delete))
    

    This causes the interactive form to be called only once, namely when my-ad-do-it is executed. The downside is that you cannot access the arguments of evil-delete in the advice (because they have not been determined, yet).

    Other solutions could keep the outer interactive form intact and replace the interactive form the original function. But don't ask me how to do this ;) In the end you have to ensure that the original interactive form of evil-delete (or a compatible replacement) is evaluated exactly once.

  11. York Zhao

    This almost worked, I think the only issue left is that if I don't use the my-ad-do-it macro, it worked perfectly:

    (defadvice evil-delete (around around-delete (&rest args) activate)
      (if (called-interactively-p 'any)
          (call-interactively (ad-get-orig-definition 'evil-change))
        ad-do-it))
    

    However, if use the macro, and call evil-delete from within another function (non-interactive call), I got the error:

    my-ad-do-it: Symbol's value as variable is void: ad-do-it

    my-ad-do-it defined as:

    (defmacro my-ad-do-it (function)
      `(if (called-interactively-p 'any)
           (call-interactively (ad-get-orig-definition ',function))
         ad-do-it))
    

    I found myself always get confused with what exactly ad-do-it is, is it a macro? Where is the definition of this strange animal? I couldn't find it in advice.el, maybe defined in C?

    Sorry for so many question.

  12. Frank Fischer

    Hm, yes, you're right. ad-do-it is not a macro, it's something special (a pseudo-variable) that is handled by the advice framework itself. So it must be plainly written in the body of the advice, not hidden within some macro. So the macro-variant does not work. (Perhaps it work with byte-compilation, but I don't know).

  13. York Zhao

    I have abandoned the approach of using ad-get-orig-definition because like you said in order to avoid the interactive form of evil-delete being executed twice, I have to override the interactive form in the advice by using (&optional rest) as arguments list and using the (interactive) in the advice, and the problem is that my advice would not be able to use the arguments of evil-delete which is unacceptable as that was the reason I advices evil-delete in the first place. Finally, I used the following approach which is not perfect but works, and wouldn't have potential problem theoretically (not that I know of) .

    ;; Save the original definition of `called-interactively-p' so that we can
    ;; always use the original one directly without getting incorrect result by
    ;; using a temporarily redefined definition of the function.
    (fset (defvar called-interactively-orig-p)
          (symbol-function 'called-interactively-p))
    
    (defadvice evil-delete (around around-delete activate)
      ...
      (if (called-interactively-orig-p)
          (flet ((called-interactively-p (&optional kind) t))
            (message "evil-delete called interactively"))
            ad-do-it
        ad-do-it))
    
    (defadvice evil-change (around around-change activate)
      ...
      (if (called-interactively-orig-p)
          (flet ((called-interactively-p (&optional kind) t))
            (message "evil-change called interactively"))
            ad-do-it
        ad-do-it))
    

    And thank you very much for your extensive responsiveness to solving this problem. Please keep me posted if you come up with any better approach to address this issue.

  14. Log in to comment
Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.