bug#36516: Wrong dynamic abbrev expansion after space

bug#36516: Wrong dynamic abbrev expansion after space

[Juri Linkov]

0. emacs -Q
1. Type these two lines:
str 1
str 2
2. On the third line type:
 s                      ;; self-insert-command
 t                      ;; self-insert-command
 r                      ;; self-insert-command
 SPC                    ;; self-insert-command
 M-/                    ;; dabbrev-expand
 M-/                    ;; dabbrev-expand
3. The result is correct, the third line is correctly expanded to:
str 1

Now the test case that exposes the error:

0. emacs -Q
1. Type these two lines:
str 1
str 2
2. On the third line type:
 s                      ;; self-insert-command
 M-/                    ;; dabbrev-expand
 SPC                    ;; self-insert-command
 M-/                    ;; dabbrev-expand
 M-/                    ;; dabbrev-expand
3. The word after "str " is expanded to some random word.

bug#36516: Wrong dynamic abbrev expansion after space

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Fri, 05 Jul 2019 21:38:39 +0300
> 
> 0. emacs -Q
> 1. Type these two lines:
> str 1
> str 2
> 2. On the third line type:
>  s                      ;; self-insert-command
>  M-/                    ;; dabbrev-expand
>  SPC                    ;; self-insert-command
>  M-/                    ;; dabbrev-expand
>  M-/                    ;; dabbrev-expand
> 3. The word after "str " is expanded to some random word.

Why did you expect something different?  The first M-/ copies "2", but
the next one after that replaces "2" with some alternative, which
exactly depends on what other buffers you have.  I see no bug here,
you are just expecting from M-/ something it cannot give you.  The
"SPC M-/" trick is already reaching too far out, IMO.

bug#36516: Wrong dynamic abbrev expansion after space

[Juri Linkov]

>> 0. emacs -Q
>> 1. Type these two lines:
>> str 1
>> str 2
>> 2. On the third line type:
>>  s                      ;; self-insert-command
>>  M-/                    ;; dabbrev-expand
>>  SPC                    ;; self-insert-command
>>  M-/                    ;; dabbrev-expand
>>  M-/                    ;; dabbrev-expand
>> 3. The word after "str " is expanded to some random word.
>
> Why did you expect something different?  The first M-/ copies "2", but
> the next one after that replaces "2" with some alternative, which
> exactly depends on what other buffers you have.  I see no bug here,
> you are just expecting from M-/ something it cannot give you.  The
> "SPC M-/" trick is already reaching too far out, IMO.

The example in the first part of my bug report demonstrates that
this useful feature works as documented in the manual.  Also
using ‘C-M-/’ (‘dabbrev-completion’) confirms that it works
as expected by displaying correct completions for both cases.

BTW, while reading the manual, I noticed the documentation bug in
(info "(emacs) Dynamic Abbrevs") that could be fixed by this patch:

diff --git a/doc/emacs/abbrevs.texi b/doc/emacs/abbrevs.texi
index 9c8a280efb..708adbb8a6 100644
--- a/doc/emacs/abbrevs.texi
+++ b/doc/emacs/abbrevs.texi
@@ -465,7 +465,7 @@ Dabbrev Customization
 generally appropriate for expanding symbols in a program source and
 also for human-readable text in many languages, but may not be what
 you want in a text buffer that includes unusual punctuation characters;
-in that case, the value @code{"\\sw"} might produce better results.
+in that case, the value @code{\\sw} might produce better results.
 
 @vindex dabbrev-abbrev-skip-leading-regexp
   In shell scripts and makefiles, a variable name is sometimes prefixed


PS: Also I noticed that the documentation of ‘M-x dissociated-press’
has no references to https://www.gutenberg.org/files/829/829-h/829-h.htm
the text starting with “He then led me to the frame, about the sides,
whereof all his pupils stood in ranks.”  But maybe the device described
in Gulliver's Travels has a different text generation algorithm than
dissociated-press, I'm not sure.

bug#36516: Wrong dynamic abbrev expansion after space

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 36516@debbugs.gnu.org
> Date: Mon, 08 Jul 2019 01:13:13 +0300
> 
> >> 0. emacs -Q
> >> 1. Type these two lines:
> >> str 1
> >> str 2
> >> 2. On the third line type:
> >>  s                      ;; self-insert-command
> >>  M-/                    ;; dabbrev-expand
> >>  SPC                    ;; self-insert-command
> >>  M-/                    ;; dabbrev-expand
> >>  M-/                    ;; dabbrev-expand
> >> 3. The word after "str " is expanded to some random word.
> >
> > Why did you expect something different?  The first M-/ copies "2", but
> > the next one after that replaces "2" with some alternative, which
> > exactly depends on what other buffers you have.  I see no bug here,
> > you are just expecting from M-/ something it cannot give you.  The
> > "SPC M-/" trick is already reaching too far out, IMO.
> 
> The example in the first part of my bug report demonstrates that
> this useful feature works as documented in the manual.

