bug#48584: 28.0.50; Incorrect hook ordering between local and global hooks with depth

bug#48584: 28.0.50; Incorrect hook ordering between local and global hooks with depth

[Philipp]

Evaluate these forms:

(add-hook 'my-hook (lambda () (message "Outer")) 20)
(with-temp-buffer
  (add-hook 'my-hook (lambda () (message "Inner")) 10 :local)
  (run-hooks 'my-hook))

Then in the *Messages* buffer, "Inner" appears *after* "Outer" even
though the local function's depth is lower than the global one's.


In GNU Emacs 28.0.50 (build 124, aarch64-apple-darwin20.4.0, NS appkit-2022.44 Version 11.3.1 (Build 20E241))
 of 2021-05-22
Repository revision: a3de48687eb28121f3dbfc20be19bd06c4cd6e98
Repository branch: master
Windowing system distributor 'Apple', version 10.3.2022
System Description:  macOS 11.3.1

Configured using:
 'configure --with-modules --without-xml2 --without-pop --with-mailutils
 --enable-gcc-warnings=warn-only --enable-checking=all
 --enable-check-lisp-object-type 'CFLAGS=-ggdb3 -O0''

Configured features:
ACL GNUTLS JSON LCMS2 MODULES NOTIFY KQUEUE NS PDUMPER PNG THREADS
TOOLKIT_SCROLL_BARS ZLIB

Important settings:
  value of $LANG: de_DE.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc dired dired-loaddefs rfc822
mml mml-sec epa epg epg-config gnus-util rmail rmail-loaddefs time-date
mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils
mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr
mail-utils phst skeleton derived edmacro kmacro pcase ffap thingatpt url
url-proxy url-privacy url-expand url-methods url-history url-cookie
url-domsuf url-util url-parse auth-source cl-seq eieio eieio-core
cl-macs eieio-loaddefs password-cache json map url-vars mailcap rx
gnutls puny dbus xml subr-x seq byte-opt gv bytecomp byte-compile cconv
compile text-property-search comint ansi-color ring cl-loaddefs cl-lib
iso-transl tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/ns-win ns-win ucs-normalize mule-util
term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice button
loaddefs faces cus-face macroexp files window text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads kqueue cocoa ns lcms2
multi-tty make-network-process emacs)

Memory information:
((conses 16 70771 6618)
 (symbols 48 8363 1)
 (strings 32 24252 2098)
 (string-bytes 1 793122)
 (vectors 16 16068)
 (vector-slots 8 212664 8593)
 (floats 8 26 28)
 (intervals 56 220 0)
 (buffers 992 10))

bug#48584: 28.0.50; Incorrect hook ordering between local and global hooks with depth

[Lars Ingebrigtsen]

Philipp <p.stephani2@gmail.com> writes:

> Evaluate these forms:
>
> (add-hook 'my-hook (lambda () (message "Outer")) 20)
> (with-temp-buffer
>   (add-hook 'my-hook (lambda () (message "Inner")) 10 :local)
>   (run-hooks 'my-hook))
>
> Then in the *Messages* buffer, "Inner" appears *after* "Outer" even
> though the local function's depth is lower than the global one's.

Looking at the code, the order is computed by `add-hook' by stashing
data in the symbol plist of my-hook, but the hook function is then
pushed onto either the local or the global version of the variable.  So
the ordering isn't global -- it's one ordering for the local and one for
the global, and `run-hooks' doesn't say anything about what order the
local and global values are run in?

(The global value is run when `t' appears in the local value.)

So...  I don't see any obvious way to fix this, and perhaps we should
just document that the order is undefined when you have both local and
global hooks with the same name.

Any opinions?

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

bug#48584: 28.0.50; Incorrect hook ordering between local and global hooks with depth

[Philipp Stephani]

Am Di., 25. Mai 2021 um 22:07 Uhr schrieb Lars Ingebrigtsen <larsi@gnus.org>:
>
> So...  I don't see any obvious way to fix this, and perhaps we should
> just document that the order is undefined when you have both local and
> global hooks with the same name.
>
> Any opinions?

The order isn't undefined, and add-hook carefully distinguishes
between negative and nonnegative depths in this case. It's just that
the relative ordering of depths with the same sign but different
"localness" isn't considered/documented.
How about documenting something along those lines?
In "Running hooks", amend the paragraph
"If the hook variable is buffer-local, the buffer-local variable will
be used instead of the global variable.  However, if the buffer-local
variable contains the element @code{t}, the global hook variable will
be run as well."
to say that the global hook is run at exactly the place where the "t" appears.

In "Setting hooks", amend the paragraph
"If @var{local} is non-@code{nil}, that says to add @var{function} to the
buffer-local hook list instead of to the global hook list.  This makes
the hook buffer-local and adds @code{t} to the buffer-local value."
to specify where the "t" is added (IIUC it's appended if depth > 0 and
prepended otherwise).

bug#48584: 28.0.50; Incorrect hook ordering between local and global hooks with depth

[Lars Ingebrigtsen]

Philipp Stephani <p.stephani2@gmail.com> writes:

> The order isn't undefined, and add-hook carefully distinguishes
> between negative and nonnegative depths in this case. It's just that
> the relative ordering of depths with the same sign but different
> "localness" isn't considered/documented.

That's a different way to say "undefined".  :-)

If I'm reading run_hook_with_args correctly, it'll loop over the local
hook first (in order), and when it happens upon a t in that value, it'll
then loop over the global value (in order), and then finish up the rest
of the local ones.

> How about documenting something along those lines?
> In "Running hooks", amend the paragraph
> "If the hook variable is buffer-local, the buffer-local variable will
> be used instead of the global variable.  However, if the buffer-local
> variable contains the element @code{t}, the global hook variable will
> be run as well."
> to say that the global hook is run at exactly the place where the "t" appears.

(Oh, I should have read your entire mail before starting to answer.)

Well, you're still not saying that that's what'll happen with the t.
And I'm not sure that's really a conscious design, but just an odd
implementation. 

> In "Setting hooks", amend the paragraph
> "If @var{local} is non-@code{nil}, that says to add @var{function} to the
> buffer-local hook list instead of to the global hook list.  This makes
> the hook buffer-local and adds @code{t} to the buffer-local value."
> to specify where the "t" is added (IIUC it's appended if depth > 0 and
> prepended otherwise).

I think I'd just prefer to say that the ordering is undefined if you
have both a global and a local hook.  Either that, or actually implement
a proper ordering system, because "do the global where the t is" is very
odd and somewhat brittle.

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

bug#48584: 28.0.50; Incorrect hook ordering between local and global hooks with depth

[Philipp]

> Am 26.05.2021 um 23:50 schrieb Lars Ingebrigtsen <larsi@gnus.org>:
> 
> Philipp Stephani <p.stephani2@gmail.com> writes:
> 
>> The order isn't undefined, and add-hook carefully distinguishes
>> between negative and nonnegative depths in this case. It's just that
>> the relative ordering of depths with the same sign but different
>> "localness" isn't considered/documented.
> 
> That's a different way to say "undefined".  :-)
> 
> If I'm reading run_hook_with_args correctly, it'll loop over the local
> hook first (in order), and when it happens upon a t in that value, it'll
> then loop over the global value (in order), and then finish up the rest
> of the local ones.

Yes - and that's a reasonable behavior to expect, and we should document it.

> 
>> How about documenting something along those lines?
>> In "Running hooks", amend the paragraph
>> "If the hook variable is buffer-local, the buffer-local variable will
>> be used instead of the global variable.  However, if the buffer-local
>> variable contains the element @code{t}, the global hook variable will
>> be run as well."
>> to say that the global hook is run at exactly the place where the "t" appears.
> 
> (Oh, I should have read your entire mail before starting to answer.)
> 
> Well, you're still not saying that that's what'll happen with the t.
> And I'm not sure that's really a conscious design, but just an odd
> implementation. 

That doesn't really matter: now that it's implemented that way, people rely on the behavior.

> 
>> In "Setting hooks", amend the paragraph
>> "If @var{local} is non-@code{nil}, that says to add @var{function} to the
>> buffer-local hook list instead of to the global hook list.  This makes
>> the hook buffer-local and adds @code{t} to the buffer-local value."
>> to specify where the "t" is added (IIUC it's appended if depth > 0 and
>> prepended otherwise).
> 
> I think I'd just prefer to say that the ordering is undefined if you
> have both a global and a local hook.  Either that, or actually implement
> a proper ordering system, because "do the global where the t is" is very
> odd and somewhat brittle.

That's not going to work.  People don't rely on documented but observable behavior; this is Hyrum's law (https://www.hyrumslaw.com).  Just stating that something is "undefined" won't change that.

Correct hook ordering is crucial for abnormal hooks.  We already rely on the very specific ordering behavior several times in the Emacs codebase alone.  I've searched a bit through the Emacs codebase, and found the following places where Emacs runs an abnormal hook with `run-hook-with-args-until-success/failure' that also gains a local part somewhere the codebase:

lisp/textmodes/fill.el:405:     (run-hook-with-args-until-success 'fill-nobreak-predicate)))))
lisp/files.el:1894:  (unless (run-hook-with-args-until-failure 'kill-buffer-query-functions)
lisp/files.el:5353:	    (unless (run-hook-with-args-until-success 'write-contents-functions)
lisp/files.el:5373:	          (run-hook-with-args-until-success 'write-file-functions)
lisp/minibuffer.el:2948:	 (run-hook-with-args-until-success 'file-name-at-point-functions)))
lisp/minibuffer.el:4079:	   (run-hook-with-args-until-success 'file-name-at-point-functions))))
lisp/progmodes/grep.el:1017:	     (run-hook-with-args-until-success 'file-name-at-point-functions)))
lisp/progmodes/which-func.el:280:	 (run-hook-with-args-until-success 'which-func-functions)))
lisp/progmodes/xref.el:266:  (run-hook-with-args-until-success 'xref-backend-functions))
lisp/emacs-lisp/eldoc.el:619:  (run-hook-with-args-until-success 'eldoc-documentation-functions

That is, we rely on this "undefined" behavior already in very basic operations such as saving buffers to file.  In Eldoc, we even explicitly tell users to add functions to the local part of this abnormal hook (eldoc-documentation-functions), thereby telling them to rely on "undefined" behavior!  This hopefully shows that the behavior is far from being undefined.

bug#48584: 28.0.50; Incorrect hook ordering between local and global hooks with depth

[Lars Ingebrigtsen]

Philipp <p.stephani2@gmail.com> writes:

>> If I'm reading run_hook_with_args correctly, it'll loop over the local
>> hook first (in order), and when it happens upon a t in that value, it'll
>> then loop over the global value (in order), and then finish up the rest
>> of the local ones.
>
> Yes - and that's a reasonable behavior to expect, and we should document it.

I think it's an implementation detail that users should not depend on.
Where that `t' ends up being depends on many subtle things like the
order of your add-hook/remove-hook calls in your .emacs file.

> Correct hook ordering is crucial for abnormal hooks.  We already rely
> on the very specific ordering behavior several times in the Emacs
> codebase alone.  I've searched a bit through the Emacs codebase, and
> found the following places where Emacs runs an abnormal hook with
> `run-hook-with-args-until-success/failure' that also gains a local
> part somewhere the codebase:

[...]

> That is, we rely on this "undefined" behavior already in very basic
> operations such as saving buffers to file.  In Eldoc, we even
> explicitly tell users to add functions to the local part of this
> abnormal hook (eldoc-documentation-functions), thereby telling them to
> rely on "undefined" behavior!  This hopefully shows that the behavior
> is far from being undefined.

I don't think it shows any such thing.

I think it'd be a good idea to implement (and document) something in
this area that actually allows users to control the hook running order
properly, which currently just isn't possible.

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

bug#48584: 28.0.50; Incorrect hook ordering between local and global hooks with depth

[Philipp]

> Am 06.06.2021 um 11:12 schrieb Lars Ingebrigtsen <larsi@gnus.org>:
> 
> Philipp <p.stephani2@gmail.com> writes:
> 
>>> If I'm reading run_hook_with_args correctly, it'll loop over the local
>>> hook first (in order), and when it happens upon a t in that value, it'll
>>> then loop over the global value (in order), and then finish up the rest
>>> of the local ones.
>> 
>> Yes - and that's a reasonable behavior to expect, and we should document it.
> 
> I think it's an implementation detail that users should not depend on.
> Where that `t' ends up being depends on many subtle things like the
> order of your add-hook/remove-hook calls in your .emacs file.

What we think about this doesn't matter much.  As I explained, people depend on observable/observed behavior.

Besides, hook ordering for abnormal hooks is crucial, so we can't just say "don't depend on it."

> 
>> Correct hook ordering is crucial for abnormal hooks.  We already rely
>> on the very specific ordering behavior several times in the Emacs
>> codebase alone.  I've searched a bit through the Emacs codebase, and
>> found the following places where Emacs runs an abnormal hook with
>> `run-hook-with-args-until-success/failure' that also gains a local
>> part somewhere the codebase:
> 
> [...]
> 
>> That is, we rely on this "undefined" behavior already in very basic
>> operations such as saving buffers to file.  In Eldoc, we even
>> explicitly tell users to add functions to the local part of this
>> abnormal hook (eldoc-documentation-functions), thereby telling them to
>> rely on "undefined" behavior!  This hopefully shows that the behavior
>> is far from being undefined.
> 
> I don't think it shows any such thing.

Why not?  Clearly there are hooks that are both abnormal and partially-local in the Emacs codebase, and for those hooks this issue arises.

> 
> I think it'd be a good idea to implement (and document) something in
> this area that actually allows users to control the hook running order
> properly, which currently just isn't possible.
> 

Yes, and arguably the current implementation with the "shadow" ordering stored in a private symbol property is already kind of a hack.  However, the second-best option is still to document the current behavior.  As I've tried to explain, just stating "it's undefined" won't fly.

bug#48584: 28.0.50; Incorrect hook ordering between local and global hooks with depth

[Lars Ingebrigtsen]

Philipp <p.stephani2@gmail.com> writes:

> Yes, and arguably the current implementation with the "shadow"
> ordering stored in a private symbol property is already kind of a
> hack.  However, the second-best option is still to document the
> current behavior.  As I've tried to explain, just stating "it's
> undefined" won't fly.

Well, whether it flies or not, that's what we have.

But we should fix this and allow proper, non-brittle ordering.

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