GNU bug report logs - #14724
24.3.50; `isearch-open-necessary-overlays' handling of overlay property 'isearch-open-invisible'

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 14724 in the body.
You can then email your comments to 14724 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: 24.3.50; `isearch-open-necessary-overlays' handling of overlay
 property 'isearch-open-invisible'
Date: Wed, 26 Jun 2013 09:24:53 -0700 (PDT)

The code would be more robust if it would gracefully handle (1) an
non-functional value and perhaps also (2) a functional value where
invoking the function raises an error.  Here is the problematic code:

(funcall (overlay-get ov 'isearch-open-invisible)  ov)

For (1), the code could be like this:

(when (functionp (overlay-get ov 'isearch-open-invisible))
  (funcall (overlay-get ov 'isearch-open-invisible) ov))

For (1+2), that could be wrapped in `ignore-errors'.  Dunno whether (2)
is an important case to handle or whether it is really advisable to
ignore such an error.

Handling (1) seems to me reasonable, and with no downside.  The behavior
is to just do nothing additional with the overlay - e.g., leave the text
invisible.

Dunno know whether similar considerations apply to
`isearch-open-invisible-temporary'.  I'm guessing no - its value seems
to be provided only by the Isearch code (whereas code far from Isearch
sets the `isearch-open-invisible' overlay property).

In GNU Emacs 24.3.50.1 (i686-pc-mingw32)
 of 2013-06-20 on ODIEONE
Bzr revision: 113100 eliz <at> gnu.org-20130620173624-w9v620tog4yacftk
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/Devel/emacs/binary --enable-checking=yes,glyphs
 CFLAGS=-O0 -g3 LDFLAGS=-Lc:/Devel/emacs/lib
 CPPFLAGS=-Ic:/Devel/emacs/include'

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 14724 <at> debbugs.gnu.org
Subject: Re: bug#14724: 24.3.50;
 `isearch-open-necessary-overlays' handling of overlay property
 'isearch-open-invisible'
Date: Wed, 26 Jun 2013 21:37:30 -0400

> The code would be more robust if it would gracefully handle (1) an
> non-functional value and perhaps

Wouldn't such a non-function non-nil value reflect an error in an
Elisp package?  If so, why should we silence it?

> For (1+2), that could be wrapped in `ignore-errors'.

I thing I disagree with ignore-errors here (at least, I don't think such
errors are normal, so they shouldn't be silenced), but using
with-demoted-errors could make sense if errors in this code could make
isearch non-functional.  Is that the case?


        Stefan

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 14724 <at> debbugs.gnu.org
Subject: RE: bug#14724: 24.3.50; `isearch-open-necessary-overlays' handling of
 overlay property 'isearch-open-invisible'
Date: Wed, 26 Jun 2013 19:38:19 -0700 (PDT)

> > The code would be more robust if it would gracefully handle (1) an
> > non-functional value and perhaps
> 
> Wouldn't such a non-function non-nil value reflect an error in an
> Elisp package?

Yes, normally.  In Elisp code, whether a package or not, and whether distributed by Emacs Dev or not.

> If so, why should we silence it?

It would allow 3rd-party code that might not be correct in this regard to at least allow Isearch to continue to function normally otherwise.

The only reasonable value is a function, since we immediately invoke funcall with it.  I'm guessing it might not be obvious to someone where the error lies.

It costs nothing for the code to protect itself from funcalling a non-function here and thus fail gracefully with a no-op.  Someone trying to use this feature will as likely dig in to figure out the problem with a non-action as with an error, but users would at least not be impacted.

Of course I agree that raising an error for incorrect code is typically TRT.  I'm not sure it is in this case.  See what I said about `isearch-open-invisible-temporary', which is different in that it is not set up in distant code but is internal to isearch.el.

> > For (1+2), that could be wrapped in `ignore-errors'.
> 
> I thing I disagree with ignore-errors here (at least, I don't think such
> errors are normal, so they shouldn't be silenced), but using
> with-demoted-errors could make sense if errors in this code could make
> isearch non-functional.  Is that the case?

An error is raised; that's all - Isearch is not broken by it.  Yes, `with-demoted-errors' makes more sense.  The same considerations apply here as for a non-function: better a no-op (a msg is OK) than an error, I think.

Whatever you decide is fine with me.  I'm essentially bringing it to your attention.  Perhaps Juri has some input on this?

Message #18 received at 14724-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 14724-done <at> debbugs.gnu.org
Subject: Re: bug#14724: 24.3.50;
 `isearch-open-necessary-overlays' handling of overlay property
 'isearch-open-invisible'
Date: Wed, 26 Jun 2013 23:00:18 -0400

tags 14724 notabug
thanks

> An error is raised; that's all - Isearch is not broken by it.  Yes,

Then there's no point adding a with-demoted-error.


        Stefan

Previous Next

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