bug#1948: confusion and bug in dabbrev.el

bug#1948: confusion and bug in dabbrev.el

[Peter Tury]

Hi,

1)
in `dabbrev--substitute-expansion' (in dabbrev.el), `t' value of
variable `dabbrev-eliminate-newlines' causes elimination of spaces
(and tabs) as well. Thus its name and its functionality are not really
compatible. In the best case it should have a better name
(`dabbrev-eliminate-whitespace'??), but as a minimum, its
documentation should be improved and mention elimination of tabs and
spaces as well, I think.

Moreover, contrary to its documentation again, it doesn't "converts
them [=newlines] to spaces": instead it just removes them. So its
documentation should be fixed here as well in my opinion.

2)
However, the more serious problem is that it "forgets" such
eliminations when it searches for `old' expansion in case of repeated
calls of  `dabbrev-expand' (i.e. when the check "(eq last-command
this-command)" in this function results t) what results in weird text
in some cases. To check this:

* customize `dabbrev-abbrev-char-regexp' to be a period (.) Thus
dabbrev-expand will replace whole lines.

* type these two lines into a buffer (with double spaces between the words!):
one  tt
one  ww

* then type "on" in the next line and call `dabbrev-expand' two times.
Your buffer should look like this at the end:
one  tt
one  ww
one tt
But instead you will get this:
one  tt
one tt
one ww
What seems to be a complete mess.

As I see the key here is the search for `old' at the end of
`dabbrev--substitute-expansion' (`old' is the first parameter of this
function). If I replace the following original code:
"    (if old
	(save-excursion
	  (search-backward old))"
to be
"    (if old
	;; Convert whitespace to single spaces.
	(progn
	(if dabbrev-eliminate-newlines
	    (let ((pos
		   (if (equal abbrev " ") 0 (length abbrev))))
	      ;; If ABBREV is real, search after the end of it.
	      ;; If ABBREV is space and we are copying successive words,
	      ;; search starting at the front.
	      (while (string-match "[\n \t]+" old pos)
		(setq pos (1+ (match-beginning 0)))
		(setq old (replace-match " " nil nil old)))))
	(save-excursion
	  (search-backward old)))"
my test (above) works well. Here I simply copy-pasted (...) original
whitespace-elimination code from few lines above, so this is clearly
not a nice solution: it just shows a way of fixing the broken
functionality.

(I "reported" this problem in gnu.emacs.help with the subject "strange
dabbrev for whole lines and double spaces in cvs Emacs 23" on
Jan.8.2009)

Could you please fix these bugs in dabbrev.el.

Thanks,
P

bug#1948: confusion and bug in dabbrev.el

[Chong Yidong]

Hi Lars, could you take a look at this bug report for dabbrev?  Thanks.

Peter Tury <tury.peter@gmail.com> wrote:

> 1) in `dabbrev--substitute-expansion' (in dabbrev.el), `t' value of
> variable `dabbrev-eliminate-newlines' causes elimination of spaces
> (and tabs) as well. Thus its name and its functionality are not really
> compatible. In the best case it should have a better name
> (`dabbrev-eliminate-whitespace'??), but as a minimum, its
> documentation should be improved and mention elimination of tabs and
> spaces as well, I think.

> Moreover, contrary to its documentation again, it doesn't "converts
> them [=newlines] to spaces": instead it just removes them. So its
> documentation should be fixed here as well in my opinion.

> 2) However, the more serious problem is that it "forgets" such
> eliminations when it searches for `old' expansion in case of repeated
> calls of `dabbrev-expand' (i.e. when the check "(eq last-command
> this-command)" in this function results t) what results in weird text
> in some cases. To check this:

> * customize `dabbrev-abbrev-char-regexp' to be a period (.) Thus
> dabbrev-expand will replace whole lines.

> * type these two lines into a buffer (with double spaces between the
>   words!):
> one  tt
> one  ww

> * then type "on" in the next line and call `dabbrev-expand' two times.
> Your buffer should look like this at the end:
> one  tt
> one  ww
> one tt
> But instead you will get this:
> one  tt
> one tt
> one ww
> What seems to be a complete mess.

