bug#74091: 31.0.50; string-pixel-width in mode line disables region

bug#74091: 31.0.50; string-pixel-width in mode line disables region

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

The new implementation of string-pixel-width has some unexpected effect
when it is called from mode-line-format, as happens for example when
mode-line-format-right-align occurs in mode-line-format:

1. emacs -Q
2. (setq-default mode-line-format
	      '("" (:eval (progn (string-pixel-width "foo") nil))))
3. C-x C-f /path/to/emacs/lisp/subr.el
4. C-SPC
5. C-n

At this point the region is expected to be active since we activated it
in step 4.  But in step 5 the mode line is updated, which calls
string-pixel-width, which in turn unexpectedly disables the region.

I'm not really sure why this happens...  Also, if I now trace
string-pixel-width, then it no longer disables the region and everything
starts working as expected.


Thanks,

Eshel

bug#74091: 31.0.50; string-pixel-width in mode line disables region

[Eli Zaretskii]

> Date: Tue, 29 Oct 2024 18:27:05 +0100
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> 
> 
> The new implementation of string-pixel-width has some unexpected effect
> when it is called from mode-line-format, as happens for example when
> mode-line-format-right-align occurs in mode-line-format:
> 
> 1. emacs -Q
> 2. (setq-default mode-line-format
> 	      '("" (:eval (progn (string-pixel-width "foo") nil))))
> 3. C-x C-f /path/to/emacs/lisp/subr.el
> 4. C-SPC
> 5. C-n
> 
> At this point the region is expected to be active since we activated it
> in step 4.  But in step 5 the mode line is updated, which calls
> string-pixel-width, which in turn unexpectedly disables the region.

Thanks, should be fixed now.

> I'm not really sure why this happens...

It happens because string-pixel-width modifies a buffer, and that sets
deactivate-mark, which then causes the region to be deactivated when
a command finishes.  When you inject string-pixel-width into
mode-line-format, you indirectly cause it to be called from C-n and
the like, because those evaluate the mode-line format.  So doing that
is quite a risky thing, in general.

bug#74091: 31.0.50; string-pixel-width in mode line disables region

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

Hi Eli,

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Tue, 29 Oct 2024 18:27:05 +0100
>> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> 
>> 
>> The new implementation of string-pixel-width has some unexpected effect
>> when it is called from mode-line-format, as happens for example when
>> mode-line-format-right-align occurs in mode-line-format:
>> 
>> 1. emacs -Q
>> 2. (setq-default mode-line-format
>> 	      '("" (:eval (progn (string-pixel-width "foo") nil))))
>> 3. C-x C-f /path/to/emacs/lisp/subr.el
>> 4. C-SPC
>> 5. C-n
>> 
>> At this point the region is expected to be active since we activated it
>> in step 4.  But in step 5 the mode line is updated, which calls
>> string-pixel-width, which in turn unexpectedly disables the region.
>
> Thanks, should be fixed now.

Great!  That seems to work.

>> I'm not really sure why this happens...
>
> It happens because string-pixel-width modifies a buffer, and that sets
> deactivate-mark, which then causes the region to be deactivated when
> a command finishes.

Hmm but string-pixel-width used to modify a buffer also in the old
implementation, and that never caused this issue...  And in both the old
implementation and in the new one, the modification is in a different
buffer, is that expected to disable the mark in the original buffer?

> When you inject string-pixel-width into mode-line-format, you
> indirectly cause it to be called from C-n and the like, because those
> evaluate the mode-line format.  So doing that is quite a risky thing,
> in general.

Well, that's how Emacs implements mode-line-format-right-align :)


Thanks for the quick response,

Eshel

bug#74091: 31.0.50; string-pixel-width in mode line disables region

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 74091@debbugs.gnu.org
> Date: Wed, 30 Oct 2024 16:26:38 +0100
> 
> >> At this point the region is expected to be active since we activated it
> >> in step 4.  But in step 5 the mode line is updated, which calls
> >> string-pixel-width, which in turn unexpectedly disables the region.
> >
> > Thanks, should be fixed now.
> 
> Great!  That seems to work.

Thanks for testing, I will close this bug.

> >> I'm not really sure why this happens...
> >
> > It happens because string-pixel-width modifies a buffer, and that sets
> > deactivate-mark, which then causes the region to be deactivated when
> > a command finishes.
> 
> Hmm but string-pixel-width used to modify a buffer also in the old
> implementation, and that never caused this issue...

The new implementation also didn't cause this issue in some buffers.
For example, in *scratch*.  Trying to understand the logic of a bug is
never a good investment of time.

> And in both the old
> implementation and in the new one, the modification is in a different
> buffer, is that expected to disable the mark in the original buffer?

The variable deactivate-mark only becomes buffer-local if set;
otherwise the global value will be changed.

> > When you inject string-pixel-width into mode-line-format, you
> > indirectly cause it to be called from C-n and the like, because those
> > evaluate the mode-line format.  So doing that is quite a risky thing,
> > in general.
> 
> Well, that's how Emacs implements mode-line-format-right-align :)

One reason why I don't like it.  mode-line-format is evaluated in many
more contexts than most people realize, so putting arbitrary calls
there without a good understanding what those calls do and how is not
the best idea, although it will mostly work.

bug#74091: 31.0.50; string-pixel-width in mode line disables region

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: 74091@debbugs.gnu.org
>> Date: Wed, 30 Oct 2024 16:26:38 +0100
>> 
>> >> At this point the region is expected to be active since we activated it
>> >> in step 4.  But in step 5 the mode line is updated, which calls
>> >> string-pixel-width, which in turn unexpectedly disables the region.
>> >
>> > Thanks, should be fixed now.
>> 
>> Great!  That seems to work.
>
> Thanks for testing, I will close this bug.

Since we don't fully understand the issue, it may manifest in more ways.
So I think closing the bug is premature.

After looking a bit more into it, it seems like the problem has to do
with the call to kill-all-local-variables in work-buffer--release, the
following patch circumvents the unexpected behavior, although I still
don't understand why:

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 5b47deb880e..b5cbe28afad 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -361,7 +361,7 @@ work-buffer--release
           (erase-buffer))
         (delete-all-overlays)
         (let (change-major-mode-hook)
-          (kill-all-local-variables t))
+          (kill-all-local-variables))
         ;; Make the buffer available again.
         (push buffer work-buffer--list)))
   ;; If the maximum number of reusable work buffers is exceeded, kill


