bug#19324: 25.0.50; add-function and nil

bug#19324: 25.0.50; add-function and nil

[Leo Liu]

1. Goto a buffer (e.g. text-mode) with eldoc-documentation-function nil
2. (add-function :after-until (local 'eldoc-documentation-function) #'ignore)
3. Move around to trigger eldoc error: (void-function nil)

Looks like the orig function is unconditionally called without checking
it's a function object.

Leo

bug#19324: 25.0.50; add-function and nil

[Stefan Monnier]

> 1. Goto a buffer (e.g. text-mode) with eldoc-documentation-function nil
> 2. (add-function :after-until (local 'eldoc-documentation-function) #'ignore)
> 3. Move around to trigger eldoc error: (void-function nil)
> Looks like the orig function is unconditionally called without checking
> it's a function object.

Indeed.  But I don't think advice.el should handle this case.
IOW the way I see it there are 2 ways to fix this problem:
- Change the default value so it's always a function.
- Make sure all the functions you add use either :override or :around
  and check that the orig is not nil before calling it.

Obviously, the first option is much better, which is why I've been
changing various foo-function variables so that their default value is
not nil.


        Stefan

bug#19324: 25.0.50; add-function and nil

[Leo Liu]

On 2014-12-10 14:54 -0500, Stefan Monnier wrote:
> - Make sure all the functions you add use either :override or :around
>   and check that the orig is not nil before calling it.

Yes, I had expected this to work and obviously not so filed this bug.
Otherwise it is not a bad workaround.

Go to a buffer in text mode where the default
eldoc-documentation-function is nil and eval

  (add-function :around (local 'eldoc-documentation-function)
                        (lambda (orig) (or (and orig (funcall orig)) (ignore))))

Move around to get the error.

Leo

bug#19324: 25.0.50; add-function and nil

[Stefan Monnier]

> Yes, I had expected this to work and obviously not so filed this bug.
> Otherwise it is not a bad workaround.

> Go to a buffer in text mode where the default
> eldoc-documentation-function is nil and eval

>   (add-function :around (local 'eldoc-documentation-function)
>                         (lambda (orig) (or (and orig (funcall orig)) (ignore))))

> Move around to get the error.

Ah, right, damn: the (local <foo>) causes the insertion of a "proxy
function" which runs the global value, but that doesn't check if
it's nil.
IOW it only works for :override.


        Stefan

bug#19324: 25.0.50; add-function and nil

[Leo Liu]

On 2014-12-10 21:48 -0500, Stefan Monnier wrote:
> Ah, right, damn: the (local <foo>) causes the insertion of a "proxy
> function" which runs the global value, but that doesn't check if it's
> nil. IOW it only works for :override.

Sounds like a bug ;)

Leo

bug#19324: 25.0.50; add-function and nil

[Stefan Monnier]

>> Ah, right, damn: the (local <foo>) causes the insertion of a "proxy
>> function" which runs the global value, but that doesn't check if it's
>> nil. IOW it only works for :override.
> Sounds like a bug ;)

Indeed, I fixed the eldoc-documentation-function not to be nil.


        Stefan

bug#19324: 25.0.50; add-function and nil

[Leo Liu]

On 2014-12-11 12:01 -0500, Stefan Monnier wrote:
> Indeed, I fixed the eldoc-documentation-function not to be nil.
>
>
>         Stefan

But :around are still broken. It promises to pass on ORIG but instead
passes a proxy. so the advice has no (public) way to tell missing
function. Do you have a fix for this? Otherwise we have to check and set
a dummy value to be safe, for example¹.

Leo

Footnotes: 
¹  https://github.com/leoliu/ggtags/commit/115194cc27dd578b986aaaf0dc390bb57e9838b2

bug#19324: 25.0.50; add-function and nil

[Stefan Monnier]

> But :around are still broken.  It promises to pass on ORIG but instead
> passes a proxy.  So the advice has no (public) way to tell missing
> function.  Do you have a fix for this?

No I don't have a fix for this.
`add-function' is designed to work on places that only hold functions,
so any other value (such as nil) will create problems.

IOW a variable that can hold "either nil or a function" is not something
that add-function supports.

I guess we could treat nil as an alias for `ignore' in the "proxy
function", which would fix this particular issue.  See patch below.
But I don't intend to handle all the cases in which a "nil function" can
show up.  Many/most uses of `foo-function' actually give a special
meaning to nil which is different from `ignore'.
So I'm not sure we should cater to this particular case.

> Otherwise we have to check and set a dummy value to be safe, for
> example¹.

That looks like a good workaround.


        Stefan


diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
index a81d3e4..7e1c236 100644
--- a/lisp/emacs-lisp/nadvice.el
+++ b/lisp/emacs-lisp/nadvice.el
@@ -234,7 +234,7 @@ different, but `function-equal' will hopefully ignore those differences.")
   (if (local-variable-p var) (symbol-value var)
     (setq advice--buffer-local-function-sample
           ;; This function acts like the t special value in buffer-local hooks.
-          (lambda (&rest args) (apply (default-value var) args)))))
+          (lambda (&rest args) (apply (or (default-value var) #'ignore) args)))))
 
 (eval-and-compile
   (defun advice--normalize-place (place)

bug#19324: 25.0.50; add-function and nil

[Leo Liu]

On 2014-12-11 14:11 -0500, Stefan Monnier wrote:
> No I don't have a fix for this.
> `add-function' is designed to work on places that only hold functions,
> so any other value (such as nil) will create problems.
>
> IOW a variable that can hold "either nil or a function" is not something
> that add-function supports.
>
> I guess we could treat nil as an alias for `ignore' in the "proxy
> function", which would fix this particular issue.  See patch below.
> But I don't intend to handle all the cases in which a "nil function" can
> show up.  Many/most uses of `foo-function' actually give a special
> meaning to nil which is different from `ignore'.
> So I'm not sure we should cater to this particular case.

Thanks, Stefan, for the explanation. In that case maybe the workaround
isn't needed because we still cannot tell if ORIG is missing. I consider
the bug closed.

Leo