bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Drew Adams]

Consider regep-searching for just $.  No problem; search goes to every
line end.

Now consider wanting to use a filter predicate that allows search hits
only for lines that are at least 80 chars long, e.g.:

 (defun line>79-p (beg end)
   "Return non-nil if END is past column 79."
   (save-excursion
     (goto-char end)
     (> (current-column) 79)))

 (defun foo ()
   (interactive)
   (let ((isearch-filter-predicate  #'line>79-p))
     (isearch-forward 4)))

That won't work.  Isearch is unnecessarily restrictive because of this
test in `isearch-search', which if non-nil prevents invoking
`isearch-filter-predicate'.

 (= (match-beginning 0) (match-end 0))

I've fixed this in my own code (isearch+.el).  But there are now many
differences between vanilla isearch.el and my code, so it's better if
you fix the vanilla code for this bug.

The fix should be pretty straightforward, but if you have trouble let me
know.  You can anyway see my fix here:

  https://www.emacswiki.org/emacs/download/isearch%2b.el


In GNU Emacs 26.3 (build 1, x86_64-w64-mingw32)
 of 2019-08-29
Repository revision: 96dd0196c28bc36779584e47fffcca433c9309cd
Windowing system distributor `Microsoft Corp.', version 10.0.19042
Configured using:
 `configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install 'CFLAGS=-O2 -static -g3''

bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Lars Ingebrigtsen]

Drew Adams <drew.adams@oracle.com> writes:

> That won't work.  Isearch is unnecessarily restrictive because of this
> test in `isearch-search', which if non-nil prevents invoking
> `isearch-filter-predicate'.
>
>  (= (match-beginning 0) (match-end 0))

The code is:

	(while retry
	  (setq isearch-success
		(isearch-search-string isearch-string nil t))
	  ;; Clear RETRY unless the search predicate says
	  ;; to skip this search hit.
	  (if (or (not isearch-success)
		  (bobp) (eobp)
		  (= (match-beginning 0) (match-end 0))
		  (funcall isearch-filter-predicate
			   (match-beginning 0) (match-end 0)))
	      (setq retry nil)))

I've tried following the logic here, but I'm not sure why we never retry
if the match is zero length.  It looks like this was introduced here:

commit 86bfaffe409c6f7398bcf2144fa8d9f436bd4002
Author:     Richard M. Stallman <rms@gnu.org>
AuthorDate: Mon Feb 10 09:41:31 1997 +0000

    (isearch-search): Refuse to match invisible text.
    (isearch-range-invisible): New function.
    (search-invisible): New user option.

And, indeed, the default predicate doesn't match on invisible text...
but I'm not sure why it's also testing the length of the match.  (The
default predicate also checks this, so removing the test seems to
produce identical results by default.)

So I've added Richard to the CCs -- Richard, you don't happen to recall
the meaning of that apparently duplicated test in a patch that's only 24
years old?

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

bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Juri Linkov]

tags 49534 fixed
close 49534 28.0.50
quit

>> That won't work.  Isearch is unnecessarily restrictive because of this
>> test in `isearch-search', which if non-nil prevents invoking
>> `isearch-filter-predicate'.
>>
>>  (= (match-beginning 0) (match-end 0))
>
> And, indeed, the default predicate doesn't match on invisible text...
> but I'm not sure why it's also testing the length of the match.  (The
> default predicate also checks this, so removing the test seems to
> produce identical results by default.)

I've tried to remove (= (match-beginning 0) (match-end 0))
and then tried the test case provided by Drew,
but it goes into an infinite loop.

So it requires advancing by 1 char - the same trick as it's used
in query-replace, etc.

Also I noticed that Drew's test case matches at 'eob'
that is wrong.  So I moved checks for bobp/eobp outside.

The third problem I noticed thanks to Drew's test case
is that lazy-highlighting incorrectly highlights empty matches.
Fixed as well.

bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Juri Linkov]