Here's another interesting data point:

--8<---------------cut here---------------start------------->8---
(defun foo ()
  (interactive)
  (with-current-buffer (get-buffer-create "some-buffer")
    (kill-all-local-variables t)
    (insert "foo")))
--8<---------------cut here---------------end--------------->8---

Invoking this command twice in a row in subr.el deactivates the region,
while the same without the argument to kill-all-local-variables keeps
the region active.

So the problem seems to be in a lower level than string-pixel-width...

>> >> I'm not really sure why this happens...
>> >
>> > It happens because string-pixel-width modifies a buffer, and that sets
>> > deactivate-mark, which then causes the region to be deactivated when
>> > a command finishes.
>> 
>> Hmm but string-pixel-width used to modify a buffer also in the old
>> implementation, and that never caused this issue...
>
> The new implementation also didn't cause this issue in some buffers.
> For example, in *scratch*.  Trying to understand the logic of a bug is
> never a good investment of time.

:)

As I'm sure you know, applying a crude fix without fully understanding
the problem is likely to hide other subtle bugs that may then be harder
to investigate.

>> And in both the old
>> implementation and in the new one, the modification is in a different
>> buffer, is that expected to disable the mark in the original buffer?
>
> The variable deactivate-mark only becomes buffer-local if set;
> otherwise the global value will be changed.

Could you perhaps elaborate?  I see that running a command that modifies
a different buffer does not deactivate the region in the current buffer,
which is basically what I would expect.


Best,

Eshel

bug#74091: 31.0.50; string-pixel-width in mode line disables region

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 74091-done@debbugs.gnu.org
> Date: Thu, 31 Oct 2024 12:09:20 +0100
> 
> Invoking this command twice in a row in subr.el deactivates the region,
> while the same without the argument to kill-all-local-variables keeps
> the region active.
> 
> So the problem seems to be in a lower level than string-pixel-width...

Killing local variables makes the global value of deactivate-mark be
in effect when the command loop decides whether to deactivate the
region after a command finishes.

> As I'm sure you know, applying a crude fix without fully understanding
> the problem is likely to hide other subtle bugs that may then be harder
> to investigate.

That's not a crude fix, that's what the ELisp manual tells us to do:

 -- Variable: deactivate-mark
     If an editor command sets this variable non-‘nil’, then the editor
     command loop deactivates the mark after the command returns (if
     Transient Mark mode is enabled).  All the primitives that change
     the buffer set ‘deactivate-mark’, to deactivate the mark when the
     command is finished.  Setting this variable makes it buffer-local.

     To write Lisp code that modifies the buffer without causing
     deactivation of the mark at the end of the command, bind
     ‘deactivate-mark’ to ‘nil’ around the code that does the
     modification.  For example:

          (let (deactivate-mark)
            (insert " "))

