bug#13305: 24.2; describe-function gives incorrect function location information (for functions changed in a hook?)

bug#13305: 24.2; describe-function gives incorrect function location information (for functions changed in a hook?)

[Mark Lillibridge]

    While working on a patch, I ran into a really weird and misleading
case where describe-function lied about where a function was located,
making me think my redefinition of the function had been ignored.

Here's a recipe to reproduce the problem:

    Start with emacs -q, do M-: (require 'rmailsum), then use C-h f on
rmail-summary-output; you should (correctly) see:

    rmail-summary-output is an interactive compiled Lisp function in
    `rmailsum.el'.
    
    (rmail-summary-output &optional FILE-NAME N)
    
    Append this message to mail file FILE-NAME.
    This works with both mbox format and Babyl format files,
    outputting in the appropriate format for each.
    The default file name comes from `rmail-default-file',
    which is updated to the name you use in this command.
    
    A prefix argument N says to output that many consecutive messages
    from those in the summary, starting with the current one.
    Deleted messages are skipped and don't count.
    When called from Lisp code, N may be omitted and defaults to 1.
    
    This command always outputs the complete message header,
    even the header display is currently pruned.

Note that the location of the function is correctly given as in rmailsum.el.


    Now start again with emacs -q then evaluate the following elisp
code:

====================  cut here for elisp ====================
(add-hook 'rmail-mode-hook
  (function (lambda()

(require 'rmailsum)

(defun rmail-summary-output (&optional file-name n)
  "MODIFIED!
Append this message to mail file FILE-NAME.
This works with both mbox format and Babyl format files,
outputting in the appropriate format for each.
The default file name comes from `rmail-default-file',
which is updated to the name you use in this command.

A prefix argument N says to output that many consecutive messages
from those in the summary, starting with the current one.
Deleted messages are skipped and don't count.
When called from Lisp code, N may be omitted and defaults to 1.

This command always outputs the complete message header,
even the header display is currently pruned.
[mdl: patched for bug #12214]"
  (interactive
   (progn (require 'rmailout)
	  (list (with-current-buffer rmail-buffer (rmail-output-read-file-name))
		(prefix-numeric-value current-prefix-arg))))
  (let ((i 0) prev-msg)
    (while
	(and (< i n)
	     (progn (rmail-summary-goto-msg)
		    (not (eq prev-msg
			     (setq prev-msg
				   (with-current-buffer rmail-buffer
				     rmail-current-message))))))
      (setq i (1+ i))
      (with-current-buffer rmail-buffer
	(let ((rmail-delete-after-output nil))
	  (with-no-warnings (rmail-output file-name 1))))
      (if rmail-delete-after-output
	  (rmail-summary-delete-forward nil)
	(if (< i n)
	    (rmail-summary-next-msg 1))))))

)))
====================  cut here for elisp ====================

    Basically, this replaces the built-in version of
rmail-summary-output with a patched version; the patching is done only
when Rmail started via the Rmail mode hook (I don't always use Rmail
mode and don't want to unnecessarily load things to save on start up
speed with slow connections).

    Now do M-x rmail then once again C-h f on rmail-summary-output; you
should (incorrectly) see:

    rmail-summary-output is an interactive Lisp function in `rmailsum.el'.
    
    (rmail-summary-output &optional FILE-NAME N)
    
    MODIFIED!
    Append this message to mail file FILE-NAME.
    This works with both mbox format and Babyl format files,
    outputting in the appropriate format for each.
    The default file name comes from `rmail-default-file',
    which is updated to the name you use in this command.
    
    A prefix argument N says to output that many consecutive messages
    from those in the summary, starting with the current one.
    Deleted messages are skipped and don't count.
    When called from Lisp code, N may be omitted and defaults to 1.
    
    This command always outputs the complete message header,
    even the header display is currently pruned.
    [mdl: patched for bug #12214]

Note that the modification has been made (see the MODIFIED! at the top)
*BUT* help still thinks the function is located in rmailsum.el.


    By contrast, if I do the patching outside of the Rmail mode hook,
(emacs -q, eval all but first 2 and last line, M-x rmail, then C-h f) I
get:

    rmail-summary-output is an interactive Lisp function in `temporary'.
    
    (rmail-summary-output &optional FILE-NAME N)
    
    MODIFIED!
    Append this message to mail file FILE-NAME.
    This works with both mbox format and Babyl format files,
    outputting in the appropriate format for each.
    The default file name comes from `rmail-default-file',
    which is updated to the name you use in this command.
    
    A prefix argument N says to output that many consecutive messages
    from those in the summary, starting with the current one.
    Deleted messages are skipped and don't count.
    When called from Lisp code, N may be omitted and defaults to 1.
    
    This command always outputs the complete message header,
    even the header display is currently pruned.
    [mdl: patched for bug #12214]

(temporary is the name of the file I evaluated the list from).

- Mark

bug#13305: 24.2; describe-function gives incorrect function location information (for functions changed in a hook?)

[Stefan Monnier]

>     Basically, this replaces the built-in version of
> rmail-summary-output with a patched version; the patching is done only

Don't do that.  Use defadvice or advice-add instead.


        Stefan

bug#13305: 24.2; describe-function gives incorrect function location information (for functions changed in a hook?)

[Mark Lillibridge]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>  >     Basically, this replaces the built-in version of
>  > rmail-summary-output with a patched version; the patching is done only
>  
>  Don't do that.  Use defadvice or advice-add instead.

    For some reason I was thinking that changes to interactive didn't
work properly with advice.  Just using defun can be more convenient than
advice, however, when you are in a hurry for a temporary patch.

    It's too bad there isn't a macro for replace-defun that acts
syntactically identical to defun except that it uses advice to patch
lazily the built-in (e.g., part of Emacs distribution) function.  This
would be a lot easier for users to understand and use.  For that matter,
what would be really useful is a small (10-20 page?) guide to patching
Emacs with this sort of useful information.  Yes, I know there is a 800+
page elisp manual that contains a lot of this information somewhere
(maybe?)  But the reality is that the average person trying to patch
Emacs doesn't have time to read the manual and limits themselves to
Google searches.  You don't discover advice that way.

- Mark

bug#13305: 24.2; describe-function gives incorrect function location information (for functions changed in a hook?)

[Stefan Monnier]

>     For some reason I was thinking that changes to interactive didn't
> work properly with advice.  Just using defun can be more convenient than
> advice, however, when you are in a hurry for a temporary patch.

Fine, but don't expect everything else to handle it just right.

>     It's too bad there isn't a macro for replace-defun that acts
> syntactically identical to defun except that it uses advice to patch
> lazily the built-in (e.g., part of Emacs distribution) function.

Instead of (replace-defun foo (ARGS) BODY), you can write
(advice-add foo :around (lambda (_ignore ARGS) BODY))
If needed we could add a :replace to advice-add, so it'd be
(advice-add foo :replace (lambda (ARGS) BODY)), but I'm not convinced
it's worth the trouble.


        Stefan

bug#13305: 24.2; describe-function gives incorrect function location information (for functions changed in a hook?)

[Stefan Monnier]

BTW, now that `defun' is macro-expanded when loading the file (thanks to
eager macro-expansion), we could actually use it to record the file where
this `defun' was found, even if that `defun' is (as in your case) only
executed at some later time.


        Stefan

bug#13305: 24.2; describe-function gives incorrect function location information (for functions changed in a hook?)

[Stefan Monnier]

> (add-hook 'rmail-mode-hook
>   (function (lambda()
> (require 'rmailsum)
> (defun rmail-summary-output (&optional file-name n)

I also *strongly* discourage the use of `defun' within another function.


        Stefan

bug#13305: 24.2; describe-function gives incorrect function location information (for functions changed in a hook?)

[Mark Lillibridge]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>  > (add-hook 'rmail-mode-hook
>  >   (function (lambda()
>  > (require 'rmailsum)
>  > (defun rmail-summary-output (&optional file-name n)
>  
>  I also *strongly* discourage the use of `defun' within another function.

    Is this in the elisp manual somewhere?  It's exactly what one would
do given experience with scheme or other lisps.

- Mark

bug#13305: 24.2; describe-function gives incorrect function location information (for functions changed in a hook?)

[Stefan Monnier]

>> > (add-hook 'rmail-mode-hook
>> >   (function (lambda()
>> > (require 'rmailsum)
>> > (defun rmail-summary-output (&optional file-name n)
>> I also *strongly* discourage the use of `defun' within another function.
>     Is this in the elisp manual somewhere?  It's exactly what one would
> do given experience with scheme or other lisps.

In Scheme, a local `define' does not affect the global definition.
So it's a completely different operation (it's more like a `let').


        Stefan