bug#36588: Unable to revert M-x apropos help buffer

bug#36588: Unable to revert M-x apropos help buffer

[Stefan Kangas]

I get an error trying to revert the apropos help buffer on Emacs 27
(current master).  I could also reproduce this on 26.1.

Steps to reproduce:
0. emacs -Q
1. M-x apropos RET foo RET
2. C-x o
3. g yes RET
4. Error:
apply: Symbol’s function definition is void: nil

Backtrace:
Debugger entered--Lisp error: (void-function nil)
  nil()
  apply(nil nil)
  help-mode-revert-buffer(t nil)
  revert-buffer(t)
  funcall-interactively(revert-buffer t)
  #<subr call-interactively>(revert-buffer nil nil)
  apply(#<subr call-interactively> revert-buffer (nil nil))
  call-interactively@ido-cr+-record-current-command(#<subr
call-interactively> revert-buffer nil nil)
  apply(call-interactively@ido-cr+-record-current-command #<subr
call-interactively> (revert-buffer nil nil))
  call-interactively(revert-buffer nil nil)
  command-execute(revert-buffer)

GNU Emacs 27.0.50 (build 5, x86_64-apple-darwin15.6.0, NS
appkit-1404.47 Version 10.11.6 (Build 15G22010)) of 2019-07-11

Thanks,
Stefan Kangas

bug#36588: Unable to revert M-x apropos help buffer

[Basil L. Contovounesios]

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

severity 36588 minor
tags 36588 + patch
quit

Stefan Kangas <stefan@marxist.se> writes:

> I get an error trying to revert the apropos help buffer on Emacs 27
> (current master).  I could also reproduce this on 26.1.
>
> Steps to reproduce:
> 0. emacs -Q
> 1. M-x apropos RET foo RET
> 2. C-x o
> 3. g yes RET
> 4. Error:
> apply: Symbol’s function definition is void: nil
>
> Backtrace:
> Debugger entered--Lisp error: (void-function nil)
>   nil()
>   apply(nil nil)
>   help-mode-revert-buffer(t nil)
>   revert-buffer(t)
>   funcall-interactively(revert-buffer t)
>   #<subr call-interactively>(revert-buffer nil nil)
>   apply(#<subr call-interactively> revert-buffer (nil nil))
>   call-interactively@ido-cr+-record-current-command(#<subr
> call-interactively> revert-buffer nil nil)
>   apply(call-interactively@ido-cr+-record-current-command #<subr
> call-interactively> (revert-buffer nil nil))
>   call-interactively(revert-buffer nil nil)
>   command-execute(revert-buffer)

This is because Apropos buffers are set up in apropos-print using
with-output-to-temp-buffer, which by default calls help-mode, which sets
revert-buffer-function permanently-locally to help-mode-revert-buffer.

[Aside: Why is revert-buffer-function permanent-local?]

help-mode-revert-buffer expects the command which generated the Help
buffer to have previously called help-setup-xref.  This should be
possible to do in Apropos, but it would be quite ugly as help-setup-xref
needs to be called with the Help buffer current, so Apropos commands
would need to set their own version of help-xref-stack-item before
apropos-print performs the help-setup-xref dance.  Besides, Apropos
buffers are not (currently) really Help buffers.

Instead, I propose emulating a simpler version of help-setup-xref
specific to Apropos:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Support-reverting-Apropos-buffers-bug-36588.patch --]
[-- Type: text/x-diff, Size: 4253 bytes --]

From 890d2d6c1447f207892be734ab7412b4b471ddb7 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Thu, 11 Jul 2019 15:11:35 +0100
Subject: [PATCH] Support reverting Apropos buffers (bug#36588)

* lisp/apropos.el (apropos--current): New variable akin to
help-xref-stack-item storing information for revert-buffer.
(apropos--revert-buffer): New function.
(apropos-mode): Use it as revert-buffer-function.
(apropos-command, apropos, apropos-library, apropos-value)
(apropos-local-value, apropos-documentation): Set apropos--current
in low-level commands, i.e., those which do not call other commands.
---
 lisp/apropos.el | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/lisp/apropos.el b/lisp/apropos.el
index 1b86f5bcde..79e5a1518f 100644
--- a/lisp/apropos.el
+++ b/lisp/apropos.el
@@ -212,6 +212,12 @@ apropos-synonyms
 Each element is a list of words where the first word is the standard Emacs
 term, and the rest of the words are alternative terms.")
 
+(defvar apropos--current nil
+  "List of current Apropos function followed by its arguments.
+Used by `apropos--revert-buffer' to regenerate the current
+Apropos buffer.  Each Apropos command should ensure it is set
+before `apropos-mode' makes it buffer-local.")
+
 \f
 ;;; Button types used by apropos
 
@@ -472,10 +478,18 @@ apropos-true-hit-doc
   "Return t if DOC is really matched by the current keywords."
   (apropos-true-hit doc apropos-all-words))
 
+(defun apropos--revert-buffer (_ignore-auto noconfirm)
+  "Regenerate current Apropos buffer using `apropos--current'.
+Intended as a value for `revert-buffer-function'."
+  (when (or noconfirm (yes-or-no-p "Revert apropos buffer? "))
+    (apply #'funcall apropos--current)))
+
 (define-derived-mode apropos-mode special-mode "Apropos"
   "Major mode for following hyperlinks in output of apropos commands.
 
-\\{apropos-mode-map}")
+\\{apropos-mode-map}"
+  (make-local-variable 'apropos--current)
+  (setq-local revert-buffer-function #'apropos--revert-buffer))
 
 (defvar apropos-multi-type t
   "If non-nil, this apropos query concerns multiple types.
@@ -550,6 +564,7 @@ apropos-command
 		      (if (or current-prefix-arg apropos-do-all)
 			  "command or function" "command"))
 		     current-prefix-arg))
+  (setq apropos--current (list #'apropos-command pattern do-all var-predicate))
   (apropos-parse-pattern pattern)
   (let ((message
 	 (let ((standard-output (get-buffer-create "*Apropos*")))
@@ -628,6 +643,7 @@ apropos
 Returns list of symbols and documentation found."
   (interactive (list (apropos-read-pattern "symbol")
 		     current-prefix-arg))
+  (setq apropos--current (list #'apropos pattern do-all))
   (apropos-parse-pattern pattern)
   (apropos-symbols-internal
    (apropos-internal apropos-regexp
@@ -670,6 +686,7 @@ apropos-library
                          libs))
                   libs)))
      (list (completing-read "Describe library: " libs nil t))))
+  (setq apropos--current (list #'apropos-library file))
   (let ((symbols nil)
 	;; (autoloads nil)
 	(provides nil)
@@ -776,6 +793,7 @@ apropos-value
 Returns list of symbols and values found."
   (interactive (list (apropos-read-pattern "value")
 		     current-prefix-arg))
+  (setq apropos--current (list #'apropos-value pattern do-all))
   (apropos-parse-pattern pattern)
   (or do-all (setq do-all apropos-do-all))
   (setq apropos-accumulator ())
@@ -815,6 +833,7 @@ apropos-local-value
 Optional arg BUFFER (default: current buffer) is the buffer to check."
   (interactive (list (apropos-read-pattern "value of buffer-local variable")))
   (unless buffer (setq buffer  (current-buffer)))
+  (setq apropos--current (list #'apropos-local-value pattern buffer))
   (apropos-parse-pattern pattern)
   (setq apropos-accumulator  ())
   (let ((var             nil))
@@ -856,6 +875,7 @@ apropos-documentation
   ;; output, but I cannot see that that is true.
   (interactive (list (apropos-read-pattern "documentation")
 		     current-prefix-arg))
+  (setq apropos--current (list #'apropos-documentation pattern do-all))
   (apropos-parse-pattern pattern)
   (or do-all (setq do-all apropos-do-all))
   (setq apropos-accumulator () apropos-files-scanned ())
-- 
2.20.1


[-- Attachment #3: Type: text/plain, Size: 284 bytes --]


I don't consider this duplication because, unless Apropos were
completely refactored, setting something like apropos--current would be
necessary either way.  This has the added perk of not coupling Apropos
with Help any more than it currently needs to be.

WDYT?

Thanks,

-- 
Basil

bug#36588: Unable to revert M-x apropos help buffer

[Stefan Kangas]

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

Basil L. Contovounesios <contovob@tcd.ie> writes:

> This is because Apropos buffers are set up in apropos-print using
> with-output-to-temp-buffer, which by default calls help-mode, which sets
> revert-buffer-function permanently-locally to help-mode-revert-buffer.
>
> [Aside: Why is revert-buffer-function permanent-local?]
>
> help-mode-revert-buffer expects the command which generated the Help
> buffer to have previously called help-setup-xref.  This should be
> possible to do in Apropos, but it would be quite ugly as help-setup-xref
> needs to be called with the Help buffer current, so Apropos commands
> would need to set their own version of help-xref-stack-item before
> apropos-print performs the help-setup-xref dance.  Besides, Apropos
> buffers are not (currently) really Help buffers.
>
> Instead, I propose emulating a simpler version of help-setup-xref
> specific to Apropos:
>
>
> I don't consider this duplication because, unless Apropos were
> completely refactored, setting something like apropos--current would be
> necessary either way.  This has the added perk of not coupling Apropos
> with Help any more than it currently needs to be.
>
> WDYT?

Not familiar with this code, so I can't speak to your solution.  I can confirm
it fixes the problem though.  Thanks for looking at it so promptly.

I've attached a small patch with a test for this.

Thanks,
Stefan Kangas

[-- Attachment #2: 0001-test-lisp-apropos-tests.el-New-file.patch --]
[-- Type: application/octet-stream, Size: 1681 bytes --]

From 562d2622048be8ac5509d8ccd4b9fd18244fc3a4 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Fri, 12 Jul 2019 06:57:29 +0200
Subject: [PATCH] * test/lisp/apropos-tests.el: New file.

---
 test/lisp/apropos-tests.el | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 test/lisp/apropos-tests.el

diff --git a/test/lisp/apropos-tests.el b/test/lisp/apropos-tests.el
new file mode 100644
index 0000000000..44737d902e
--- /dev/null
+++ b/test/lisp/apropos-tests.el
@@ -0,0 +1,34 @@
+;;; apropos-tests.el --- Tests for apropos.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2019 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'apropos)
+
+(ert-deftest apropos-tests-revert-buffer ()
+  (with-temp-buffer
+    (apropos "foo")
+    (revert-buffer nil t)
+    (should (looking-at "Type RET on a type label"))))
+
+(provide 'apropos-tests)
+;;; apropos-tests.el ends here
-- 
2.21.0

bug#36588: Unable to revert M-x apropos help buffer

[Basil L. Contovounesios]

tags 36588 fixed
close 36588 27.1
quit

Stefan Kangas <stefan@marxist.se> writes:

> Not familiar with this code, so I can't speak to your solution.  I can confirm
> it fixes the problem though.  Thanks for looking at it so promptly.

Thanks, I've now pushed the patch to master[1] and am therefore closing
this report.

[1]: Support reverting Apropos buffers (bug#36588)
4cd41ba8de 2019-08-05 01:19:51 +0300
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=4cd41ba8def704ce3bd2f3806176815fd696fa57

> I've attached a small patch with a test for this.

Thanks, but I'm not sure it's applicable here; see below.

> From 562d2622048be8ac5509d8ccd4b9fd18244fc3a4 Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Fri, 12 Jul 2019 06:57:29 +0200
> Subject: [PATCH] * test/lisp/apropos-tests.el: New file.

[...]

> +(ert-deftest apropos-tests-revert-buffer ()
> +  (with-temp-buffer
> +    (apropos "foo")
> +    (revert-buffer nil t)
> +    (should (looking-at "Type RET on a type label"))))

[Nit: Use looking-at-p unless subsequently using the match data.]

I don't currently see a clean way to test this behaviour
non-interactively, as the apropos command takes over the hard-coded
*Apropos* buffer and can pop up new windows and frames (which also
renders the call to with-temp-buffer a no-op, AFAICT).  This is far too
intrusive for the ERT case.

Alternatives include refactoring apropos.el to make it more testable,
and adding manual tests under test/manual/, but either of these deserves
its own bug report.

Thanks,

-- 
Basil