bug#66908: Exposing more public nadvice API

bug#66908: Exposing more public nadvice API

[Philip Kaludercic]

I would like to ask if it would be possible to have a public and
"official" way to access information about advised functions, since all
the accessory functions defined by `oclosure-define' appear to be marked
as internal.

Specifically `advice-cd*r' would be useful, though it might be that we
(Visuwesh and I) are trying to do the wrong thing, since we want the
return value to get a more sensible response from `func-arity' -- and I
recall we had conversations about the complexity of this issue in the
past before.

bug#66908: Exposing more public nadvice API

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I would like to ask if it would be possible to have a public and
> "official" way to access information about advised functions, since all
> the accessory functions defined by `oclosure-define' appear to be marked
> as internal.

[ `oclosure-define` always define accessors as internal, indeed.
  I decided it was simpler this way, since it's easy enough to add
  aliases for those accessors we want to export.  ]

> Specifically `advice-cd*r' would be useful,

It's clearly the internal function most frequently used outside of
`nadvice.el`, indeed.
I think it'd be OK to promote that function to a non-internal name.

> though it might be that we (Visuwesh and I) are trying to do the wrong
> thing, since we want the return value to get a more sensible response
> from `func-arity' -- and I recall we had conversations about the
> complexity of this issue in the past before.

There are very few places where `func-arity` can be used reliably,
indeed and most of those cases are better served by
`help-function-arglist`.

In the case where `func-arity` is used for backward compatibility
purposes (where reliability is not really possible anyway), I've
generally recommend the use of

    (condition-case nil
        ...
      (wrong-number-of-aruments
        ...))

instead.  It comes with its own failure modes, of course, but it's
usually easier to use and I found it to fail less often in practice
(because it's not affected by wrappers like those introduced by advice
or `apply-partially`).


        Stefan

bug#66908: Exposing more public nadvice API

[Visuwesh]

[வெள்ளி நவம்பர் 03, 2023] Stefan Monnier wrote:

>> Specifically `advice-cd*r' would be useful,
>
> It's clearly the internal function most frequently used outside of
> `nadvice.el`, indeed.
> I think it'd be OK to promote that function to a non-internal name.

It would be nice if you could since it would also provide some guarantee
of it being a fairly stable interface.

The other function that I saw get usage outside of nadvice and bytecomp
was advice--symbol-function.  Should we use its return value, or is
indirect-function's return value good enough for advice--cd*r?  IIRC,
help-function-arglist uses indirect-function and it also works for our
use case.

>> though it might be that we (Visuwesh and I) are trying to do the wrong
>> thing, since we want the return value to get a more sensible response
>> from `func-arity' -- and I recall we had conversations about the
>> complexity of this issue in the past before.
>
> There are very few places where `func-arity` can be used reliably,
> indeed and most of those cases are better served by
> `help-function-arglist`.
>
> In the case where `func-arity` is used for backward compatibility
> purposes (where reliability is not really possible anyway), I've
> generally recommend the use of
>
>     (condition-case nil
>         ...
>       (wrong-number-of-aruments
>         ...))
>
> instead.  It comes with its own failure modes, of course, but it's
> usually easier to use and I found it to fail less often in practice
> (because it's not affected by wrappers like those introduced by advice
> or `apply-partially`).
>
>
>         Stefan

bug#66908: Exposing more public nadvice API

[Drew Adams]

> The other function that I saw get usage outside of
> nadvice and bytecomp was advice--symbol-function.

advice--car
advice--cdr
advice--cd*r
advice--p
advice--symbol-function

bug#66908: Exposing more public nadvice API

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> It's clearly the internal function most frequently used outside of
>> `nadvice.el`, indeed.
>> I think it'd be OK to promote that function to a non-internal name.
> It would be nice if you could since it would also provide some guarantee
> of it being a fairly stable interface.

Could you describe the circumstance where you need it?

> The other function that I saw get usage outside of nadvice and bytecomp
> was advice--symbol-function.  Should we use its return value, or is
> indirect-function's return value good enough for advice--cd*r?  IIRC,
> help-function-arglist uses indirect-function and it also works for our
> use case.

