bug#44864: 27.1; advice.el: ad-with-originals deprecated, but no advice on replacement

[Julian Gilbey <julian-gnu@d-and-j.net>]

On Thu, Nov 26, 2020 at 10:39:10AM -0500, Stefan Monnier wrote:
> >             ;; Use file's local variables section to set variables in
> >             ;; this buffer.  (Don't just copy local variables from the
> >             ;; base buffer because it may have set things locally that
> >             ;; we don't want in the other modes.)  We need to prevent
> >             ;; `mode' being processed and re-setting the major mode.
> >             ;; It all goes badly wrong if `hack-one-local-variable' is
> >             ;; advised.  The appropriate mechanism to get round this
> >             ;; appears to be `ad-with-originals', but we don't want to
> >             ;; pull in the advice package unnecessarily.  `flet'-like
> >             ;; mechanisms lose with advice because `fset' acts on the
> >             ;; advice anyway.
> >             (if (featurep 'advice)
> >                 (ad-with-originals (hack-one-local-variable)
> >                   (multi-hack-local-variables))
> >               (multi-hack-local-variables))
> 
> The comment doesn't explain why Dave thought that "It all goes badly
> wrong if `hack-one-local-variable' is advised", so there's not much we
> can do.  AFAIK it will only go wrong with some advice, so he must have
> had some particular pieces of advice in mind.
> 
> `advice.el` is deprecated anyway, so my recommendation is to remove this
> test and just call `multi-hack-local-variables` without paying attention
> to any potential advice there.  If/when you bump into a problem with
> that, then we'll be able to see what should be done (the best option
> might be to change the advice itself).

Thanks Stefan!

That sounds eminently sensible.  Scouring the rest of Dave's file, I
found the following comment at the beginning of the function you've
just quoted from:

(defun multi-install-mode (mode &optional chunk-fn base)
  "Add MODE to the multiple major modes supported by the current buffer.
CHUNK-FN, if non-nil, is a function to select the mode of a chunk,
added to the list `multi-chunk-fns'.  BASE non-nil means that this
is the base mode."
  (unless (memq mode multi-indirect-buffers-alist) ; be idempotent
    ;; This is part of a grim hack for lossage in AUCTeX, which
    ;; bogusly advises `hack-one-local-variable'.  This loses, due to
    ;; the way advice works, when we run `multi-hack-local-variables'
    ;; below -- there ought to be a way round this, probably with CL's
    ;; flet.  Any subsequent use of it then fails because advice has
    ;; captured the now-unbound variable `late-hack'...  Thus ensure
    ;; we've loaded the mode in advance to get any autoloads sorted
    ;; out.  Do it generally in case other modes have similar
    ;; problems.  [The AUCTeX stuff is in support of an undocumented
    ;; feature which is unnecessary and, anyway, wouldn't need advice
    ;; to implement.  Unfortunately the maintainer seems not to
    ;; understand the local variables mechanism and wouldn't remove
    ;; this.  To invoke minor modes, you should just use `mode:' in
    ;; `local variables'.]

And in auctex/tex.el, we have:

(defadvice hack-one-local-variable (after TeX-hack-one-local-variable-after
                                          activate)
  "Call minor mode function if minor mode variable is found."
  (let ((var (ad-get-arg 0))
        (val (ad-get-arg 1)))
    ;; Instead of checking for each mode explicitely `minor-mode-list'
    ;; could be used.  But this may make the byte compiler pop up.
    (when (memq var '(TeX-PDF-mode
                      TeX-source-correlate-mode TeX-interactive-mode
                      TeX-fold-mode LaTeX-math-mode))
      (if (symbol-value val) (funcall var 1) (funcall var 0)))))

So I'm guessing that's what he's referring to.

> > But there is no advice on what to do instead.
> 
> I think that's because `ad-with-originals` was used on it ;-)

Ah, that's beginning to make some sense!

Many thanks,

   Julian