It does?  Please explain how it does, because I don't see it.

bug#36516: Wrong dynamic abbrev expansion after space

[Juri Linkov]

>> The example in the first part of my bug report demonstrates that
>> this useful feature works as documented in the manual.
>
> It does?  Please explain how it does, because I don't see it.

For example:

0. emacs -Q
1. Type these two lines:
str 1
str 2
2. On the third line type:
 s                      ;; self-insert-command
 M-/                    ;; dabbrev-expand
 SPC                    ;; self-insert-command
 C-M-/                  ;; dabbrev-completion

This correctly displays all available completions
“str 1” and “str 2”.  But ‘s M-/ SPC M-/ M-/’ doesn't get
the second completion.

bug#36516: Wrong dynamic abbrev expansion after space

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 36516@debbugs.gnu.org
> Date: Tue, 09 Jul 2019 00:35:44 +0300
> 
> 0. emacs -Q
> 1. Type these two lines:
> str 1
> str 2
> 2. On the third line type:
>  s                      ;; self-insert-command
>  M-/                    ;; dabbrev-expand
>  SPC                    ;; self-insert-command
>  C-M-/                  ;; dabbrev-completion
> 
> This correctly displays all available completions
> “str 1” and “str 2”.  But ‘s M-/ SPC M-/ M-/’ doesn't get
> the second completion.

It does, if you type M-/ a couple more times.

bug#36516: Wrong dynamic abbrev expansion after space

[Noam Postavsky]

Juri Linkov <juri@linkov.net> writes:

> 2. On the third line type:
>  s                      ;; self-insert-command
>  M-/                    ;; dabbrev-expand
>  SPC                    ;; self-insert-command
>  M-/                    ;; dabbrev-expand
>  M-/                    ;; dabbrev-expand
> 3. The word after "str " is expanded to some random word.

It's due to this code in dabbrev.el:

