bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

[Dmitry Gutov]

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

1. Do a search that has several matches in one buffer, e.g. using
dired-do-find-regexp.
2. Press `r' in the *xref* buffer, to initiate replacement.  Input `.*'
and `abcd', for instance.  This value of FROM is important.
3. Replace a couple then undo that (press y, y, u, u).
4. Try agreeing with the subsequent prompts.  The replacements performed
then will be wrong.

I've tried to come up with a patch but stopped short of really delving
into the code of perform-replace. Which seems really necessary at this
point. Attaching what I already have.

The big problem is that perform-replace does not consistently use
replace-re-search-function. With the new undo code, it's became worse.
After you press `u', it seems to switch to moving around the saved match
data stack and using a plain looking-at, without checking with
isearch-filter-predicate.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xref-undo-fix-attempt.diff --]
[-- Type: text/x-diff, Size: 2597 bytes --]

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index f651dc9..5686cc9 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -552,6 +552,7 @@ xref--buf-pairs-iterator
                       (end (move-marker (make-marker)
                                         (+ beg (xref-match-length item))
                                         (marker-buffer beg))))
+                 (set-marker-insertion-type end t)
                  (let ((pair (cons beg end)))
                    (push pair all-pairs)
                    ;; Perform sanity check first.
@@ -593,7 +594,7 @@ xref--outdated-p
 (defun xref--query-replace-1 (from to iter)
   (let* ((query-replace-lazy-highlight nil)
          (continue t)
-         did-it-once buf-pairs pairs
+         did-it-once buf-pairs
          current-beg current-end
          ;; Counteract the "do the next match now" hack in
          ;; `perform-replace'.  And still, it'll report that those
@@ -605,23 +606,24 @@ xref--query-replace-1
                  (<= end current-end))))
          (replace-re-search-function
           (lambda (from &optional _bound noerror)
-            (let (found pair)
-              (while (and (not found) pairs)
-                (setq pair (pop pairs)
-                      current-beg (car pair)
-                      current-end (cdr pair))
-                (goto-char current-beg)
-                (when (re-search-forward from current-end noerror)
-                  (setq found t)))
-              found))))
+            (let ((tmp-pairs (cdr buf-pairs)) pair)
+              (while (and (not pair) tmp-pairs)
+                (setq current-beg (caar tmp-pairs)
+                      current-end (cdar tmp-pairs))
+                (unless (< current-beg (point))
+                  (goto-char current-beg)
+                  (when (re-search-forward from current-end noerror)
+                    (setq pair (car tmp-pairs))))
+                (setq tmp-pairs (cdr tmp-pairs)))
+              pair))))
     (while (and continue (setq buf-pairs (funcall iter :next)))
       (if did-it-once
           ;; Reuse the same window for subsequent buffers.
           (switch-to-buffer (car buf-pairs))
         (xref--with-dedicated-window
-         (pop-to-buffer (car buf-pairs)))
+         (pop-to-buffer (car buf-pairs))
+         (goto-char (point-min)))
         (setq did-it-once t))
-      (setq pairs (cdr buf-pairs))
       (setq continue
             (perform-replace from to t t nil nil multi-query-replace-map)))
     (unless did-it-once (user-error "No suitable matches here"))))

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


In GNU Emacs 25.1.50.5 (x86_64-unknown-linux-gnu, GTK+ Version 3.18.9)
 of 2016-05-08 built on axl
Repository revision: 2eb6817ba971184cc109f8530f4b3b38f65650ea
Windowing system distributor 'The X.Org Foundation', version 11.0.11803000
System Description:	Ubuntu 16.04 LTS

bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

[Juri Linkov]

> 1. Do a search that has several matches in one buffer, e.g. using
> dired-do-find-regexp.
> 2. Press `r' in the *xref* buffer, to initiate replacement.  Input `.*'
> and `abcd', for instance.  This value of FROM is important.
> 3. Replace a couple then undo that (press y, y, u, u).
> 4. Try agreeing with the subsequent prompts.  The replacements performed
> then will be wrong.
>
> I've tried to come up with a patch but stopped short of really delving
> into the code of perform-replace. Which seems really necessary at this
> point. Attaching what I already have.

I can't reproduce this when using normal query-replace.
Is this caused by binding local variables in xref--query-replace-1?
Then maybe there is a better way to achieve the same.

> The big problem is that perform-replace does not consistently use
> replace-re-search-function. With the new undo code, it's became worse.
> After you press `u', it seems to switch to moving around the saved match
> data stack and using a plain looking-at, without checking with
> isearch-filter-predicate.

You don't need isearch-filter-predicate when doing undo
because a previous replacement already checked it, no?

bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

[Dmitry Gutov]

On 05/08/2016 11:19 PM, Juri Linkov wrote:

> I can't reproduce this when using normal query-replace.

Correct.

> Is this caused by binding local variables in xref--query-replace-1?

No, it's caused by perform-replace not using the said variables in a 
consistent fashion.

 > Then maybe there is a better way to achieve the same.

Maybe there is, but I don't know it.

> You don't need isearch-filter-predicate when doing undo
> because a previous replacement already checked it, no?

Not sure. It's possible the problem is caused by FROM being `.*'. The 
post-undo code initiates new searches at the positions of the previous 
matches using `looking-at'.

bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

[Juri Linkov]

>> Then maybe there is a better way to achieve the same.
>
> Maybe there is, but I don't know it.
>
>> You don't need isearch-filter-predicate when doing undo
>> because a previous replacement already checked it, no?
>
> Not sure. It's possible the problem is caused by FROM being `.*'. The
> post-undo code initiates new searches at the positions of the previous
> matches using `looking-at'.

Why not using an original search string instead of ‘.*’ thus avoiding
tricks with isearch-filter-predicate/replace-re-search-function?

perform-replace will have no problem finding the same matches
as found by xref.

bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

[Dmitry Gutov]

On 05/09/2016 11:01 PM, Juri Linkov wrote:

> Why not using an original search string instead of ‘.*’ thus avoiding
> tricks with isearch-filter-predicate/replace-re-search-function?

A few reasons:

- We don't actually save it.
- Being able to replace _any_ regexp inside the matches is a feature 
(though maybe it's not going to be very popular).
- We're replacing the given list of matches, not all regexp occurrences.

> perform-replace will have no problem finding the same matches
> as found by xref.

That's not true, xref is not limited by Grep is regexp searching in the 
ways that its results are found.

E.g. xref-find-references is not necessarily the result of a regexp 
search. The default implementation is a bit basic, but different 
backends can implement smarter logic (so that the search takes into 
account the context at point, and only returns the uses of the local 
variable within its scope, or only method calls, etc). We'll still want 
to be able to replace them.

bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

[Dmitry Gutov]

Sorry,

> That's not true, xref is not limited by Grep is regexp searching in the
                                                ^^ or
> ways that its results are found.

bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

[Juri Linkov]

>> Why not using an original search string instead of ‘.*’ thus avoiding
>> tricks with isearch-filter-predicate/replace-re-search-function?
>
> A few reasons:
>
> - We don't actually save it.
> - Being able to replace _any_ regexp inside the matches is a feature
> (though maybe it's not going to be very popular).

Honestly speaking, the ‘.*’ thing is quite confusing.
More natural syntax is ‘\&’ like is used in e.g.
‘occur-read-primary-args’ to collect the entire match.

bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

[Dmitry Gutov]

On 05/11/2016 12:34 AM, Juri Linkov wrote:

>> - Being able to replace _any_ regexp inside the matches is a feature
>> (though maybe it's not going to be very popular).
>
> Honestly speaking, the ‘.*’ thing is quite confusing.

You're probably right. We could provide a separate command with that 
advanced feature (or only use ask for FROM with prefix argument). We 
would still have `.*' under the covers in the default case, though.

> More natural syntax is ‘\&’ like is used in e.g.
> ‘occur-read-primary-args’ to collect the entire match.

I'm not sure how \& would be used. And the ability to use \1, \2, etc, 
seems to depend on the original regexp. Which is a) currently unknowable 
(and the original search may have been performed not using a regexp, but 
e.g. by asking a specialized external program), b) not something the 
user can choose when the command is e.g. xref-find-references.

bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

[Juri Linkov]

>>> - Being able to replace _any_ regexp inside the matches is a feature
>>> (though maybe it's not going to be very popular).
>>
>> Honestly speaking, the ‘.*’ thing is quite confusing.
>
> You're probably right. We could provide a separate command with that
> advanced feature (or only use ask for FROM with prefix argument). We would
> still have `.*' under the covers in the default case, though.

What is the purpose of asking FROM?  If to be able to replace
a substring of the original xref search string, then anyway it's
inconvenient for the user to type a part of the already typed string
again, e.g. after searching with xref for the string “abracadabra”,
pressing ‘r’ requires typing a substring “abracadabr”, etc.

More useful would be to prefill the original string in the minibuffer
(here INITIAL-CONTENTS of read-from-minibuffer is justified)
for easy editing (removing parts of the xref search string).

This will greatly simplify replacement with perform-replace.

And then why limit to only boundaries of matches?  The user sees
a list of matching lines in the *xref* buffer.  Let's allow the users
to replace any text within the displayed matching lines (WYSIWYG).

bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

[Dmitry Gutov]

On 05/11/2016 11:48 PM, Juri Linkov wrote:

>> You're probably right. We could provide a separate command with that
>> advanced feature (or only use ask for FROM with prefix argument). We would
>> still have `.*' under the covers in the default case, though.
>
> What is the purpose of asking FROM?  If to be able to replace
> a substring of the original xref search string, then anyway it's
> inconvenient for the user to type a part of the already typed string
> again, e.g. after searching with xref for the string “abracadabra”,
> pressing ‘r’ requires typing a substring “abracadabr”, etc.

See the quote above from my previous message.

> More useful would be to prefill the original string in the minibuffer
> (here INITIAL-CONTENTS of read-from-minibuffer is justified)
> for easy editing (removing parts of the xref search string).

Again: we don't have the access to the "original string", and there 
might not have been one.

> And then why limit to only boundaries of matches?  The user sees
> a list of matching lines in the *xref* buffer.  Let's allow the users
> to replace any text within the displayed matching lines (WYSIWYG).

It's not something I've ever had an urge to do, but sure, the more 
features the better.

However, how would you allow doing this *without* losing the ability to 
replace the matches and just the matches? That should be the default choice.

bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

[Juri Linkov]

> However, how would you allow doing this *without* losing the ability to
> replace the matches and just the matches? That should be the
> default choice.

So I see your aim is to use regexp replacements without regexps,
i.e. with only a list of region boundaries like is used by
region-noncontiguous-p in perform-replace.  I guess this could be
achieved with more hacking in real-match-data (maybe to use a layer
like replace-match-data).

bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

[Dmitry Gutov]

On 05/12/2016 11:57 PM, Juri Linkov wrote:

> So I see your aim is to use regexp replacements without regexps,
> i.e. with only a list of region boundaries like is used by
> region-noncontiguous-p in perform-replace.

This may sound crazy, but shouldn't just using region-extract-function 
to return the list of region bounds work? FROM-STRING can then be `.*' 
without a problem.

> I guess this could be
> achieved with more hacking in real-match-data (maybe to use a layer
> like replace-match-data).

A lot code in perform-replace is really beyond my comprehension, but why 
would the undo code muck around with replaying match data? It could just 
as well repeat the searches.

bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

[Juri Linkov]

>> So I see your aim is to use regexp replacements without regexps,
>> i.e. with only a list of region boundaries like is used by
>> region-noncontiguous-p in perform-replace.
>
> This may sound crazy, but shouldn't just using region-extract-function to
> return the list of region bounds work? FROM-STRING can then be `.*' without
> a problem.

By default .* tries to match whole lines.  For partial matches you need
to use a combination of goto-line and BOUND arg of re-search-forward.

But I still don't see a case where a search regexp used to find matches
in the *xref* buffer can't be re-used in xref-query-replace-in-results.

I tried ‘xref-find-definitions’ with a string identifier, then typed ‘r’
in the *xref* buffer - it asked “Xref query-replace (regexp) (default .*): ”,
then I typed RET and entered a replacement string, but eventually
it failed with “user-error: No suitable matches here”.

Then I thought that maybe you meant the case of ‘xref-find-apropos’
that might find quite different strings with partial matches of
the original pattern, but its replacements fail with the same
“No suitable matches here”.

It's difficult to find a solution without a real use case.

>> I guess this could be
>> achieved with more hacking in real-match-data (maybe to use a layer
>> like replace-match-data).
>
> A lot code in perform-replace is really beyond my comprehension, but why
> would the undo code muck around with replaying match data? It could just as
> well repeat the searches.

Maybe undo could use a search function too.  And with using
a search function should also avoid the need to disable
query-replace-lazy-highlight in xref--query-replace-1.

But it seems there is no hurry in fixing undo because undo of replacements
is a new feature existing only in master, not in the release branch.

If you are looking for a solution for the next release, I recommend
to not expose .* in the minibuffer to the users - that causes too many
problems.  Either leave the initial FROM regexp empty, so the user types
an explicit string, or don't ask FROM at all, using the same regexp
provided to find matches in the *xref* buffer, thus removing all tricks
with isearch-filter-predicate/replace-re-search-function, i.e. since the
.* replacement is not a release-ready feature, just continue its development
in master.

bug#23484: 25.1.50; undo doesn't work properly in xref-query-replace-in-results

[Dmitry Gutov]

On 05/14/2016 11:34 PM, Juri Linkov wrote:

> By default .* tries to match whole lines.

If you have a rectangular region, shouldn't replacing .* with whatever 
only touch the parts of each line within that rectangle?

That's what region-noncontiguous-p means.

 > For partial matches you need
 > to use a combination of goto-line and BOUND arg of re-search-forward.

Either you are referring to the current implementation of 
xref--query-replace-1, or I don't know what else you want me to do.

> But I still don't see a case where a search regexp used to find matches
> in the *xref* buffer can't be re-used in xref-query-replace-in-results.

I have mentioned xref-find-references several times in the recent 
messages alone. An like I said, several times, an xref backend is 
allowed to implement it with something more complex than using a plain 
regexp search.

In fact, if the current directory has a CScope or Global index 
generated, and the relevant program is installed, 
xref-collect-references *will* use something different than a regexp search.

> I tried ‘xref-find-definitions’ with a string identifier, then typed ‘r’
>
> Then I thought that maybe you meant the case of ‘xref-find-apropos’

Replacing the results of xref-find-definitions or xref-find-apropos 
would be a useless nonsense (all usages of the function(s) or 
variable(s) in question would be left intact). This is one of a few 
reasons why it isn't supported.

> It's difficult to find a solution without a real use case.

If my explanations are too complicated, can't you just take it on faith 
that the only data the replacement command can use here is a list of 
pairs of markers?

> Maybe undo could use a search function too.  And with using
> a search function should also avoid the need to disable
> query-replace-lazy-highlight in xref--query-replace-1.

Not sure how one follows from the other, but great.

> But it seems there is no hurry in fixing undo because undo of replacements
> is a new feature existing only in master, not in the release branch.

Sure. I never implied that this bug is a blocker for 25.1.

> If you are looking for a solution for the next release, I recommend
> to not expose .* in the minibuffer to the users - that causes too many
> problems.

Since one release will happen in the meantime, we'll probably hear about 
this from the users anyway. But this is orthogonal to the solution for 
the current problem.

> Either leave the initial FROM regexp empty, so the user types
> an explicit string,

So they'll always *have to* type something? This is very impractical, 
which is definitely worse than just being confusing.

> or don't ask FROM at all,

...and implicitly use `.*' as FROM.

I think I've replied to this suggestion twice already.

> using the same regexp
> provided to find matches in the *xref* buffer, thus removing all tricks
> with isearch-filter-predicate/replace-re-search-function, i.e. since the
> .* replacement is not a release-ready feature, just continue its development
> in master.

FFS, no. This is not feasible.