bug#66032: [PATCH] Inline advice documentation into advised function's docstring, after all

bug#66032: [PATCH] Inline advice documentation into advised function's docstring, after all

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

[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]

Severity: wishlist
X-Debbugs-CC: drew.adams@oracle.com, monnier@iro.umontreal.ca

I'm aware that I'm late to the party (got unstuck from Emacs 23
only recently) and that there have been some reports on that
already (bug#14734 and merged).

But at least on one occasion Stefan has asked for a patch, and I
haven't seen yet patches that got declined.  So here is one, so
far the plain code patch only, without NEWS, tests, etc.  It was
actually easier to implement than I thought, I hope I haven't
overseen any edge cases.

Here are two examples of how the generated documentation looks
like.  The former shows documentation of a legacy-advised
function, the latter shows documentation of one of Stefan's
test advice to demonstrate that these cases are handled as well.

------------------------- snip -------------------------
ediff-quit is an interactive byte-compiled Lisp function in
‘ediff-util.el’.

(ediff-quit REVERSE-DEFAULT-KEEP-VARIANTS)

Finish an Ediff session and exit Ediff.
[...]
With prefix argument REVERSE-DEFAULT-KEEP-VARIANTS, temporarily
reverse the meaning of this variable.

<font-lock-warning-face>This function is advised.</f-l-w-f>

This function has :around advice: ‘ad-Advice-ediff-quit’
Does not ask any stupid questions.

Pops to buffer A.

[back]
------------------------- snip -------------------------

------------------------- snip -------------------------
sm-test1 is a Lisp macro.

(sm-test1 A)

<font-lock-warning-face>This function is advised.</f-l-w-f>

This macro has :override advice: No documentation

This is an :override advice, which means that ‘sm-test1’ isn’t
run at all, and the documentation below may be irrelevant.

This macro has :around advice: No documentation

[back]
------------------------- snip -------------------------

What do you think?

[-- Attachment #2: 0001-Improve-documentation-of-advised-macros-or-functions.patch --]
[-- Type: text/x-patch, Size: 3155 bytes --]

From 5b7c14242c1efad6d5ab05bf159ae0a5b7f41c32 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Sat, 16 Sep 2023 14:53:18 +0200
Subject: [PATCH] Improve documentation of advised macros or functions

* lisp/emacs-lisp/nadvice.el (advice--make-single-doc): Inline advice
documentation where possible.
(advice--make-docstring): Add a prominent marker that the macro or
function is advised.
---
 lisp/emacs-lisp/nadvice.el | 47 +++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 11 deletions(-)

diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
index cd80df2c41d..f04a584cf74 100644
--- a/lisp/emacs-lisp/nadvice.el
+++ b/lisp/emacs-lisp/nadvice.el
@@ -93,17 +93,30 @@ advice--make-single-doc
      (format "This %s has %s advice: "
              (if macrop "macro" "function")
              how)
-     (let ((fun (advice--car flist)))
-       (if (symbolp fun) (format-message "`%S'." fun)
-         (let* ((name (cdr (assq 'name (advice--props flist))))
-                (doc (documentation fun t))
-                (usage (help-split-fundoc doc function)))
-           (if usage (setq doc (cdr usage)))
-           (if name
-               (if doc
-                   (format "%s\n%s" name doc)
-                 (format "%s" name))
-             (or doc "No documentation")))))
+     (let* ((fun (advice--car flist))
+            (doc (documentation fun t))
+            (usage (help-split-fundoc doc function))
+            name)
+       (if usage (setq doc (cdr usage)))
+       (if (symbolp fun)
+           (if doc
+               (concat
+                (format-message "`%S'\n" fun)
+                ;; Remove first line possibly added by
+                ;; `ad-make-single-advice-docstring' for
+                ;; legacy advices.
+                (if (string-match
+                     "^\\(?:Before\\|Around\\|After\\)-advice `.*?':\n"
+                     doc)
+                    (substring doc (match-end 0))
+                  doc))
+             (format-message "`%S'." fun))
+         (setq name (cdr (assq 'name (advice--props flist))))
+         (if name
+             (if doc
+                 (format "%s\n%s" name doc)
+               (format "%s" name))
+           (or doc "No documentation"))))
      "\n"
      (and
       (eq how :override)
@@ -155,10 +168,22 @@ advice--make-docstring
       (help-add-fundoc-usage
        (with-temp-buffer
          (when before
+           (insert
+            (propertize
+             (concat "This " (if macrop "macro" "function") " is advised.")
+             'face 'font-lock-warning-face))
+           (ensure-empty-lines 1)
            (insert before)
            (ensure-empty-lines 1))
          (when origdoc
            (insert origdoc))
+         (when (not before)
+           (ensure-empty-lines 1)
+           (insert
+            (propertize
+             (concat "This " (if macrop "macro" "function") " is advised.")
+             'face 'font-lock-warning-face))
+           (ensure-empty-lines 1))
          (when after
            (ensure-empty-lines 1)
            (insert after))
-- 
2.30.2

bug#66032: [PATCH] Inline advice documentation into advised function's docstring, after all

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

> I'm aware that I'm late to the party (got unstuck from Emacs 23
> only recently) and that there have been some reports on that
> already (bug#14734 and merged).

:-)

> But at least on one occasion Stefan has asked for a patch, and I
> haven't seen yet patches that got declined.

It does happen, tho.

> ------------------------- snip -------------------------
> sm-test1 is a Lisp macro.
>
> (sm-test1 A)
>
> <font-lock-warning-face>This function is advised.</f-l-w-f>
>
> This macro has :override advice: No documentation
>
> This is an :override advice, which means that ‘sm-test1’ isn’t
> run at all, and the documentation below may be irrelevant.
>
> This macro has :around advice: No documentation
>
> [back]
> ------------------------- snip -------------------------

Hmm... why does it say "This function is advised" here where the rest
talks about a macro instead?

[ This is not your fault, but the handling of :override isn't right
  currently, so if you could fix it while you're there, it would be
  great: the problem is that the mention of the specific override advice
  is placed at the beginning and all the others at the end, which makes
  it impossible to see the relative order of the :override advice w.r.t
  to the others.  ]

> What do you think?

I got used to the single line where I have to click to get more info, so
I'm not the target audience, but see comments below.

> +       (if (symbolp fun)
> +           (if doc
> +               (concat
> +                (format-message "`%S'\n" fun)
> +                ;; Remove first line possibly added by
> +                ;; `ad-make-single-advice-docstring' for
> +                ;; legacy advices.
> +                (if (string-match
> +                     "^\\(?:Before\\|Around\\|After\\)-advice `.*?':\n"
> +                     doc)
> +                    (substring doc (match-end 0))
> +                  doc))
> +             (format-message "`%S'." fun))

We don't want code to cater to the old `advice.el` here.
If you don't like that extra line, remove it in `advice.el` instead.
But now that `defadvice` is deprecated, I'm not sure it's worth the
trouble anyway.

The main problem I see, tho, is how to clearly "delimit" the docstring.
Maybe we should indent the advice's docstring by two spaces or so?

> +         (setq name (cdr (assq 'name (advice--props flist))))
> +         (if name
> +             (if doc
> +                 (format "%s\n%s" name doc)
> +               (format "%s" name))

[ I realize this is not your fault either, but we should say

      This function has :before advice named "NAME"

  rather than

      This function has :before advice: NAME
]

Fun situation where your code misfires:

    (defun sm-foo1 (&rest _) "Hello, this is foo1\n\nhere." nil)
    (advice-add 'diff-mode :around #'sm-foo1)
    (advice-add 'sm-foo1 :after #'ignore)

makes `C-h f diff-mode RET` show:

    [...]
    This mode runs the hook ‘diff-mode-hook’, as the final or penultimate
    step during initialization.
    
    This function is advised.
    
    This function has :around advice: ‘sm-foo1’
    Hello, this is foo1
    
    here.
    
    This function is advised.
    
    This function has :after advice: ‘ignore’
    Ignore ARGUMENTS, do nothing, and return nil.
    This function accepts any number of arguments in ARGUMENTS.
    Also see ‘always’.
    
      Probably introduced at or before Emacs version 21.1.

We can up the ante even further and advise `ignore`:

    (advice-add 'ignore :after #'sm-foo1)

such that `C-h f diff-mode RET` now tells us:

    Lisp nesting exceeds ‘max-lisp-eval-depth’

:-)

> @@ -155,10 +168,22 @@ advice--make-docstring
>        (help-add-fundoc-usage
>         (with-temp-buffer
>           (when before
> +           (insert
> +            (propertize
> +             (concat "This " (if macrop "macro" "function") " is advised.")
> +             'face 'font-lock-warning-face))
> +           (ensure-empty-lines 1)

I like this warning, but I don't like its code duplication.
The positive side is that I think this line should always come before
the main doc (and should be merged with the warning about "... override
... documentation below may be irrelevant" when applicable).


        Stefan

bug#66032: [PATCH] Inline advice documentation into advised function's docstring, after all

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

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

>> But at least on one occasion Stefan has asked for a patch, and I
>> haven't seen yet patches that got declined.
>
> It does happen, tho.

From your detailed comments (thanks for those) I understand that a patch
for *this* bug would be, in principle, still welcome.

I'll re-comment only where I think it's needed, everything else I try to
implement as you seem to suggest.

> I got used to the single line where I have to click to get more info,
> so I'm not the target audience, but see comments below.

My advices' docstrings all start with verb in 3rd person singular, and
that looks awful if viewed standalone.  (Showing off my standard
adherence here.)  More seriously, I have been thinking about a
customizable option to control the inclusion of advice docstrings, but
nadvice.el seems to be too infrastructure-y for that.

> The main problem I see, tho, is how to clearly "delimit" the
> docstring.  Maybe we should indent the advice's docstring by two
> spaces or so?

Probably OK, but I'm afraid that lines get too long, then.  OTOH, advice
docstrings are probably not that lengthy, in general.  I'll give it a
try and consider alternatives as well.

Do you think this docstring generation should be covered by ERT tests?

bug#66032: [PATCH] Inline advice documentation into advised function's docstring, after all

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

>>> But at least on one occasion Stefan has asked for a patch, and I
>>> haven't seen yet patches that got declined.
>> It does happen, tho.
> From your detailed comments (thanks for those) I understand that a patch
> for *this* bug would be, in principle, still welcome.

It's been requested a few times, so we may as well, yes.
I think ideally it should be fold(ed|able), but we don't have the
infrastructure for that yet.

> My advices' docstrings all start with verb in 3rd person singular, and
> that looks awful if viewed standalone.  (Showing off my standard
> adherence here.)  More seriously, I have been thinking about a
> customizable option to control the inclusion of advice docstrings, but
> nadvice.el seems to be too infrastructure-y for that.

Maybe a halfway point is to include only the first line of the
advice's docstring?

>> The main problem I see, tho, is how to clearly "delimit" the
>> docstring.  Maybe we should indent the advice's docstring by two
>> spaces or so?
> Probably OK, but I'm afraid that lines get too long, then.

We could consider this as a bug in the advice's docstring
(i.e. decide&document that advices' docstrings should use fewer columns
because they'll be displayed indented).

> Do you think this docstring generation should be covered by ERT tests?

ERT tests are always welcome,


        Stefan

bug#66032: [PATCH] Inline advice documentation into advised function's docstring, after all

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

[Sorry if this is resent - thought I sent it yesterday already.]

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

> [...]

Thanks for your comments again, I'll consider them.

>> Do you think this docstring generation should be covered by ERT tests?
>
> ERT tests are always welcome,

I have been trying to understand these quirks and previous bugs of
functions `advice--make-single-doc' and `advice--make-docstring' better,
also to probably provide some ERT tests, and would like to focus on one
issue I came across.  Namely, it seems that your work-around:

  ;; Hack attack!  For advices installed before calling
  ;; Snarf-documentation, the integer offset into the DOC file will not
  ;; be installed in the "core unadvised function" but in the advice
  ;; object instead!  So here we try to undo the damage.
  (when (integerp (aref flist 4))
    (setq docfun flist))

is no longer efficient.  When I modify uniquifiy.el as follows in emacs
master:

diff --git a/lisp/uniquify.el b/lisp/uniquify.el
index 2ad2fb0eeac..e43d61a3be6 100644
--- a/lisp/uniquify.el
+++ b/lisp/uniquify.el
@@ -489,7 +489,7 @@ uniquify-kill-buffer-function
 ;; rename-buffer and create-file-buffer.  (Setting find-file-hook isn't
 ;; sufficient.)

-;; (advice-add 'rename-buffer :around #'uniquify--rename-buffer-advice)
+(advice-add 'rename-buffer :before #'ignore)
 (defun uniquify--rename-buffer-advice (newname &optional unique)
   ;; BEWARE: This is called directly from `buffer.c'!
   "Uniquify buffer names with parts of directory name."

and remake emacs, I do not get the doc string for `rename-buffer' in
that Emacs.  Furthermore, if I execute

  (Snarf-documentation "DOC")

I get a message

  Docstring slot busy for rename-buffer

which makes me think that function store_function_docstring now knows
better than simply overwriting the COMPILED_DOC_STRING slot.  Most
likely because of this test:

  /* Don't overwrite a non-docstring value placed there,
   * such as the symbols used for Oclosures.  */
  && VALID_DOCSTRING_P (AREF (fun, COMPILED_DOC_STRING))

What do you think?  Strictly speaking, this is not even a bug since
Emacs does not seem to use pre-dump advices.

bug#66032: [PATCH] Inline advice documentation into advised function's docstring, after all

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

> I have been trying to understand these quirks and previous bugs of
> functions `advice--make-single-doc' and `advice--make-docstring' better,
> also to probably provide some ERT tests, and would like to focus on one
> issue I came across.  Namely, it seems that your work-around:
>
>   ;; Hack attack!  For advices installed before calling
>   ;; Snarf-documentation, the integer offset into the DOC file will not
>   ;; be installed in the "core unadvised function" but in the advice
>   ;; object instead!  So here we try to undo the damage.
>   (when (integerp (aref flist 4))
>     (setq docfun flist))
>
> is no longer efficient.

Indeed, this is not needed any more (since we don't use pre-dump advice
any more) nor is it working (ever since nadvice.el was made to use OClosures).

>   (Snarf-documentation "DOC")
>
> I get a message
>
>   Docstring slot busy for rename-buffer

[ BTW, I consider this to a be long standing bug in
  `Snarf-documentation`.  But since it's very unusual to call this
  function after dumping, it's a low-priority issue.  ]


        Stefan

bug#66032: [PATCH] Inline advice documentation into advised function's docstring, after all

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

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

> I think ideally it should be fold(ed|able), but we don't have the
> infrastructure for that yet.

With advice documentation looking like this:

------------------------- snip -------------------------
* :after advice ‘js-test-3’

  Doc string js-doc-3.

* :around advice ‘js-test-2’

  Doc string js-doc-2.

* :before advice ‘js-test-1’

  Doc string js-doc-1.
------------------------- snip -------------------------

and `outline-minor-mode' we can get even that.  Downsides:

- The documentation of function `*' looks funny when folded.

- The "[back] [forward]" navigation buttons get folded.

NB: I don't want to switch on `o-m-m' by default in *Help* buffers - we
could just advertise that somewhere.


But here is one other idea I wanted to discuss, which could both
simplify that whole business *and* make it to some extent customizable:

I propose to limit the docstring generation in `advice--make-docstring'
to something like this:

------------------------- snip -------------------------
js-test-0 is a Lisp function.

<f-l-w-f>This function is advised.</f-l-w-f>

(js-test-0 &rest _)

Doc string js-doc-0.
------------------------- snip -------------------------

that is, just include the *original function's docstring* and
some warning in it.

The documentation of the *function's advices*, however, is handled by
some function on `help-fns-describe-function-functions'.  By default we
add a function `advice-describe-function-advices' (for you) but offer an
alternative one `advice-describe-detailed-function-advices' (for me and
others who have been asking for inlined docstrings).  In principle this
works, as I have confirmed with a very rough prototype.


There are some pros and cons to this, but before I go into details I
wanted to get your opinion on that.

bug#66032: [PATCH] Inline advice documentation into advised function's docstring, after all

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

> The documentation of the *function's advices*, however, is handled by
> some function on `help-fns-describe-function-functions'.  By default we
> add a function `advice-describe-function-advices' (for you) but offer an
> alternative one `advice-describe-detailed-function-advices' (for me and
> others who have been asking for inlined docstrings).  In principle this
> works, as I have confirmed with a very rough prototype.

That's an option as well (and it comes with the advantage that the list
of advice can come after things like obsoletion info rather than before,
which would be an improvement as well).

I don't see any reason to reduce what is displayed by default (at least,
nobody has asked for that, AFAIK), so the question is how to make it
sufficiently easy to switch to a more detailed output.  `remove-hook` of
the default followed by `add-hook` of the other seems
rather inconvenient.


        Stefan

bug#66032: [PATCH] Inline advice documentation into advised function's docstring, after all

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

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

>> The documentation of the *function's advices*, however, is handled by
>> some function on `help-fns-describe-function-functions'.  By default we
>> add a function `advice-describe-function-advices' (for you) but offer an
>> alternative one `advice-describe-detailed-function-advices' (for me and
>> others who have been asking for inlined docstrings).  In principle this
>> works, as I have confirmed with a very rough prototype.
>
> That's an option as well (and it comes with the advantage that the list
> of advice can come after things like obsoletion info rather than before,
> which would be an improvement as well).

Exactly that was how I learned about `help-fns-d-f-f': The obsoletion
information currently being "misplaced" w.r.t. the advice documentation,
which gets more pronounced when the advice documentation gets longer.

> I don't see any reason to reduce what is displayed by default (at least,
> nobody has asked for that, AFAIK), so the question is how to make it
> sufficiently easy to switch to a more detailed output.  `remove-hook` of
> the default followed by `add-hook` of the other seems
> rather inconvenient.

I think anything more customizable than that would require a new
customization group, a defcustom, etc, which is probably too much effort
for a rather arcane feature.  So I will go simply for a more detailed
output by default on `help-fns-describe-function-functions'.

Here is one minor obstacle, though: The documentation of `h-f-d-f-f'
says:

  By convention they should indent their output by 2 spaces.

I think it is OK to break that convention for the advice headlines,
which I would like to start at column zero to set them off visibly
(and make them usable for outlining).

bug#66032: [PATCH] Inline advice documentation into advised function's docstring, after all

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

> Exactly that was how I learned about `help-fns-d-f-f': The obsoletion
> information currently being "misplaced" w.r.t. the advice documentation,
> which gets more pronounced when the advice documentation gets longer.

:-)

> I think anything more customizable than that would require a new
> customization group, a defcustom, etc, which is probably too much effort
> for a rather arcane feature.

We could get away without a customization group, but... agreed.

> So I will go simply for a more detailed
> output by default on `help-fns-describe-function-functions'.

Fine by me.

> Here is one minor obstacle, though: The documentation of `h-f-d-f-f'
> says:
>
>   By convention they should indent their output by 2 spaces.
>
> I think it is OK to break that convention for the advice headlines,
> which I would like to start at column zero to set them off visibly
> (and make them usable for outlining).

I think that's OK, yes.  Just make sure that the `add-hook` you use
specifies an appropriate `depth` so that the advice info always stays at
the bottom.


        Stefan