> >> And in both the old
> >> implementation and in the new one, the modification is in a different
> >> buffer, is that expected to disable the mark in the original buffer?
> >
> > The variable deactivate-mark only becomes buffer-local if set;
> > otherwise the global value will be changed.
> 
> Could you perhaps elaborate?  I see that running a command that modifies
> a different buffer does not deactivate the region in the current buffer,
> which is basically what I would expect.

You are asking me to elaborate about what? about the local value of
deactivate-mark or about why you see what you see (in a scenario you
haven't described)?

Look, you are welcome to keep debugging this if you are interested.  I
invested enough of my time into figuring out why the region was
deactivated by C-n, and the solution I installed satisfies me.  But
you are welcome to keep digging, and let me tell you what I found to
save you some non-trivial tinkering:

 . The region is deactivated because of this in our command loop:

	  if (!NILP (Vdeactivate_mark))
	    /* If `select-active-regions' is non-nil, this call to
	       `deactivate-mark' also sets the PRIMARY selection.  */
	    call0 (Qdeactivate_mark);

   This is consistent with what the ELisp manual says, see the above
   citation.

 . Deactivate-mark is set non-nil by the low-level subroutines that
   modify the buffer, again according to the manual.  Two such
   buffer-modification calls were present in string-pixel-width: one
   in string-pixel-width itself, when it inserts the string into a
   work buffer, the other in work-buffer--release where it erases the
   work buffer.  This happens in in prepare_to_modify_buffer_1:

    signal_before_change (start, end, preserve_ptr);
    Fset (Qdeactivate_mark, Qt);

 . Here is a Lisp-level backtrace from one call which sets
   deactivate-mark in your recipe, as collected by GDB:

    Lisp Backtrace:
    "string-pixel-width" (0x65a8940)
    "progn" (0x65a8c40)
    "eval" (0x65a8f90)
    "posn-at-point" (0xa084200)
    "line-move-visual" (0xa084170)
    "line-move" (0xa084108)
    "next-line" (0x65aea10)
    "funcall-interactively" (0x65aea08)
    "call-interactively" (0xa084078)
    "command-execute" (0x65af798)

   As you see, next-line calls posn-at-point, which formats the mode
   line (to calculate is height), and that invokes the :eval form.
   Here's the C backtrace which captures the details missing from the
   above Lisp backtrace:

   Thread 1 hit Hardware watchpoint 4: Vdeactivate_mark

   Old value = XIL(0)
   New value = XIL(0x30)
   0x00bd2da5 in store_symval_forwarding (valcontents=..., newval=XIL(0x30),
       buf=0x8f4c48) at data.c:1430
   1430          *XOBJFWD (valcontents)->objvar = newval;
   (gdb) bt
   #0  0x00bd2da5 in store_symval_forwarding (valcontents=..., newval=XIL(0x30),
       buf=0x8f4c48) at data.c:1430
   #1  0x00bd3da3 in set_internal (symbol=XIL(0x6210), newval=XIL(0x30),
       where=XIL(0xa0000000008f4c48), bindflag=SET_INTERNAL_SET) at data.c:1759
   #2  0x00bd37c5 in Fset (symbol=XIL(0x6210), newval=XIL(0x30)) at data.c:1630
   #3  0x00b5b757 in prepare_to_modify_buffer_1 (start=1, end=1, preserve_ptr=0x0)
       at insdel.c:2073
   #4  0x00b5b784 in prepare_to_modify_buffer (start=1, end=1, preserve_ptr=0x0)
       at insdel.c:2083
   #5  0x00b57e1b in insert_from_string_1 (string=XIL(0x800000000bc07ad8), pos=0,
       pos_byte=0, nchars=3, nbytes=3, inherit=false, before_markers=false)
       at insdel.c:1023
   #6  0x00b57c15 in insert_from_string (string=XIL(0x800000000bc07ad8), pos=0,
       pos_byte=0, length=3, length_byte=3, inherit=false) at insdel.c:974
   #7  0x00be5dbf in general_insert_function (insert_func=0xb57249 <insert>,
       insert_from_string_func=0xb57b92 <insert_from_string>, inherit=false,
       nargs=1, args=0xa084250) at editfns.c:1336
   #8  0x00be5e59 in Finsert (nargs=1, args=0xa084250) at editfns.c:1372
   #9  0x00c71edc in exec_byte_code (fun=XIL(0xa0000000008f45b0),
       args_template=513, nargs=1, args=0x65a8948) at bytecode.c:1417
   #10 0x00c019a3 in funcall_lambda (fun=XIL(0xa0000000008f45b0), nargs=1,
       arg_vector=0x65a8940) at eval.c:3238
   #11 0x00c017be in apply_lambda (fun=XIL(0xa0000000008f45b0),
       args=XIL(0xc00000000071c2d0), count=640) at eval.c:3201
   #12 0x00bff56b in eval_sub (form=XIL(0xc00000000071c2e0)) at eval.c:2631
   #13 0x00bf7cde in Fprogn (body=XIL(0xc00000000071c2b0)) at eval.c:439
   #14 0x00bfec17 in eval_sub (form=XIL(0xc00000000071c2f0)) at eval.c:2535
   #15 0x00bfe644 in Feval (form=XIL(0xc00000000071c2f0), lexical=XIL(0x30))
       at eval.c:2448
   #16 0x00c010df in funcall_subr (subr=0x128a1c0 <Seval>, numargs=2,
       args=0x65a8f90) at eval.c:3149
   #17 0x00c00a02 in funcall_general (fun=XIL(0xa00000000128a1c0), numargs=2,
       args=0x65a8f90) at eval.c:3026
   #18 0x00c00d92 in Ffuncall (nargs=3, args=0x65a8f88) at eval.c:3079
   #19 0x00bfbd5f in internal_condition_case_n (bfun=0xc00c46 <Ffuncall>,
       nargs=3, args=0x65a8f88, handlers=XIL(0x30),
       hfun=0x9dfc51 <dsafe_eval_handler>) at eval.c:1687
   #20 0x009dfd7d in dsafe__call (inhibit_quit=true, f=0xc00c46 <Ffuncall>,
       nargs=3, args=0x65a8f88) at xdisp.c:3093
   #21 0x009dfef9 in dsafe_eval (sexpr=XIL(0xc00000000071c2f0)) at xdisp.c:3129
   #22 0x00a3033e in display_mode_element (it=0x65a93d0, depth=2, field_width=0,
       precision=0, elt=XIL(0xc00000000071c300), props=XIL(0), risky=false)
       at xdisp.c:28039
   #23 0x00a308f2 in display_mode_element (it=0x65a93d0, depth=1, field_width=0,
       precision=0, elt=XIL(0xc00000000071c290), props=XIL(0), risky=false)
       at xdisp.c:28125
   #24 0x00a2e976 in display_mode_line (w=0xbb0b428,
       face_id=MODE_LINE_ACTIVE_FACE_ID, format=XIL(0xc00000000071c310))
       at xdisp.c:27550
   #25 0x009d54b2 in pos_visible_p (w=0xbb0b428, charpos=1, x=0x65adfec,
       y=0x65adfe8, rtop=0x65adffc, rbot=0x65adff8, rowh=0x65adff4,
       vpos=0x65adff0) at xdisp.c:1732
   #26 0x00a65e8e in Fpos_visible_in_window_p (pos=XIL(0),
       window=XIL(0xa00000000bb0b428), partially=XIL(0x30)) at window.c:2018
   #27 0x00b2810d in Fposn_at_point (pos=XIL(0), window=XIL(0xa00000000bb0b428))
       at keyboard.c:12552

   Translation: posn-at-point called pos-visible-in-window-p, , which
   called pos_visible_p, which called display_mode_line.  That
   eventually called the :eval form, and inserted the string via
   insert_from_string_1, which called prepare_to_modify_buffer_1,
   which set deactivate-mark to t.

That's what I saw and what led me to my solution, according to what
the ELisp manual says.

bug#74091: 31.0.50; string-pixel-width in mode line disables region

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

Eli Zaretskii <eliz@gnu.org> writes:

>> > The variable deactivate-mark only becomes buffer-local if set;
>> > otherwise the global value will be changed.
>> 
>> Could you perhaps elaborate?  I see that running a command that modifies
>> a different buffer does not deactivate the region in the current buffer,
>> which is basically what I would expect.
>
> You are asking me to elaborate about what? about the local value of
> deactivate-mark or about why you see what you see (in a scenario you
> haven't described)?

What I'm asking is: under what circumstances is it expected that
after changing deactivate-mark in another buffer the mark is deactivated
in the current buffer?

> Look, you are welcome to keep debugging this if you are interested.  I
> invested enough of my time into figuring out why the region was
> deactivated by C-n, and the solution I installed satisfies me.  But
> you are welcome to keep digging, and let me tell you what I found to
> save you some non-trivial tinkering:

Thank you, I'll keep digging and let you know if I figure it out.