Subject: 31.0.50; Problems with dabbrev-expand
From: Stephen Berman <[email-masked]>
To: [email-masked]
Date: Tue, 29 Oct 2024 18:06:18 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

If an expansion found by dabbrev-expand is located in a different buffer
than the buffer from which dabbrev-expand was invoked, the following
kinds of problems can occur:

First problem:
0. emacs -Q
1. Visit (for example) the INSTALL file in the Emacs source tree.
2. Switch to a new buffer (either empty or at least not containing any
   text that can be a dabbrev expansion), type into it the string "Ind"
   and then type `M-/' (dabbrev-expand), which expands "Ind" to "Indic".
3. Type `SPC M-/'
=> This does not expand the buffer substring "Indic and" but to
   "Indicmacs", which is not a string in any live buffer.

A variant of this problem:
After switching to a new buffer replace the rest of step 2 by the
following:
2a. Type `Indic SPC M-/'.  This correctly expands to "Indic and".
3. Type `SPC M-/'
=> This does not expand the buffer substring "Indic and Khmer" but to
   "Indic and Installation", which is not a string in any live buffer.

Another variant of this problem:
After visiting INSTALL, type `M-x find-library RET dabbrev RET' to visit
the dabbrev.el source file, then switch to a new buffer, e.g. `C-x b a'
and type `M-x emacs-lisp-mode'.  Now continue:
2b. Type `Ind SPC M-/'.  This expands to "Indicate" and displays the
    message "Expansion found in ‘dabbrev.el’".
3b. Type `M-/' to replace the expansion.  This now expands to "Indic"
    and displays the message "Expansion found in ‘INSTALL’".
4. Type `SPC M-/'
=> This expands as above to "Indicmacs", which is not a string in any
   live buffer.

Second problem:
0. emacs -Q
1. Visit (for example) the INSTALL file in the Emacs source tree.
2. Type `C-s Ind M-a C-SPC M-} C-x n n' and at the prompt type SPC to
   narrow the buffer to the paragraph containing "Indic".
3. As in step 2 of the first problem, switch to a new buffer, e.g. `C-x
   b a' and then type `Ind M-/'.  As above, this expands to "Indic".
4. Type `SPC M-/'.
=> This raises the error "Args out of range: #<buffer INSTALL>, #<marker
   at 6 in a>, 4886".

These problems also occur if, instead of invoking dabbrev-expand from an
ordinary buffer, you invoke a command that accepts text input in the
minibuffer, e.g `M-%', and then invoke dabbrev-expand from there:

- For the first case, typing `M-% Ind M-/' in the minibuffer expands to
  "Indic", but now typing `SPC M-/' expands the string to "Indicon",
  which is not a string in INSTALL or any other live buffer.

- For the first variant of the first case, typing `M-% Indic SPC M-/' in
  the minibuffer correctly expands to "Indic and", but typing `SPC M-/'
  again expands the string to "Indic anduide", which is not a string in
  INSTALL or any other live buffer.

- For the second variant of the first case, after visiting INSTALL and
  dabbrev.el, in the latter buffer first typing `M-% Ind M-/' in the
  minibuffer expands to "Indicate", then typing `M-/' replaces the
  expansion with "Indic", and now typing `SPC M-/' expands string to
  "Indicon", which is not a string in any live buffer.

- For the second case, after narrowing buffer INSTALL as above, typing
  `M-% Ind M-/' expands to "Indic" and then typing SPC M-/' results in
  the error "Args out of range: #<buffer INSTALL>, #<marker at 31 in
  *Minibuf-1*>, 4886".

(For the record, I'm pretty sure I've encountered the first problem and
its variants in my normal use of Emacs, though I can't recall specific
instances.  I don't recall encountering problems with dabbrev-expand
when using query-replace (but I probably seldom use dabbrev-expand with
query-replace); however, a number of times I have hit the
args-out-of-range error on invoking dabbrev-expand in the minibuffer in
todo-mode, and that is what motivated me to debug the problem, which led
to finding the other dabbrev-expand problems and variants.  I used
query-replace instead of todo-mode in the above recipes to keep them
simpler by not having to include the additional steps needed to use
todo-mode.)

