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.