I don't think `advice--symbol-function` is a good candidate because its
semantics is not very clearly defined.  E.g. I'd be hard pressed to give
a comprehensible documentation of it without either being too vague,
or promising things I can't always provide, or getting too much into the
the nitty gritty details of the various possible situations.
That's why I haven't promoted the comment in its body
to an actual docstring :-(

Most callers are in only one of the many different situations, in which
case they usually don't need that functionality.


        Stefan

bug#66908: Exposing more public nadvice API

[Visuwesh]

[வெள்ளி நவம்பர் 03, 2023] Stefan Monnier wrote:

>>> It's clearly the internal function most frequently used outside of
>>> `nadvice.el`, indeed.
>>> I think it'd be OK to promote that function to a non-internal name.
>> It would be nice if you could since it would also provide some guarantee
>> of it being a fairly stable interface.
>
> Could you describe the circumstance where you need it?

We need to get the func-arity of the original function and not its
advice.  I have xref-find-definitions adviced but

    (func-arity 'xref-find-definitions);; ⇒ (0 . many)

returns the func-arity of the advice meanwhile, what we are really
interested in is the func-arity of xref-find-definitions as shown in the
*Help* buffer i.e.,

    (func-arity (advice--cd*r (indirect-function 'xref-find-definitions))) ;; ⇒ (1 . 1)

which is the right return value.  It might be nice to not have to call
`indirect-function' here for the "global" function but you can be a
better judge of that.

>> The other function that I saw get usage outside of nadvice and bytecomp
>> was advice--symbol-function.  Should we use its return value, or is
>> indirect-function's return value good enough for advice--cd*r?  IIRC,
>> help-function-arglist uses indirect-function and it also works for our
>> use case.
>
> I don't think `advice--symbol-function` is a good candidate because its
> semantics is not very clearly defined.  E.g. I'd be hard pressed to give
> a comprehensible documentation of it without either being too vague,
> or promising things I can't always provide, or getting too much into the
> the nitty gritty details of the various possible situations.
> That's why I haven't promoted the comment in its body
> to an actual docstring :-(
>
> Most callers are in only one of the many different situations, in which
> case they usually don't need that functionality.

In our case, the functions that will be checked for its arity should be
defined at the time of func-arity call.  Or at least auto-loaded AFAIU.
Philip can correct me if I'm wrong.

bug#66908: Exposing more public nadvice API

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Could you describe the circumstance where you need it?
> We need to get the func-arity of the original function and not its
> advice.

That's the part I'd been (indirectly) told already.  What I meant was
why do you need to find the arity of that(those) function(s)?

>     (func-arity (advice--cd*r (indirect-function 'xref-find-definitions))) ;; ⇒ (1 . 1)
>
> which is the right return value.  It might be nice to not have to call
> `indirect-function' here for the "global" function but you can be a
> better judge of that.

Don't know what you mean by "global" function.

Side note: an advice may also be installed specifically to change the
arity, e.g. to add support for some new calling convention.

> In our case, the functions that will be checked for its arity should be
> defined at the time of func-arity call.  Or at least auto-loaded AFAIU.

By "autoloaded" do you mean "setup to be loaded on demand but not yet
loaded", or do you mean "had been setup to be loaded on demand and has
been loaded already"?

The second case is "irrelevant" in the sense that it doesn't matter if
the function had been autoloaded before it was defined.


        Stefan

bug#66908: Exposing more public nadvice API

[Visuwesh]

[சனி நவம்பர் 04, 2023] Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" wrote:

>>> Could you describe the circumstance where you need it?
>> We need to get the func-arity of the original function and not its
>> advice.
>
> That's the part I'd been (indirectly) told already.  What I meant was
> why do you need to find the arity of that(those) function(s)?

Sure, in Philip's package do-at-point, a function defined to "act" on
the `thing' at point are given different arguments depending on the
minimum number of arguments required by the function:

    (let* ((thing (overlay-get do-at-point--overlay 'do-at-point-thing))
           (options (do-at-point--actions thing))
           (choice ...)
           (func (cadr (alist-get (car choice) options)))
           (bound (cons (overlay-start do-at-point--overlay)
                        (overlay-end do-at-point--overlay))))
      (when func
        (pcase (car (func-arity func))
                    ^^^^^^^^^^^^^^^^^
          (0 (funcall func))
          (1 (funcall func (buffer-substring (car bound) (cdr bound))))
          (2 (funcall func (car bound) (cdr bound)))
          (_ (error "Unsupported signature: %S" func)))))

Currently the func-arity call fails when the function is adviced.  This
is a nice format to follow since it

    1. allows reuse of existing functions to be executed as actions
       without the need for wrapper functions 
       [ i.e., (lambda (str) (xref-find-definitions str))  ]
    2. allows the "action" function to specify what it ends by changing
       its required arguments.

I hope this is clear.

>>     (func-arity (advice--cd*r (indirect-function 'xref-find-definitions))) ;; ⇒ (1 . 1)
>>
>> which is the right return value.  It might be nice to not have to call
>> `indirect-function' here for the "global" function but you can be a
>> better judge of that.
>
> Don't know what you mean by "global" function.

By "global", I mean the new global function advice-cd*r or somesuch that
might eventually be added from this discussion.

> Side note: an advice may also be installed specifically to change the
> arity, e.g. to add support for some new calling convention.

Ahhhhhh... now we have a complication that I never thought about.

>> In our case, the functions that will be checked for its arity should be
>> defined at the time of func-arity call.  Or at least auto-loaded AFAIU.
>
> By "autoloaded" do you mean "setup to be loaded on demand but not yet
> loaded", or do you mean "had been setup to be loaded on demand and has
> been loaded already"?

The former obviously.

> The second case is "irrelevant" in the sense that it doesn't matter if
> the function had been autoloaded before it was defined.
>
>
>         Stefan

bug#66908: Exposing more public nadvice API

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Sure, in Philip's package do-at-point, a function defined to "act" on
> the `thing' at point are given different arguments depending on the
> minimum number of arguments required by the function:

Ah :-(

So a kind of "unimplementable semantics" for DWIM purposes.

>         (pcase (car (func-arity func))
>                     ^^^^^^^^^^^^^^^^^
>           (0 (funcall func))
>           (1 (funcall func (buffer-substring (car bound) (cdr bound))))
>           (2 (funcall func (car bound) (cdr bound)))
>           (_ (error "Unsupported signature: %S" func)))))

I recommend:

    (condition-case nil
        (funcall func (car bound) (cdr bound))
      (wrong-number-of-arguments
       (condition-case nil
           (funcall func (buffer-substring (car bound) (cdr bound)))
         (wrong-number-of-arguments
          (funcall func)))))

:-)

Works with advice and other wrappers without having to worry about
`indirect-function`, autoloading, etc...

>>>     (func-arity (advice--cd*r (indirect-function 'xref-find-definitions))) ;; ⇒ (1 . 1)
>>> which is the right return value.  It might be nice to not have to call
>>> `indirect-function' here for the "global" function but you can be a
>>> better judge of that.
>> Don't know what you mean by "global" function.
> By "global", I mean the new global function advice-cd*r or somesuch that
> might eventually be added from this discussion.

Ah, I see.  I don't think `advice-cd*r` should follow aliases in
general.  But indeed, you may have an `advice` object whose
`advice-cd*r` is a symbol, whose definition is another advice object,
etc...

Another good reason to prefer the `condition-case` approach :-)

>> By "autoloaded" do you mean "setup to be loaded on demand but not yet
>> loaded", or do you mean "had been setup to be loaded on demand and has
>> been loaded already"?
> The former obviously.

In that case, `indirect-function` would not see the advice, then
`func-arity` would cause the target to be (auto)loaded, during which
a previously pending advice could be installed and it would return the
dreaded (0 . many) from the advice it sees in the definition.

Again, using `condition-case` side steps the issue.


        Stefan

bug#66908: Exposing more public nadvice API

[Philip Kaludercic]

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

>> Sure, in Philip's package do-at-point, a function defined to "act" on
>> the `thing' at point are given different arguments depending on the
>> minimum number of arguments required by the function:
>
> Ah :-(
>
> So a kind of "unimplementable semantics" for DWIM purposes.
>
>>         (pcase (car (func-arity func))
>>                     ^^^^^^^^^^^^^^^^^
>>           (0 (funcall func))
>>           (1 (funcall func (buffer-substring (car bound) (cdr bound))))
>>           (2 (funcall func (car bound) (cdr bound)))
>>           (_ (error "Unsupported signature: %S" func)))))
>
> I recommend:
>
>     (condition-case nil
>         (funcall func (car bound) (cdr bound))
>       (wrong-number-of-arguments
>        (condition-case nil
>            (funcall func (buffer-substring (car bound) (cdr bound)))
>          (wrong-number-of-arguments
>           (funcall func)))))
>
> :-)
>
> Works with advice and other wrappers without having to worry about
> `indirect-function`, autoloading, etc...

The main issue here is that this checks if a function accepts up to two
arguments, but we are interested in the minimal number of arguments.
I guess that turning this around should work, right:

     (condition-case nil
         (funcall func)
       (wrong-number-of-arguments
        (condition-case nil
            (funcall func (buffer-substring (car bound) (cdr bound)))
          (wrong-number-of-arguments
           (funcall func (car bound) (cdr bound))))))

?

>>>>     (func-arity (advice--cd*r (indirect-function 'xref-find-definitions))) ;; ⇒ (1 . 1)
>>>> which is the right return value.  It might be nice to not have to call
>>>> `indirect-function' here for the "global" function but you can be a
>>>> better judge of that.
>>> Don't know what you mean by "global" function.
>> By "global", I mean the new global function advice-cd*r or somesuch that
>> might eventually be added from this discussion.
>
> Ah, I see.  I don't think `advice-cd*r` should follow aliases in
> general.  But indeed, you may have an `advice` object whose
> `advice-cd*r` is a symbol, whose definition is another advice object,
> etc...
>
> Another good reason to prefer the `condition-case` approach :-)
>
>>> By "autoloaded" do you mean "setup to be loaded on demand but not yet
>>> loaded", or do you mean "had been setup to be loaded on demand and has
>>> been loaded already"?
>> The former obviously.
>
> In that case, `indirect-function` would not see the advice, then
> `func-arity` would cause the target to be (auto)loaded, during which
> a previously pending advice could be installed and it would return the
> dreaded (0 . many) from the advice it sees in the definition.
>
> Again, using `condition-case` side steps the issue.
>
>
>         Stefan

bug#66908: Exposing more public nadvice API

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> The main issue here is that this checks if a function accepts up to two
> arguments, but we are interested in the minimal number of arguments.

Ah, yes you checked the lower bound, sorry.

> I guess that turning this around should work, right:
>
>      (condition-case nil
>          (funcall func)
>        (wrong-number-of-arguments
>         (condition-case nil
>             (funcall func (buffer-substring (car bound) (cdr bound)))
>           (wrong-number-of-arguments
>            (funcall func (car bound) (cdr bound))))))

Yup.


        Stefan