Subject: bug#75626: 31.0.50; Dired misses or double-processes files when auto-revert-mode is enabled

From: Michael Heerdegen <[email-masked]>
Date: Wed, 05 Feb 2025 15:44:35 +0100

Michael Albinus <[email-masked]> writes:

> Cut'n'waste error. Thanks for the heads-up, I've fixed it.

Thanks.

> > | + (let ((buf (and (not (memq (current-buffer)
> > | inhibit-auto-revert-buffers))
> > | +                       (current-buffer))))
> > | +         (unwind-protect
> > | +             (progn
> > | + (when buf (add-to-list 'inhibit-auto-revert-buffers buf))
> > | +               ,@body)
> >
> > I think at least in this part where body is inserted the variable BUF
> > should be uninterned for the obvious reason.  Or at least have a more
> > obscure name.
>
> Yes, I've seen such constructs somewhere else in Emacs' code. I don't
> understand why a simple (lexical) let-binding of buf isn't
> sufficient. The problem would exist for any macro. Could you pls show me
> an example how it could go wrong?

Have you ever read (info "(elisp) Surprising Local Vars") - and the whole
(info "(elisp) Problems with Macros") chapter?  (If not, you should do...)

Short version: Lisp macro expansion is dumb ("unhygienic"): if your
macro uses a variable that accidentally has the same name as a variable
in the "inserted" code (like a BODY), when expanding the macro these
unrelated variables will become one and the same variable in the code
("variable" or "name clash") causing a variety of funny and unexpected
surprises that are weird and nasty to debug.

In case of `inhibit-auto-revert', the macro could interfere with a
variable named `buf' that may occur in BODY.  A variable named `buf' may
already be introduced in the surrounding code, e.g.

#+begin_src emacs-lisp
(let ((buf (get-buffer "*scratch*")))
  (inhibit-auto-revert
    (setq-local this-buffers-friend buf)
    (do-this-and-that...)))
#+end_src

The expansion would look like:

#+begin_src emacs-lisp
(let ((buf (get-buffer "*scratch*")))
 ...
 (let ((buf (and (not (memq (current-buffer) inhibit-auto-revert-buffers))
                 (current-buffer))))
   (unwind-protect
       (progn
         (if buf
             (add-to-list 'inhibit-auto-revert-buffers buf))
         (setq this-buffers-friend buf) ;; Ouch!! Refers to the wrong `buf'!
         (do-this-and-that...))
     ...)))
#+end_src

> (If you believe it is needed, you can just patch autorevert.el yourself,
> with a comment in the code.)

If I didn't misunderstand and this was new to you - am I allowed to
suggest this as an exercise?

The idea for fixing is that the macro instead introduces a new
uninterned symbol instead of an interned one, and uses that in the
expansion (see manual): a new uninterned symbol can't clash with any
existing variable.  `macroexpand' and `macroexpand-all' are your friend:
use these to see what you get.  Later you can do that in your mind only.
If you don't do that _all_the_time_, you'll get crazy with this stuff.

Ok - was that understandable?

Thx,

Michael.