(defun dabbrev-expand (arg)
   ...
        ;; If the user inserts a space after expanding
        ;; and then asks to expand again, always fetch the next word.
        (if (and (eq (preceding-char) ?\s)
                 (markerp dabbrev--last-abbrev-location)
                 (marker-position dabbrev--last-abbrev-location)
                 (= (point) (1+ dabbrev--last-abbrev-location)))
            (progn
              ;; The "abbrev" to expand is just the space.
              (setq abbrev " ")
              ...

This triggers on the second M-/ in your recipe above, which causes the
third M-/ to expand " " instead of "str ".  Not sure what to do about it
though.  Removing this `if' will fix your case, but probably break
others.

bug#36516: Wrong dynamic abbrev expansion after space

[Juri Linkov]

>> 2. On the third line type:
>>  s                      ;; self-insert-command
>>  M-/                    ;; dabbrev-expand
>>  SPC                    ;; self-insert-command
>>  M-/                    ;; dabbrev-expand
>>  M-/                    ;; dabbrev-expand
>> 3. The word after "str " is expanded to some random word.
>
> It's due to this code in dabbrev.el:
>
> (defun dabbrev-expand (arg)
>    ...
>         ;; If the user inserts a space after expanding
>         ;; and then asks to expand again, always fetch the next word.
>         (if (and (eq (preceding-char) ?\s)
>                  (markerp dabbrev--last-abbrev-location)
>                  (marker-position dabbrev--last-abbrev-location)
>                  (= (point) (1+ dabbrev--last-abbrev-location)))
>             (progn
>               ;; The "abbrev" to expand is just the space.
>               (setq abbrev " ")
>               ...
>
> This triggers on the second M-/ in your recipe above, which causes the
> third M-/ to expand " " instead of "str ".  Not sure what to do about it
> though.  Removing this `if' will fix your case, but probably break
> others.

Thanks for investigating this.  Out of curiosity, I checked now
the test directory, and here it is: test/lisp/dabbrev-tests.el -
the already existing test with exactly the same recipe as in my report.
But for some reason this test doesn't fail.

bug#36516: Wrong dynamic abbrev expansion after space

[Noam Postavsky]

Juri Linkov <juri@linkov.net> writes:
>
>  Out of curiosity, I checked now
> the test directory, and here it is: test/lisp/dabbrev-tests.el -
> the already existing test with exactly the same recipe as in my report.
> But for some reason this test doesn't fail.

Having 2 spaces between the tokens and expanding from the middle line
make the difference (not sure why exactly).

bug#36516: Wrong dynamic abbrev expansion after space

[Alan Third]

On Wed, Jul 24, 2019 at 12:15:16AM +0300, Juri Linkov wrote:
> >> 2. On the third line type:
> >>  s                      ;; self-insert-command
> >>  M-/                    ;; dabbrev-expand
> >>  SPC                    ;; self-insert-command
> >>  M-/                    ;; dabbrev-expand
> >>  M-/                    ;; dabbrev-expand
> >> 3. The word after "str " is expanded to some random word.
> >
> > It's due to this code in dabbrev.el:
> >
> > (defun dabbrev-expand (arg)
> >    ...
> >         ;; If the user inserts a space after expanding
> >         ;; and then asks to expand again, always fetch the next word.
> >         (if (and (eq (preceding-char) ?\s)
> >                  (markerp dabbrev--last-abbrev-location)
> >                  (marker-position dabbrev--last-abbrev-location)
> >                  (= (point) (1+ dabbrev--last-abbrev-location)))
> >             (progn
> >               ;; The "abbrev" to expand is just the space.
> >               (setq abbrev " ")
> >               ...
> >
> > This triggers on the second M-/ in your recipe above, which causes the
> > third M-/ to expand " " instead of "str ".  Not sure what to do about it
> > though.  Removing this `if' will fix your case, but probably break
> > others.
> 
> Thanks for investigating this.  Out of curiosity, I checked now
> the test directory, and here it is: test/lisp/dabbrev-tests.el -
> the already existing test with exactly the same recipe as in my report.
> But for some reason this test doesn't fail.

I remembered writing that test and checked previously. It’s not
exactly the same recipe. That one was where there were two spaces and
subsequent calls to dabbrev-expand would replace the wrong text with
the correct expansion, not replace the correct text with the wrong
expansion.

I hope that makes sense... A bit more info here:

    https://lists.gnu.org/archive/html/emacs-devel/2016-01/msg01721.html
-- 
Alan Third

bug#36516: Wrong dynamic abbrev expansion after space

[Alan Third]

On Tue, Jul 09, 2019 at 12:35:44AM +0300, Juri Linkov wrote:
> >> The example in the first part of my bug report demonstrates that
> >> this useful feature works as documented in the manual.
> >
> > It does?  Please explain how it does, because I don't see it.
> 
> For example:
> 
> 0. emacs -Q
> 1. Type these two lines:
> str 1
> str 2
> 2. On the third line type:
>  s                      ;; self-insert-command
>  M-/                    ;; dabbrev-expand
>  SPC                    ;; self-insert-command
>  C-M-/                  ;; dabbrev-completion
> 
> This correctly displays all available completions
> “str 1” and “str 2”.  But ‘s M-/ SPC M-/ M-/’ doesn't get
> the second completion.

’s M-/ SPC M-/’ doesn’t do a normal completion, it does this (from 
the manual):

    After you have expanded a dynamic abbrev, you can copy additional
    words that follow the expansion in its original context. Simply type
    <SPC> M-/ for each additional word you want to copy. The spacing and
    punctuation between words is copied along with the words.

I suppose that subsequent M-/’s are undocumented, but the current
behaviour is, IMO, definitely confusing as it seems to search other
buffers for completions even though there are valid completions in the
current buffer.

-- 
Alan Third

bug#36516: Wrong dynamic abbrev expansion after space

[Juri Linkov]

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

>> 0. emacs -Q
>> 1. Type these two lines:
>> str 1
>> str 2
>> 2. On the third line type:
>>  s                      ;; self-insert-command
>>  M-/                    ;; dabbrev-expand
>>  SPC                    ;; self-insert-command
>>  C-M-/                  ;; dabbrev-completion
>>
>> This correctly displays all available completions
>> “str 1” and “str 2”.  But ‘s M-/ SPC M-/ M-/’ doesn't get
>> the second completion.
>
> ’s M-/ SPC M-/’ doesn’t do a normal completion, it does this (from 
> the manual):
>
>     After you have expanded a dynamic abbrev, you can copy additional
>     words that follow the expansion in its original context. Simply type
>     <SPC> M-/ for each additional word you want to copy. The spacing and
>     punctuation between words is copied along with the words.

Oh, this special-casing unfortunately breaks the natural workflow.

> I suppose that subsequent M-/’s are undocumented, but the current
> behaviour is, IMO, definitely confusing as it seems to search other
> buffers for completions even though there are valid completions in the
> current buffer.

Maybe it's possible to break out of the vicious cycle of ‘SPC M-/’
repetitions by making ‘SPC M-/’ look like a normal expansion, e.g.:


[-- Attachment #2: dabbrev.patch --]
[-- Type: text/x-diff, Size: 4293 bytes --]

diff --git a/test/lisp/dabbrev-tests.el b/test/lisp/dabbrev-tests.el
index a6ab2e7201..39fd37db1d 100644
--- a/test/lisp/dabbrev-tests.el
+++ b/test/lisp/dabbrev-tests.el
@@ -40,3 +40,27 @@
      ;; M-/ SPC M-/ M-/
      (execute-kbd-macro "\257 \257\257"))
    (should (string= (buffer-string) "ab  x\nab y\nab  y"))))
+
+(ert-deftest dabbrev-with-space-word ()
+  "Test for bug#36516.
+Check that the ‘<SPC> M-/’ feature still works as documented
+in `(emacs) Dynamic Abbrevs'."
+  (with-temp-buffer
+   (insert "sub a b\nstr a c\ns")
+   (save-window-excursion
+     (set-window-buffer nil (current-buffer))
+     ;; M-/ M-/ SPC M-/ SPC M-/
+     (execute-kbd-macro "\257\257 \257 \257"))
+   (should (string= (buffer-string) "sub a b\nstr a c\nsub a b"))))
+
+(ert-deftest dabbrev-without-space-word ()
+  "Test for bug#36516.
+Check that ‘<SPC> M-/ M-/’ doesn't conflict with the ‘<SPC> M-/’ feature
+documented in `(emacs) Dynamic Abbrevs'."
+  (with-temp-buffer
+   (insert "str 1\nstr 2\ns")
+   (save-window-excursion
+     (set-window-buffer nil (current-buffer))
+     ;; M-/ SPC M-/ M-/
+     (execute-kbd-macro "\257 \257\257"))
+   (should (string= (buffer-string) "str 1\nstr 2\nstr 1"))))
diff --git a/lisp/dabbrev.el b/lisp/dabbrev.el
index 23abe7ae16..ffb1b7d819 100644
--- a/lisp/dabbrev.el
+++ b/lisp/dabbrev.el
@@ -472,43 +472,35 @@ dabbrev-expand
 		 (markerp dabbrev--last-abbrev-location)
 		 (marker-position dabbrev--last-abbrev-location)
 		 (= (point) (1+ dabbrev--last-abbrev-location)))
-	    (progn
-	      ;; The "abbrev" to expand is just the space.
-	      (setq abbrev " ")
-	      (save-excursion
-		(save-restriction
-		  (widen)
-		  (if dabbrev--last-buffer
-		      (set-buffer dabbrev--last-buffer))
-		  ;; Find the end of the last "expansion" word.
-		  (if (or (eq dabbrev--last-direction 1)
-			  (and (eq dabbrev--last-direction 0)
-			       (< dabbrev--last-expansion-location (point))))
-		      (setq dabbrev--last-expansion-location
-			    (+ dabbrev--last-expansion-location
-			       (length dabbrev--last-expansion))))
-		  (goto-char dabbrev--last-expansion-location)
-		  ;; Take the following word, with intermediate separators,
-		  ;; as our expansion this time.
-		  (re-search-forward
-		   (concat "\\(?:" dabbrev--abbrev-char-regexp "\\)+"))
-		  (setq expansion (buffer-substring-no-properties
-				   dabbrev--last-expansion-location (point)))
-
-		  ;; Record the end of this expansion, in case we repeat this.
-		  (setq dabbrev--last-expansion-location (point))))
-	      ;; Indicate that dabbrev--last-expansion-location is
-	      ;; at the end of the expansion.
-	      (setq dabbrev--last-direction -1))
+            (save-excursion
+	      (save-restriction
+	        (widen)
+	        (if dabbrev--last-buffer
+		    (set-buffer dabbrev--last-buffer))
+	        ;; Find the end of the last "expansion" word.
+	        (if (or (eq dabbrev--last-direction 1)
+		        (and (eq dabbrev--last-direction 0)
+			     (< dabbrev--last-expansion-location (point))))
+		    (setq dabbrev--last-expansion-location
+			  (+ dabbrev--last-expansion-location
+			     (length dabbrev--last-expansion))))
+	        (goto-char dabbrev--last-expansion-location)
+	        ;; Take the following word, with intermediate separators,
+	        ;; as our expansion this time.
+	        (re-search-forward
+	         (concat "\\(?:" dabbrev--abbrev-char-regexp "\\)+"))
 
+	        ;; Record the end of this expansion, in case we repeat this.
+	        (setq dabbrev--last-expansion-location (point))))
 	  ;; We have a different abbrev to expand.
-	  (dabbrev--reset-global-variables)
-	  (setq direction (if (null arg)
-			      (if dabbrev-backward-only 1 0)
-			    (prefix-numeric-value arg)))
-	  (setq abbrev (dabbrev--abbrev-at-point))
-	  (setq record-case-pattern t)
-	  (setq old nil)))
+          (dabbrev--reset-global-variables))
+
+	(setq direction (if (null arg)
+			    (if dabbrev-backward-only 1 0)
+			  (prefix-numeric-value arg)))
+	(setq abbrev (dabbrev--abbrev-at-point))
+	(setq record-case-pattern t)
+	(setq old nil))
 
       ;;--------------------------------
       ;; Find the expansion

bug#36516: Wrong dynamic abbrev expansion after space

[Alan Third]

On Sun, Jul 28, 2019 at 11:47:53PM +0300, Juri Linkov wrote:
> > I suppose that subsequent M-/’s are undocumented, but the current
> > behaviour is, IMO, definitely confusing as it seems to search other
> > buffers for completions even though there are valid completions in the
> > current buffer.
> 
> Maybe it's possible to break out of the vicious cycle of ‘SPC M-/’
> repetitions by making ‘SPC M-/’ look like a normal expansion, e.g.:

This looks good to me. The only problem is it breaks the test for
bug#1948. I think it no longer squashes all the spaces.
-- 
Alan Third

bug#36516: Wrong dynamic abbrev expansion after space

[Juri Linkov]

>> Maybe it's possible to break out of the vicious cycle of ‘SPC M-/’
>> repetitions by making ‘SPC M-/’ look like a normal expansion, e.g.:
>
> This looks good to me. The only problem is it breaks the test for
> bug#1948. I think it no longer squashes all the spaces.

Sorry for breaking the test, I'll try to find a fix that will pass all tests.

Meanwhile, I started to rely heavily on consequent calls of M-/ and
today this feature reared its ugly head (so then could it be called
a misfeature).  Here's is the case that raises the error:

0. emacs -Q

1. Type these two lines:
str 1
str 2

(optionally set debug-on-error to t)

2. On the third line type:
 s			;; self-insert-command
 M-/			;; dabbrev-expand
 SPC			;; self-insert-command
 M-/			;; dabbrev-expand
 M-/			;; dabbrev-expand
 <backspace>		;; backward-delete-char-untabify
 <backspace>		;; backward-delete-char-untabify
 <backspace>		;; backward-delete-char-untabify
 <backspace>		;; backward-delete-char-untabify
 <backspace>		;; backward-delete-char-untabify

The amount of <backspace> keypresses depends on the length
of the random word expanded, so the intention is to delete
that word and the space character.

 SPC			;; self-insert-command
 M-/			;; dabbrev-expand

and the bug is revealed that signals this error:

Debugger entered--Lisp error: (search-failed "\\(?:\\sw\\|\\s_\\)+")
  re-search-forward("\\(?:\\sw\\|\\s_\\)+")
  dabbrev-expand(nil)
  funcall-interactively(dabbrev-expand nil)
  call-interactively(dabbrev-expand nil nil)
  command-execute(dabbrev-expand)

bug#36516: Wrong dynamic abbrev expansion after space

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

>>> Maybe it's possible to break out of the vicious cycle of ‘SPC M-/’
>>> repetitions by making ‘SPC M-/’ look like a normal expansion, e.g.:
>>
>> This looks good to me. The only problem is it breaks the test for
>> bug#1948. I think it no longer squashes all the spaces.
>
> Sorry for breaking the test, I'll try to find a fix that will pass all tests.

This was a year ago -- Juri, did you get any further with this?

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

bug#36516: Wrong dynamic abbrev expansion after space

[Juri Linkov]

>>>> Maybe it's possible to break out of the vicious cycle of ‘SPC M-/’
>>>> repetitions by making ‘SPC M-/’ look like a normal expansion, e.g.:
>>>
>>> This looks good to me. The only problem is it breaks the test for
>>> bug#1948. I think it no longer squashes all the spaces.
>>
>> Sorry for breaking the test, I'll try to find a fix that will pass all tests.
>
> This was a year ago -- Juri, did you get any further with this?

No progress, sorry.  And no ideas how to fix this.