From my debugging of these problems, I think they arise because the code
in dabbrev-expand that sets up looking for an expansion (either
directly, or after a space, or as a replacement for the current
expansion) wrongly using positions in the buffer from which
dabbrev-expand was invoked instead of the buffer in which the expansion
is found.  The attached patch fixes these problems, according to my
tests.  The patch includes comments justifying or at least motivating
the changes.

The patch also includes new tests for dabbrev-tests.el, both for the
cases described above as well as for the same operations but confined to
a single buffer, for which the current code yields correct results (the
tests for the problematic cases fail with the current code, of course).
After applying my fix to dabbrev.el all tests pass with `make check' as
well as in a batch run.  However, the four "other-buffer" tests fail
when manually running ert in the dabbrev-tests.el buffer, and I don't
know why; from the ERT output it looks like dabbrev-expand is treating
the test file buffer as the current buffer when manually running these
tests, but I don't see how that happens.  Another unresolved issue is
with the test dabbrev-expand-test-minibuffer-3: I could only get this to
pass by invoking `dabbrev--reset-global-variables', and I don't know why
that is needed here but not in the other tests.  (The patch omits the
two new resource files used in the tests, which are copies of parts of
INSTALL and dabbrev.el from the Emacs sources).

In GNU Emacs 31.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.43, cairo version 1.18.2) of 2024-10-29 built on strobelfssd
Repository revision: 9aa186592634212fcdb2dbafdfd0c52a2475ba96
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101013
System Description: Linux From Scratch r12.2-17-systemd

Configured using:
 'configure -C 'CFLAGS=-Og -g3' PKG_CONFIG_PATH=/opt/qt6/lib/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
LCMS2 LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG
RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER
WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB
diff --git a/lisp/dabbrev.el b/lisp/dabbrev.el
index 7b6cbb78cef..f9f9fc8bb27 100644
--- a/lisp/dabbrev.el
+++ b/lisp/dabbrev.el
@@ -464,8 +464,21 @@ dabbrev-expand

 See also `dabbrev-abbrev-char-regexp' and \\[dabbrev-completion]."
   (interactive "*P")
-  (let (abbrev record-case-pattern
-	       expansion old direction (orig-point (point)))
+  ;; There are three possible sources of the expansion, which we need to
+  ;; check in a specific order:
+  (let ((buf (cond ((window-minibuffer-p)
+                    ;; If we invoked dabbrev-expand in the minibuffer,
+                    ;; this is the buffer from which we entered the
+                    ;; minibuffer.
+                    (window-buffer (get-mru-window)))
+                   ;; Otherwise, if we found the expansion in another
+                   ;; buffer, use that buffer for further expansions.
+                   (dabbrev--last-buffer-found dabbrev--last-buffer-found)
+                   ;; Otherwise, use the buffer where we invoked
+                   ;; dabbrev-expand.
+                   (t (current-buffer))))
+        abbrev record-case-pattern expansion old direction
+        (orig-point (point)))
     ;; abbrev -- the abbrev to expand
     ;; expansion -- the expansion found (eventually) or nil until then
     ;; old -- the text currently in the buffer
@@ -480,6 +493,7 @@ dabbrev-expand
 			   (point)))))
 	  ;; Find a different expansion for the same abbrev as last time.
 	  (progn
+            (setq dabbrev--last-buffer-found nil)
 	    (setq abbrev dabbrev--last-abbreviation)
 	    (setq old dabbrev--last-expansion)
 	    (setq direction dabbrev--last-direction))
@@ -488,7 +502,14 @@ dabbrev-expand
 	(if (and (eq (preceding-char) ?\s)
 		 (markerp dabbrev--last-abbrev-location)
 		 (marker-position dabbrev--last-abbrev-location)
-		 (= (point) (1+ dabbrev--last-abbrev-location)))
+                 ;; Comparing with point only makes sense in the buffer
+                 ;; where we called dabbrev-exand, but if that differs
+                 ;; from the buffer containing the expansion, we want to
+                 ;; get the next word in the latter buffer, so we skip
+                 ;; the comparison.
+		 (if (eq buf (current-buffer))
+                     (= (point) (1+ dabbrev--last-abbrev-location))
+                   t))
 	    (progn
 	      ;; The "abbrev" to expand is just the space.
 	      (setq abbrev " ")
@@ -549,29 +570,43 @@ dabbrev-expand
                   (if old " further" "") abbrev))
      (t
       (if (not (or (eq dabbrev--last-buffer dabbrev--last-buffer-found)
-		   (minibuffer-window-active-p (selected-window))))
+                   ;; If we are in the minibuffer and an expansion has
+                   ;; been found but dabbrev--last-buffer-found is not
+                   ;; yet set, we need to set it now.
+                   (and dabbrev--last-buffer-found
+		        (minibuffer-window-active-p (selected-window)))))
 	  (progn
             (when (buffer-name dabbrev--last-buffer)
 	      (message "Expansion found in `%s'"
 		       (buffer-name dabbrev--last-buffer)))
 	    (setq dabbrev--last-buffer-found dabbrev--last-buffer))
 	(message nil))
