Re: Last use of defadvice in Emacs

[Stefan Monnier]

> Could you give some tips on what issues one might see with defadvice
> and its disrespect of lexical bindings?

IIRC for existing pieces of advice the main issue can occur if an advice
accesses the formal arguments by their name rather than via things like
`ad-get-arg`.  IIRC we managed to get it working like it used to in most
cases, but it's not very robust.

The other issue is that the body of `defadvice` is always evaluated in
the dynamically scoped dialect of ELisp.  This means that its local vars
can "leak" to some of the functions they call (as is normal with
dynamic scoping) and that they can't refer to surrounding lexical
variables (more common with `ad-make-advice` than with `defadvice` since
that one is usually used at top-level), ...

Those problems aren't new.  What's "new" is that `advice-add` doesn't
suffer from them.

> For the record, I switched emacspeak to lexical binding a couple of
> years ago and fixed any/all bytecomp warnings it generated --- and
> I've not hit any bugs to date.

That's indeed how it often turns out :-)
[ Sadly, not all problems are caught by warnings, but the vast
  majority.  ]

> But it would be helpful to understand the types of issues defadvice
> might cause with lexical binding if something inexplicable does
> show up.

For people who grew up on dynamically scoped ELisp, there are usually no
bad surprises.  The surprises come up when you take lexical scoping as
a given.

> This next is likely not lexical binding related, but the only issue I
> needed to create an emacspeak specific check was for the equivalent of
> (called-interactively 'interactive) which exhibited strange behavior
> with defadvice -- I created a "tighter" check that is
> emacspeak-specific.

`called-interactively-p` is inherently brittle.  We go through some
trouble to make it work "right" (this is sometimes hard to define) for
as many cases as possible, but it's fundamentally an impossible job.
Sadly, it's particularly hard to do for pieces of advice yet also
particular useful there :-(


        Stefan


>>>> The patch below replaces the last remaining use of `defadvice` in Emacs
>>>> (well, except for Org where this has already been fixed upstream but
>>>> we're waiting for the change to trickle down to `master`).
>>>
>>> Why would we want to replace defadvice with advice-add?
>>> Don't all the objections to advice apply equally to both forms?
>>
>> Both should be avoided, indeed.  But `defadvice` is slowly being
>> replaced because it cannot obey `lexical-binding` (along with a bunch
>> of more minor annoyances) so it gets a few more objections.
>>
>>> I've spent an hour trying various combinations of eval-when-compile and
>>> (boundp 'font-lock-extend-after-change-region-function), to try and get
>>> the stuff compiled or ignored based on the presence/absence of that
>>> variable.  Something like C's #ifdef.  I didn't manage it, and don't
>>> think it's possible.  That's another C facility Emacs Lisp seems to be
>>> missing.
>>
>> I thought the code was written specifically to perform the test at
>> runtime, so that the code compiled on Emacs-21 would still skip the
>> advice when run on Emacs≥22 (and also so code compiled on Emacs-28 would
>> still activate the advice when run on some hypothetical future Emacs
>> where `font-lock-extend-after-change-region-function` has been removed).
>>
>> If you want to perform the test at compile time then I think something
>> like the following should work.
>>
>>
>>         Stefan
>>
>>
>> diff --git a/lisp/progmodes/cc-mode.el b/lisp/progmodes/cc-mode.el
>> index 957a0b8a7c5..05da61dbb2f 100644
>> --- a/lisp/progmodes/cc-mode.el
>> +++ b/lisp/progmodes/cc-mode.el
>> @@ -2565,18 +2565,18 @@
>>  ;; Emacs < 22 and XEmacs
>>  (defmacro c-advise-fl-for-region (function)
>>    (declare (debug t))
>> +  (unless (boundp 'font-lock-extend-after-change-region-function)
>>    `(defadvice ,function (before get-awk-region activate)
>>       ;; Make sure that any string/regexp is completely font-locked.
>>       (when c-buffer-is-cc-mode
>>         (save-excursion
>>       (ad-set-arg 1 c-new-END)   ; end
>> -     (ad-set-arg 0 c-new-BEG)))))   ; beg
>> +       (ad-set-arg 0 c-new-BEG)))))) ; beg
>>  
>> -(unless (boundp 'font-lock-extend-after-change-region-function)
>> -  (c-advise-fl-for-region font-lock-after-change-function)
>> -  (c-advise-fl-for-region jit-lock-after-change)
>> -  (c-advise-fl-for-region lazy-lock-defer-rest-after-change)
>> -  (c-advise-fl-for-region lazy-lock-defer-line-after-change))
>> +(c-advise-fl-for-region font-lock-after-change-function)
>> +(c-advise-fl-for-region jit-lock-after-change)
>> +(c-advise-fl-for-region lazy-lock-defer-rest-after-change)
>> +(c-advise-fl-for-region lazy-lock-defer-line-after-change)
>>  
>>  ;; Connect up to `electric-indent-mode' (Emacs 24.4 and later).
>>  (defun c-electric-indent-mode-hook ()
>>
>>