GNU bug report logs - #21091
25.0.50; `isearch-done' called before `isearch-update' raises wrong-type-arg error

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 21091 in the body.
You can then email your comments to 21091 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.0.50; `isearch-done' called before `isearch-update' raises
 wrong-type-arg error
Date: Sat, 18 Jul 2015 20:57:32 -0700 (PDT)

Dunno whether I'll be able to convince you that this is a bug, but here
goes.

Recently someone added `isearch--current-buffer' to isearch.el.

This is initially nil.  It is given a string value (buffer name) only in
`isearch-update'.  But it is called in `isearch-done' and expected to
have a string value there.  If it does not, a wrong-type-arg error is
raised.

I have code that defines some Isearch commands that each start out by
calling (isearch-done) - they can be called at top level or from
`isearch-mode-map'.  Here is the beginning of one:

(defun foo (arg)
  (interactive "P")
  (bar 'isearch-forward arg))

(defun bar (arg)
  (isearch-done)
  (setq isearch-success   t
        isearch-adjusted  t)
  (let* ((enable-recursive-minibuffers t)
         ...)
    ...
    (setq isearch-filter-predicate ...)
    ...)
  (funcall search-fn))

When called at top level, `isearch--current-buffer' is nil, and the
wrong-type arg error is raised.

I can "fix" the problem that was introduced by wrapping the
`isearch-done' call in `ignore-errors'.  But I think it would be better
for isearch.el not to assume that `isearch-done' is called after
`isearch-update'.  I don't think there is a reason why the two need to
be coupled in that way.  Adding variable `isearch--current-buffer' in
the way it was done makes the isearch.el code more fragile than it needs
to be, I think.

Anyway, please consider somehow ensuring that `isearch-done' does not
care whether `isearch--current-buffer' has a string value.

Perhaps one possibility would be something like this - dunno.

  (when isearch--current-buffer
    (with-current-buffer isearch--current-buffer
      (setq isearch--current-buffer nil)
      (setq cursor-sensor-inhibit (delq 'isearch cursor-sensor-inhibit))))

Or maybe even:

  (when (and isearch--current-buffer
             (get-buffer isearch--current-buffer))
    ...)


In GNU Emacs 25.0.50.1 (i686-pc-mingw32)
 of 2015-07-03 on LEG570
Bzr revision: 2b848fadd51e805b2f46da64c5958ea7f009048a
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --host=i686-pc-mingw32 --enable-checking=yes,glyphs'

Message #8 received at 21091 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 21091 <at> debbugs.gnu.org
Subject: Re: bug#21091: 25.0.50;
 `isearch-done' called before `isearch-update' raises wrong-type-arg
 error
Date: Sat, 30 Apr 2016 23:27:39 +0200

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

> Dunno whether I'll be able to convince you that this is a bug, but here
> goes.
>
> Recently someone added `isearch--current-buffer' to isearch.el.
>
> This is initially nil.  It is given a string value (buffer name) only in
> `isearch-update'.  But it is called in `isearch-done' and expected to
> have a string value there.  If it does not, a wrong-type-arg error is
> raised.

If I understand you correctly, I don't think this is a bug.  If somebody
else disagrees, please reopen this bug report.

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

Message #19 received at 21091 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 21091 <at> debbugs.gnu.org
Subject: RE: bug#21091: 25.0.50; `isearch-done' called before `isearch-update'
 raises wrong-type-arg error
Date: Sat, 3 Sep 2016 17:08:41 -0700 (PDT)

> > Recently someone added `isearch--current-buffer' to isearch.el.
> >
> > This is initially nil.  It is given a string value (buffer name) only in
> > `isearch-update'.  But it is called in `isearch-done' and expected to
> > have a string value there.  If it does not, a wrong-type-arg error is
> > raised...
> > 
> > When called at top level, `isearch--current-buffer' is nil, and the
> > wrong-type arg error is raised.
> > 
> > I can "fix" the problem that was introduced by wrapping the
> > `isearch-done' call in `ignore-errors'.  But I think it would be better
> > for isearch.el not to assume that `isearch-done' is called after
> > `isearch-update'.  I don't think there is a reason why the two need to
> > be coupled in that way.  Adding variable `isearch--current-buffer' in
> > the way it was done makes the isearch.el code more fragile than it needs
> > to be, I think.
> > 
> > Anyway, please consider somehow ensuring that `isearch-done' does not
> > care whether `isearch--current-buffer' has a string value.
> 
> If I understand you correctly, I don't think this is a bug.  If somebody
> else disagrees, please reopen this bug report.

No reason given?  Why do you think it is not a bug?  Why do you not
think that "adding variable `isearch--current-buffer' in the way it
was done makes the isearch.el code more fragile than it needs to be"?
Why should the code assume that `isearch-done' is called only after
`isearch-update'?  There is nothing inherent in `isearch-done' that
suggests that.

Message #22 received at 21091 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 21091 <at> debbugs.gnu.org
Subject: Re: bug#21091: 25.0.50;
 `isearch-done' called before `isearch-update' raises wrong-type-arg
 error
Date: Sat, 03 Sep 2016 23:45:07 -0400

[Message part 1 (text/plain, inline)]

tags 21091 patch
quit

Drew Adams <drew.adams <at> oracle.com> writes:
>
> Anyway, please consider somehow ensuring that `isearch-done' does not
> care whether `isearch--current-buffer' has a string value.

Make sense to me, I suggest the following (isearch-update checks for a
buffer value, so I went with that to be consistent):