-      (if (and (or (eq (current-buffer) dabbrev--last-buffer)
-		   (null dabbrev--last-buffer)
-                   (buffer-live-p dabbrev--last-buffer))
-	       (numberp dabbrev--last-expansion-location)
-	       (and (> dabbrev--last-expansion-location (point))))
-	  (setq dabbrev--last-expansion-location
-		(copy-marker dabbrev--last-expansion-location)))
+      ;; To get correct further expansions we have to be sure to use the
+      ;; buffer containing the already found expansions.
+      (when dabbrev--last-buffer-found
+        (setq buf dabbrev--last-buffer-found))
+      ;; If the buffer where we called dabbrev-expand differs from the
+      ;; buffer containing the expansion, make sure copy-marker is
+      ;; called in the latter buffer.
+      (with-current-buffer buf
+        (if (and (or (eq (current-buffer) dabbrev--last-buffer)
+		     (null dabbrev--last-buffer)
+                     (buffer-live-p dabbrev--last-buffer))
+	         (numberp dabbrev--last-expansion-location)
+	         (and (> dabbrev--last-expansion-location (point))))
+	    (setq dabbrev--last-expansion-location
+		  (copy-marker dabbrev--last-expansion-location))))
       ;; Success: stick it in and return.
       (setq buffer-undo-list (cons orig-point buffer-undo-list))
       (setq expansion (dabbrev--substitute-expansion old abbrev expansion
                                                      record-case-pattern))

-      ;; Save state for re-expand.
-      (setq dabbrev--last-expansion expansion)
-      (setq dabbrev--last-abbreviation abbrev)
-      (setq dabbrev--last-abbrev-location (point-marker))))))
+      ;; Save state for re-expand (making sure it's the state of the
+      ;; buffer containing the already found expansions).
+      (with-current-buffer buf
+        (setq dabbrev--last-expansion expansion)
+        (setq dabbrev--last-abbreviation abbrev)
+        (setq dabbrev--last-abbrev-location (point-marker)))))))

 ;;----------------------------------------------------------------
 ;; Local functions
diff --git a/test/lisp/dabbrev-tests.el b/test/lisp/dabbrev-tests.el
index c7574403949..987106aa5af 100644
--- a/test/lisp/dabbrev-tests.el
+++ b/test/lisp/dabbrev-tests.el
@@ -25,6 +25,7 @@
 ;;; Code:

 (require 'ert)
+(require 'ert-x)
 (require 'dabbrev)

 (ert-deftest dabbrev-expand-test ()
@@ -68,4 +69,210 @@ dabbrev-completion-test-with-argument
         (execute-kbd-macro (kbd "C-u C-u C-M-/")))
       (should (string= (buffer-string) "abc\na")))))