> As I see the key here is the search for `old' at the end of
> `dabbrev--substitute-expansion' (`old' is the first parameter of this
> function). If I replace the following original code:
> "    (if old
>      (save-excursion
>        (search-backward old))"
> to be
> "    (if old
>      ;; Convert whitespace to single spaces.
>      (progn
>      (if dabbrev-eliminate-newlines
>          (let ((pos
>                  (if (equal abbrev " ") 0 (length abbrev))))
>                        ;; If ABBREV is real, search after the end of it.
>                              ;; If ABBREV is space and we are copying
>                              successive words,
>                                    ;; search starting at the front.
>                                          (while (string-match "[\n \t]+"
>                                          old pos)
>                                              (setq pos (1+
>                                              (match-beginning 0)))
>                                                               (setq old
>                                                               (replace-match
>                                                               " " nil
>                                                               nil
>                                                               old)))))
>                                                               (save-excursion
>                                                                 (search-backward
>                                                                 old)))"
> my test (above) works well. Here I simply copy-pasted (...) original
> whitespace-elimination code from few lines above, so this is clearly
> not a nice solution: it just shows a way of fixing the broken
> functionality.

> (I "reported" this problem in gnu.emacs.help with the subject "strange
> dabbrev for whole lines and double spaces in cvs Emacs 23" on
> Jan.8.2009)

bug#1948: confusion and bug in dabbrev.el

[Fred]

Follow-up to http://debbugs.gnu.org/cgi/bugreport.cgi?bug=1948

I seem to be experiencing a bad version of this bug. In some instances,
dabbrev seems to feel like not only inserting text at point (which is
expected) but also modifying some text in other parts of the buffer. 
For
some reason, TCL code seems to trigger the bug quite easily. To see 
what
I experienced:

Save the following as blah.tcl:

====== beginning of blah.tcl
proc check { topology } {
     set network [ blahblah ]
     preconfig $topology $networks
     do_check  $topology $networks
}

# Apply the default pre-config steps for each device
proc preconfig { topology networks } {
     foreach node [ keylkeys topology ] {
	set config [ get_config -device $node -type pre ]
	apply_config $node $config
     }
}