[v1-0001-Don-t-require-isearch-update-before-isearch-done.patch (text/plain, inline)]

From 5d7697546800ad3494df1d06d24e12f2fe987350 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs <at> gmail.com>
Date: Sat, 3 Sep 2016 23:38:35 -0400
Subject: [PATCH v1] Don't require isearch-update before isearch-done

It is useful to be able to call `isearch-done' unconditionally to
ensure a non-isearching state.

* lisp/isearch.el (isearch-done): Check that `isearch--current-buffer'
is a live buffer before using it (Bug #21091).
---
 lisp/isearch.el | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index b50379a..39ed8af 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1045,9 +1045,10 @@ isearch-done
   (remove-hook 'mouse-leave-buffer-hook 'isearch-done)
   (remove-hook 'kbd-macro-termination-hook 'isearch-done)
   (setq isearch-lazy-highlight-start nil)
-  (with-current-buffer isearch--current-buffer
-    (setq isearch--current-buffer nil)
-    (setq cursor-sensor-inhibit (delq 'isearch cursor-sensor-inhibit)))
+  (when (buffer-live-p isearch--current-buffer)
+    (with-current-buffer isearch--current-buffer
+      (setq isearch--current-buffer nil)
+      (setq cursor-sensor-inhibit (delq 'isearch cursor-sensor-inhibit))))
 
   ;; Called by all commands that terminate isearch-mode.
   ;; If NOPUSH is non-nil, we don't push the string on the search ring.
-- 
2.9.3

Message #27 received at 21091 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: npostavs <at> users.sourceforge.net
Cc: 21091 <at> debbugs.gnu.org
Subject: RE: bug#21091: 25.0.50; `isearch-done' called before `isearch-update'
 raises wrong-type-arg error
Date: Sat, 3 Sep 2016 22:43:54 -0700 (PDT)

> > Anyway, please consider somehow ensuring that `isearch-done' does not
> > care whether `isearch--current-buffer' has a string value.
> 
> Make sense to me, I suggest the following (isearch-update checks for a
> buffer value, so I went with that to be consistent):

Haven't tested, but it looks good to me.  I think that's all that's
needed.  Thanks for doing this.

Message #30 received at 21091 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 21091 <at> debbugs.gnu.org
Subject: Re: bug#21091: 25.0.50;
 `isearch-done' called before `isearch-update' raises wrong-type-arg
 error
Date: Sun, 04 Sep 2016 16:47:59 -0400

[Message part 1 (text/plain, inline)]

tags 21091 - notabug
found 21091 25.1
quit

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

>> > Anyway, please consider somehow ensuring that `isearch-done' does not
>> > care whether `isearch--current-buffer' has a string value.
>> 
>> Make sense to me, I suggest the following (isearch-update checks for a
>> buffer value, so I went with that to be consistent):
>
> Haven't tested, but it looks good to me.  I think that's all that's
> needed.  Thanks for doing this.

Speaking of testing, I think this sort thing would benefit from a
regression test, so I added that to the patch.  I will to master push in
a few days if there are no objections.

[v2-0001-Don-t-require-isearch-update-before-isearch-done.patch (text/plain, inline)]

From 2f00da3a255a2fb46ce4819a3153e04d9d9d59c9 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs <at> gmail.com>
Date: Sat, 3 Sep 2016 23:38:35 -0400
Subject: [PATCH v2] Don't require isearch-update before isearch-done

It is useful to be able to call `isearch-done' unconditionally to
ensure a non-isearching state.

* lisp/isearch.el (isearch-done): Check that `isearch--current-buffer'
is a live buffer before using it (Bug #21091).
* test/lisp/isearch-tests.el (isearch--test-done): Test it.
---
 lisp/isearch.el            | 7 ++++---
 test/lisp/isearch-tests.el | 8 ++++++++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index b50379a..39ed8af 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -1045,9 +1045,10 @@ isearch-done
   (remove-hook 'mouse-leave-buffer-hook 'isearch-done)
   (remove-hook 'kbd-macro-termination-hook 'isearch-done)
   (setq isearch-lazy-highlight-start nil)
-  (with-current-buffer isearch--current-buffer
-    (setq isearch--current-buffer nil)
-    (setq cursor-sensor-inhibit (delq 'isearch cursor-sensor-inhibit)))
+  (when (buffer-live-p isearch--current-buffer)
+    (with-current-buffer isearch--current-buffer
+      (setq isearch--current-buffer nil)
+      (setq cursor-sensor-inhibit (delq 'isearch cursor-sensor-inhibit))))
 
   ;; Called by all commands that terminate isearch-mode.
   ;; If NOPUSH is non-nil, we don't push the string on the search ring.
diff --git a/test/lisp/isearch-tests.el b/test/lisp/isearch-tests.el
index 48c3424..52f312d 100644
--- a/test/lisp/isearch-tests.el
+++ b/test/lisp/isearch-tests.el
@@ -28,5 +28,13 @@
     (isearch-update)
     (should (equal isearch--current-buffer (current-buffer)))))
 
+(ert-deftest isearch--test-done ()
+  ;; Normal operation.
+  (isearch-update)
+  (isearch-done)
+  (should-not isearch--current-buffer)
+  ;; Bug #21091: let `isearch-done' work without `isearch-update'.
+  (isearch-done))
+
 (provide 'isearch-tests)
 ;;; isearch-tests.el ends here
-- 
2.9.3

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.