>>>  (= (match-beginning 0) (match-end 0))
>>
>> And, indeed, the default predicate doesn't match on invisible text...
>> but I'm not sure why it's also testing the length of the match.  (The
>> default predicate also checks this, so removing the test seems to
>> produce identical results by default.)
>
> I've tried to remove (= (match-beginning 0) (match-end 0))
> and then tried the test case provided by Drew,
> but it goes into an infinite loop.
>
> So it requires advancing by 1 char - the same trick as it's used
> in query-replace, etc.

Oh, I realized that

  (= (match-beginning 0) (match-end 0))

was moved below after

  (funcall isearch-filter-predicate
           (match-beginning 0) (match-end 0))

This means that if isearch-filter-predicate does own matching,
it will break later (= (match-beginning 0) (match-end 0)).

What would be better: to remember its result in a let-bound variable,
or to use save-match-data?  Probably, save-match-data:

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 3337d9be68..9113e94c3b 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -3591,8 +3591,9 @@ isearch-search
 	  ;; Clear RETRY unless the search predicate says
 	  ;; to skip this search hit.
 	  (if (or (not isearch-success)
-		  (funcall isearch-filter-predicate
-			   (match-beginning 0) (match-end 0)))
+		  (save-match-data
+                    (funcall isearch-filter-predicate
+			     (match-beginning 0) (match-end 0))))
 	      (setq retry nil)
 	    ;; Advance point on empty matches before retrying
 	    (when (= (match-beginning 0) (match-end 0))

bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> What would be better: to remember its result in a let-bound variable,
> or to use save-match-data?  Probably, save-match-data:

save-match-data would be pretty natural here, I think.

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

bug#49534: [External] : bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Drew Adams]

>   (= (match-beginning 0) (match-end 0))
> 
> was moved below after
> 
>   (funcall isearch-filter-predicate
>            (match-beginning 0) (match-end 0))
> 
> This means that if isearch-filter-predicate does own matching,
> it will break later (= (match-beginning 0) (match-end 0)).
> 
> What would be better: to remember its result in a let-bound variable,
> or to use save-match-data?  Probably, save-match-data:

Yes, that's probably the right thing; good catch.
___

[On the other hand, a filter predicate can really
do more than just act as a predicate (it can
perform useful side effects).  There could
presumably be reasons we'd want to let it change
the match data.

No, I don't have any concrete use case in mind;
and yes, without some other changes, that could
mess up code that follows using the predicate.
I just wonder about further restricting the
predicate.  The point of this bug fix was to
_remove_ an unnecessary restriction, but now
we're adding another restriction. ;-)]

bug#49534: [External] : Re: bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Drew Adams]

> I've tried to remove (= (match-beginning 0) (match-end 0))
> and then tried the test case provided by Drew,
> but it goes into an infinite loop.
> 
> So it requires advancing by 1 char - the same trick as it's used
> in query-replace, etc.

Yes, my code does that.

> Also I noticed that Drew's test case matches at 'eob'
> that is wrong.  So I moved checks for bobp/eobp outside.

What do you mean by Drew's test case?

Do you mean the updated isearch+.el definition of `isearch-search', with the bug fix?  That is, do you mean this code?

 (when (or (not isearch-success)
           (isearchp-reached-limit-p) ; <==============
           (funcall isearch-filter-predicate
                    (match-beginning 0)
                    (match-end 0)))
   (setq retry  nil))

where

(defun isearchp-reached-limit-p ()
  "Return non-nil if at search-boundary limit in current search direction."
  (if isearch-forward
      (or (eobp) ; <==================
          (and isearchp-reg-end
               (> (point) isearchp-reg-end)))
      (or (bobp) ; <==================
          (and isearchp-reg-beg
               (< (point) isearchp-reg-beg)))))

Is that the use of `eobp' you're talking about?
(If not, what is?)  That `isearchp-reached-limit-p'
just replaces the original isearch.el code, which
just tested (or (bobp) (eobp)):

(or (not isearch-success)
    (bobp) (eobp) ; <=================
    (= (match-beginning 0) (match-end 0))
    (funcall isearch-filter-predicate
             (match-beginning 0) (match-end 0))) 

My code just uses the (original) region limits
instead of the buffer limits, when searching the
active region.

Your code (in master) removes that boundary test
altogether (no `bobp' or `eobp' test).  Why is
that the right thing?  Is it because the match
should be allowed to match up to `bobp' or `eobp'?
If so, why was that test in isearch.el in the
first place?

Actually, you do still test for reaching the
boundary, but only for an empty match and after
filter failure.  Why is that?

And why do you not need to back up a char after
the loop, if the match was empty the last time
around and the next time it fails?  It'll have
advanced a char; should it stay there instead
of backing up?  (Dunno, but I supposed not.)

Our code in those spots is slightly different.
I'd like to know why you did just as you did.

> The third problem I noticed thanks to Drew's test case
> is that lazy-highlighting incorrectly highlights empty matches.
> Fixed as well.

Yes, I left that code alone for the moment, but
your change is no doubt the right thing there.

bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Juri Linkov]

