bug#23127: 25.0.92; Assertion failure when editing history in query-replace in cc-mode

bug#23127: 25.0.92; Assertion failure when editing history in query-replace in cc-mode

[Richard Copley]

In cc-mode, while doing query-replace, edit a previous replacement in
the minibuffer, and insert characters after the " → " separator. The
subsequent emacs-lisp chokes on the text properties of the resulting
string, with an assertion failure.

Recipe starting from 'emacs -Q':

M-x c-mode RET
M-<
C-% a RET b RET !
M-<
C-% M-p C-e <backspace> c RET

==> error "cl--assertion-failed: Assertion failed: (not
(text-property-any (1+ split-pos) length (quote separator) t string))"

The callstack looks like this (except that here I've replaced a NUL with "^@"):

Debugger entered: ((cl-assertion-failed (not (text-property-any (1+
split-pos) length (quote separator) t string)) nil))
  cl--assertion-failed((not (text-property-any (1+ split-pos) length
(quote separator) t string)))
  query-replace--split-string(#("a^@c" 1 2 (display #(" → " 0 3 (face
minibuffer-prompt)) separator t) 2 3 (separator t)))
  query-replace-read-from("Query replace" nil)
  query-replace-read-args("Query replace" nil)
  (let ((common (query-replace-read-args (concat "Query replace" (if
current-prefix-arg (if (eq current-prefix-arg ...) " backward" "
word") "") (if (use-region-p) " in region" "")) nil))) (list (nth 0
common) (nth 1 common) (nth 2 common) (if (use-region-p)
(region-beginning)) (if (use-region-p) (region-end)) (nth 3 common)
(if (use-region-p) (region-noncontiguous-p))))
  call-interactively(query-replace nil nil)
  command-execute(query-replace)

As you can see, the "c" inherited the separator property, and
query-replace--split-string choked on that.
It doesn't happen in all modes. It does happen in c-mode and c++-mode.

In GNU Emacs 25.0.92.1 (x86_64-w64-mingw32)
 of 2016-03-21 built on MACHINE
Repository revision: 76ef52267cf887e3e1aa6d25b3b16dd0601dd459
Windowing system distributor 'Microsoft Corp.', version 10.0.10586
Configured using:
 'configure --prefix /c/emacs/emacs-20160321-102640
 --without-imagemagick --disable-dependency-tracking
 --enable-locallisppath=%emacs_dir%/../site-lisp 'CFLAGS=-Og -g -ggdb''

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND DBUS NOTIFY ACL GNUTLS LIBXML2 ZLIB
TOOLKIT_SCROLL_BARS

Important settings:
  value of $LANG: ENG
  locale-coding-system: cp1252

Major mode: C++/l

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-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
  abbrev-mode: t

Recent messages:
Mark set
cl--assertion-failed: Assertion failed: (not (text-property-any (1+
split-pos) length (quote separator) t string))

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message dired format-spec rfc822 mml
mml-sec password-cache epg epg-config gnus-util mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util help-fns mail-prsvr mail-utils
thingatpt cl-extra help-mode cc-mode cc-fonts easymenu cc-guess cc-menus
cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs cl-loaddefs pcase
cl-lib kmacro time-date mule-util tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel dos-w32 ls-lisp disp-table
w32-win w32-vars term/common-win tool-bar dnd fontset image regexp-opt
fringe tabulated-list newcomment elisp-mode lisp-mode prog-mode register
page menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock
font-lock syntax facemenu font-core frame cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese charscript case-table epa-hook
jka-cmpr-hook help simple abbrev minibuffer cl-preloaded nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote w32notify dbusbind w32 multi-tty
make-network-process emacs)

Memory information:
((conses 16 110212 13535)
 (symbols 56 21829 0)
 (miscs 48 49 129)
 (strings 32 21738 3341)
 (string-bytes 1 713085)
 (vectors 16 15026)
 (vector-slots 8 455559 3738)
 (floats 8 164 97)
 (intervals 56 275 18)
 (buffers 976 12))

bug#23127: 25.0.92; Assertion failure when editing history in query-replace in cc-mode

[Alan Mackenzie]

Hello, Richard.

In article <mailman.8469.1459108386.843.bug-gnu-emacs@gnu.org> you wrote:
> In cc-mode, while doing query-replace, edit a previous replacement in
> the minibuffer, and insert characters after the " → " separator. The
> subsequent emacs-lisp chokes on the text properties of the resulting
> string, with an assertion failure.

> Recipe starting from 'emacs -Q':

> M-x c-mode RET
> M-<
> C-% a RET b RET !
> M-<
> C-% M-p C-e <backspace> c RET

> ==> error "cl--assertion-failed: Assertion failed: (not
> (text-property-any (1+ split-pos) length (quote separator) t string))"

> The callstack looks like this (except that here I've replaced a NUL with "^@"):

> Debugger entered: ((cl-assertion-failed (not (text-property-any (1+
> split-pos) length (quote separator) t string)) nil))
>   cl--assertion-failed((not (text-property-any (1+ split-pos) length
> (quote separator) t string)))
>   query-replace--split-string(#("a^@c" 1 2 (display #(" → " 0 3 (face
> minibuffer-prompt)) separator t) 2 3 (separator t)))
>   query-replace-read-from("Query replace" nil)
>   query-replace-read-args("Query replace" nil)
>   (let ((common (query-replace-read-args (concat "Query replace" (if
> current-prefix-arg (if (eq current-prefix-arg ...) " backward" "
> word") "") (if (use-region-p) " in region" "")) nil))) (list (nth 0
> common) (nth 1 common) (nth 2 common) (if (use-region-p)
> (region-beginning)) (if (use-region-p) (region-end)) (nth 3 common)
> (if (use-region-p) (region-noncontiguous-p))))
>   call-interactively(query-replace nil nil)
>   command-execute(query-replace)

> As you can see, the "c" inherited the separator property, and
> query-replace--split-string choked on that.
> It doesn't happen in all modes. It does happen in c-mode and c++-mode.

Thanks for such a high quality bug report.  The cause was a critical
variable being buffer local in C Mode and C++ Mode.  This buffer local
binding was not available in `read-from-minibuffer', with the result that
the `separator' text property got spread to the newly inserted 'c'.

The following patch should fix the bug.  Would you please try it out and
confirm that the bug is indeed fixed, or report what's still wrong.
Thanks!

> In GNU Emacs 25.0.92.1 (x86_64-w64-mingw32)
>  of 2016-03-21 built on MACHINE
> Repository revision: 76ef52267cf887e3e1aa6d25b3b16dd0601dd459
> Windowing system distributor 'Microsoft Corp.', version 10.0.10586
> Configured using:
>  'configure --prefix /c/emacs/emacs-20160321-102640
>  --without-imagemagick --disable-dependency-tracking
>  --enable-locallisppath=%emacs_dir%/../site-lisp 'CFLAGS=-Og -g -ggdb''



diff --git a/lisp/replace.el b/lisp/replace.el
index 428be3c..412f827 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -167,8 +167,6 @@ query-replace-read-from
     ;; unavailable while preparing to dump.
     (custom-reevaluate-setting 'query-replace-from-to-separator)
     (let* ((history-add-new-input nil)
-	   (text-property-default-nonsticky
-	    (cons '(separator . t) text-property-default-nonsticky))
 	   (separator
 	    (when query-replace-from-to-separator
 	      (propertize "\0"
@@ -195,9 +193,16 @@ query-replace-read-from
 	    (save-excursion
 	      (if regexp-flag
 		  (read-regexp prompt nil 'query-replace-from-to-history)
-		(read-from-minibuffer
-		 prompt nil nil nil 'query-replace-from-to-history
-		 (car (if regexp-flag regexp-search-ring search-ring)) t))))
+                ;; The `with-current-buffer' ensures that the binding
+                ;; for `text-property-default-nonsticky' isn't a
+                ;; buffer local binding in the current buffer, which
+                ;; `read-from-minibuffer' wouldn't see.
+                (with-current-buffer (window-buffer (minibuffer-window))
+                  (let ((text-property-default-nonsticky
+                         (cons '(separator . t) text-property-default-nonsticky)))
+                    (read-from-minibuffer
+                     prompt nil nil nil 'query-replace-from-to-history
+                     (car (if regexp-flag regexp-search-ring search-ring)) t))))))
            (to))
       (if (and (zerop (length from)) query-replace-defaults)
 	  (cons (caar query-replace-defaults)



-- 
Alan Mackenzie (Nuremberg, Germany).

bug#23127: 25.0.92; Assertion failure when editing history in query-replace in cc-mode

[Richard Copley]

On 28 March 2016 at 23:02, Alan Mackenzie <acm@muc.de> wrote:
> Hello, Richard.
>
> In article <mailman.8469.1459108386.843.bug-gnu-emacs@gnu.org> you wrote:
>> In cc-mode, while doing query-replace, edit a previous replacement in
>> the minibuffer, and insert characters after the " → " separator. The
>> subsequent emacs-lisp chokes on the text properties of the resulting
>> string, with an assertion failure.
>
>> Recipe starting from 'emacs -Q':
>
>> M-x c-mode RET
>> M-<
>> C-% a RET b RET !
>> M-<
>> C-% M-p C-e <backspace> c RET
>
>> ==> error "cl--assertion-failed: Assertion failed: (not
>> (text-property-any (1+ split-pos) length (quote separator) t string))"
>
>> The callstack looks like this (except that here I've replaced a NUL with "^@"):
>
>> Debugger entered: ((cl-assertion-failed (not (text-property-any (1+
>> split-pos) length (quote separator) t string)) nil))
>>   cl--assertion-failed((not (text-property-any (1+ split-pos) length
>> (quote separator) t string)))
>>   query-replace--split-string(#("a^@c" 1 2 (display #(" → " 0 3 (face
>> minibuffer-prompt)) separator t) 2 3 (separator t)))
>>   query-replace-read-from("Query replace" nil)
>>   query-replace-read-args("Query replace" nil)
>>   (let ((common (query-replace-read-args (concat "Query replace" (if
>> current-prefix-arg (if (eq current-prefix-arg ...) " backward" "
>> word") "") (if (use-region-p) " in region" "")) nil))) (list (nth 0
>> common) (nth 1 common) (nth 2 common) (if (use-region-p)
>> (region-beginning)) (if (use-region-p) (region-end)) (nth 3 common)
>> (if (use-region-p) (region-noncontiguous-p))))
>>   call-interactively(query-replace nil nil)
>>   command-execute(query-replace)
>
>> As you can see, the "c" inherited the separator property, and
>> query-replace--split-string choked on that.
>> It doesn't happen in all modes. It does happen in c-mode and c++-mode.
>
> Thanks for such a high quality bug report.  The cause was a critical
> variable being buffer local in C Mode and C++ Mode.  This buffer local
> binding was not available in `read-from-minibuffer', with the result that
> the `separator' text property got spread to the newly inserted 'c'.
>
> The following patch should fix the bug.  Would you please try it out and
> confirm that the bug is indeed fixed, or report what's still wrong.
> Thanks!
>
>> In GNU Emacs 25.0.92.1 (x86_64-w64-mingw32)
>>  of 2016-03-21 built on MACHINE
>> Repository revision: 76ef52267cf887e3e1aa6d25b3b16dd0601dd459
>> Windowing system distributor 'Microsoft Corp.', version 10.0.10586
>> Configured using:
>>  'configure --prefix /c/emacs/emacs-20160321-102640
>>  --without-imagemagick --disable-dependency-tracking
>>  --enable-locallisppath=%emacs_dir%/../site-lisp 'CFLAGS=-Og -g -ggdb''
>
>
>
> diff --git a/lisp/replace.el b/lisp/replace.el
> index 428be3c..412f827 100644
> --- a/lisp/replace.el
> +++ b/lisp/replace.el
> @@ -167,8 +167,6 @@ query-replace-read-from
>      ;; unavailable while preparing to dump.
>      (custom-reevaluate-setting 'query-replace-from-to-separator)
>      (let* ((history-add-new-input nil)
> -          (text-property-default-nonsticky
> -           (cons '(separator . t) text-property-default-nonsticky))
>            (separator
>             (when query-replace-from-to-separator
>               (propertize "\0"
> @@ -195,9 +193,16 @@ query-replace-read-from
>             (save-excursion
>               (if regexp-flag
>                   (read-regexp prompt nil 'query-replace-from-to-history)
> -               (read-from-minibuffer
> -                prompt nil nil nil 'query-replace-from-to-history
> -                (car (if regexp-flag regexp-search-ring search-ring)) t))))
> +                ;; The `with-current-buffer' ensures that the binding
> +                ;; for `text-property-default-nonsticky' isn't a
> +                ;; buffer local binding in the current buffer, which
> +                ;; `read-from-minibuffer' wouldn't see.
> +                (with-current-buffer (window-buffer (minibuffer-window))
> +                  (let ((text-property-default-nonsticky
> +                         (cons '(separator . t) text-property-default-nonsticky)))
> +                    (read-from-minibuffer
> +                     prompt nil nil nil 'query-replace-from-to-history
> +                     (car (if regexp-flag regexp-search-ring search-ring)) t))))))
>             (to))
>        (if (and (zerop (length from)) query-replace-defaults)
>           (cons (caar query-replace-defaults)

Thanks very much Alan.
I tested your patch and noticed that it fixes query-replace but not
query-replace-regexp. I won't submit a new patch because I don't have
the paperwork (sorry), but I reckon you just need to move the
"(with-current-buffer (...) (let (...) ...))" construct outside the
"(if regexp-flag ...)". With that change it works fine for me.

bug#23127: 25.0.92; Assertion failure when editing history in query-replace in cc-mode

[Alan Mackenzie]

Hello, Richard.

On Tue, Mar 29, 2016 at 12:24:20AM +0100, Richard Copley wrote:

> Thanks very much Alan.
> I tested your patch and noticed that it fixes query-replace but not
> query-replace-regexp.

Whoops!

> I won't submit a new patch because I don't have the paperwork (sorry),
> but I reckon you just need to move the "(with-current-buffer (...)
> (let (...) ...))" construct outside the "(if regexp-flag ...)". With
> that change it works fine for me.

And it works fine for me, too.  Thanks for spotting that.  I've
committed the fix (to the emacs-25 branch in the savannah repository),
and I'm closing the bug.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#23127: 25.0.92; Assertion failure when editing history in query-replace in cc-mode

[Alan Mackenzie]

Bug fixed.

-- 
Alan Mackenzie (Nuremberg, Germany).