+(defmacro with-dabbrev-test (&rest body)
+  "Set up an isolated `dabbrev' test environment."
+  (declare (debug (body)))
+  `(ert-with-temp-directory dabbrev-test-home
+     (let* (;; Since we change HOME, clear this to avoid a conflict
+            ;; e.g. if Emacs runs within the user's home directory.
+            (abbreviated-home-dir nil)
+            (process-environment (cons (format "HOME=%s" dabbrev-test-home)
+                                       process-environment))
+            (dabbrev-directory (ert-resource-directory)))
+       (unwind-protect
+           (progn ,@body)
+         ;; Restore pre-test-run state of test files.
+         (dolist (f (directory-files dabbrev-directory))
+           (let ((buf (get-file-buffer f)))
+             (when buf
+               (with-current-buffer buf
+                 (restore-buffer-modified-p nil)
+                 (kill-buffer)))))
+         (dabbrev--reset-global-variables)))))
+
+(ert-deftest dabbrev-expand-test-same-buffer-1 ()
+  "Test expanding a string twice within a single buffer.
+The first expansion should expand the input (a prefix-string) to a
+string in the buffer containing no whitespace character, the second
+expansion, after adding a space to the first expansion, should extend
+the string with the following string in the buffer up to the next
+whitespace character."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (goto-char (point-max))
+   (terpri)
+   (execute-kbd-macro (kbd "Ind M-/"))
+   (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic"))
+   (execute-kbd-macro (kbd "SPC M-/"))
+   (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic and"))))
+
+(ert-deftest dabbrev-expand-test-same-buffer-2 ()
+  "Test expanding a string plus space twice within a single buffer.
+Each expansion should extend the string with the following string in the
+buffer up to the next whitespace character."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (goto-char (point-max))
+   (terpri)
+   (execute-kbd-macro (kbd "Indic SPC M-/"))
+   (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic and"))
+   (execute-kbd-macro (kbd "SPC M-/"))
+   (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic and Khmer"))))
+
+(ert-deftest dabbrev-expand-test-same-buffer-3 ()
+  "Test replacing an expansion within a single buffer."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (goto-char (point-max))
+   (terpri)
+   (insert-file-contents (ert-resource-file "dabbrev-expand.el"))
+   (goto-char (point-max))
+   (terpri)
+   (execute-kbd-macro (kbd "Ind M-/"))
+   (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indicate"))
+   (kill-whole-line)
+   (execute-kbd-macro (kbd "Ind M-/ M-/"))
+   (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic"))
+   (execute-kbd-macro (kbd "SPC M-/"))
+   (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic and"))))
+
+(ert-deftest dabbrev-expand-test-same-buffer-4 ()
+  "Test expanding a string in a narrowed-region."
+  (with-dabbrev-test
+   (let (disabled-command-function) ; Enable narrow-to-region.
+     (find-file (ert-resource-file "INSTALL_BEGIN"))
+     (goto-char (point-min))
+     (execute-kbd-macro (kbd "C-s Ind M-a C-SPC M-} C-x n n"))
+     (goto-char (point-max))
+     (terpri)
+     (execute-kbd-macro (kbd "Ind M-/"))
+     (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic"))
+     (execute-kbd-macro (kbd "SPC M-/"))
+     (should (string= (buffer-substring (pos-bol) (pos-eol)) "Indic and")))))
+
+(ert-deftest dabbrev-expand-test-other-buffer-1 ()
+  "Test expanding a prefix string to a string from another buffer."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (switch-to-buffer (get-buffer-create "a" t))
+   (execute-kbd-macro (kbd "Ind M-/"))
+   (should (string= (buffer-string) "Indic"))
+   (execute-kbd-macro (kbd "SPC M-/"))
+   (should (string= (buffer-string) "Indic and"))
+   (kill-buffer "a")))
+
+(ert-deftest dabbrev-expand-test-other-buffer-2 ()
+  "Test expanding a string + space to a string from another buffer."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (switch-to-buffer (get-buffer-create "a" t))
+   (execute-kbd-macro (kbd "Indic SPC M-/"))
+   (should (string= (buffer-string) "Indic and"))
+   (execute-kbd-macro (kbd "SPC M-/"))
+   (should (string= (buffer-string) "Indic and Khmer"))
+   (kill-buffer "a")))
+
+(ert-deftest dabbrev-expand-test-other-buffer-3 ()
+  "Test replacing an expansion with three different buffers.
+A prefix string in a buffer should find the first expansion in a
+different buffer and then find a replacement expansion is yet another
+buffer."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (find-file (ert-resource-file "dabbrev-expand.el"))
+   (switch-to-buffer (get-buffer-create "a" t))
+   (emacs-lisp-mode)
+   (execute-kbd-macro (kbd "Ind M-/"))
+   (should (string= (buffer-string) "Indicate"))
+   (erase-buffer)
+   (execute-kbd-macro (kbd "Ind M-/ M-/"))
+   (should (string= (buffer-string) "Indic"))
+   (execute-kbd-macro (kbd "SPC M-/"))
+   (should (string= (buffer-string) "Indic and"))
+   (kill-buffer "a")))
+
+(ert-deftest dabbrev-expand-test-other-buffer-4 ()
+  "Test expanding a string using another narrowed buffer."
+  (with-dabbrev-test
+   (let (disabled-command-function) ; Enable narrow-to-region.
+     (find-file (ert-resource-file "INSTALL_BEGIN"))
+     (goto-char (point-min))
+     (execute-kbd-macro (kbd "C-s Ind M-a C-SPC M-} C-x n n"))
+     (switch-to-buffer (get-buffer-create "a" t))
+     (execute-kbd-macro (kbd "Ind M-/"))
+     (should (string= (buffer-string) "Indic"))
+     (execute-kbd-macro (kbd "SPC M-/"))
+     (should (string= (buffer-string) "Indic and"))
+     (kill-buffer "a"))))
+
+(ert-deftest dabbrev-expand-test-minibuffer-1 ()
+  "Test expanding a prefix string twice in the minibuffer.
+Both expansions should come from the buffer from which the minibuffer
+was entered."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (with-selected-window (minibuffer-window)
+     (insert "Ind")
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indic"))
+     (insert " ")
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indic and"))
+     (delete-minibuffer-contents))))
+
+(ert-deftest dabbrev-expand-test-minibuffer-2 ()
+  "Test expanding a string + space in the minibuffer.
+The expansions should come from the buffer from which the minibuffer was
+entered."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (with-selected-window (minibuffer-window)
+     (insert "Indic ")
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indic and"))
+     (insert " ")
+     (dabbrev-expand nil)
+     (should (string= (buffer-string) "Indic and Khmer"))
+     (delete-minibuffer-contents))))
+
+;; FIXME: Why is dabbrev--reset-global-variables needed here?
+(ert-deftest dabbrev-expand-test-minibuffer-3 ()
+  "Test replacing an expansion in the minibuffer using two buffers.
+The first expansion should befound in the buffer from which the
+minibuffer was entered, the replacement should found in another buffer."
+  (with-dabbrev-test
+   (find-file (ert-resource-file "INSTALL_BEGIN"))
+   (find-file (ert-resource-file "dabbrev-expand.el"))
+   (with-selected-window (minibuffer-window)
+     (insert "Ind")
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indicate"))
+     (kill-whole-line)
+     (dabbrev--reset-global-variables)
+     (insert "Ind")
+     (dabbrev-expand nil)
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indic"))
+     (dabbrev--reset-global-variables)
+     (insert " ")
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indic and"))
+     (delete-minibuffer-contents))))
+
+(ert-deftest dabbrev-expand-test-minibuffer-4 ()
+  "Test expansion in the minibuffer using another narrowed buffer."
+  (with-dabbrev-test
+   (let (disabled-command-function) ; Enable narrow-to-region.
+     (find-file (ert-resource-file "INSTALL_BEGIN"))
+     (goto-char (point-min))
+     (execute-kbd-macro (kbd "C-s Ind M-a C-SPC M-} C-x n n")))
+   (with-selected-window (minibuffer-window)
+     (insert "Ind")
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indic"))
+     (insert " ")
+     (dabbrev-expand nil)
+     (should (string= (minibuffer-contents) "Indic and"))
+     (delete-minibuffer-contents))))
+
 ;;; dabbrev-tests.el ends here