bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

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

  What happened:
In Dired isearch-{forward,backward} will no longer match anything after running
`rectangle-mark-mode' and `query-replace'.

  What I thought would happen:
In Dired after running `rectangle-mark-mode' and `query-replace',
isearch-{forward,backward} would match again.

  To reproduce:
1. emacs -Q
2. C-x d <RET>
3. C-x C-q
4. C-x <SPC> C-n
5. M-% <RET> <RET> (entering nothing)
6. ^s (searching for anything will show "Failing I-search: ANYTHING" in minibuffer)


In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.30, cairo version 1.17.4)
 of 2021-08-09 built on hcra
Repository revision: 799a66c454f4dbb06aeab744f92321d95e5043d0
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12013000
System Description: Arch Linux

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY INOTIFY
PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE
XIM XPM GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg epg-config gnus-util rmail
rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json map text-property-search time-date
subr-x seq byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs
cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
iso-transl tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads dbusbind
inotify lcms2 dynamic-setting system-font-setting font-render-setting
cairo move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 50823 6567)
 (symbols 48 6607 1)
 (strings 32 18207 1416)
 (string-bytes 1 601772)
 (vectors 16 13438)
 (vector-slots 8 178370 10146)
 (floats 8 21 36)
 (intervals 56 293 1)
 (buffers 992 11))

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

[Juri Linkov]

>   What happened:
> In Dired isearch-{forward,backward} will no longer match anything after running
> `rectangle-mark-mode' and `query-replace'.
>
>   What I thought would happen:
> In Dired after running `rectangle-mark-mode' and `query-replace',
> isearch-{forward,backward} would match again.
>
>   To reproduce:
> 1. emacs -Q
> 2. C-x d <RET>
> 3. C-x C-q
> 4. C-x <SPC> C-n
> 5. M-% <RET> <RET> (entering nothing)
> 6. ^s (searching for anything will show "Failing I-search: ANYTHING" in minibuffer)

