bug#13780: 24.2; Making find-file-hook customizable seems to cause troublle

bug#13780: 24.2; Making find-file-hook customizable seems to cause troublle

[Reuben Thomas]

I just spent a while trying to work out why a third-party mode I had
just installed was not automatically added to my find-file-hook, even
though the mode's file calls add-hook when it is loaded, and I took care
to ensure the mode was autoloaded. I discovered that the problem was
that the mode was loaded by my customization file, custom.el, but later,
the same file overwrote find-file-hook, because I had customized
find-file-hook.

I found that the reason for the customization was to get flymake-mode
automatically activated; I've filed a separate issue about this
(#13779). After removing the customization, my find-file-hook was set up
correctly (except for flymake-mode).

The real problem here is that find-file-hook is clearly meant to be
customizable (there is even a check box for auto-indent in its
customization dialog), yet customizing it seems to cause a race
condition where depending in what order files are autoloaded, hooks may
fail to be added properly to find-file-hook.

For the moment, I have added an add-hook call to my Emacs init file to
set up flymake-mode, and removed the customization, but it would be nice
if customizing the hook were both possible (perhaps by ensuring that
customized hooks were added to rather than overwritten?) and safe.


In GNU Emacs 24.2.1 (x86_64-pc-linux-gnu, GTK+ Version 2.24.13)
 of 2012-12-13 on komainu, modified by Debian
Windowing system distributor `The X.Org Foundation', version 11.0.11300000
Configured using:
 `configure '--build' 'x86_64-linux-gnu' '--build' 'x86_64-linux-gnu'
 '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib'
 '--localstatedir=/var/lib' '--infodir=/usr/share/info'
 '--mandir=/usr/share/man' '--with-pop=yes'
 '--enable-locallisppath=/etc/emacs24:/etc/emacs:/usr/local/share/emacs/24.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.2/site-lisp:/usr/share/emacs/site-lisp'
 '--with-crt-dir=/usr/lib/x86_64-linux-gnu' '--with-x=yes'
 '--with-x-toolkit=gtk' '--with-toolkit-scroll-bars'
 'build_alias=x86_64-linux-gnu' 'CFLAGS=-g -O2 -fstack-protector
 --param=ssp-buffer-size=4 -Wformat -Werror=format-security -Wall'
 'CPPFLAGS=-D_FORTIFY_SOURCE=2''

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: en_GB.UTF-8
  value of $LC_NUMERIC: en_GB.UTF-8
  value of $LC_TIME: en_GB.UTF-8
  value of $LANG: en_GB.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Emacs-Lisp

Minor modes in effect:
  diff-auto-refine-mode: t
  recentf-mode: t
  show-paren-mode: t
  server-mode: t
  savehist-mode: t
  minibuffer-electric-default-mode: t
  iswitchb-mode: t
  icomplete-mode: t
  global-auto-revert-mode: t
  dtrt-indent-mode: t
  desktop-save-mode: t
  TeX-PDF-mode: t
  TeX-source-correlate-mode: t
  tooltip-mode: t
  mouse-wheel-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
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
SPC i t SPC s h o u l d SPC b e SPC r e m o v e d SPC 
, SPC a n d <backspace> <backspace> <backspace> <backspace> 
<backspace> , <backspace> <backspace> , SPC a n d SPC 
a SPC m o r e SPC s t a <backspace> <backspace> <backspace> 
u s e r - f r i e n d l y SPC a n d SPC s t a n d a 
r d SPC w a y SPC o f SPC a c t i v a t i n g SPC f 
l y m a k e - m o d e SPC g l o b v a <backspace> <backspace> 
<backspace> a <backspace> b a l l y SPC <M-backspace> 
a u t o m a t i c a l l y SPC a d d e d ? M-q C-c C-c 
y e s <return> C-x k <return> C-x k <return> C-x k 
<return> C-x k <return> C-x k <return> C-a C-SPC C-n 
M-w C-x k <return> C-a C-x k <return> y e s <return> 
C-x b v a r s C-g C-x C-f v a <tab> <return> C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-p C-p C-p C-p C-p 
C-p C-p C-p C-p C-y C-p C-d C-d C-d C-d C-d C-d C-d 
C-d C-d C-x C-s C-n C-n C-n C-n C-n C-n C-n C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
C-n C-n M-x r e p o r t - e m a c s - b u g <retur
n>

Recent messages:
Sending...
Sending via mail...
Sending...done
Mark set
Quit
byte-code: End of buffer [16 times]
Mark set
Saving file /home/rrt/.emacs.d/vars.el...
Wrote /home/rrt/.emacs.d/vars.el
byte-code: End of buffer [13 times]
byte-code: End of buffer

Load-path shadows:
/home/rrt/local/share/emacs/site-lisp/browse-kill-ring hides /usr/share/emacs24/site-lisp/emacs-goodies-el/browse-kill-ring
/home/rrt/local/share/emacs/site-lisp/dict hides /usr/share/emacs24/site-lisp/emacs-goodies-el/dict
/home/rrt/.emacs.d/elpa/dictionary-1.8.7/dictionary-init hides /usr/share/emacs24/site-lisp/dictionary-el/dictionary-init
/home/rrt/.emacs.d/elpa/dictionary-1.8.7/dictionary hides /usr/share/emacs24/site-lisp/dictionary-el/dictionary
/home/rrt/.emacs.d/elpa/dictionary-1.8.7/link hides /usr/share/emacs24/site-lisp/dictionary-el/link
/home/rrt/.emacs.d/elpa/dictionary-1.8.7/connection hides /usr/share/emacs24/site-lisp/dictionary-el/connection
/usr/share/emacs24/site-lisp/auctex/tex-style hides /usr/share/emacs/site-lisp/auctex/tex-style
/usr/share/emacs24/site-lisp/auctex/tex-mik hides /usr/share/emacs/site-lisp/auctex/tex-mik
/usr/share/emacs24/site-lisp/auctex/multi-prompt hides /usr/share/emacs/site-lisp/auctex/multi-prompt
/usr/share/emacs24/site-lisp/auctex/tex-jp hides /usr/share/emacs/site-lisp/auctex/tex-jp
/usr/share/emacs24/site-lisp/auctex/tex-info hides /usr/share/emacs/site-lisp/auctex/tex-info
/usr/share/emacs24/site-lisp/auctex/plain-tex hides /usr/share/emacs/site-lisp/auctex/plain-tex
/usr/share/emacs24/site-lisp/auctex/latex hides /usr/share/emacs/site-lisp/auctex/latex
/usr/share/emacs24/site-lisp/auctex/preview hides /usr/share/emacs/site-lisp/auctex/preview
/usr/share/emacs24/site-lisp/auctex/tex hides /usr/share/emacs/site-lisp/auctex/tex
/usr/share/emacs24/site-lisp/auctex/texmathp hides /usr/share/emacs/site-lisp/auctex/texmathp
/usr/share/emacs24/site-lisp/auctex/context-nl hides /usr/share/emacs/site-lisp/auctex/context-nl
/usr/share/emacs24/site-lisp/auctex/tex-font hides /usr/share/emacs/site-lisp/auctex/tex-font
/usr/share/emacs24/site-lisp/auctex/toolbar-x hides /usr/share/emacs/site-lisp/auctex/toolbar-x
/usr/share/emacs24/site-lisp/auctex/tex-buf hides /usr/share/emacs/site-lisp/auctex/tex-buf
/usr/share/emacs24/site-lisp/auctex/bib-cite hides /usr/share/emacs/site-lisp/auctex/bib-cite
/usr/share/emacs24/site-lisp/auctex/context-en hides /usr/share/emacs/site-lisp/auctex/context-en
/usr/share/emacs24/site-lisp/auctex/tex-fold hides /usr/share/emacs/site-lisp/auctex/tex-fold
/usr/share/emacs24/site-lisp/auctex/tex-bar hides /usr/share/emacs/site-lisp/auctex/tex-bar
/usr/share/emacs24/site-lisp/auctex/context hides /usr/share/emacs/site-lisp/auctex/context
/usr/share/emacs24/site-lisp/auctex/prv-emacs hides /usr/share/emacs/site-lisp/auctex/prv-emacs
/usr/share/emacs24/site-lisp/auctex/font-latex hides /usr/share/emacs/site-lisp/auctex/font-latex
/usr/share/emacs/site-lisp/golang-mode/go-mode-load hides /usr/share/emacs/24.2/site-lisp/golang-mode/go-mode-load
/usr/share/emacs/site-lisp/golang-mode/go-mode hides /usr/share/emacs/24.2/site-lisp/golang-mode/go-mode
/usr/share/emacs/24.2/site-lisp/cmake-data/cmake-mode hides /usr/share/emacs/site-lisp/cmake-mode
/usr/share/emacs/24.2/site-lisp/cdargs hides /usr/share/emacs/site-lisp/cdargs
/usr/share/emacs/site-lisp/rst hides /usr/share/emacs/24.2/lisp/textmodes/rst
/usr/share/emacs24/site-lisp/dictionaries-common/ispell hides /usr/share/emacs/24.2/lisp/textmodes/ispell
/usr/share/emacs24/site-lisp/dictionaries-common/flyspell hides /usr/share/emacs/24.2/lisp/textmodes/flyspell

Features:
(time-stamp cus-edit info mule-util shadow sort mail-extr emacsbug
message format-spec rfc822 mml mml-sec mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mail-utils add-log log-view pcvs-util vc-annotate vc
ediff-merg ediff-diff ediff-wind ediff-help ediff-util ediff-mult
ediff-init ediff vc-dispatcher vc-bzr find-func etags latexenc preview
prv-emacs tex-buf font-latex latex tex-style misearch multi-isearch
m4-mode conf-mode newcomment make-mode pp help-mode view lua-mode
nroff-mode autoconf autoconf-mode sh-script executable todoo noutline
outline flymake compile comint ansi-color ring inform-mode diff-git
diff-mode vc-git cc-mode cc-fonts cc-guess cc-menus cc-cmds cc-styles
cc-align cc-engine cc-vars cc-defs jka-compr face-remap flyspell ispell
smart-quotes auto-dictionary-autoloads c-eldoc-autoloads
dictionary-autoloads diff-git-autoloads dired-isearch-autoloads
full-ack-autoloads guess-style-autoloads kill-ring-search-autoloads
magit-autoloads mv-shell-autoloads tumble-autoloads
http-post-simple-autoloads package tabulated-list completing-help
recentf tree-widget wid-edit uniquify paren server savehist
minibuf-eldef iswitchb icomplete autorevert dtrt-indent desktop
cus-start cus-load ropemacs pymacs warnings url-util url-parse
auth-source eieio byte-opt bytecomp byte-compile cconv macroexp
gnus-util password-cache url-vars mm-util mail-prsvr tex dbus xml
regexp-opt remember user-site-loaddefs advice advice-preload yasnippet
help-fns derived edmacro kmacro easymenu assoc cl muse-autoloads
go-mode-load emacs-goodies-el emacs-goodies-custom
emacs-goodies-loaddefs easy-mmode preview-latex tex-site auto-loads
time-date tooltip ediff-hook vc-hooks lisp-float-type mwheel x-win x-dnd
tool-bar dnd fontset image fringe lisp-mode register page menu-bar
rfn-eshadow timer select scroll-bar mouse jit-lock font-lock syntax
facemenu font-core frame cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese hebrew greek romanian slovak
czech european ethiopic indian cyrillic chinese case-table epa-hook
jka-cmpr-hook help simple abbrev minibuffer loaddefs button faces
cus-face files text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget hashtable-print-readable backquote
make-network-process dbusbind dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty emacs)

-- 
http://rrt.sc3d.org/

bug#13780: 24.2; Making find-file-hook customizable seems to cause troublle

[Stefan Monnier]

> The real problem here is that find-file-hook is clearly meant to be
> customizable (there is even a check box for auto-indent in its

But Custom only properly handles hooks which is only sets via Custom,
whereas it's reasonably common for find-file-hook to be modified by
packages (via add-hook, of course).
So until Custom is fixed to actually properly handle customisation of
hooks (e.g. by storing "add functions foo, bar, and baz" rather than
"ignore current value and just set it to (foo bar baz)").


        Stefan

bug#13780: 24.2; Making find-file-hook customizable seems to cause troublle

[Reuben Thomas]

On 22 February 2013 02:14, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> So until Custom is fixed to actually properly handle customisation of
> hooks (e.g. by storing "add functions foo, bar, and baz" rather than
> "ignore current value and just set it to (foo bar baz)").

This sentence seems to be missing its main clause…

-- 
http://rrt.sc3d.org

bug#13780: 24.2; Making find-file-hook customizable seems to cause troublle

[Stefan Monnier]

>> So until Custom is fixed to actually properly handle customisation of
>> hooks (e.g. by storing "add functions foo, bar, and baz" rather than
>> "ignore current value and just set it to (foo bar baz)").
> This sentence seems to be missing its main clause…

That's not just an appearance.  Its main clause is:

   So until Custom ...blabla... I think it's better to use defcustom for
   hooks very sparingly, i.e. only for those hook we can reasonably
   expect will only be modified by the user's customization.


        Stefan

bug#13780: 24.2; Making find-file-hook customizable seems to cause troublle

[Reuben Thomas]

On 22 February 2013 14:22, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
>    So until Custom ...blabla... I think it's better to use defcustom for
>    hooks very sparingly, i.e. only for those hook we can reasonably
>    expect will only be modified by the user's customization.

So do you think the existing customization ability for find-file-hook
should be removed? Or at least some kind of warning attached?

--
http://rrt.sc3d.org

bug#13780: 24.2; Making find-file-hook customizable seems to cause troublle

[Stefan Monnier]

>> So until Custom ...blabla... I think it's better to use defcustom for
>> hooks very sparingly, i.e. only for those hook we can reasonably
>> expect will only be modified by the user's customization.
> So do you think the existing customization ability for find-file-hook
> should be removed? Or at least some kind of warning attached?

Yes,


        Stefan

bug#13780: 24.2; Making find-file-hook customizable seems to cause troublle

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

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

>>> So until Custom ...blabla... I think it's better to use defcustom for
>>> hooks very sparingly, i.e. only for those hook we can reasonably
>>> expect will only be modified by the user's customization.
>> So do you think the existing customization ability for find-file-hook
>> should be removed? Or at least some kind of warning attached?
>
> Yes,

So how about the attached?

[-- Attachment #2: 0001-Don-t-allow-customizing-find-file-hook.patch --]
[-- Type: text/x-diff, Size: 1300 bytes --]

From d504f7a02ff1adcee60b9569e6a161d50e7e37c7 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Wed, 28 Oct 2020 09:51:05 +0100
Subject: [PATCH] Don't allow customizing find-file-hook

* lisp/files.el (find-file-hook): Make into defvar.  (Bug#13780)
---
 lisp/files.el | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index fdf758ad92..3d3dece691 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -498,15 +498,11 @@ find-file-not-found-functions
 ;;;(put 'find-file-hook 'permanent-local t)
 ;; I found some external files still using the obsolete form in 2018.
 (define-obsolete-variable-alias 'find-file-hooks 'find-file-hook "22.1")
-(defcustom find-file-hook nil
+(defvar find-file-hook nil
   "List of functions to be called after a buffer is loaded from a file.
 The buffer's local variables (if any) will have been processed before the
 functions are called.  This includes directory-local variables, if any,
-for the file's directory."
-  :group 'find-file
-  :type 'hook
-  :options '(auto-insert)
-  :version "22.1")
+for the file's directory.")
 
 ;; I found some external files still using the obsolete form in 2018.
 (define-obsolete-variable-alias 'write-file-hooks 'write-file-functions "22.1")
-- 
2.28.0

bug#13780: 24.2; Making find-file-hook customizable seems to cause troublle

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 28 Oct 2020 01:52:42 -0700
> Cc: 13780@debbugs.gnu.org, Reuben Thomas <rrt@sc3d.org>
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> >>> So until Custom ...blabla... I think it's better to use defcustom for
> >>> hooks very sparingly, i.e. only for those hook we can reasonably
> >>> expect will only be modified by the user's customization.
> >> So do you think the existing customization ability for find-file-hook
> >> should be removed? Or at least some kind of warning attached?
> >
> > Yes,
> 
> So how about the attached?

That's a backward-incompatible change, isn't it?  Now a user who types
"M-x set-variable RET" will not be able to set this variable, it can
only be set via "M-:".  Do we have a serious enough reason for that?

bug#13780: 24.2; Making find-file-hook customizable seems to cause troublle

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>
>> >>> So until Custom ...blabla... I think it's better to use defcustom for
>> >>> hooks very sparingly, i.e. only for those hook we can reasonably
>> >>> expect will only be modified by the user's customization.
>> >> So do you think the existing customization ability for find-file-hook
>> >> should be removed? Or at least some kind of warning attached?
>> >
>> > Yes,
>>
>> So how about the attached?
>
> That's a backward-incompatible change, isn't it?  Now a user who types
> "M-x set-variable RET" will not be able to set this variable, it can
> only be set via "M-:".  Do we have a serious enough reason for that?

It will be a backward-incompatible change, yes.

The reasons are the ones given in the discussion of this bug.
I think they make sense, FWIW:

Reuben Thomas <rrt@sc3d.org> writes:

> I just spent a while trying to work out why a third-party mode I had
> just installed was not automatically added to my find-file-hook, even
> though the mode's file calls add-hook when it is loaded, and I took care
> to ensure the mode was autoloaded. I discovered that the problem was
> that the mode was loaded by my customization file, custom.el, but later,
> the same file overwrote find-file-hook, because I had customized
> find-file-hook.
>
> I found that the reason for the customization was to get flymake-mode
> automatically activated; I've filed a separate issue about this
> (#13779). After removing the customization, my find-file-hook was set up
> correctly (except for flymake-mode).
>
> The real problem here is that find-file-hook is clearly meant to be
> customizable (there is even a check box for auto-indent in its
> customization dialog), yet customizing it seems to cause a race
> condition where depending in what order files are autoloaded, hooks may
> fail to be added properly to find-file-hook.

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

>>> So until Custom is fixed to actually properly handle customisation of
>>> hooks (e.g. by storing "add functions foo, bar, and baz" rather than
>>> "ignore current value and just set it to (foo bar baz)").
>> This sentence seems to be missing its main clause…
>
> That's not just an appearance.  Its main clause is:
>
>    So until Custom ...blabla... I think it's better to use defcustom for
>    hooks very sparingly, i.e. only for those hook we can reasonably
>    expect will only be modified by the user's customization.

bug#13780: 24.2; Making find-file-hook customizable seems to cause troublle

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Tue, 24 Nov 2020 18:18:58 -0800
> Cc: 13780@debbugs.gnu.org, monnier@iro.umontreal.ca, rrt@sc3d.org
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> >>> So until Custom is fixed to actually properly handle customisation of
> >>> hooks (e.g. by storing "add functions foo, bar, and baz" rather than
> >>> "ignore current value and just set it to (foo bar baz)").
> >> This sentence seems to be missing its main clause…
> >
> > That's not just an appearance.  Its main clause is:
> >
> >    So until Custom ...blabla... I think it's better to use defcustom for
> >    hooks very sparingly, i.e. only for those hook we can reasonably
> >    expect will only be modified by the user's customization.

IMO, this rationale is good for future additions, not for
already-existing user options.  We cannot un-defcustom a user option
just because someone became confused by some subtle use case.  This is
the price we pay for the flexibility and power we have in Emacs.

bug#13780: 24.2; Making find-file-hook customizable seems to cause troublle

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>
>> >>> So until Custom is fixed to actually properly handle customisation of
>> >>> hooks (e.g. by storing "add functions foo, bar, and baz" rather than
>> >>> "ignore current value and just set it to (foo bar baz)").
>> >> This sentence seems to be missing its main clause…
>> >
>> > That's not just an appearance.  Its main clause is:
>> >
>> >    So until Custom ...blabla... I think it's better to use defcustom for
>> >    hooks very sparingly, i.e. only for those hook we can reasonably
>> >    expect will only be modified by the user's customization.
>
> IMO, this rationale is good for future additions, not for
> already-existing user options.  We cannot un-defcustom a user option
> just because someone became confused by some subtle use case.  This is
> the price we pay for the flexibility and power we have in Emacs.

Fine by me.

bug#13780: 24.2; Making find-file-hook customizable seems to cause troublle

[Stefan Kangas]

tags 13780 + wontfix
close 13780
thanks

Stefan Kangas <stefan@marxist.se> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>>
>>> >>> So until Custom is fixed to actually properly handle customisation of
>>> >>> hooks (e.g. by storing "add functions foo, bar, and baz" rather than
>>> >>> "ignore current value and just set it to (foo bar baz)").
>>> >> This sentence seems to be missing its main clause…
>>> >
>>> > That's not just an appearance.  Its main clause is:
>>> >
>>> >    So until Custom ...blabla... I think it's better to use defcustom for
>>> >    hooks very sparingly, i.e. only for those hook we can reasonably
>>> >    expect will only be modified by the user's customization.
>>
>> IMO, this rationale is good for future additions, not for
>> already-existing user options.  We cannot un-defcustom a user option
>> just because someone became confused by some subtle use case.  This is
>> the price we pay for the flexibility and power we have in Emacs.
>
> Fine by me.

No further comments within 7 days, closing as wontfix.