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.