Looking at function

Looking at function

[Juri Linkov]

Any use of the function ‘looking-at’ is incompatible with a non-default value
of ‘isearch-search-fun-function’.  So there are following problematic uses
of ‘looking-at’:

1. There is 1 call in isearch.el in ‘isearch-search-and-update’:

		   (looking-at (cond
				((functionp isearch-regexp-function)
				 (funcall isearch-regexp-function isearch-string t))
				(isearch-regexp-function (word-search-regexp isearch-string t))
				(isearch-regexp isearch-string)
				(t (regexp-quote isearch-string))))

   It doesn't call a non-default search function when using a key sequence like
   ‘C-M-r ^’ on rectangular regions — it matches outside of regions
   when the search function restricts the search within the region bounds.

2. In replace.el ‘looking-at/back’ are used only in ‘perform-replace’
   to check if the next match is adjacent.  This causes problems during
   rectangular replacements to find matches based on a non-default
   search function.

These possible solutions were proposed in bug#54733:

1. Replace ‘looking-at’ with a call to the search function,
   but keep it at point by prepending ‘\\=’ to the regexp.
   Can it break a complex regexp?

2. Call the search function as above but afterwards check if
   (match-beginning 0) is equal to the original position.
   Less efficient.

3. Use looking-at only when the search function is default.

4. Add a new variable ‘looking-at-function’.
   It could be like ‘isearch-search-fun-function’, so redefining
   the search function will also require redefining the
   looking-at function with similar code that doesn't move point.

Any better ideas how to replace looking-at with something
that uses a search function?

Re: Looking at function

[Stefan Monnier]

> 1. Replace ‘looking-at’ with a call to the search function,
>    but keep it at point by prepending ‘\\=’ to the regexp.
>    Can it break a complex regexp?

I can't see why.  Of course, you want to use \\=\\(?:...\\) but
that should be reliable.


        Stefan

Re: Looking at function

[Juri Linkov]

>> 1. Replace ‘looking-at’ with a call to the search function,
>>    but keep it at point by prepending ‘\\=’ to the regexp.
>>    Can it break a complex regexp?
>
> I can't see why.  Of course, you want to use \\=\\(?:...\\) but
> that should be reliable.

I confirm that \\= works reliable, but discovered that not all
search functions can be used instead of looking-at
in isearch-search-and-update.  When calling a search function
that searches through the minibuffer history in the minibuffer,
it tries to find \\= in every minibuffer history item and
eventually messes up the minibuffer contents.
So looking-at can't be replaced in isearch-search-and-update.

There is no problem with replacing looking-at with searching \\=
in perform-replace since it uses the default search function
that can be overridden by replace-re-search-function, and
no one sets replace-re-search-function to a function that
tries to do replacements through all minibuffer history items.

Re: Looking at function

[Dmitry Gutov]

Hi Juri,

On 28.06.2022 21:01, Juri Linkov wrote:
> Any use of the function ‘looking-at’ is incompatible with a non-default value
> of ‘isearch-search-fun-function’.  So there are following problematic uses
> of ‘looking-at’:
> 
> 1. There is 1 call in isearch.el in ‘isearch-search-and-update’:
> 
> 		   (looking-at (cond
> 				((functionp isearch-regexp-function)
> 				 (funcall isearch-regexp-function isearch-string t))
> 				(isearch-regexp-function (word-search-regexp isearch-string t))
> 				(isearch-regexp isearch-string)
> 				(t (regexp-quote isearch-string))))
> 
>     It doesn't call a non-default search function when using a key sequence like
>     ‘C-M-r ^’ on rectangular regions — it matches outside of regions
>     when the search function restricts the search within the region bounds.
> 
> 2. In replace.el ‘looking-at/back’ are used only in ‘perform-replace’
>     to check if the next match is adjacent.  This causes problems during
>     rectangular replacements to find matches based on a non-default
>     search function.
> 
> These possible solutions were proposed in bug#54733:
> 
> 1. Replace ‘looking-at’ with a call to the search function,
>     but keep it at point by prepending ‘\\=’ to the regexp.
>     Can it break a complex regexp?

I suppose it can. Even a simple one (that has \\| inside without a 
grouping).

> 2. Call the search function as above but afterwards check if
>     (match-beginning 0) is equal to the original position.
>     Less efficient.

I don't think efficiency would be a problem here, but tricky search 
functions could be. Like ones that expect to be called a specific number 
of times (replace-re-search-function inside xref--query-replace-1 is one 
such example).

