bug#8387: 24.0.50; `run-hook-with-args' return value has changed

bug#8387: 24.0.50; `run-hook-with-args' return value has changed

[Michael Welsh Duggan]

emacs -Q
(defun foo (arg) 'test) C-j
(defvar foo-hook '(foo)) C-j
(run-hook-with-args 'foo-hook 'foo) C-j

In the past, this would return 'test instead of nil.  This breaks some
code, in particular bbdb, which has the following:

(defun bbdb-canonicalize-address (net)
  ;; call the bbdb-canonicalize-net-hook repeatedly until it returns a
  ;; value eq to the value passed in.  This implies that it can't
  ;; destructively modify the string.

  ;; Hysterical Raisins: This is a function, not a hook. In order to
  ;; make this hook a hook, we'll quietly convert a single function
  ;; into a hook list.  We should really warn the user that we're
  ;; doing this, and advise them to update their configuration
  ;; accordingly. For the release, maybe.
  (if (functionp bbdb-canonicalize-net-hook)
      (setq bbdb-canonicalize-net-hook (list bbdb-canonicalize-net-hook)))

  ;; Now, do the hook run. Note, if you mess up, it's possible that
  ;; BBDB will get stuck here oscillating between various definitions
  ;; of the canonical address.
  (while (not (equal net (setq net (run-hook-with-args
                                    'bbdb-canonicalize-net-hook net)))))

  net)

I don't have time to test this right now, but I would guess this started
happening in the following commit:

2011-03-28  Stefan Monnier  <monnier@iro.umontreal.ca>

	* keyboard.c (safe_run_hook_funcall): New function.
	(safe_run_hooks_1, safe_run_hooks_error, safe_run_hooks): On error,
	don't set the hook to nil, but remove the offending function instead.
	(Qcommand_hook_internal): Remove, unused.
	(syms_of_keyboard): Don't initialize Qcommand_hook_internal nor define
	Vcommand_hook_internal.

	* eval.c (enum run_hooks_condition): Remove.
	(funcall_nil, funcall_not): New functions.
	(run_hook_with_args): Call each function through a `funcall' argument.
	Remove `cond' argument, now redundant.
	(Frun_hooks, Frun_hook_with_args, Frun_hook_with_args_until_success)
	(Frun_hook_with_args_until_failure): Adjust accordingly.
	(run_hook_wrapped_funcall, Frun_hook_wrapped): New functions.



In GNU Emacs 24.0.50.1 (i686-pc-linux-gnu, GTK+ Version 2.24.3)
 of 2011-03-30 on maru
Windowing system distributor `The X.Org Foundation', version 11.0.10707000
configured using `configure  '--without-toolkit-scroll-bars''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_US.utf8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

-- 
Michael Welsh Duggan
(md5i@md5i.com)

bug#8387: 24.0.50; `run-hook-with-args' return value has changed

[Juanma Barranquero]

On Thu, Mar 31, 2011 at 08:25, Michael Welsh Duggan <md5i@md5i.com> wrote:

> (run-hook-with-args 'foo-hook 'foo) C-j
>
> In the past, this would return 'test instead of nil.  This breaks some
> code, in particular bbdb, which has the following:

I'm closing this one, as it is not a bug.

The docstring of `run-hook-with-args' contains this bit, since at
least Emacs 22.1:

"It is best not to depend on the value returned by `run-hook-with-args',
as that may change."

    Juanma

bug#8387: 24.0.50; `run-hook-with-args' return value has changed

[Stefan Monnier]

> (defun foo (arg) 'test) C-j
> (defvar foo-hook '(foo)) C-j
> (run-hook-with-args 'foo-hook 'foo) C-j

> In the past, this would return 'test instead of nil.  This breaks some
[...]
> I don't have time to test this right now, but I would guess this started
> happening in the following commit:
[...]
> 	(run_hook_with_args): Call each function through a `funcall' argument.
> 	Remove `cond' argument, now redundant.
> 	(Frun_hooks, Frun_hook_with_args, Frun_hook_with_args_until_success)
> 	(Frun_hook_with_args_until_failure): Adjust accordingly.

No doubt it's the origin of the new behavior.
But note that the above change has not touched the docstring of
run-hook-with-args which says:

   It is best not to depend on the value returned by `run-hook-with-args',
   as that may change.

Looking at the BBDB code, I think we don't need to try and accomodate it
because it is fundamentally broken, not just because of the above
docstring warning:

  ;; Hysterical Raisins: This is a function, not a hook. In order to
  ;; make this hook a hook, we'll quietly convert a single function
  ;; into a hook list.  We should really warn the user that we're
  ;; doing this, and advise them to update their configuration
  ;; accordingly. For the release, maybe.
  (if (functionp bbdb-canonicalize-net-hook)
      (setq bbdb-canonicalize-net-hook (list bbdb-canonicalize-net-hook)))

  ;; Now, do the hook run. Note, if you mess up, it's possible that
  ;; BBDB will get stuck here oscillating between various definitions
  ;; of the canonical address.
  (while (not (equal net (setq net (run-hook-with-args
                                    'bbdb-canonicalize-net-hook net)))))

So bbdb-canonicalize-net-hook can start as a single function or a list
of functions.  But the code is explicit about wanting a list of
functions.  Now if this list contains N>1 function, then
run-hook-with-args has to call all those functions and discard at least
N-1 of the results.  I.e. only one of the functions on
bbdb-canonicalize-net-hook can work (in Emacs-23, one of them happened
to work, and on the current trunk, none of them work).


        Stefan