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.

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: Thu, 31 Oct 2024 13:24:34 +0100
> 
> 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?

AFAIU, whenever deactivate-mark is not local to the buffer which is
being changed.

> > 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.

TIA.

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 Zaretskii <eliz@gnu.org> writes:

>> > 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.
>
> TIA.

FYI after spending a bit more time on this issue, I concluded that
(kill-all-local-variables t) is inherently problematic: it breaks
assumptions that Emacs relies on.  (See bug#73005 for another example.)

It doesn't seem like killing permanent-local variables in the work
buffers is necessary ATM, so the fix I'm using is the following:

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



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@debbugs.gnu.org
> Date: Wed, 06 Nov 2024 09:01:16 +0100
> 
> FYI after spending a bit more time on this issue, I concluded that
> (kill-all-local-variables t) is inherently problematic: it breaks
> assumptions that Emacs relies on.  (See bug#73005 for another example.)
> 
> It doesn't seem like killing permanent-local variables in the work
> buffers is necessary ATM, so the fix I'm using is the following:

Can't _any_ variable become permanent-local, by virtue of some Lisp
program or the user giving it a permanent-local property?

More generally, how do we know that there are no permanent-local
variables out there that affect layout, and thus affect the results of
this function?

I believe these considerations were those which led the author to use
kill-all-local-variables like this: we want to be sure that nothing
will dupe string-pixel-width into producing unexpected results.

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, 06 Nov 2024 09:01:16 +0100
>> 
>> FYI after spending a bit more time on this issue, I concluded that
>> (kill-all-local-variables t) is inherently problematic: it breaks
>> assumptions that Emacs relies on.  (See bug#73005 for another example.)
>> 
>> It doesn't seem like killing permanent-local variables in the work
>> buffers is necessary ATM, so the fix I'm using is the following:
>
> Can't _any_ variable become permanent-local, by virtue of some Lisp
> program or the user giving it a permanent-local property?
>
> More generally, how do we know that there are no permanent-local
> variables out there that affect layout, and thus affect the results of
> this function?
>
> I believe these considerations were those which led the author to use
> kill-all-local-variables like this: we want to be sure that nothing
> will dupe string-pixel-width into producing unexpected results.

Yes, this reasoning is perfectly understandable.  However, the current
way of killing all permanent-local variables has bad consequences that
that author, naturally, did not expect.  So my suggestion is to avoid
(kill-all-local-variables t) here and elsewhere.

For string-pixel-width specifically, we take care to clear variables
that may affect width calculation.  If other variables may interfere, we
should just clear those too.


Cheers,

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, 06 Nov 2024 15:12:24 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Eshel Yaron <me@eshelyaron.com>
> >> Cc: 74091@debbugs.gnu.org
> >> Date: Wed, 06 Nov 2024 09:01:16 +0100
> >> 
> >> FYI after spending a bit more time on this issue, I concluded that
> >> (kill-all-local-variables t) is inherently problematic: it breaks
> >> assumptions that Emacs relies on.  (See bug#73005 for another example.)
> >> 
> >> It doesn't seem like killing permanent-local variables in the work
> >> buffers is necessary ATM, so the fix I'm using is the following:
> >
> > Can't _any_ variable become permanent-local, by virtue of some Lisp
> > program or the user giving it a permanent-local property?
> >
> > More generally, how do we know that there are no permanent-local
> > variables out there that affect layout, and thus affect the results of
> > this function?
> >
> > I believe these considerations were those which led the author to use
> > kill-all-local-variables like this: we want to be sure that nothing
> > will dupe string-pixel-width into producing unexpected results.
> 
> Yes, this reasoning is perfectly understandable.  However, the current
> way of killing all permanent-local variables has bad consequences that
> that author, naturally, did not expect.  So my suggestion is to avoid
> (kill-all-local-variables t) here and elsewhere.
> 
> For string-pixel-width specifically, we take care to clear variables
> that may affect width calculation.  If other variables may interfere, we
> should just clear those too.

Let's see what Stefan (CC'ed) thinks about these issues.

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

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

> 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.

The thing I don't understand is this:

    Why is the global value of `deactivate-mark` non-nil?

After all, since it is buffer-local when set, it should only be non-nil
buffer-locally, so the `kill-all-local-variables` should just through
away the non-nil setting, whereas it seems that somehow the non-nil
setting gets "promoted" to global.

I suspect the problem might be a bug in `reset_buffer_local_variables` around:

    /* Reset all (or most) per-buffer variables to their defaults.  */
    if (permanent_too)
      bset_local_var_alist (b, Qnil);

I suspect this was OK in the "original" uses of `permanent_too` because
"by construction" none of those vars could be "swapped in" (i.e. have
their value held in a C variable like `Vdeactivate_mark`), but we now
have cases where this is not the case any more, so that if the value of
`Vdeactivate_mark` held a buffer-local value, it ends up "promoted"
to global.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eshel Yaron <me@eshelyaron.com>,  74091@debbugs.gnu.org
> Date: Sat, 09 Nov 2024 11:26:48 -0500
> 
> I suspect the problem might be a bug in `reset_buffer_local_variables` around:
> 
>     /* Reset all (or most) per-buffer variables to their defaults.  */
>     if (permanent_too)
>       bset_local_var_alist (b, Qnil);
> 
> I suspect this was OK in the "original" uses of `permanent_too` because
> "by construction" none of those vars could be "swapped in" (i.e. have
> their value held in a C variable like `Vdeactivate_mark`), but we now
> have cases where this is not the case any more, so that if the value of
> `Vdeactivate_mark` held a buffer-local value, it ends up "promoted"
> to global.

Which development in Emacs made such a "promotion" possible?

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

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

>> I suspect the problem might be a bug in `reset_buffer_local_variables` around:
>> 
>>     /* Reset all (or most) per-buffer variables to their defaults.  */
>>     if (permanent_too)
>>       bset_local_var_alist (b, Qnil);
>> 
>> I suspect this was OK in the "original" uses of `permanent_too` because
>> "by construction" none of those vars could be "swapped in" (i.e. have
>> their value held in a C variable like `Vdeactivate_mark`), but we now
>> have cases where this is not the case any more, so that if the value of
>> `Vdeactivate_mark` held a buffer-local value, it ends up "promoted"
>> to global.

Hmm... I was wrong, it's not actually promoted to global, it's just that
the former local value "lingers".

> Which development in Emacs made such a "promotion" possible?

AFAICT the problem is new with the addition of the `kill-permanent`
arg to `kill-all-local-variables`.

Here's an example of a problem:

    src/emacs -Q --batch --eval '(message "%s" (list (setq-default foo nil) (setq-local foo t) (kill-all-local-variables t) (local-variable-p `foo) foo (default-value `foo) (with-temp-buffer foo) foo))'

=>

    (nil t nil t t nil nil nil)

If we call `kill-all-local-variables` without the additional argument, we
get the correct answer:

    (nil t nil nil nil nil nil nil)


- Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: me@eshelyaron.com,  74091@debbugs.gnu.org
> Date: Sat, 09 Nov 2024 13:06:13 -0500
> 
> AFAICT the problem is new with the addition of the `kill-permanent`
> arg to `kill-all-local-variables`.
> 
> Here's an example of a problem:
> 
>     src/emacs -Q --batch --eval '(message "%s" (list (setq-default foo nil) (setq-local foo t) (kill-all-local-variables t) (local-variable-p `foo) foo (default-value `foo) (with-temp-buffer foo) foo))'
> 
> =>
> 
>     (nil t nil t t nil nil nil)
> 
> If we call `kill-all-local-variables` without the additional argument, we
> get the correct answer:
> 
>     (nil t nil nil nil nil nil nil)

This feature was introduced in Emacs 29, so we should preferably fix
it in Emacs 30.  Any suggestions?

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

[Eli Zaretskii]

> Cc: me@eshelyaron.com, 74091@debbugs.gnu.org
> Date: Sat, 09 Nov 2024 20:14:43 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Stefan Monnier <monnier@iro.umontreal.ca>
> > Cc: me@eshelyaron.com,  74091@debbugs.gnu.org
> > Date: Sat, 09 Nov 2024 13:06:13 -0500
> > 
> > AFAICT the problem is new with the addition of the `kill-permanent`
> > arg to `kill-all-local-variables`.
> > 
> > Here's an example of a problem:
> > 
> >     src/emacs -Q --batch --eval '(message "%s" (list (setq-default foo nil) (setq-local foo t) (kill-all-local-variables t) (local-variable-p `foo) foo (default-value `foo) (with-temp-buffer foo) foo))'
> > 
> > =>
> > 
> >     (nil t nil t t nil nil nil)
> > 
> > If we call `kill-all-local-variables` without the additional argument, we
> > get the correct answer:
> > 
> >     (nil t nil nil nil nil nil nil)
> 
> This feature was introduced in Emacs 29, so we should preferably fix
> it in Emacs 30.  Any suggestions?

I think calling reset_buffer_local_variables with 2nd arg non-zero is
not TRT to implement this new argument of kill-all-local-variables,
because it does this:

  /* Reset all (or most) per-buffer variables to their defaults.  */
  if (permanent_too)
    bset_local_var_alist (b, Qnil);

I think we should instead ignore the permanent-local property of
variables.  How about the patch below?

diff --git a/src/buffer.c b/src/buffer.c
index 744b0ef..9e93e0f 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -113,7 +113,7 @@ #define OVERLAY_COUNT_MAX						\
 static void call_overlay_mod_hooks (Lisp_Object list, Lisp_Object overlay,
                                     bool after, Lisp_Object arg1,
                                     Lisp_Object arg2, Lisp_Object arg3);
-static void reset_buffer_local_variables (struct buffer *, bool);
+static void reset_buffer_local_variables (struct buffer *, int);
 
 /* Alist of all buffer names vs the buffers.  This used to be
    a Lisp-visible variable, but is no longer, to prevent lossage
@@ -1112,10 +1112,11 @@ reset_buffer (register struct buffer *b)
    Instead, use Fkill_all_local_variables.
 
    If PERMANENT_TOO, reset permanent buffer-local variables.
-   If not, preserve those.  */
+   If not, preserve those.  PERMANENT_TOO = 2 means ignore
+   the permanent-local property of non-builtin variables.  */
 
 static void
-reset_buffer_local_variables (struct buffer *b, bool permanent_too)
+reset_buffer_local_variables (struct buffer *b, int permanent_too)
 {
   int offset, i;
 
@@ -1141,7 +1142,7 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
   bset_invisibility_spec (b, Qt);
 
   /* Reset all (or most) per-buffer variables to their defaults.  */
-  if (permanent_too)
+  if (permanent_too == 1)
     bset_local_var_alist (b, Qnil);
   else
     {
@@ -1170,7 +1171,7 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
 	      swap_in_global_binding (XSYMBOL (sym));
 	    }
 
-          if (!NILP (prop))
+          if (!NILP (prop) && permanent_too)
             {
               /* If permanent-local, keep it.  */
               last = tmp;
@@ -3001,7 +3002,7 @@ DEFUN ("kill-all-local-variables", Fkill_all_local_variables,
 
   /* Actually eliminate all local bindings of this buffer.  */
 
-  reset_buffer_local_variables (current_buffer, !NILP (kill_permanent));
+  reset_buffer_local_variables (current_buffer, !NILP (kill_permanent) ? 2 : 0);
 
   /* Force mode-line redisplay.  Useful here because all major mode
      commands call this function.  */

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

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

> I think we should instead ignore the permanent-local property of
> variables.  How about the patch below?

Funny: I went through 3 different versions of a patch and ended up with
basically the same patch as yours.

> @@ -1170,7 +1171,7 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
>  	      swap_in_global_binding (XSYMBOL (sym));
>  	    }
>  
> -          if (!NILP (prop))
> +          if (!NILP (prop) && permanent_too)
>              {
>                /* If permanent-local, keep it.  */
>                last = tmp;

This should be an || rather than an &&, no?


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: me@eshelyaron.com,  74091@debbugs.gnu.org
> Date: Sat, 09 Nov 2024 17:11:19 -0500
> 
> > I think we should instead ignore the permanent-local property of
> > variables.  How about the patch below?
> 
> Funny: I went through 3 different versions of a patch and ended up with
> basically the same patch as yours.

;-)

> > @@ -1170,7 +1171,7 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
> >  	      swap_in_global_binding (XSYMBOL (sym));
> >  	    }
> >  
> > -          if (!NILP (prop))
> > +          if (!NILP (prop) && permanent_too)
> >              {
> >                /* If permanent-local, keep it.  */
> >                last = tmp;
> 
> This should be an || rather than an &&, no?

No, I don't think so.  If the permanent-local property is nil, we
should not take this branch, even if permanent_too is non-zero.  Or
what am I missing?

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

[Eli Zaretskii]

> Cc: me@eshelyaron.com, 74091@debbugs.gnu.org
> Date: Sun, 10 Nov 2024 07:50:47 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > > @@ -1170,7 +1171,7 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
> > >  	      swap_in_global_binding (XSYMBOL (sym));
> > >  	    }
> > >  
> > > -          if (!NILP (prop))
> > > +          if (!NILP (prop) && permanent_too)
> > >              {
> > >                /* If permanent-local, keep it.  */
> > >                last = tmp;
> > 
> > This should be an || rather than an &&, no?
> 
> No, I don't think so.  If the permanent-local property is nil, we
> should not take this branch, even if permanent_too is non-zero.  Or
> what am I missing?

Hmm... but something is still amiss, because Emacs built with my patch
comes up without fontifications, although font-lock-mode is t.  Any
ideas?

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

[Eli Zaretskii]

> Cc: me@eshelyaron.com, 74091@debbugs.gnu.org
> Date: Sun, 10 Nov 2024 08:50:49 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > Cc: me@eshelyaron.com, 74091@debbugs.gnu.org
> > Date: Sun, 10 Nov 2024 07:50:47 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> > 
> > > > @@ -1170,7 +1171,7 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
> > > >  	      swap_in_global_binding (XSYMBOL (sym));
> > > >  	    }
> > > >  
> > > > -          if (!NILP (prop))
> > > > +          if (!NILP (prop) && permanent_too)
> > > >              {
> > > >                /* If permanent-local, keep it.  */
> > > >                last = tmp;
> > > 
> > > This should be an || rather than an &&, no?
> > 
> > No, I don't think so.  If the permanent-local property is nil, we
> > should not take this branch, even if permanent_too is non-zero.  Or
> > what am I missing?
> 
> Hmm... but something is still amiss, because Emacs built with my patch
> comes up without fontifications, although font-lock-mode is t.  Any
> ideas?

Sorry, here's the correct patch:

diff --git a/src/buffer.c b/src/buffer.c
index 744b0ef..995984e 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -113,7 +113,7 @@ #define OVERLAY_COUNT_MAX						\
 static void call_overlay_mod_hooks (Lisp_Object list, Lisp_Object overlay,
                                     bool after, Lisp_Object arg1,
                                     Lisp_Object arg2, Lisp_Object arg3);
-static void reset_buffer_local_variables (struct buffer *, bool);
+static void reset_buffer_local_variables (struct buffer *, int);
 
 /* Alist of all buffer names vs the buffers.  This used to be
    a Lisp-visible variable, but is no longer, to prevent lossage
@@ -1112,10 +1112,11 @@ reset_buffer (register struct buffer *b)
    Instead, use Fkill_all_local_variables.
 
    If PERMANENT_TOO, reset permanent buffer-local variables.
-   If not, preserve those.  */
+   If not, preserve those.  PERMANENT_TOO = 2 means ignore
+   the permanent-local property of non-builtin variables.  */
 
 static void
-reset_buffer_local_variables (struct buffer *b, bool permanent_too)
+reset_buffer_local_variables (struct buffer *b, int permanent_too)
 {
   int offset, i;
 
@@ -1141,7 +1142,7 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
   bset_invisibility_spec (b, Qt);
 
   /* Reset all (or most) per-buffer variables to their defaults.  */
-  if (permanent_too)
+  if (permanent_too == 1)
     bset_local_var_alist (b, Qnil);
   else
     {
@@ -1170,7 +1171,7 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
 	      swap_in_global_binding (XSYMBOL (sym));
 	    }
 
-          if (!NILP (prop))
+          if (!NILP (prop) && !permanent_too)
             {
               /* If permanent-local, keep it.  */
               last = tmp;
@@ -3001,7 +3002,7 @@ DEFUN ("kill-all-local-variables", Fkill_all_local_variables,
 
   /* Actually eliminate all local bindings of this buffer.  */
 
-  reset_buffer_local_variables (current_buffer, !NILP (kill_permanent));
+  reset_buffer_local_variables (current_buffer, !NILP (kill_permanent) ? 2 : 0);
 
   /* Force mode-line redisplay.  Useful here because all major mode
      commands call this function.  */

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

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

> Sorry, here's the correct patch:

Oh, right, I had !(...||...)
Thanks, LGTM!


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: me@eshelyaron.com,  74091@debbugs.gnu.org
> Date: Sun, 10 Nov 2024 11:46:47 -0500
> 
> > Sorry, here's the correct patch:
> 
> Oh, right, I had !(...||...)
> Thanks, LGTM!

Thanks, installed on the emacs-30 branch.

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: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: me@eshelyaron.com,  74091@debbugs.gnu.org
>> Date: Sun, 10 Nov 2024 11:46:47 -0500
>> 
>> > Sorry, here's the correct patch:
>> 
>> Oh, right, I had !(...||...)
>> Thanks, LGTM!
>
> Thanks, installed on the emacs-30 branch.

Thank you both for getting to the bottom of this issue.  Note that once
this fix lands on master, we should be able to revert 57fe24961fd.
Works well for me so far.

Best,

Eshel

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

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  74091@debbugs.gnu.org
> Date: Sun, 10 Nov 2024 21:19:58 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Stefan Monnier <monnier@iro.umontreal.ca>
> >> Cc: me@eshelyaron.com,  74091@debbugs.gnu.org
> >> Date: Sun, 10 Nov 2024 11:46:47 -0500
> >> 
> >> > Sorry, here's the correct patch:
> >> 
> >> Oh, right, I had !(...||...)
> >> Thanks, LGTM!
> >
> > Thanks, installed on the emacs-30 branch.
> 
> Thank you both for getting to the bottom of this issue.  Note that once
> this fix lands on master, we should be able to revert 57fe24961fd.
> Works well for me so far.

I see no need to revert that commit, it doesn't cause any problems
AFAICT.

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: Stefan Monnier <monnier@iro.umontreal.ca>,  74091@debbugs.gnu.org
>> Date: Sun, 10 Nov 2024 21:19:58 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> >> Cc: me@eshelyaron.com,  74091@debbugs.gnu.org
>> >> Date: Sun, 10 Nov 2024 11:46:47 -0500
>> >> 
>> >> > Sorry, here's the correct patch:
>> >> 
>> >> Oh, right, I had !(...||...)
>> >> Thanks, LGTM!
>> >
>> > Thanks, installed on the emacs-30 branch.
>> 
>> Thank you both for getting to the bottom of this issue.  Note that once
>> this fix lands on master, we should be able to revert 57fe24961fd.
>> Works well for me so far.
>
> I see no need to revert that commit, it doesn't cause any problems
> AFAICT.

The potential problem is misleading or confusing folks reading this code
in the future, since it implies (quite explicitly with the comments)
that there's a need to bind deactivate-mark, where in fact there isn't.
But OK, it's a small concern, so if you find it harmless I won't insist.


Eshel