But if the code could be rearranged such that the search function is 
called the same number of times but does something different when it's 
found right away. Or of course we could mandate that the search 
functions are never written this way (it's pretty ad-hoc).

> 3. Use looking-at only when the search function is default.

Probably would fail some of the scenarios similar to which the 
looking-at hack was added for.

> 4. Add a new variable ‘looking-at-function’.
>     It could be like ‘isearch-search-fun-function’, so redefining
>     the search function will also require redefining the
>     looking-at function with similar code that doesn't move point.

I suppose this is a safe alternative.

> Any better ideas how to replace looking-at with something
> that uses a search function?

Do we have a clear understanding of the idea behind this looking-at call?

The comment says:

	  ;; Otherwise, if matching a regular expression, do the next
	  ;; match now, since the replacement for this match may
	  ;; affect whether the next match is adjacent to this one.
	  ;; If that match is empty, don't use it.

What happens if there are multiple adjacent matches in a row, not just 
2? I suppose the replacement could be performed for the first one, then 
the next one is "popped" becoming the current and looking-at is called 
again near its end?

If so, perhaps a good alternative is to stop caring about whether those 
matches are adjacent and always store the latest two matches, whether 
they are next to each other or not.

Re: Looking at function

[Juri Linkov]

>> 1. Replace ‘looking-at’ with a call to the search function,
>>     but keep it at point by prepending ‘\\=’ to the regexp.
>>     Can it break a complex regexp?
>
> I suppose it can. Even a simple one (that has \\| inside without
> a grouping).

This is what the fix for xref successfully uses in bug#53758
with changes in perform-replace from bug#14013.  (However,
none of these variants is suitable for replacing another call
of looking-at in isearch-search-and-update.)

> Do we have a clear understanding of the idea behind this looking-at call?
>
> The comment says:
>
> 	  ;; Otherwise, if matching a regular expression, do the next
> 	  ;; match now, since the replacement for this match may
> 	  ;; affect whether the next match is adjacent to this one.
> 	  ;; If that match is empty, don't use it.
>
> What happens if there are multiple adjacent matches in a row, not just 2?
> I suppose the replacement could be performed for the first one, then the
> next one is "popped" becoming the current and looking-at is called again
> near its end?
>
> If so, perhaps a good alternative is to stop caring about whether those
> matches are adjacent and always store the latest two matches, whether they
> are next to each other or not.

The sole purpose of this "do the next match now" hack
is to handle a special use case that is tested
in test/lisp/replace-tests.el:

    ;; Test case from commit 5632eb272c7
    ("a a a " "C-M-% \\ba SPC RET c RET !" "ccc") ; not "ca c"

    ;; Test case from commit 5632eb272c7
    ("a a a " "\\ba " "c" nil t nil nil nil nil nil nil nil "ccc") ; not "ca c"

Re: Looking at function

[Dmitry Gutov]

On 11.07.2022 22:14, Juri Linkov wrote:
>>> 1. Replace ‘looking-at’ with a call to the search function,
>>>      but keep it at point by prepending ‘\\=’ to the regexp.
>>>      Can it break a complex regexp?
>>
>> I suppose it can. Even a simple one (that has \\| inside without
>> a grouping).
> 
> This is what the fix for xref successfully uses in bug#53758
> with changes in perform-replace from bug#14013.  (However,
> none of these variants is suitable for replacing another call
> of looking-at in isearch-search-and-update.)

Right. Because xref basically uses literal matching, no alternations.

But it will break more complex cases.

>> Do we have a clear understanding of the idea behind this looking-at call?
>>
>> The comment says:
>>
>> 	  ;; Otherwise, if matching a regular expression, do the next
>> 	  ;; match now, since the replacement for this match may
>> 	  ;; affect whether the next match is adjacent to this one.
>> 	  ;; If that match is empty, don't use it.
>>
>> What happens if there are multiple adjacent matches in a row, not just 2?
>> I suppose the replacement could be performed for the first one, then the
>> next one is "popped" becoming the current and looking-at is called again
>> near its end?
>>
>> If so, perhaps a good alternative is to stop caring about whether those
>> matches are adjacent and always store the latest two matches, whether they
>> are next to each other or not.
> 
> The sole purpose of this "do the next match now" hack
> is to handle a special use case that is tested
> in test/lisp/replace-tests.el:
> 
>      ;; Test case from commit 5632eb272c7
>      ("a a a " "C-M-% \\ba SPC RET c RET !" "ccc") ; not "ca c"
> 
>      ;; Test case from commit 5632eb272c7
>      ("a a a " "\\ba " "c" nil t nil nil nil nil nil nil nil "ccc") ; not "ca c"

All right. So it seems the idea of keeping references to the two latest 
can work.

No idea how big the changes will have to be, though.

Re: Looking at function

[Stefan Monnier]

Dmitry Gutov [2022-07-12 03:19:42] wrote:
> On 11.07.2022 22:14, Juri Linkov wrote:
>>>> 1. Replace ‘looking-at’ with a call to the search function,
>>>>      but keep it at point by prepending ‘\\=’ to the regexp.
>>>>      Can it break a complex regexp?
>>>
>>> I suppose it can. Even a simple one (that has \\| inside without
>>> a grouping).
>> This is what the fix for xref successfully uses in bug#53758
>> with changes in perform-replace from bug#14013.  (However,
>> none of these variants is suitable for replacing another call
>> of looking-at in isearch-search-and-update.)
>
> Right. Because xref basically uses literal matching, no alternations.
> But it will break more complex cases.

I can't see why "\\=\\(?:...\\)" would break anything, personally.


        Stefan

Re: Looking at function

[Juri Linkov]