>> What would be better: to remember its result in a let-bound variable,
>> or to use save-match-data?  Probably, save-match-data:
>
> save-match-data would be pretty natural here, I think.

So now pushed as well.

bug#49534: [External] : bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Juri Linkov]

>> What would be better: to remember its result in a let-bound variable,
>> or to use save-match-data?  Probably, save-match-data:
>
> Yes, that's probably the right thing; good catch.
> ___
>
> [On the other hand, a filter predicate can really
> do more than just act as a predicate (it can
> perform useful side effects).  There could
> presumably be reasons we'd want to let it change
> the match data.
>
> No, I don't have any concrete use case in mind;
> and yes, without some other changes, that could
> mess up code that follows using the predicate.
> I just wonder about further restricting the
> predicate.  The point of this bug fix was to
> _remove_ an unnecessary restriction, but now
> we're adding another restriction. ;-)]

If you want to mess with the match data,
you can use own isearch-search-fun-function.

bug#49534: [External] : Re: bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Juri Linkov]

> What do you mean by Drew's test case?

I meant:

 (defun line>79-p (beg end)
   "Return non-nil if END is past column 79."
   (save-excursion
     (goto-char end)
     (> (current-column) 79)))

 (defun foo ()
   (interactive)
   (let ((isearch-filter-predicate  #'line>79-p))
     (isearch-forward 4)))

> Your code (in master) removes that boundary test
> altogether (no `bobp' or `eobp' test).  Why is
> that the right thing?  Is it because the match
> should be allowed to match up to `bobp' or `eobp'?
> If so, why was that test in isearch.el in the
> first place?

If you think that you found a problem, please provide
a test that exposes it.

> Actually, you do still test for reaching the
> boundary, but only for an empty match and after
> filter failure.  Why is that?

Please provide a test case if you think there is a problem.

> And why do you not need to back up a char after
> the loop, if the match was empty the last time
> around and the next time it fails?  It'll have
> advanced a char; should it stay there instead
> of backing up?  (Dunno, but I supposed not.)

Please provide more tests that confirm your doubts.

bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> This means that if isearch-filter-predicate does own matching,
> it will break later (= (match-beginning 0) (match-end 0)).
> What would be better: to remember its result in a let-bound variable,
> or to use save-match-data?  Probably, save-match-data:

Remembering the result in a var might be more efficient.
Also if `isearch-filter-predicate` clobbers the match data we have more
problems (unless it tells us to skip this match).

But whichever choice we make I think the docstring of
`isearch-filter-predicate` should clarify what happens with the match
data (i.e. whether it needs to be careful not to clobber it or not).


        Stefan

bug#49534: [External] : bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Drew Adams]

> > [On the other hand, a filter predicate can really
> > do more than just act as a predicate (it can
> > perform useful side effects).  There could
> > presumably be reasons we'd want to let it change
> > the match data.
> >
> > No, I don't have any concrete use case in mind;
> > and yes, without some other changes, that could
> > mess up code that follows using the predicate.
> > I just wonder about further restricting the
> > predicate.  The point of this bug fix was to
> > _remove_ an unnecessary restriction, but now
> > we're adding another restriction. ;-)]
> 
> If you want to mess with the match data,
> you can use own isearch-search-fun-function.

No, that doesn't speak to whether a filter
predicate should be able to do that.  The
question is not about how to accomplish this
or that; it's about what a filter predicate
should be able to do, as part of its intended
effect.

I'm OK with not letting it use changing match
data as a side effect.

But in general I'm in favor of it being free
to perform arbitrary actions (whether or not
it also acts effectively as a filter).
___

See "A Filter Predicate Can Perform Side Effects":

https://www.emacswiki.org/emacs/DynamicIsearchFiltering#AFilterPredicateCanPerformSideEffects

bug#49534: [External] : Re: bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Drew Adams]

> > Your code (in master) removes that boundary test
> > altogether (no `bobp' or `eobp' test).  Why is
> > that the right thing?  Is it because the match
> > should be allowed to match up to `bobp' or `eobp'?
> > If so, why was that test in isearch.el in the
> > first place?
> 
> If you think that you found a problem, please provide
> a test that exposes it.

I'm asking why you removed that boundary test.
And peripherally, why it was there to begin with?

> > Actually, you do still test for reaching the
> > boundary, but only for an empty match and after
> > filter failure.  Why is that?
> 
> Please provide a test case if you think there is a problem.

I'm asking why you test for reaching the boundary,
and only for an empty match and after a filter
failure.

> > And why do you not need to back up a char after
> > the loop, if the match was empty the last time
> > around and the next time it fails?  It'll have
> > advanced a char; should it stay there instead
> > of backing up?  (Dunno, but I supposed not.)
> 
> Please provide more tests that confirm your doubts.

I'm asking why you don't need to back up a char
after having advanced a char in that case.

bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Juri Linkov]

>> This means that if isearch-filter-predicate does own matching,
>> it will break later (= (match-beginning 0) (match-end 0)).
>> What would be better: to remember its result in a let-bound variable,
>> or to use save-match-data?  Probably, save-match-data:
>
> Remembering the result in a var might be more efficient.
> Also if `isearch-filter-predicate` clobbers the match data we have more
> problems (unless it tells us to skip this match).
>
> But whichever choice we make I think the docstring of
> `isearch-filter-predicate` should clarify what happens with the match
> data (i.e. whether it needs to be careful not to clobber it or not).

Then there is no need to remember the result in a var either
since `isearch-filter-predicate` might want intentionally
force forward-char as if the original match was empty.

So the last change fixed a non-problem and now is reverted.
Also I clarified the docstring of `isearch-filter-predicate`.

bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Drew Adams]

(Resending after unarchiving.)

Juri, could you please answer the questions
I asked you here?  Thx.

> > > Your code (in master) removes that boundary test
> > > altogether (no `bobp' or `eobp' test).  Why is
> > > that the right thing?  Is it because the match
> > > should be allowed to match up to `bobp' or `eobp'?
> > > If so, why was that test in isearch.el in the
> > > first place?
> >
> > If you think that you found a problem, please provide
> > a test that exposes it.
> 
> I'm asking why you removed that boundary test.
> And peripherally, why it was there to begin with?

ping.  I'd really like to know.

> > > Actually, you do still test for reaching the
> > > boundary, but only for an empty match and after
> > > filter failure.  Why is that?
> >
> > Please provide a test case if you think there is a problem.
> 
> I'm asking why you test for reaching the boundary,
> and only for an empty match and after a filter
> failure.

ping.  I'd really like to know.
 
> > > And why do you not need to back up a char after
> > > the loop, if the match was empty the last time
> > > around and the next time it fails?  It'll have
> > > advanced a char; should it stay there instead
> > > of backing up?  (Dunno, but I supposed not.)
> >
> > Please provide more tests that confirm your doubts.
> 
> I'm asking why you don't need to back up a char
> after having advanced a char in that case.

ping.  I'd really like to know.

bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Juri Linkov]

>> > > Your code (in master) removes that boundary test
>> > > altogether (no `bobp' or `eobp' test).  Why is
>> > > that the right thing?  Is it because the match
>> > > should be allowed to match up to `bobp' or `eobp'?
>> > > If so, why was that test in isearch.el in the
>> > > first place?
>> >
>> > If you think that you found a problem, please provide
>> > a test that exposes it.
>>
>> I'm asking why you removed that boundary test.
>> And peripherally, why it was there to begin with?
>
> ping.  I'd really like to know.
>
>> > > Actually, you do still test for reaching the
>> > > boundary, but only for an empty match and after
>> > > filter failure.  Why is that?
>> >
>> > Please provide a test case if you think there is a problem.
>>
>> I'm asking why you test for reaching the boundary,
>> and only for an empty match and after a filter
>> failure.
>
> ping.  I'd really like to know.
>
>> > > And why do you not need to back up a char after
>> > > the loop, if the match was empty the last time
>> > > around and the next time it fails?  It'll have
>> > > advanced a char; should it stay there instead
>> > > of backing up?  (Dunno, but I supposed not.)
>> >
>> > Please provide more tests that confirm your doubts.
>>
>> I'm asking why you don't need to back up a char
>> after having advanced a char in that case.
>
> ping.  I'd really like to know.

All these changes fixed the test case that you presented in the bug report.

bug#49534: [External] : Re: bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Drew Adams]

> All these changes fixed the test case that you
> presented in the bug report.

Yes, I understand that.  You've said that.  
But that doesn't help me understand the code.

Could you please answer the specific questions
I asked about the code?  That will help me
understand what's going on.

I'm in no way arguing that your fix is in some
way incorrect or insufficient.  I'm just trying
to understand the code here.

I'd appreciate this; thx.

bug#49534: [External] : Re: bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Juri Linkov]

>> All these changes fixed the test case that you
>> presented in the bug report.
>
> Yes, I understand that.  You've said that.  
> But that doesn't help me understand the code.
>
> Could you please answer the specific questions
> I asked about the code?  That will help me
> understand what's going on.
>
> I'm in no way arguing that your fix is in some
> way incorrect or insufficient.  I'm just trying
> to understand the code here.
>
> I'd appreciate this; thx.

If I designed a new function from scratch then I could explain all
decisions made while implementing it.  But I only fixed a few places
in the existing function just to pass test cases that you reported.

bug#49534: [External] : Re: bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Drew Adams]

> > I'm in no way arguing that your fix is in some
> > way incorrect or insufficient.  I'm just trying
> > to understand the code here.  I'd appreciate this; thx.
> 
> If I designed a new function from scratch then I could explain all
> decisions made while implementing it.  But I only fixed a few places
> in the existing function just to pass test cases that you reported.

Understood.  But you must at least understand why
you made the changes you made.  That's what I'm
asking about - those particular changes.

bug#49534: [External] : Re: bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Juri Linkov]

>> > I'm in no way arguing that your fix is in some
>> > way incorrect or insufficient.  I'm just trying
>> > to understand the code here.  I'd appreciate this; thx.
>>
>> If I designed a new function from scratch then I could explain all
>> decisions made while implementing it.  But I only fixed a few places
>> in the existing function just to pass test cases that you reported.
>
> Understood.  But you must at least understand why
> you made the changes you made.  That's what I'm
> asking about - those particular changes.

I made these changes because you reported a bug
and provided test cases.  So these changes correspond
to your test cases one to one.