# do the actual check
proc do_check {
====== end of blah.tcl

Start `emacs -Q', and either look at the `view-lossage' output included
below, or in plain English:

- Open blah.tcl
- Put the point after the opening brace of do_check;
- Insert `SPC top' and complete with `dabbrev-expand' (M-/) to get
   `topology';
- Insert `SPC' and try to complete (M-/) until you get `topology 
network'.

Expected:

After a round or two, dabbrev inserts networks, based on the prototype
of `preconfig'. At worse, it inserts rubbish and I have to give a 
longer
hint to dabbrev.

Actual result:

Some text after the last occurence of `topology' is replaced, point has
been moved, and procedure prototype has not been completed.

With Peter Tury's patch to `dabbrev--substitute-expansion', point does
not seem to start jumping around, replacing text in remote places
(though I don't have the correct replacement, but whatever)


In GNU Emacs 24.2.1 (x86_64-pc-linux-gnu, GTK+ Version 2.24.10)
  of 2012-09-09 on trouble, modified by Debian
Windowing system distributor `The X.Org Foundation', version 
11.0.11203902
Configured using:
  `configure '--build' 'x86_64-linux-gnu' '--build' 'x86_64-linux-gnu'
  '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib'
  '--localstatedir=/var/lib' '--infodir=/usr/share/info'
  '--mandir=/usr/share/man' '--with-pop=yes'
  
'--enable-locallisppath=/etc/emacs24:/etc/emacs:/usr/local/share/emacs/24.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.2/site-lisp:/usr/share/emacs/site-lisp'
  '--with-crt-dir=/usr/lib/x86_64-linux-gnu' '--with-x=yes'
  '--with-x-toolkit=gtk' '--with-toolkit-scroll-bars'
  'build_alias=x86_64-linux-gnu' 'CFLAGS=-g -O2 -fstack-protector
  --param=ssp-buffer-size=4 -Wformat -Werror=format-security -Wall'
  'CPPFLAGS=-D_FORTIFY_SOURCE=2''

Important settings:
   value of $LC_ALL: nil
   value of $LC_COLLATE: nil
   value of $LC_CTYPE: nil
   value of $LC_MESSAGES: nil
   value of $LC_MONETARY: nil
   value of $LC_NUMERIC: nil
   value of $LC_TIME: nil
   value of $LANG: en_GB.UTF-8
   value of $XMODIFIERS: nil
   locale-coding-system: utf-8-unix
   default enable-multibyte-characters: t

Major mode: Tcl

Minor modes in effect:
   tooltip-mode: t
   mouse-wheel-mode: t
   tool-bar-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   blink-cursor-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t
   line-number-mode: t
   transient-mark-mode: t

Recent input:
C-x C-f / t m p / b l a h . t c l <return> C-s d o
_ c C-s C-e SPC t o p M-/ SPC M-/ M-/ C-/ C-/ M-x r
e p o <tab> r t - e m <tab> <return>

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Mark saved where search started
Undo! [2 times]
Making completion list...

Load-path shadows:
/usr/share/emacs/24.2/site-lisp/flim/hex-util hides 
/usr/share/emacs/24.2/lisp/hex-util
/usr/share/emacs/24.2/site-lisp/flim/md4 hides 
/usr/share/emacs/24.2/lisp/md4
/usr/share/emacs/24.2/site-lisp/dictionaries-common/flyspell hides 
/usr/share/emacs/24.2/lisp/textmodes/flyspell
/usr/share/emacs/24.2/site-lisp/dictionaries-common/ispell hides 
/usr/share/emacs/24.2/lisp/textmodes/ispell
/usr/share/emacs/24.2/site-lisp/flim/sasl-ntlm hides 
/usr/share/emacs/24.2/lisp/net/sasl-ntlm
/usr/share/emacs/24.2/site-lisp/flim/sasl-digest hides 
/usr/share/emacs/24.2/lisp/net/sasl-digest
/usr/share/emacs/24.2/site-lisp/flim/hmac-def hides 
/usr/share/emacs/24.2/lisp/net/hmac-def
/usr/share/emacs/24.2/site-lisp/flim/sasl hides 
/usr/share/emacs/24.2/lisp/net/sasl
/usr/share/emacs/24.2/site-lisp/flim/hmac-md5 hides 
/usr/share/emacs/24.2/lisp/net/hmac-md5
/usr/share/emacs/24.2/site-lisp/flim/ntlm hides 
/usr/share/emacs/24.2/lisp/net/ntlm
/usr/share/emacs/24.2/site-lisp/flim/sasl-cram hides 
/usr/share/emacs/24.2/lisp/net/sasl-cram

Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 
mml
mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev
gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils help-mode view dabbrev misearch multi-isearch tcl
easymenu comint regexp-opt ansi-color ring time-date tooltip ediff-hook
vc-hooks lisp-float-type mwheel x-win x-dnd tool-bar dnd fontset image
fringe lisp-mode register page menu-bar rfn-eshadow timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core frame 
cham
georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese hebrew greek romanian slovak czech european ethiopic
indian cyrillic chinese case-table epa-hook jka-cmpr-hook help simple
abbrev minibuffer loaddefs button faces cus-face files text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process dbusbind
dynamic-setting system-font-setting font-render-setting move-toolbar 
gtk
x-toolkit x multi-tty emacs)

-- 
Fred -- http://tar-jx.bz

bug#1948: confusion and bug in dabbrev.el

[Alan J Third]

Peter Tury <tury.peter@gmail.com> writes:

> * type these two lines into a buffer (with double spaces between the words!):
> one  tt
> one  ww
>
> * then type "on" in the next line and call `dabbrev-expand' two times.
> Your buffer should look like this at the end:
> one  tt
> one  ww
> one tt
> But instead you will get this:
> one  tt
> one tt
> one ww

I'm still getting this weird behaviour in 25.1, but the above doesn't
work exactly for me.

Set dabbrev-eliminate-newlines to "t".

Type:

one  tt
one  ww

Then in the next line I have to type:

one 
   ^- space

and M-/ (dabbrev-expand) completes it to:

one ww

Then if I hit M-/ again, point jumps up to the line above and replaces
"ww". Subsequent M-/'s continue to fiddle with the same line.
-- 
Alan Third

bug#1948: confusion and bug in dabbrev.el

[Alan J Third]

Alan J Third <alan@idiocy.org> writes:

> Type:
>
> one  tt
> one  ww
>
> Then in the next line I have to type:
>
> one 
>    ^- space
>
> and M-/ (dabbrev-expand) completes it to:
>
> one ww
>
> Then if I hit M-/ again, point jumps up to the line above and replaces
> "ww". Subsequent M-/'s continue to fiddle with the same line.

I got this slightly wrong. The full set of commands is:

Type:

one  tt
one  ww
on M-/ SPC M-/ M-/

I've written a test, but I'm not sure what to do with it:

(ert-deftest dabbrev-expand-test ()
  "Test for bug#1948.
When DABBREV-ELIMINATE-NEWLINES is non-nil (the default),
repeated calls to DABBREV-EXPAND can result in the source of
first expansion being replaced rather than the destination."
  (with-temp-buffer
   (insert "ab  x\na\nab  y")
   (goto-char 8)
   (save-window-excursion
     (set-window-buffer nil (current-buffer))
     ;; M-/ SPC M-/ M-/
     (execute-kbd-macro "\257 \257\257"))
   (should (string= (buffer-string) "ab  x\nab y\nab  y"))))

I haven't signed the copyright assignment stuff.
-- 
Alan Third

bug#1948: confusion and bug in dabbrev.el

[Alan Third]

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

Peter Tury said:
> However, the more serious problem is that it "forgets" such
> eliminations when it searches for `old' expansion in case of repeated
> calls of  `dabbrev-expand' (i.e. when the check "(eq last-command
> this-command)" in this function results t) what results in weird text
> in some cases.

I think I've got a fix for this.

The problem is that dabbrev--substitute-expansion alters EXPANSION
before using it in the substitution, but dabbrev-expand has no idea that
it's done this, so it assumes the unaltered EXPANSION is still valid and
saves it.

The patch makes dabbrev--substitute-expansion return EXPANSION, and
dabbrev-expand saves that version instead.

I considered making a new function to remove the whitespace and using it
in both dabbrev-expand and dabbrev--substitute-expansion, but returning
the value actually used strikes me as safer, in case anything else is
done to it.


[-- Attachment #2: save actual expansion --]
[-- Type: text/plain, Size: 1470 bytes --]

From 9ef22a911d58f772265bc1fde2608ea135a4dea2 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Sat, 30 Jan 2016 00:38:32 +0000
Subject: [PATCH] Always save the actual expansion

lisp/dabbrev.el (dabbrev--substitute-expansion): Return EXPANSION after
any processing.
lisp/dabbrev.el (dabbrev-expand): Set EXPANSION to the return value of
DABBREV--SUBSTITUTE-EXPANSION.
---
 lisp/dabbrev.el | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lisp/dabbrev.el b/lisp/dabbrev.el
index 3557041..d9f36b1 100644
--- a/lisp/dabbrev.el
+++ b/lisp/dabbrev.el
@@ -546,8 +546,8 @@ dabbrev-expand
 		(copy-marker dabbrev--last-expansion-location)))
       ;; Success: stick it in and return.
       (setq buffer-undo-list (cons orig-point buffer-undo-list))
-      (dabbrev--substitute-expansion old abbrev expansion
-				     record-case-pattern)
+      (setq expansion (dabbrev--substitute-expansion old abbrev expansion
+                                                     record-case-pattern))
 
       ;; Save state for re-expand.
       (setq dabbrev--last-expansion expansion)
@@ -902,7 +902,9 @@ dabbrev--substitute-expansion
     ;; and (2) the replacement itself is all lower case.
     (dabbrev--safe-replace-match expansion
 				 (not use-case-replace)
-				 t)))
+				 t))
+  ;; Return the expansion actually used.
+  expansion)
 
 
 ;;;----------------------------------------------------------------
-- 
2.5.4 (Apple Git-61)


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


-- 
Alan Third

bug#1948: confusion and bug in dabbrev.el

[Lars Ingebrigtsen]

Alan Third <alan@idiocy.org> writes:

> I think I've got a fix for this.
>
> The problem is that dabbrev--substitute-expansion alters EXPANSION
> before using it in the substitution, but dabbrev-expand has no idea that
> it's done this, so it assumes the unaltered EXPANSION is still valid and
> saves it.
>
> The patch makes dabbrev--substitute-expansion return EXPANSION, and
> dabbrev-expand saves that version instead.
>
> I considered making a new function to remove the whitespace and using it
> in both dabbrev-expand and dabbrev--substitute-expansion, but returning
> the value actually used strikes me as safer, in case anything else is
> done to it.

If I understand the code correctly, this looks like a good fix to me.
Could you resubmit it with the test case you sent earlier as one patch?

Also I see that we have a copyright disclaimer on file for your work.
Is that sufficient to accept code into Emacs?  (I'm asking the other
Emacs hackers here...)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#1948: confusion and bug in dabbrev.el

[Glenn Morris]

(Insert standard "why is this on two mailing lists?" blurb here)

Lars Ingebrigtsen wrote:

> Also I see that we have a copyright disclaimer on file for your work.
> Is that sufficient to accept code into Emacs?  (I'm asking the other
> Emacs hackers here...)

A disclaimer is fine, though an assignment is preferred.
In this case, I see an assignment in the file.
Maybe you are looking at the employer disclaimer?

(I usually try grep "First.*Last" rather than just "First Last".)

bug#1948: confusion and bug in dabbrev.el

[Lars Ingebrigtsen]

Glenn Morris <rgm@gnu.org> writes:

> A disclaimer is fine, though an assignment is preferred.
> In this case, I see an assignment in the file.
> Maybe you are looking at the employer disclaimer?

Ah, yes.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#1948: [PATCH] Always save the actual expansion (bug#1948)

[Alan Third]

lisp/dabbrev.el (dabbrev--substitute-expansion): Return EXPANSION after
any processing.
lisp/dabbrev.el (dabbrev-expand): Set EXPANSION to the return value of
DABBREV--SUBSTITUTE-EXPANSION.
test/automated/dabbrev-tests.el (dabbrev-expand-test): Test for bug#1948.
---
 lisp/dabbrev.el                 |  8 +++++---
 test/automated/dabbrev-tests.el | 42 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 3 deletions(-)
 create mode 100644 test/automated/dabbrev-tests.el

diff --git a/lisp/dabbrev.el b/lisp/dabbrev.el
index 3557041..d9f36b1 100644
--- a/lisp/dabbrev.el
+++ b/lisp/dabbrev.el
@@ -546,8 +546,8 @@ dabbrev-expand
 		(copy-marker dabbrev--last-expansion-location)))
       ;; Success: stick it in and return.
       (setq buffer-undo-list (cons orig-point buffer-undo-list))
-      (dabbrev--substitute-expansion old abbrev expansion
-				     record-case-pattern)
+      (setq expansion (dabbrev--substitute-expansion old abbrev expansion
+                                                     record-case-pattern))
 
       ;; Save state for re-expand.
       (setq dabbrev--last-expansion expansion)
@@ -902,7 +902,9 @@ dabbrev--substitute-expansion
     ;; and (2) the replacement itself is all lower case.
     (dabbrev--safe-replace-match expansion
 				 (not use-case-replace)
-				 t)))
+				 t))
+  ;; Return the expansion actually used.
+  expansion)
 
 
 ;;;----------------------------------------------------------------
diff --git a/test/automated/dabbrev-tests.el b/test/automated/dabbrev-tests.el
new file mode 100644
index 0000000..9c7a838
--- /dev/null
+++ b/test/automated/dabbrev-tests.el
@@ -0,0 +1,42 @@
+;;; dabbrev-tests.el --- Test suite for dabbrev.
+
+;; Copyright (C) 2016 Free Software Foundation, Inc.
+
+;; Author: Alan Third <alan@idiocy.org>
+;; Keywords: dabbrev
+
+;; 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 <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'dabbrev)
+
+(ert-deftest dabbrev-expand-test ()
+  "Test for bug#1948.
+When DABBREV-ELIMINATE-NEWLINES is non-nil (the default),
+repeated calls to DABBREV-EXPAND can result in the source of
+first expansion being replaced rather than the destination."
+  (with-temp-buffer
+   (insert "ab  x\na\nab  y")
+   (goto-char 8)
+   (save-window-excursion
+     (set-window-buffer nil (current-buffer))
+     ;; M-/ SPC M-/ M-/
+     (execute-kbd-macro "\257 \257\257"))
+   (should (string= (buffer-string) "ab  x\nab y\nab  y"))))
-- 

Hope I've put the test file in the right place.
-- 
Alan Third

bug#1948: [PATCH] Always save the actual expansion (bug#1948)

[Lars Ingebrigtsen]

Alan Third <alan@idiocy.org> writes:

> lisp/dabbrev.el (dabbrev--substitute-expansion): Return EXPANSION after
> any processing.
> lisp/dabbrev.el (dabbrev-expand): Set EXPANSION to the return value of
> DABBREV--SUBSTITUTE-EXPANSION.
> test/automated/dabbrev-tests.el (dabbrev-expand-test): Test for bug#1948.

Thanks; applied to emacs-25.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no