Stefan, please help.  Wdired adds advice on a buffer-local isearch-filter-predicate,
and perform-replace adds advice on a let-bound isearch-filter-predicate
that changes the global value.  This is the minimal test case:

  (add-function :after-while (local 'isearch-filter-predicate)
                (lambda (&rest _) 'filter-local))

  (let ((isearch-filter-predicate isearch-filter-predicate))
    (add-function :after-while isearch-filter-predicate
                  (lambda (&rest _) 'filter-letbound)))

C-h v isearch-filter-predicate:

  Value:
  #f(advice-wrapper :after-while #f(compiled-function
                                    (&rest args)
                                    #<bytecode -0x1d2ab1d0859aebb6>)
                    (closure
                     (t)
                     (&rest _)
                     'filter-local))
  Local in buffer *scratch*; global value is
  #f(advice-wrapper :after-while isearch-filter-visible
                     (closure
                      (t)
                      (&rest _)
                      'filter-letbound))

Let-binding was supposed to protect from changing the global value,
but with a buffer-local value it still changes the global value.

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Tue, 10 Aug 2021 10:03:39 +0300
> Cc: 49963@debbugs.gnu.org, laslydone <laslydone@protonmail.com>
> 
> Stefan, please help.  Wdired adds advice on a buffer-local isearch-filter-predicate,
> and perform-replace adds advice on a let-bound isearch-filter-predicate
> that changes the global value.  This is the minimal test case:
> 
>   (add-function :after-while (local 'isearch-filter-predicate)
>                 (lambda (&rest _) 'filter-local))
> 
>   (let ((isearch-filter-predicate isearch-filter-predicate))
>     (add-function :after-while isearch-filter-predicate
>                   (lambda (&rest _) 'filter-letbound)))

Regardless of how we solve this issue, could we please NOT use advices
in such cases?  is there really no other reasonable solution for this,
like perhaps using some existing Isearch hook or (gasp!) introducing a
new one?

TIA

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

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

> Regardless of how we solve this issue, could we please NOT use advices
> in such cases?  is there really no other reasonable solution for this,
> like perhaps using some existing Isearch hook or (gasp!) introducing a
> new one?

These are not "advices", they are using a hook.


        Stefan

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

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

>   (add-function :after-while (local 'isearch-filter-predicate)
>                 (lambda (&rest _) 'filter-local))
>
>   (let ((isearch-filter-predicate isearch-filter-predicate))
>     (add-function :after-while isearch-filter-predicate
>                   (lambda (&rest _) 'filter-letbound)))
>
> C-h v isearch-filter-predicate:
>
>   Value:
>   #f(advice-wrapper :after-while #f(compiled-function
>                                     (&rest args)
>                                     #<bytecode -0x1d2ab1d0859aebb6>)
>                     (closure
>                      (t)
>                      (&rest _)
>                      'filter-local))
>   Local in buffer *scratch*; global value is
>   #f(advice-wrapper :after-while isearch-filter-visible
>                      (closure
>                       (t)
>                       (&rest _)
>                       'filter-letbound))
>
> Let-binding was supposed to protect from changing the global value,

But it doesn't, does it?

So don't use `let`.
Instead use `remove-function` with `unwind-protect`.


        Stefan

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Juri Linkov <juri@linkov.net>,  49963@debbugs.gnu.org,
>   laslydone@protonmail.com
> Date: Tue, 10 Aug 2021 10:28:10 -0400
> 
> > Regardless of how we solve this issue, could we please NOT use advices
> > in such cases?  is there really no other reasonable solution for this,
> > like perhaps using some existing Isearch hook or (gasp!) introducing a
> > new one?
> 
> These are not "advices", they are using a hook.

I meant can we please not use add-function and friends.  Please?

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I meant can we please not use add-function and friends.  Please?

There isn't much difference between using add-function and add-hook most
of the time (although here we trip over a `let' binding not doing what
the author thought it did).

add-function is better than add-hook in most ways, in my opinion, so I
think using it is fine.

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

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

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

Lars Ingebrigtsen [2021-08-10 17:44:50] wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
>> I meant can we please not use add-function and friends.  Please?
> There isn't much difference between using add-function and add-hook most
> of the time (although here we trip over a `let' binding not doing what
> the author thought it did).

FWIW, `add-hook` has the exact same problem with `let` (and we already
tripped against this exact same situation with `add-hook` and `let`).


        Stefan

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  49963@debbugs.gnu.org,
>   juri@linkov.net,  laslydone@protonmail.com
> Date: Tue, 10 Aug 2021 17:44:50 +0200
> 
> There isn't much difference between using add-function and add-hook most
> of the time (although here we trip over a `let' binding not doing what
> the author thought it did).
> 
> add-function is better than add-hook in most ways, in my opinion, so I
> think using it is fine.

Well, it prevented me from immediately seeing which code is stealing
C-s, so I beg to differ.

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  49963@debbugs.gnu.org,  juri@linkov.net,
>   laslydone@protonmail.com
> Date: Tue, 10 Aug 2021 11:56:58 -0400
> 
> Lars Ingebrigtsen [2021-08-10 17:44:50] wrote:
> > Eli Zaretskii <eliz@gnu.org> writes:
> >> I meant can we please not use add-function and friends.  Please?
> > There isn't much difference between using add-function and add-hook most
> > of the time (although here we trip over a `let' binding not doing what
> > the author thought it did).
> 
> FWIW, `add-hook` has the exact same problem with `let` (and we already
> tripped against this exact same situation with `add-hook` and `let`).

This is a misunderstanding of what bothers me.  The problem is
discoverability: add-function is not easily discoverable, if it is
used on an internal function.  IOW, the documentation of C-s doesn't
tell you that its operation could be affected by that "hook".  By
contrast, if we use some other mechanism, like call a function via a
variable or call a hook, and if we document these in the doc string of
C-s, that problem will have disappeared, and finding causes of
problems such as this one would be easier.

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

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

Eli Zaretskii [2021-08-10 19:42:17] wrote:
>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  49963@debbugs.gnu.org,  juri@linkov.net,
>>   laslydone@protonmail.com
>> Date: Tue, 10 Aug 2021 11:56:58 -0400
>> 
>> Lars Ingebrigtsen [2021-08-10 17:44:50] wrote:
>> > Eli Zaretskii <eliz@gnu.org> writes:
>> >> I meant can we please not use add-function and friends.  Please?
>> > There isn't much difference between using add-function and add-hook most
>> > of the time (although here we trip over a `let' binding not doing what
>> > the author thought it did).
>> 
>> FWIW, `add-hook` has the exact same problem with `let` (and we already
>> tripped against this exact same situation with `add-hook` and `let`).
>
> This is a misunderstanding of what bothers me.  The problem is
> discoverability: add-function is not easily discoverable, if it is
> used on an internal function.

I'm sorry, I don't understand.  AFAIK it's used on variables (in the
present case it's used on the variable `isearch-filter-predicate`)
holding functions, not on internal functions.

> IOW, the documentation of C-s doesn't tell you that its operation
> could be affected by that "hook".

You mean the doc of `C-s` should state that it's affected by
`isearch-filter-predicate`?

Fine by me, but I don't know what this has to do with `add-function`
(`isearch-filter-predicate` existed before `add-function` was invented).

> By contrast, if we use some other mechanism, like call a function via
> a variable or call a hook,

Hmm... again I don't understand: we do "call a function via a variable",
the variable being `isearch-filter-predicate`.


        Stefan

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > This is a misunderstanding of what bothers me.  The problem is
  > discoverability: add-function is not easily discoverable, if it is
  > used on an internal function.  IOW, the documentation of C-s doesn't
  > tell you that its operation could be affected by that "hook".  By
  > contrast, if we use some other mechanism, like call a function via a
  > variable or call a hook, and if we document these in the doc string of
  > C-s, that problem will have disappeared, and finding causes of
  > problems such as this one would be easier.

For this reason, we had the rule that packages or libraries should not
define any advice.  Advice is for users to play with, or for code not
ready to release; it is not meant for released code to communicate
through.

However, that rule seems to have been deleted from all the
places it ought to be.  I can't find it in advice.el (though I may
have missed it; the file starts with 1500 lines of documentation).
And I can't find it in functions.texi either.

We need to document this rule again.  Can someone find it
in an old version of the Emacs Lisp Reference Manual?

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

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

>   > This is a misunderstanding of what bothers me.  The problem is
>   > discoverability: add-function is not easily discoverable, if it is
>   > used on an internal function.  IOW, the documentation of C-s doesn't
>   > tell you that its operation could be affected by that "hook".  By
>   > contrast, if we use some other mechanism, like call a function via a
>   > variable or call a hook, and if we document these in the doc string of
>   > C-s, that problem will have disappeared, and finding causes of
>   > problems such as this one would be easier.
>
> For this reason, we had the rule that packages or libraries should not
> define any advice.

Again, this bug report is not about the use of an advice.
There is no advice used in this code.


        Stefan

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

[Juri Linkov]

> However, that rule seems to have been deleted from all the
> places it ought to be.  I can't find it in advice.el (though I may
> have missed it; the file starts with 1500 lines of documentation).
> And I can't find it in functions.texi either.
>
> We need to document this rule again.  Can someone find it
> in an old version of the Emacs Lisp Reference Manual?

The manual still contains this rule in the node Coding Standards:

  Avoid using ‘defadvice’ or ‘with-eval-after-load’ for Lisp code to
  be included in Emacs.

I'm sorry that I made a mistake when used the wrong word "advice"
while describing the problem that is not about the use of an advice.

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

[Juri Linkov]

tags 49963 fixed
close 49963 28.0.50
thanks

>> Let-binding was supposed to protect from changing the global value,
>
> But it doesn't, does it?
>
> So don't use `let`.
> Instead use `remove-function` with `unwind-protect`.

Thanks, now added `remove-function` in `unwind-protect`.

bug#49963: 28.0.50; isearch failing in Dired after rectangle-mark-mode and query-replace

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: larsi@gnus.org,  49963@debbugs.gnu.org,  juri@linkov.net,
>   laslydone@protonmail.com
> Date: Tue, 10 Aug 2021 16:37:22 -0400
> 
> Eli Zaretskii [2021-08-10 19:42:17] wrote:
> >> From: Stefan Monnier <monnier@iro.umontreal.ca>
> >> Cc: Eli Zaretskii <eliz@gnu.org>,  49963@debbugs.gnu.org,  juri@linkov.net,
> >>   laslydone@protonmail.com
> >> Date: Tue, 10 Aug 2021 11:56:58 -0400
> >> 
> >> Lars Ingebrigtsen [2021-08-10 17:44:50] wrote:
> >> > Eli Zaretskii <eliz@gnu.org> writes:
> >> >> I meant can we please not use add-function and friends.  Please?
> >> > There isn't much difference between using add-function and add-hook most
> >> > of the time (although here we trip over a `let' binding not doing what
> >> > the author thought it did).
> >> 
> >> FWIW, `add-hook` has the exact same problem with `let` (and we already
> >> tripped against this exact same situation with `add-hook` and `let`).
> >
> > This is a misunderstanding of what bothers me.  The problem is
> > discoverability: add-function is not easily discoverable, if it is
> > used on an internal function.
> 
> I'm sorry, I don't understand.  AFAIK it's used on variables (in the
> present case it's used on the variable `isearch-filter-predicate`)
> holding functions, not on internal functions.

And if I've said instead

  This is a misunderstanding of what bothers me.  The problem is
  discoverability: add-function is not easily discoverable, if it is
  used on an internal variable.

then you'd understand?  If so, please excuse my silly typo, and please
reply to the amended text above.

> > IOW, the documentation of C-s doesn't tell you that its operation
> > could be affected by that "hook".
> 
> You mean the doc of `C-s` should state that it's affected by
> `isearch-filter-predicate`?
> 
> Fine by me, but I don't know what this has to do with `add-function`
> (`isearch-filter-predicate` existed before `add-function` was invented).

It makes the rabbit hole much deeper and darker.  Because even if and
when I find that isearch-filter-predicate is involved in this (which
isn't easy, see below), its "C-h v" shows the following gobbledygook:

  isearch-filter-predicate is a variable defined in ‘isearch.el’.

  Its value is
  #f(advice-wrapper :after-while #f(compiled-function
				    (&rest args)
				    #<bytecode 0x18003e4619de40ca>)
		    wdired-isearch-filter-read-only)

And that's the second try, after "M-: isearch-filter-predicate RET",
a standard way of figuring out the values of variables,which curses
thusly:

  #[128 "\300\302\x02\"\205\v^@\300\301\x02\"\207" [apply wdired-isearch-filter-read-only #[128 "\301\302\300!\x02\"\207" [isearch-filter-predicate apply default-value] 4 "
  
  (fn &rest ARGS)"] nil] 4 nil]

Why does wdired need to use add-function? why couldn't it simply put
its own function on isearch-filter-predicate's value?

The code in isearch.el is notoriously hard to debug, because it is
written in many layers, uses a lot of function variables and indirect
calls, and almost none of its important subroutines have any useful
doc strings or comments.  And stepping with Edebug through its code is
also not easy because of the way Isearch reads input, which conflicts
with Edebug's SPC-stepping.  So tracking an issue related to
isearch.el is an endless mess of guessing the next candidate for being
part of the puzzle, following the chain of indirect calls through
variables and hooks, then finding the next suspect, etc. etc.

Using add-function on top of that makes a bad problem much worse.  So
I'm asking whether we could improve that by not using add-function.
Bonus points for adding meaningful doc strings and/or comments to
important isearch.el functions and variables so that one could easier
find the possible suspects by following the code and the documentation
instead of stepping blindly through complex code.  TIA.