>>> Do we have a clear understanding of the idea behind this looking-at call?
>>>
>>> The comment says:
>>>
>>> 	  ;; Otherwise, if matching a regular expression, do the next
>>> 	  ;; match now, since the replacement for this match may
>>> 	  ;; affect whether the next match is adjacent to this one.
>>> 	  ;; If that match is empty, don't use it.
>>>
>>> What happens if there are multiple adjacent matches in a row, not just 2?
>>> I suppose the replacement could be performed for the first one, then the
>>> next one is "popped" becoming the current and looking-at is called again
>>> near its end?

Actually, this is how it worked until now.

>>> If so, perhaps a good alternative is to stop caring about whether those
>>> matches are adjacent and always store the latest two matches, whether they
>>> are next to each other or not.

And this is how it's implemented by the patches in bug#14013 and bug#53758.

>> The sole purpose of this "do the next match now" hack
>> is to handle a special use case that is tested
>> in test/lisp/replace-tests.el:
>>      ;; Test case from commit 5632eb272c7
>>      ("a a a " "C-M-% \\ba SPC RET c RET !" "ccc") ; not "ca c"
>>      ;; Test case from commit 5632eb272c7
>>      ("a a a " "\\ba " "c" nil t nil nil nil nil nil nil nil "ccc") ; not "ca c"
>
> All right. So it seems the idea of keeping references to the two latest can
> work.
>
> No idea how big the changes will have to be, though.

Shouldn't keeping only one reference be sufficient?

Re: Looking at function

[Dmitry Gutov]

On 12.07.2022 03:29, Stefan Monnier wrote:
> Dmitry Gutov [2022-07-12 03:19:42] wrote:
>> On 11.07.2022 22:14, Juri Linkov wrote:
>>>>> 1. Replace ‘looking-at’ with a call to the search function,
>>>>>       but keep it at point by prepending ‘\\=’ to the regexp.
>>>>>       Can it break a complex regexp?
>>>> I suppose it can. Even a simple one (that has \\| inside without
>>>> a grouping).
>>> This is what the fix for xref successfully uses in bug#53758
>>> with changes in perform-replace from bug#14013.  (However,
>>> none of these variants is suitable for replacing another call
>>> of looking-at in isearch-search-and-update.)
>> Right. Because xref basically uses literal matching, no alternations.
>> But it will break more complex cases.
> I can't see why "\\=\\(?:...\\)" would break anything, personally.

Right.

But I suppose this approach could be problematic when 
isearch-regexp-function or isearch-search-fun-function have non-default 
and some distinctly weird values/specialized behaviors.

Re: Looking at function

[Dmitry Gutov]

Hi Juri,

Sorry for the abysmal rate of replies here. isearch is really hard to 
fit in my head.

On 12.07.2022 10:50, Juri Linkov wrote:
>>>> Do we have a clear understanding of the idea behind this looking-at call?
>>>>
>>>> The comment says:
>>>>
>>>> 	  ;; Otherwise, if matching a regular expression, do the next
>>>> 	  ;; match now, since the replacement for this match may
>>>> 	  ;; affect whether the next match is adjacent to this one.
>>>> 	  ;; If that match is empty, don't use it.
>>>>
>>>> What happens if there are multiple adjacent matches in a row, not just 2?
>>>> I suppose the replacement could be performed for the first one, then the
>>>> next one is "popped" becoming the current and looking-at is called again
>>>> near its end?
> 
> Actually, this is how it worked until now.
> 
>>>> If so, perhaps a good alternative is to stop caring about whether those
>>>> matches are adjacent and always store the latest two matches, whether they
>>>> are next to each other or not.
> 
> And this is how it's implemented by the patches in bug#14013 and bug#53758.

Sounds good, then. So this is a workable solution?

Or have you come across some pitfalls?

>>> The sole purpose of this "do the next match now" hack
>>> is to handle a special use case that is tested
>>> in test/lisp/replace-tests.el:
>>>       ;; Test case from commit 5632eb272c7
>>>       ("a a a " "C-M-% \\ba SPC RET c RET !" "ccc") ; not "ca c"
>>>       ;; Test case from commit 5632eb272c7
>>>       ("a a a " "\\ba " "c" nil t nil nil nil nil nil nil nil "ccc") ; not "ca c"
>>
>> All right. So it seems the idea of keeping references to the two latest can
>> work.
>>
>> No idea how big the changes will have to be, though.
> 
> Shouldn't keeping only one reference be sufficient?

By "two references" I meant the current one and the next one. Seems like 
the necessary minimum.

Re: Looking at function

[Juri Linkov]

>>>>> If so, perhaps a good alternative is to stop caring about whether those
>>>>> matches are adjacent and always store the latest two matches, whether they
>>>>> are next to each other or not.
>> And this is how it's implemented by the patches in bug#14013 and
>> bug#53758.
>
> Sounds good, then. So this is a workable solution?
>
> Or have you come across some pitfalls?

Everything works fine, but might require adding more conditionals
like '(>= emacs-major-version 29)' in 'xref--query-replace-1'
as suggested at the end of bug#54733.