bug#1948: confusion and bug in dabbrev.el

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

Re: 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

RE: bug#1948: confusion and bug in dabbrev.el

[Drew Adams]

Please don't cross-post to the bugs list and emacs-devel.  Thx.

Re: bug#1948: confusion and bug in dabbrev.el

[Alan Third]

Drew Adams <drew.adams@oracle.com> writes:

> Please don't cross-post to the bugs list and emacs-devel.  Thx.

Apologies. Should I even be posting small patches for bug fixes in
emacs-devel, or should they just go to the bug's list?
-- 
Alan Third

Re: bug#1948: confusion and bug in dabbrev.el

[Eli Zaretskii]

> From: Alan Third <alan@idiocy.org>
> Date: Sat, 30 Jan 2016 11:59:08 +0000
> Cc: emacs-devel@gnu.org
> 
> Should I even be posting small patches for bug fixes in emacs-devel,
> or should they just go to the bug's list?

It is best to post only to the bug address.  All the relevant people
should read bug-gnu-emacs anyway, and if you need to CC someone
specifically, you can always do that while posting there.

Thanks.

RE: bug#1948: confusion and bug in dabbrev.el

[Drew Adams]

> > Please don't cross-post to the bugs list and emacs-devel.  Thx.
> 
> Apologies. Should I even be posting small patches for bug fixes in
> emacs-devel, or should they just go to the bug's list?

Someone will correct me if I'm wrong, but I believe that either
is OK.  I'd suggest the bug list, unless you think that some
general discussion about it might be in order.

Re: 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

Re: 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".)