bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Pip Cet]

In emacs -Q, evaluating:

(let ((buffer1 (generate-new-buffer "A"))
      (buffer2 (generate-new-buffer "B")))
  (with-current-buffer buffer2
    (insert "BBB"))
  (with-current-buffer buffer1
    (add-hook 'after-change-functions
          (lambda (beg end len)
        (message "%S %S %S"
             beg end len))
          nil t)
    (put-text-property 1 4 'read-only t buffer2)))

results in a "1 4 3" message. I would have expected no message, as
buffer2 was modified and buffer1, whose after-change-functions I'd
set, wasn't.

I've looked at the code, and it appears no particular provisions are
being made to make sure we switch to the modified buffer before
calling signal_after_change().

As far as I can tell, this makes `put-text-property' with a buffer
argument pretty useless. Am I missing something? Is this expected
behavior somehow?

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Thu, 13 Jun 2019 13:48:40 +0000
> 
> I've looked at the code, and it appears no particular provisions are
> being made to make sure we switch to the modified buffer before
> calling signal_after_change().
> 
> As far as I can tell, this makes `put-text-property' with a buffer
> argument pretty useless.

Only if you have a buffer-local value of after-change-functions.

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Pip Cet]

[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]

On Thu, Jun 13, 2019 at 4:36 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Thu, 13 Jun 2019 13:48:40 +0000
> >
> > I've looked at the code, and it appears no particular provisions are
> > being made to make sure we switch to the modified buffer before
> > calling signal_after_change().
> >
> > As far as I can tell, this makes `put-text-property' with a buffer
> > argument pretty useless.
>
> Only if you have a buffer-local value of after-change-functions.

I'm not sure what you're saying. I'm seeing weird behavior in these cases:
 - buffer-local value of after-change-functions
 - global value of after-change functions (current-buffer is wrong!)
 - overlay property modification-hooks

(let ((buffer1 (generate-new-buffer "A"))
      (buffer2 (generate-new-buffer "B")))
  (with-current-buffer buffer2
    (insert "BBB"))
  (with-current-buffer buffer1
    (insert "AAA")
    (overlay-put (make-overlay 1 4 buffer1) 'modification-hooks
         (list (lambda (&rest args)
           (message "%S"
                args))))
    (goto-char 2)
    (insert "AAA")
    (put-text-property 1 4 'read-only t buffer2)))

produces three calls to the lambda, but should produce only two. (The
last_overlay_modification_hooks logic is a bit weird, thus the second
insertion).

That seems pretty wrong to me. In which cases do you think we're
seeing the right behavior?

Here's a first patch, which adds a "buffer" argument to
signal_after_change, to be explicit about where the change happens. It
should be pretty cheap in the case where we don't switch buffers.

[-- Attachment #2: 0001-Switch-to-modified-buffer-in-signal_after_change.patch --]
[-- Type: text/x-patch, Size: 17265 bytes --]

From ff40a90cc1e4ade9848b6cb01ea9e415d4e82d8f Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Thu, 13 Jun 2019 14:06:46 +0000
Subject: [PATCH] Switch to modified buffer in signal_after_change

* src/insdel.c (signal_after_change): Add buffer argument.
* src/casefiddle.c (casify_region),
  src/decompress.c (unwind_decompress, Fzlib_decompress_region),
  src/editfns.c (Freplace_buffer_contents, Ftranslate_region_internal,
  Ftranspose_regions), src/fileio.c (Finsert_file_contents),
  src/fns.c (Fbase64_decode_region), src/insdel.c (insert,
  insert_and_inherit, insert_before_markers,
  insert_before_markers_and_inherit, insert_from_string,
  insert_from_string_before_markers, insert_from_buffer,
  replace_range, del_range_1, del_range_byte, del_range_both,
  Fcombine_after_change_execute), src/json.c (Fjson_insert),
  src/print.c (PRINTFINISH): use current_buffer.
* src/textprop.c (add_text_properties_1, set_text_properties,
  Fremove_text_properties, Fremove_list_of_text_properties): use
  correct buffer.
---
 src/casefiddle.c |  2 +-
 src/decompress.c |  4 ++--
 src/editfns.c    |  8 ++++----
 src/fileio.c     |  2 +-
 src/fns.c        |  2 +-
 src/insdel.c     | 30 +++++++++++++++++-------------
 src/json.c       |  2 +-
 src/lisp.h       |  2 +-
 src/print.c      |  3 ++-
 src/textprop.c   | 22 +++++++++++-----------
 10 files changed, 41 insertions(+), 36 deletions(-)

diff --git a/src/casefiddle.c b/src/casefiddle.c
index 3f407eaded..d7e29d10ae 100644
--- a/src/casefiddle.c
+++ b/src/casefiddle.c
@@ -509,7 +509,7 @@ casify_region (enum case_action flag, Lisp_Object b, Lisp_Object e)
 
   if (start >= 0)
     {
-      signal_after_change (start, end - start - added, end - start);
+      signal_after_change (current_buffer, start, end - start - added, end - start);
       update_compositions (start, end, CHECK_ALL);
     }
 
diff --git a/src/decompress.c b/src/decompress.c
index 4ca6a50b2a..831cc78fe5 100644
--- a/src/decompress.c
+++ b/src/decompress.c
@@ -88,7 +88,7 @@ unwind_decompress (void *ddata)
       update_compositions (data->start, data->start, CHECK_HEAD);
       /* "Balance" the before-change-functions call, which would
          otherwise be left "hanging". */
-      signal_after_change (data->orig, data->start - data->orig,
+      signal_after_change (current_buffer, data->orig, data->start - data->orig,
                            data->start - data->orig);
     }
   /* Put point where it was, or if the buffer has shrunk because the
@@ -227,7 +227,7 @@ DEFUN ("zlib-decompress-region", Fzlib_decompress_region,
   del_range_2 (istart, istart, /* byte and char offsets are the same. */
                iend, iend, 0);
 
-  signal_after_change (istart, iend - istart, unwind_data.nbytes);
+  signal_after_change (current_buffer, istart, iend - istart, unwind_data.nbytes);
   update_compositions (istart, istart, CHECK_HEAD);
 
   return unbind_to (count, ret);
diff --git a/src/editfns.c b/src/editfns.c
index ee538e50e2..9ada23ff61 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -2148,7 +2148,7 @@ DEFUN ("replace-buffer-contents", Freplace_buffer_contents,
 
   if (modification_hooks_inhibited)
     {
-      signal_after_change (BEGV, size_a, ZV - BEGV);
+      signal_after_change (current_buffer, BEGV, size_a, ZV - BEGV);
       update_compositions (BEGV, ZV, CHECK_INSIDE);
     }
 
@@ -2412,7 +2412,7 @@ #define COMBINING_BOTH (COMBINING_BEFORE | COMBINING_AFTER)
 
   if (changed > 0)
     {
-      signal_after_change (changed,
+      signal_after_change (current_buffer, changed,
 			   last_changed - changed, last_changed - changed);
       update_compositions (changed, last_changed, CHECK_ALL);
     }
@@ -2600,7 +2600,7 @@ DEFUN ("translate-region-internal", Ftranslate_region_internal,
 		  record_change (pos, 1);
 		  while (str_len-- > 0)
 		    *p++ = *str++;
-		  signal_after_change (pos, 1, 1);
+		  signal_after_change (current_buffer, pos, 1, 1);
 		  update_compositions (pos, pos + 1, CHECK_BORDER);
 		}
 	      characters_changed++;
@@ -4445,7 +4445,7 @@ DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5,
       adjust_markers_bytepos (start1, start1_byte, end2, end2_byte, 0);
     }
 
-  signal_after_change (start1, end2 - start1, end2 - start1);
+  signal_after_change (current_buffer, start1, end2 - start1, end2 - start1);
   return Qnil;
 }
 
diff --git a/src/fileio.c b/src/fileio.c
index b141f568d7..056da2cb9d 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -4555,7 +4555,7 @@ because (1) it preserves some marker positions and (2) it puts less data
   if (inserted > 0 && total > 0
       && (NILP (visit) || !NILP (replace)))
     {
-      signal_after_change (PT, 0, inserted);
+      signal_after_change (current_buffer, PT, 0, inserted);
       update_compositions (PT, PT, CHECK_BORDER);
     }
 
diff --git a/src/fns.c b/src/fns.c
index eaa2c07fbe..4b0c271b46 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -3577,7 +3577,7 @@ DEFUN ("base64-decode-region", Fbase64_decode_region, Sbase64_decode_region,
      and delete the old.  (Insert first in order to preserve markers.)  */
   TEMP_SET_PT_BOTH (XFIXNAT (beg), ibeg);
   insert_1_both (decoded, inserted_chars, decoded_length, 0, 1, 0);
-  signal_after_change (XFIXNAT (beg), 0, inserted_chars);
+  signal_after_change (current_buffer, XFIXNAT (beg), 0, inserted_chars);
   SAFE_FREE ();
 
   /* Delete the original text.  */
diff --git a/src/insdel.c b/src/insdel.c
index 85fffd8fd1..f9499a6ab3 100644
--- a/src/insdel.c
+++ b/src/insdel.c
@@ -669,7 +669,7 @@ insert (const char *string, ptrdiff_t nbytes)
       ptrdiff_t len = chars_in_text ((unsigned char *) string, nbytes), opoint;
       insert_1_both (string, len, nbytes, 0, 1, 0);
       opoint = PT - len;
-      signal_after_change (opoint, 0, len);
+      signal_after_change (current_buffer, opoint, 0, len);
       update_compositions (opoint, PT, CHECK_BORDER);
     }
 }
@@ -684,7 +684,7 @@ insert_and_inherit (const char *string, ptrdiff_t nbytes)
       ptrdiff_t len = chars_in_text ((unsigned char *) string, nbytes), opoint;
       insert_1_both (string, len, nbytes, 1, 1, 0);
       opoint = PT - len;
-      signal_after_change (opoint, 0, len);
+      signal_after_change (current_buffer, opoint, 0, len);
       update_compositions (opoint, PT, CHECK_BORDER);
     }
 }
@@ -729,7 +729,7 @@ insert_before_markers (const char *string, ptrdiff_t nbytes)
       ptrdiff_t len = chars_in_text ((unsigned char *) string, nbytes), opoint;
       insert_1_both (string, len, nbytes, 0, 1, 1);
       opoint = PT - len;
-      signal_after_change (opoint, 0, len);
+      signal_after_change (current_buffer, opoint, 0, len);
       update_compositions (opoint, PT, CHECK_BORDER);
     }
 }
@@ -745,7 +745,7 @@ insert_before_markers_and_inherit (const char *string,
       ptrdiff_t len = chars_in_text ((unsigned char *) string, nbytes), opoint;
       insert_1_both (string, len, nbytes, 1, 1, 1);
       opoint = PT - len;
-      signal_after_change (opoint, 0, len);
+      signal_after_change (current_buffer, opoint, 0, len);
       update_compositions (opoint, PT, CHECK_BORDER);
     }
 }
@@ -959,7 +959,7 @@ insert_from_string (Lisp_Object string, ptrdiff_t pos, ptrdiff_t pos_byte,
 
   insert_from_string_1 (string, pos, pos_byte, length, length_byte,
 			inherit, 0);
-  signal_after_change (opoint, 0, PT - opoint);
+  signal_after_change (current_buffer, opoint, 0, PT - opoint);
   update_compositions (opoint, PT, CHECK_BORDER);
 }
 
@@ -979,7 +979,7 @@ insert_from_string_before_markers (Lisp_Object string,
 
   insert_from_string_1 (string, pos, pos_byte, length, length_byte,
 			inherit, 1);
-  signal_after_change (opoint, 0, PT - opoint);
+  signal_after_change (current_buffer, opoint, 0, PT - opoint);
   update_compositions (opoint, PT, CHECK_BORDER);
 }
 
@@ -1135,7 +1135,7 @@ insert_from_buffer (struct buffer *buf,
   ptrdiff_t opoint = PT;
 
   insert_from_buffer_1 (buf, charpos, nchars, inherit);
-  signal_after_change (opoint, 0, PT - opoint);
+  signal_after_change (current_buffer, opoint, 0, PT - opoint);
   update_compositions (opoint, PT, CHECK_BORDER);
 }
 
@@ -1530,7 +1530,7 @@ replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new,
   if (adjust_match_data)
     update_search_regs (from, to, from + SCHARS (new));
 
-  signal_after_change (from, nchars_del, GPT - from);
+  signal_after_change (current_buffer, from, nchars_del, GPT - from);
   update_compositions (from, GPT, CHECK_BORDER);
 }
 \f
@@ -1698,7 +1698,7 @@ del_range_1 (ptrdiff_t from, ptrdiff_t to, bool prepare, bool ret_string)
   to_byte = CHAR_TO_BYTE (to);
 
   deletion = del_range_2 (from, from_byte, to, to_byte, ret_string);
-  signal_after_change (from, to - from, 0);
+  signal_after_change (current_buffer, from, to - from, 0);
   update_compositions (from, from, CHECK_HEAD);
   return deletion;
 }
@@ -1740,7 +1740,7 @@ del_range_byte (ptrdiff_t from_byte, ptrdiff_t to_byte)
   }
 
   del_range_2 (from, from_byte, to, to_byte, 0);
-  signal_after_change (from, to - from, 0);
+  signal_after_change (current_buffer, from, to - from, 0);
   update_compositions (from, from, CHECK_HEAD);
 }
 
@@ -1784,7 +1784,7 @@ del_range_both (ptrdiff_t from, ptrdiff_t from_byte,
     }
 
   del_range_2 (from, from_byte, to, to_byte, 0);
-  signal_after_change (from, to - from, 0);
+  signal_after_change (current_buffer, from, to - from, 0);
   update_compositions (from, from, CHECK_HEAD);
 }
 
@@ -2174,7 +2174,8 @@ signal_before_change (ptrdiff_t start_int, ptrdiff_t end_int,
    after the change.  */
 
 void
-signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
+signal_after_change (struct buffer *buffer,
+                     ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
 {
   ptrdiff_t count = SPECPDL_INDEX ();
   struct rvoe_arg rvoe_arg;
@@ -2183,6 +2184,9 @@ signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
   if (inhibit_modification_hooks)
     return;
 
+  record_unwind_current_buffer ();
+  set_buffer_internal (buffer);
+
   /* If we are deferring calls to the after-change functions
      and there are no before-change functions,
      just record the args that we were going to use.  */
@@ -2338,7 +2342,7 @@ DEFUN ("combine-after-change-execute", Fcombine_after_change_execute,
      Turn off the flag that defers them.  */
   record_unwind_protect (Fcombine_after_change_execute_1,
 			 Vcombine_after_change_calls);
-  signal_after_change (begpos, endpos - begpos - change, endpos - begpos);
+  signal_after_change (current_buffer, begpos, endpos - begpos - change, endpos - begpos);
   update_compositions (begpos, endpos, CHECK_ALL);
 
   return unbind_to (count, Qnil);
diff --git a/src/json.c b/src/json.c
index e2a4424463..ecb47f3ff4 100644
--- a/src/json.c
+++ b/src/json.c
@@ -789,7 +789,7 @@ DEFUN ("json-insert", Fjson_insert, Sjson_insert, 1, MANY,
     }
 
   /* Call after-change hooks.  */
-  signal_after_change (PT, 0, inserted);
+  signal_after_change (current_buffer, PT, 0, inserted);
   if (inserted > 0)
     {
       update_compositions (PT, PT, CHECK_BORDER);
diff --git a/src/lisp.h b/src/lisp.h
index 77fc22d118..a1ed4a3ce9 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3685,7 +3685,7 @@ #define CONS_TO_INTEGER(cons, type, var)				\
 extern void prepare_to_modify_buffer (ptrdiff_t, ptrdiff_t, ptrdiff_t *);
 extern void prepare_to_modify_buffer_1 (ptrdiff_t, ptrdiff_t, ptrdiff_t *);
 extern void invalidate_buffer_caches (struct buffer *, ptrdiff_t, ptrdiff_t);
-extern void signal_after_change (ptrdiff_t, ptrdiff_t, ptrdiff_t);
+extern void signal_after_change (struct buffer *, ptrdiff_t, ptrdiff_t, ptrdiff_t);
 extern void adjust_after_insert (ptrdiff_t, ptrdiff_t, ptrdiff_t,
 				 ptrdiff_t, ptrdiff_t);
 extern void adjust_markers_for_delete (ptrdiff_t, ptrdiff_t,
diff --git a/src/print.c b/src/print.c
index 406abbf4a3..e94eaf36f9 100644
--- a/src/print.c
+++ b/src/print.c
@@ -178,7 +178,8 @@ #define PRINTFINISH							\
        else								\
 	 insert_1_both (print_buffer, print_buffer_pos,			\
 			print_buffer_pos_byte, 0, 1, 0);		\
-       signal_after_change (PT - print_buffer_pos, 0, print_buffer_pos);\
+       signal_after_change (current_buffer, PT - print_buffer_pos,      \
+                            0, print_buffer_pos);                       \
      }									\
    if (free_print_buffer)						\
      {									\
diff --git a/src/textprop.c b/src/textprop.c
index ae42c44185..293ac02d9c 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -1215,7 +1215,7 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	  if (interval_has_all_properties (properties, i))
 	    {
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+		signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				     XFIXNUM (end) - XFIXNUM (start));
 
 	      eassert (modified);
@@ -1226,7 +1226,7 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	    {
 	      add_properties (properties, i, object, set_type);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+		signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				     XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
@@ -1237,7 +1237,7 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	  copy_properties (unchanged, i);
 	  add_properties (properties, i, object, set_type);
 	  if (BUFFERP (object))
-	    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+	    signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				 XFIXNUM (end) - XFIXNUM (start));
 	  return Qt;
 	}
@@ -1398,7 +1398,7 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
   set_text_properties_1 (start, end, properties, object, i);
 
   if (BUFFERP (object) && !NILP (coherent_change_p))
-    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+    signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 			 XFIXNUM (end) - XFIXNUM (start));
   return Qt;
 }
@@ -1565,7 +1565,7 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	    {
 	      eassert (modified);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+		signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				     XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
@@ -1574,7 +1574,7 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	    {
 	      remove_properties (properties, Qnil, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+		signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				     XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
@@ -1585,7 +1585,7 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	  copy_properties (unchanged, i);
 	  remove_properties (properties, Qnil, i, object);
 	  if (BUFFERP (object))
-	    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+	    signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				 XFIXNUM (end) - XFIXNUM (start));
 	  return Qt;
 	}
@@ -1663,7 +1663,7 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 	      if (modified)
 		{
 		  if (BUFFERP (object))
-		    signal_after_change (XFIXNUM (start),
+		    signal_after_change (XBUFFER (object), XFIXNUM (start),
 					 XFIXNUM (end) - XFIXNUM (start),
 					 XFIXNUM (end) - XFIXNUM (start));
 		  return Qt;
@@ -1677,7 +1677,7 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 		modify_text_properties (object, start, end);
 	      remove_properties (Qnil, properties, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+		signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				     XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
@@ -1690,7 +1690,7 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 		modify_text_properties (object, start, end);
 	      remove_properties (Qnil, properties, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+		signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
 				     XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
@@ -1709,7 +1709,7 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
           if (modified)
             {
               if (BUFFERP (object))
-                signal_after_change (XFIXNUM (start),
+                signal_after_change (XBUFFER (object), XFIXNUM (start),
                                      XFIXNUM (end) - XFIXNUM (start),
                                      XFIXNUM (end) - XFIXNUM (start));
               return Qt;
-- 
2.20.1

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Thu, 13 Jun 2019 18:48:23 +0000
> Cc: 36190@debbugs.gnu.org
> 
> > > As far as I can tell, this makes `put-text-property' with a buffer
> > > argument pretty useless.
> >
> > Only if you have a buffer-local value of after-change-functions.
> 
> I'm not sure what you're saying.

I'm saying that the buffer argument to put-text-property is pretty
useless only if you consider after-change-functions.  The primary
purpose of put-text-property is to modify text properties, not to call
after-change-functions.  For that primary purpose, the buffer argument
is not useless.

> That seems pretty wrong to me. In which cases do you think we're
> seeing the right behavior?

Where did I say that this behavior was right?

> Here's a first patch, which adds a "buffer" argument to
> signal_after_change, to be explicit about where the change happens. It
> should be pretty cheap in the case where we don't switch buffers.

Not sure I have a clear idea of how you intend to use that additional
argument.  Are you suggesting that we switch to that buffer?  If so,
how is that different from not using the buffer argument at all, and
instead wrapping the call to put-text-property with
with-current-buffer?

Also, passing current_buffer sounds redundant to me anyway, because in
that case signal_after_change will not need to do anything that it
doesn't already do.  I would pass NULL instead.

Thanks.

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Eli Zaretskii]

> Date: Thu, 13 Jun 2019 22:05:48 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 36190@debbugs.gnu.org
> 
> > Here's a first patch, which adds a "buffer" argument to
> > signal_after_change, to be explicit about where the change happens. It
> > should be pretty cheap in the case where we don't switch buffers.
> 
> Not sure I have a clear idea of how you intend to use that additional
> argument.

Found it:

> +  record_unwind_current_buffer ();
> +  set_buffer_internal (buffer);

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Pip Cet]

On Thu, Jun 13, 2019 at 7:06 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Thu, 13 Jun 2019 18:48:23 +0000
> > Cc: 36190@debbugs.gnu.org
> >
> > > > As far as I can tell, this makes `put-text-property' with a buffer
> > > > argument pretty useless.
> > >
> > > Only if you have a buffer-local value of after-change-functions.
> >
> > I'm not sure what you're saying.
>
> I'm saying that the buffer argument to put-text-property is pretty
> useless only if you consider after-change-functions.  The primary
> purpose of put-text-property is to modify text properties, not to call
> after-change-functions.  For that primary purpose, the buffer argument
> is not useless.

Thanks for clarifying. I suppose you could say that it's
after-change-functions (local, global, plus overlay modification
hooks) that have become useless, or will be called spuriously and with
potentially nonsensical arguments.

For example, this would break:

(push (lambda (beg end len)
        (message "%S" (buffer-substring beg end)))
      after-change-functions)

> > That seems pretty wrong to me. In which cases do you think we're
> > seeing the right behavior?
>
> Where did I say that this behavior was right?

You said "only if", so I assumed you were asserting the contrapositive.

> > Here's a first patch, which adds a "buffer" argument to
> > signal_after_change, to be explicit about where the change happens. It
> > should be pretty cheap in the case where we don't switch buffers.
>
> Not sure I have a clear idea of how you intend to use that additional
> argument.  Are you suggesting that we switch to that buffer?

Yes:

@@ -2183,6 +2184,9 @@ signal_after_change (ptrdiff_t charpos,
ptrdiff_t lendel, ptrdiff_t lenins)
   if (inhibit_modification_hooks)
     return;

+  record_unwind_current_buffer ();
+  set_buffer_internal (buffer);
+
   /* If we are deferring calls to the after-change functions
      and there are no before-change functions,
      just record the args that we were going to use.  */

> If so,
> how is that different from not using the buffer argument at all, and
> instead wrapping the call to put-text-property with
> with-current-buffer?

I don't think they're usefully different, but put-text-property
doesn't appear to check the buffer is still live.

> Also, passing current_buffer sounds redundant to me anyway, because in
> that case signal_after_change will not need to do anything that it
> doesn't already do.  I would pass NULL instead.

May I ask why? I think passing current_buffer is the clearest signal
we can send to someone reusing the code that they might have to change
this if they're dealing with more than one buffer.

As a practical matter, it's hard to change the text property functions
to use NULL when passed a nil argument, so we'd have functions using
current_buffer and others using NULL, and that seems needlessly
inconsistent.

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Thu, 13 Jun 2019 19:42:29 +0000
> Cc: 36190@debbugs.gnu.org
> 
> > Not sure I have a clear idea of how you intend to use that additional
> > argument.  Are you suggesting that we switch to that buffer?
> 
> Yes:
> 
> @@ -2183,6 +2184,9 @@ signal_after_change (ptrdiff_t charpos,
> ptrdiff_t lendel, ptrdiff_t lenins)
>    if (inhibit_modification_hooks)
>      return;
> 
> +  record_unwind_current_buffer ();
> +  set_buffer_internal (buffer);

Ugh! switching buffers just to run a hook!  This will kill performance
in some cases.  We had something similar with JSON parsing a few
months ago.  I wish we had a better alternative.  Maybe we should warn
in the documentation that calling these functions with BUFFER being
other than the current buffer might hurt performance when
after-change-functions is non-nil.

> > Also, passing current_buffer sounds redundant to me anyway, because in
> > that case signal_after_change will not need to do anything that it
> > doesn't already do.  I would pass NULL instead.
> 
> May I ask why?

To make the code speak for itself.  With passing current_buffer, you
now rely on subroutines of set_buffer_internal two or 3 levels down to
test whether we are already in that buffer and do nothing.  Meanwhile,
you wasted cycles on 2 or 3 function calls, and forced someone who
reads the code to go down that rabbit hole if they want to understand
what happens in that particular case.

> I think passing current_buffer is the clearest signal we can send to
> someone reusing the code that they might have to change this if
> they're dealing with more than one buffer.

Each function has commentary, where you can say that NULL means not to
switch buffers because we are already there.  That is a more clear
signal, IME.

> As a practical matter, it's hard to change the text property functions
> to use NULL when passed a nil argument

How is it harder than passing current_buffer?

> so we'd have functions using current_buffer and others using NULL,
> and that seems needlessly inconsistent.

Sorry, I don't see any inconsistency.  We do such things (for other
kinds of arguments) all over the place.

It's really a matter of stylistic preferences, but you did ask why...

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Pip Cet]

On Thu, Jun 13, 2019 at 8:01 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Thu, 13 Jun 2019 19:42:29 +0000
> > Cc: 36190@debbugs.gnu.org
> >
> > > Not sure I have a clear idea of how you intend to use that additional
> > > argument.  Are you suggesting that we switch to that buffer?
> >
> > Yes:
> >
> > @@ -2183,6 +2184,9 @@ signal_after_change (ptrdiff_t charpos,
> > ptrdiff_t lendel, ptrdiff_t lenins)
> >    if (inhibit_modification_hooks)
> >      return;
> >
> > +  record_unwind_current_buffer ();
> > +  set_buffer_internal (buffer);
>
> Ugh! switching buffers just to run a hook!
>  This will kill performance
> in some cases.

I really don't think it will have a noticeable impact on performance,
but if you can think of a scenario, we could try to fix it.

> We had something similar with JSON parsing a few
> months ago.
> I wish we had a better alternative.

(Such as not calling regular modification hooks for text property changes?)

>  Maybe we should warn
> in the documentation that calling these functions with BUFFER being
> other than the current buffer might hurt performance when
> after-change-functions is non-nil.

It'll hurt performance even when after-change-functions is nil, so
such a warning would be overspecific.

> > As a practical matter, it's hard to change the text property functions
> > to use NULL when passed a nil argument
>
> How is it harder than passing current_buffer?

The code path goes through

  if (NILP (object))
    XSETBUFFER (object, current_buffer);

> It's really a matter of stylistic preferences, but you did ask why...

It was out of genuine interest, because passing NULL to implicitly
specify a default argument is something that people advocate against,
and that C in particular has a history of avoiding (stdout isn't NULL,
and post-VAX NULL isn't an empty string, for example). Thank you for
responding, and I'll change my patch accordingly.

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Pip Cet]

[-- Attachment #1: Type: text/plain, Size: 103 bytes --]

Here's a simplified patch (I'm unsure about the right tab convention
to use; I hope I got this right).

[-- Attachment #2: 0001-Switch-to-correct-buffer-in-put-text-property-etc.patch --]
[-- Type: text/x-patch, Size: 8227 bytes --]

From 8664be1b0aa55731774fe2b010d8dd7259ca2d28 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Thu, 13 Jun 2019 21:21:50 +0000
Subject: [PATCH] Switch to correct buffer in put-text-property etc.

* src/textprop.c (add_text_properties_1, set_text_properties)
(Fremove_text_properties, Fremove_list_of_text_properties):
use `signal_after_change_in_buffer'.

* src/insdel.c (signal_after_change_in_buffer): New function.
---
 src/insdel.c   | 19 +++++++++++++
 src/lisp.h     |  1 +
 src/textprop.c | 73 +++++++++++++++++++++++++++-----------------------
 3 files changed, 60 insertions(+), 33 deletions(-)

diff --git a/src/insdel.c b/src/insdel.c
index 85fffd8fd1..0552044447 100644
--- a/src/insdel.c
+++ b/src/insdel.c
@@ -2253,6 +2253,25 @@ signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
   unbind_to (count, Qnil);
 }
 
+/* Signal a change immediately after it happens.
+   BUFFER is the buffer in which the change happened.
+   CHARPOS is the character position of the start of the changed text.
+   LENDEL is the number of characters of the text before the change.
+   (Not the whole buffer; just the part that was changed.)
+   LENINS is the number of characters in that part of the text
+   after the change.  */
+
+void
+signal_after_change_in_buffer (struct buffer *buffer, ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
+{
+  ptrdiff_t count = SPECPDL_INDEX ();
+
+  record_unwind_current_buffer ();
+  set_buffer_internal (buffer);
+  signal_after_change (charpos, lendel, lenins);
+  unbind_to (count, Qnil);
+}
+
 static void
 Fcombine_after_change_execute_1 (Lisp_Object val)
 {
diff --git a/src/lisp.h b/src/lisp.h
index 77fc22d118..4e05c9555b 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3686,6 +3686,7 @@ #define CONS_TO_INTEGER(cons, type, var)				\
 extern void prepare_to_modify_buffer_1 (ptrdiff_t, ptrdiff_t, ptrdiff_t *);
 extern void invalidate_buffer_caches (struct buffer *, ptrdiff_t, ptrdiff_t);
 extern void signal_after_change (ptrdiff_t, ptrdiff_t, ptrdiff_t);
+extern void signal_after_change_in_buffer (struct buffer *, ptrdiff_t, ptrdiff_t, ptrdiff_t);
 extern void adjust_after_insert (ptrdiff_t, ptrdiff_t, ptrdiff_t,
 				 ptrdiff_t, ptrdiff_t);
 extern void adjust_markers_for_delete (ptrdiff_t, ptrdiff_t,
diff --git a/src/textprop.c b/src/textprop.c
index ae42c44185..1781440b10 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -1215,8 +1215,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	  if (interval_has_all_properties (properties, i))
 	    {
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 
 	      eassert (modified);
 	      return Qt;
@@ -1226,8 +1226,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	    {
 	      add_properties (properties, i, object, set_type);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 
@@ -1237,8 +1237,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	  copy_properties (unchanged, i);
 	  add_properties (properties, i, object, set_type);
 	  if (BUFFERP (object))
-	    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				 XFIXNUM (end) - XFIXNUM (start));
+	    signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					   XFIXNUM (end) - XFIXNUM (start));
 	  return Qt;
 	}
 
@@ -1398,8 +1398,9 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
   set_text_properties_1 (start, end, properties, object, i);
 
   if (BUFFERP (object) && !NILP (coherent_change_p))
-    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-			 XFIXNUM (end) - XFIXNUM (start));
+    signal_after_change_in_buffer (XBUFFER (object),
+				   XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+				   XFIXNUM (end) - XFIXNUM (start));
   return Qt;
 }
 
@@ -1565,8 +1566,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	    {
 	      eassert (modified);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object),
+					       XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 
@@ -1574,8 +1576,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	    {
 	      remove_properties (properties, Qnil, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object),
+					       XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 
@@ -1585,8 +1588,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	  copy_properties (unchanged, i);
 	  remove_properties (properties, Qnil, i, object);
 	  if (BUFFERP (object))
-	    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				 XFIXNUM (end) - XFIXNUM (start));
+	    signal_after_change_in_buffer (XBUFFER (object),
+					   XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					   XFIXNUM (end) - XFIXNUM (start));
 	  return Qt;
 	}
 
@@ -1663,9 +1667,10 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 	      if (modified)
 		{
 		  if (BUFFERP (object))
-		    signal_after_change (XFIXNUM (start),
-					 XFIXNUM (end) - XFIXNUM (start),
-					 XFIXNUM (end) - XFIXNUM (start));
+		    signal_after_change_in_buffer (XBUFFER (object),
+						   XFIXNUM (start),
+						   XFIXNUM (end) - XFIXNUM (start),
+						   XFIXNUM (end) - XFIXNUM (start));
 		  return Qt;
 		}
 	      else
@@ -1677,8 +1682,9 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 		modify_text_properties (object, start, end);
 	      remove_properties (Qnil, properties, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object),
+					       XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 	  else
@@ -1690,8 +1696,9 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 		modify_text_properties (object, start, end);
 	      remove_properties (Qnil, properties, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object),
+					       XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 	}
@@ -1705,18 +1712,18 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
       len -= LENGTH (i);
       i = next_interval (i);
       if (!i)
-        {
-          if (modified)
-            {
-              if (BUFFERP (object))
-                signal_after_change (XFIXNUM (start),
-                                     XFIXNUM (end) - XFIXNUM (start),
-                                     XFIXNUM (end) - XFIXNUM (start));
-              return Qt;
-            }
-          else
-            return Qnil;
-        }
+	{
+	  if (modified)
+	    {
+	      if (BUFFERP (object))
+		signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
+	      return Qt;
+	    }
+	  else
+	    return Qnil;
+	}
     }
 }
 \f
-- 
2.20.1

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Thu, 13 Jun 2019 20:57:08 +0000
> Cc: 36190@debbugs.gnu.org
> 
> > > +  record_unwind_current_buffer ();
> > > +  set_buffer_internal (buffer);
> >
> > Ugh! switching buffers just to run a hook!  This will kill
> >  performance in some cases.
> 
> I really don't think it will have a noticeable impact on performance,
> but if you can think of a scenario, we could try to fix it.

Switching buffers means rebinding values of all the buffer-local
variables, of which there could be quite a few.  Or am I missing
something?

One scenario where this could be painful could be reading a stream of
data that results in many changes in text properties, such as
fontifying a buffer of program source by using syntactical analysis
data received from a language server.  If you read and apply the input
one object at a time, this will result in many buffer switches.

> > I wish we had a better alternative.
> 
> (Such as not calling regular modification hooks for text property changes?)

I thought about that, but I don't think this would be acceptable.

> >  Maybe we should warn
> > in the documentation that calling these functions with BUFFER being
> > other than the current buffer might hurt performance when
> > after-change-functions is non-nil.
> 
> It'll hurt performance even when after-change-functions is nil, so
> such a warning would be overspecific.

We could avoid switching buffers if the hook is nil, at least in
principle.  If not, it's even worse than I feared.

> > > As a practical matter, it's hard to change the text property functions
> > > to use NULL when passed a nil argument
> >
> > How is it harder than passing current_buffer?
> 
> The code path goes through
> 
>   if (NILP (object))
>     XSETBUFFER (object, current_buffer);

I meant in the cases where you pass the literal current_buffer.

But even the above is not a problem:

  struct buffer *b;
  if (NILP (object))
    {
      XSETBUFFER (object, current_buffer);
      b = NULL;
    }
  else if (BUFFERP (object))
    b = XBUFFER (object);
  [...]
  signal_after_change (b, ...);

> It was out of genuine interest, because passing NULL to implicitly
> specify a default argument is something that people advocate against,

Not to specify the default, but to indicate that no action is needed
at all wrt the buffer.  It is similar to the last argument to
'strtol', for example.

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Thu, 13 Jun 2019 21:37:40 +0000
> Cc: 36190@debbugs.gnu.org
> 
> +/* Signal a change immediately after it happens.
> +   BUFFER is the buffer in which the change happened.
> +   CHARPOS is the character position of the start of the changed text.
> +   LENDEL is the number of characters of the text before the change.
> +   (Not the whole buffer; just the part that was changed.)
> +   LENINS is the number of characters in that part of the text
> +   after the change.  */

I would just say "Like signal_after_change, but ..." and describe only
the BUFFER argument.

> +void
> +signal_after_change_in_buffer (struct buffer *buffer, ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
> +{
> +  ptrdiff_t count = SPECPDL_INDEX ();
> +
> +  record_unwind_current_buffer ();
> +  set_buffer_internal (buffer);
> +  signal_after_change (charpos, lendel, lenins);
> +  unbind_to (count, Qnil);
> +}

I still think we should explicitly detect the current_buffer case here
and if so, avoid the calls to everything else except
signal_after_change itself.

But I indeed like this patch better, although the concerns over the
performance hit are still present.  Thanks.

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Pip Cet]

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

On Fri, Jun 14, 2019 at 7:41 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Thu, 13 Jun 2019 21:37:40 +0000
> > Cc: 36190@debbugs.gnu.org
> >
> > +/* Signal a change immediately after it happens.
> > +   BUFFER is the buffer in which the change happened.
> > +   CHARPOS is the character position of the start of the changed text.
> > +   LENDEL is the number of characters of the text before the change.
> > +   (Not the whole buffer; just the part that was changed.)
> > +   LENINS is the number of characters in that part of the text
> > +   after the change.  */
>
> I would just say "Like signal_after_change, but ..." and describe only
> the BUFFER argument.
>
> > +void
> > +signal_after_change_in_buffer (struct buffer *buffer, ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
> > +{
> > +  ptrdiff_t count = SPECPDL_INDEX ();
> > +
> > +  record_unwind_current_buffer ();
> > +  set_buffer_internal (buffer);
> > +  signal_after_change (charpos, lendel, lenins);
> > +  unbind_to (count, Qnil);
> > +}
>
> I still think we should explicitly detect the current_buffer case here
> and if so, avoid the calls to everything else except
> signal_after_change itself.
>
> But I indeed like this patch better, although the concerns over the
> performance hit are still present.  Thanks.

Okay, I fixed those two issues. As for the performance problem, should
we amend the documentation to state that if many changes are made,
it's better to use `with-current-buffer' instead of repeatedly calling
put-text-property with a buffer argument?

[-- Attachment #2: 0001-Switch-to-correct-buffer-in-put-text-property-etc.patch --]
[-- Type: text/x-patch, Size: 7946 bytes --]

From b23ebe451664b0f50a02fb2e4ee5a41877dfe32d Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Fri, 14 Jun 2019 11:05:31 +0000
Subject: [PATCH] Switch to correct buffer in put-text-property etc.

* src/textprop.c (add_text_properties_1, set_text_properties)
(Fremove_text_properties, Fremove_list_of_text_properties):
use `signal_after_change_in_buffer'.
* src/insdel.c (signal_after_change_in_buffer): New function.
---
 src/insdel.c   | 16 +++++++++++
 src/lisp.h     |  1 +
 src/textprop.c | 73 +++++++++++++++++++++++++++-----------------------
 3 files changed, 57 insertions(+), 33 deletions(-)

diff --git a/src/insdel.c b/src/insdel.c
index 85fffd8fd1..49aca94e03 100644
--- a/src/insdel.c
+++ b/src/insdel.c
@@ -2253,6 +2253,22 @@ signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
   unbind_to (count, Qnil);
 }
 
+/* Like `signal_after_change', but switch to BUFFER first.  */
+
+void
+signal_after_change_in_buffer (struct buffer *buffer, ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
+{
+  ptrdiff_t count = SPECPDL_INDEX ();
+
+  if (buffer != current_buffer)
+    {
+      record_unwind_current_buffer ();
+      set_buffer_internal (buffer);
+    }
+  signal_after_change (charpos, lendel, lenins);
+  unbind_to (count, Qnil);
+}
+
 static void
 Fcombine_after_change_execute_1 (Lisp_Object val)
 {
diff --git a/src/lisp.h b/src/lisp.h
index 77fc22d118..4e05c9555b 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3686,6 +3686,7 @@ #define CONS_TO_INTEGER(cons, type, var)				\
 extern void prepare_to_modify_buffer_1 (ptrdiff_t, ptrdiff_t, ptrdiff_t *);
 extern void invalidate_buffer_caches (struct buffer *, ptrdiff_t, ptrdiff_t);
 extern void signal_after_change (ptrdiff_t, ptrdiff_t, ptrdiff_t);
+extern void signal_after_change_in_buffer (struct buffer *, ptrdiff_t, ptrdiff_t, ptrdiff_t);
 extern void adjust_after_insert (ptrdiff_t, ptrdiff_t, ptrdiff_t,
 				 ptrdiff_t, ptrdiff_t);
 extern void adjust_markers_for_delete (ptrdiff_t, ptrdiff_t,
diff --git a/src/textprop.c b/src/textprop.c
index ae42c44185..1781440b10 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -1215,8 +1215,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	  if (interval_has_all_properties (properties, i))
 	    {
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 
 	      eassert (modified);
 	      return Qt;
@@ -1226,8 +1226,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	    {
 	      add_properties (properties, i, object, set_type);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 
@@ -1237,8 +1237,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	  copy_properties (unchanged, i);
 	  add_properties (properties, i, object, set_type);
 	  if (BUFFERP (object))
-	    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				 XFIXNUM (end) - XFIXNUM (start));
+	    signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					   XFIXNUM (end) - XFIXNUM (start));
 	  return Qt;
 	}
 
@@ -1398,8 +1398,9 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
   set_text_properties_1 (start, end, properties, object, i);
 
   if (BUFFERP (object) && !NILP (coherent_change_p))
-    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-			 XFIXNUM (end) - XFIXNUM (start));
+    signal_after_change_in_buffer (XBUFFER (object),
+				   XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+				   XFIXNUM (end) - XFIXNUM (start));
   return Qt;
 }
 
@@ -1565,8 +1566,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	    {
 	      eassert (modified);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object),
+					       XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 
@@ -1574,8 +1576,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	    {
 	      remove_properties (properties, Qnil, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object),
+					       XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 
@@ -1585,8 +1588,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	  copy_properties (unchanged, i);
 	  remove_properties (properties, Qnil, i, object);
 	  if (BUFFERP (object))
-	    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				 XFIXNUM (end) - XFIXNUM (start));
+	    signal_after_change_in_buffer (XBUFFER (object),
+					   XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					   XFIXNUM (end) - XFIXNUM (start));
 	  return Qt;
 	}
 
@@ -1663,9 +1667,10 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 	      if (modified)
 		{
 		  if (BUFFERP (object))
-		    signal_after_change (XFIXNUM (start),
-					 XFIXNUM (end) - XFIXNUM (start),
-					 XFIXNUM (end) - XFIXNUM (start));
+		    signal_after_change_in_buffer (XBUFFER (object),
+						   XFIXNUM (start),
+						   XFIXNUM (end) - XFIXNUM (start),
+						   XFIXNUM (end) - XFIXNUM (start));
 		  return Qt;
 		}
 	      else
@@ -1677,8 +1682,9 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 		modify_text_properties (object, start, end);
 	      remove_properties (Qnil, properties, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object),
+					       XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 	  else
@@ -1690,8 +1696,9 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 		modify_text_properties (object, start, end);
 	      remove_properties (Qnil, properties, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object),
+					       XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 	}
@@ -1705,18 +1712,18 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
       len -= LENGTH (i);
       i = next_interval (i);
       if (!i)
-        {
-          if (modified)
-            {
-              if (BUFFERP (object))
-                signal_after_change (XFIXNUM (start),
-                                     XFIXNUM (end) - XFIXNUM (start),
-                                     XFIXNUM (end) - XFIXNUM (start));
-              return Qt;
-            }
-          else
-            return Qnil;
-        }
+	{
+	  if (modified)
+	    {
+	      if (BUFFERP (object))
+		signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
+	      return Qt;
+	    }
+	  else
+	    return Qnil;
+	}
     }
 }
 \f
-- 
2.20.1

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Fri, 14 Jun 2019 11:14:53 +0000
> Cc: 36190@debbugs.gnu.org
> 
> Okay, I fixed those two issues.

Thanks.

> As for the performance problem, should we amend the documentation to
> state that if many changes are made, it's better to use
> `with-current-buffer' instead of repeatedly calling
> put-text-property with a buffer argument?

I think so, yes.

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Pip Cet]

[-- Attachment #1: Type: text/plain, Size: 497 bytes --]

On Fri, Jun 14, 2019 at 12:11 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Fri, 14 Jun 2019 11:14:53 +0000
> > Cc: 36190@debbugs.gnu.org
> >
> > Okay, I fixed those two issues.
>
> Thanks.

Except that calls set_buffer_internal twice for a single call to
put-text-property. That's too slow, right?

How about simply emulating (with-current-buffer buffer
(put-text-property ...)), as the attached patch does? That should fix
the immediate problem.

Thanks!

[-- Attachment #2: 0001-Update-current-buffer-when-changing-text-properties.patch --]
[-- Type: text/x-patch, Size: 3781 bytes --]

From c822e562aaa51af2fc6ffc7708124fec6bb333e4 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sat, 15 Jun 2019 15:12:14 +0000
Subject: [PATCH] Update current buffer when changing text properties.

* src/textprop.c (add_text_properties_1, set_text_properties)
(set_text_properties_1, Fremove_text_properties):
---
 src/textprop.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/src/textprop.c b/src/textprop.c
index ae42c44185..46de372b61 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -1141,6 +1141,15 @@ DEFUN ("previous-single-property-change", Fprevious_single_property_change,
 add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 		       Lisp_Object properties, Lisp_Object object,
 		       enum property_set_type set_type) {
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+      return unbind_to (count, add_text_properties_1 (start, end, properties,
+						      object, set_type));
+    }
+
   INTERVAL i, unchanged;
   ptrdiff_t s, len;
   bool modified = false;
@@ -1342,6 +1351,15 @@ face(s) are retained.  This is done by setting the `face' property to
 set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
 		     Lisp_Object object, Lisp_Object coherent_change_p)
 {
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+      return unbind_to (count, set_text_properties (start, end, properties,
+						    object, coherent_change_p));
+    }
+
   INTERVAL i;
   bool first_time = true;
 
@@ -1412,6 +1430,17 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
 set_text_properties_1 (Lisp_Object start, Lisp_Object end,
 		       Lisp_Object properties, Lisp_Object object, INTERVAL i)
 {
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+
+      set_text_properties_1 (start, end, properties, object, i);
+      unbind_to (count, Qnil);
+      return;
+    }
+
   INTERVAL prev_changed = NULL;
   ptrdiff_t s = XFIXNUM (start);
   ptrdiff_t len = XFIXNUM (end) - s;
@@ -1494,6 +1523,15 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 Use `set-text-properties' if you want to remove all text properties.  */)
   (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object)
 {
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+      return unbind_to (count, Fremove_text_properties (start, end, properties,
+							object));
+    }
+
   INTERVAL i, unchanged;
   ptrdiff_t s, len;
   bool modified = false;
@@ -1606,6 +1644,16 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 Return t if any property was actually removed, nil otherwise.  */)
   (Lisp_Object start, Lisp_Object end, Lisp_Object list_of_properties, Lisp_Object object)
 {
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+      return unbind_to (count, Fremove_list_of_text_properties (start, end,
+								list_of_properties,
+								object));
+    }
+
   INTERVAL i, unchanged;
   ptrdiff_t s, len;
   bool modified = false;
-- 
2.20.1

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 15 Jun 2019 15:14:43 +0000
> Cc: 36190@debbugs.gnu.org
> 
> Except that calls set_buffer_internal twice for a single call to
> put-text-property. That's too slow, right?
> 
> How about simply emulating (with-current-buffer buffer
> (put-text-property ...)), as the attached patch does? That should fix
> the immediate problem.

Looks better to me, but I'd suggest a comment explaining why we do
such an unusual thing.

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Pip Cet]

[-- Attachment #1: Type: text/plain, Size: 724 bytes --]

On Sat, Jun 15, 2019 at 3:23 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sat, 15 Jun 2019 15:14:43 +0000
> > Cc: 36190@debbugs.gnu.org
> >
> > How about simply emulating (with-current-buffer buffer
> > (put-text-property ...)), as the attached patch does? That should fix
> > the immediate problem.
>
> Looks better to me, but I'd suggest a comment explaining why we do
> such an unusual thing.

Sounds good to me. The attached patch contains the comments and a hint
in the documentation. (Maybe the comments should be a FIXME, because
there's really no need to switch buffers at all when there are no
applicable hooks?)

Obviously, feel free to reword as you consider appropriate.

[-- Attachment #2: 0001-Update-current-buffer-when-changing-text-properties.patch --]
[-- Type: text/x-patch, Size: 4965 bytes --]

From 01784e6b16c40aef362f39519a4e8b29982c8fcd Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sat, 15 Jun 2019 15:12:14 +0000
Subject: [PATCH] Update current buffer when changing text properties.

* src/textprop.c (add_text_properties_1, set_text_properties)
(set_text_properties_1, Fremove_text_properties):
---
 doc/lispref/text.texi |  2 +-
 src/textprop.c        | 58 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/doc/lispref/text.texi b/doc/lispref/text.texi
index 2e7c497f57..035fea6a6b 100644
--- a/doc/lispref/text.texi
+++ b/doc/lispref/text.texi
@@ -2800,7 +2800,7 @@ Examining Properties
 
   These functions handle both strings and buffers.  Keep in mind that
 positions in a string start from 0, whereas positions in a buffer start
-from 1.
+from 1.  Passing a buffer other than the current buffer may be slow.
 
 @defun get-text-property pos prop &optional object
 This function returns the value of the @var{prop} property of the
diff --git a/src/textprop.c b/src/textprop.c
index ae42c44185..56f276b9ad 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -1141,6 +1141,17 @@ DEFUN ("previous-single-property-change", Fprevious_single_property_change,
 add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 		       Lisp_Object properties, Lisp_Object object,
 		       enum property_set_type set_type) {
+  /* Ensure we run the modification hooks for the right buffer,
+     without switching buffers twice (bug 36190). */
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+      return unbind_to (count, add_text_properties_1 (start, end, properties,
+						      object, set_type));
+    }
+
   INTERVAL i, unchanged;
   ptrdiff_t s, len;
   bool modified = false;
@@ -1342,6 +1353,17 @@ face(s) are retained.  This is done by setting the `face' property to
 set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
 		     Lisp_Object object, Lisp_Object coherent_change_p)
 {
+  /* Ensure we run the modification hooks for the right buffer,
+     without switching buffers twice (bug 36190). */
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+      return unbind_to (count, set_text_properties (start, end, properties,
+						    object, coherent_change_p));
+    }
+
   INTERVAL i;
   bool first_time = true;
 
@@ -1412,6 +1434,19 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
 set_text_properties_1 (Lisp_Object start, Lisp_Object end,
 		       Lisp_Object properties, Lisp_Object object, INTERVAL i)
 {
+  /* Ensure we run the modification hooks for the right buffer,
+     without switching buffers twice (bug 36190). */
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+
+      set_text_properties_1 (start, end, properties, object, i);
+      unbind_to (count, Qnil);
+      return;
+    }
+
   INTERVAL prev_changed = NULL;
   ptrdiff_t s = XFIXNUM (start);
   ptrdiff_t len = XFIXNUM (end) - s;
@@ -1494,6 +1529,17 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 Use `set-text-properties' if you want to remove all text properties.  */)
   (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object)
 {
+  /* Ensure we run the modification hooks for the right buffer,
+     without switching buffers twice (bug 36190). */
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+      return unbind_to (count, Fremove_text_properties (start, end, properties,
+							object));
+    }
+
   INTERVAL i, unchanged;
   ptrdiff_t s, len;
   bool modified = false;
@@ -1606,6 +1652,18 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 Return t if any property was actually removed, nil otherwise.  */)
   (Lisp_Object start, Lisp_Object end, Lisp_Object list_of_properties, Lisp_Object object)
 {
+  /* Ensure we run the modification hooks for the right buffer,
+     without switching buffers twice (bug 36190). */
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+      return unbind_to (count, Fremove_list_of_text_properties (start, end,
+								list_of_properties,
+								object));
+    }
+
   INTERVAL i, unchanged;
   ptrdiff_t s, len;
   bool modified = false;
-- 
2.20.1

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Pip Cet]

On Fri, Jun 14, 2019 at 7:36 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Thu, 13 Jun 2019 20:57:08 +0000
> > Cc: 36190@debbugs.gnu.org
> >
> > > > +  record_unwind_current_buffer ();
> > > > +  set_buffer_internal (buffer);
> > >
> > > Ugh! switching buffers just to run a hook!  This will kill
> > >  performance in some cases.
> >
> > I really don't think it will have a noticeable impact on performance,
> > but if you can think of a scenario, we could try to fix it.
>
> Switching buffers means rebinding values of all the buffer-local
> variables, of which there could be quite a few.  Or am I missing
> something?

I just don't see how the requirement to switch buffers for modifying
text properties is so different, performance-wise, from the case of
modifying buffer text; in the latter case, we simply accept we can do
so only for the current buffer.

In any case, the current code already switches buffers, it's just a
question of doing so twice rather than once.

> One scenario where this could be painful could be reading a stream of
> data that results in many changes in text properties, such as
> fontifying a buffer of program source by using syntactical analysis
> data received from a language server.  If you read and apply the input
> one object at a time, this will result in many buffer switches.

Yes, I agree. However, half of those buffer switches are probably
because the language server output would be directed into a buffer;
you could avoid those using a filter function, I suppose.

> > > I wish we had a better alternative.
> >
> > (Such as not calling regular modification hooks for text property changes?)
>
> I thought about that, but I don't think this would be acceptable.

It's certainly not something to be done on the spur of the moment, but
it is something I feel Emacs did wrongly, perhaps because XEmacs did
things differently, if I understand correctly. I'm not sure I'm aware
of even a single place where text properties are used for something
that's integrally part of buffer text.

> > >  Maybe we should warn
> > > in the documentation that calling these functions with BUFFER being
> > > other than the current buffer might hurt performance when
> > > after-change-functions is non-nil.
> >
> > It'll hurt performance even when after-change-functions is nil, so
> > such a warning would be overspecific.
>
> We could avoid switching buffers if the hook is nil, at least in
> principle.  If not, it's even worse than I feared.

We could. I've looked at the code and I think the right thing to do,
when someone has time to test things properly, is to rewrite all
buffer-modifying functions to look like this:

  Lisp_Object hooks = run_before_change_hooks (...);
  modify_buffer ();
  run_after_change_hooks (hooks, ...);

where run_before_change_hooks runs the before-change hooks but
collects the modification hooks to be run after the modification in
the same iteration. Right now, we're using global variables to achieve
something similar, but, among other problems, that means modification
hooks aren't reentrant. Modifying buffer B from buffer A's
modification hooks sounds like it should be safe to me even when B has
modification hooks, but it isn't. (In fact, I don't see why
inhibit-modification-hooks isn't buffer-local).

> > > > As a practical matter, it's hard to change the text property functions
> > > > to use NULL when passed a nil argument
> > >
> > > How is it harder than passing current_buffer?
> >
> > The code path goes through
> >
> >   if (NILP (object))
> >     XSETBUFFER (object, current_buffer);
>
> I meant in the cases where you pass the literal current_buffer.
>
> But even the above is not a problem:
>
>   struct buffer *b;
>   if (NILP (object))
>     {
>       XSETBUFFER (object, current_buffer);
>       b = NULL;
>     }
>   else if (BUFFERP (object))
>     b = XBUFFER (object);
>   [...]
>   signal_after_change (b, ...);

I find the above much less readable than the current version, I must say.

> > It was out of genuine interest, because passing NULL to implicitly
> > specify a default argument is something that people advocate against,
>
> Not to specify the default, but to indicate that no action is needed
> at all wrt the buffer.  It is similar to the last argument to
> 'strtol', for example.

The `base' argument, you mean? If that's what you're saying, I agree
that using 0 as a short-hand for "use implicit base" is an odd
decision for C to have made, but I'm not sure I see the similarity to
the current argument.

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Mon, 17 Jun 2019 11:38:38 +0000
> Cc: 36190@debbugs.gnu.org
> 
> > Switching buffers means rebinding values of all the buffer-local
> > variables, of which there could be quite a few.  Or am I missing
> > something?
> 
> I just don't see how the requirement to switch buffers for modifying
> text properties is so different, performance-wise, from the case of
> modifying buffer text; in the latter case, we simply accept we can do
> so only for the current buffer.

It isn't different.  It's just that (a) modifying another buffer's
text is relatively rare, and (b) this is one more such switch.

> In any case, the current code already switches buffers, it's just a
> question of doing so twice rather than once.

Yes.  IOW, we get hit by that one more time.

> > > > I wish we had a better alternative.
> > >
> > > (Such as not calling regular modification hooks for text property changes?)
> >
> > I thought about that, but I don't think this would be acceptable.
> 
> It's certainly not something to be done on the spur of the moment, but
> it is something I feel Emacs did wrongly, perhaps because XEmacs did
> things differently, if I understand correctly. I'm not sure I'm aware
> of even a single place where text properties are used for something
> that's integrally part of buffer text.

I don't think this i a part of the problem: applications that don't
want the side effects of text properties can use overlays instead.

> when someone has time to test things properly, is to rewrite all
> buffer-modifying functions to look like this:
> 
>   Lisp_Object hooks = run_before_change_hooks (...);
>   modify_buffer ();
>   run_after_change_hooks (hooks, ...);

I think that'd be a welcome refactoring, if indeed this paradigm
doesn't break in some subtle use case (Emacs internals are frequently
like that).

> >   struct buffer *b;
> >   if (NILP (object))
> >     {
> >       XSETBUFFER (object, current_buffer);
> >       b = NULL;
> >     }
> >   else if (BUFFERP (object))
> >     b = XBUFFER (object);
> >   [...]
> >   signal_after_change (b, ...);
> 
> I find the above much less readable than the current version, I must say.

I guess we will have to disagree then, because this is boilerplate C
ion Emacs sources.

> > > It was out of genuine interest, because passing NULL to implicitly
> > > specify a default argument is something that people advocate against,
> >
> > Not to specify the default, but to indicate that no action is needed
> > at all wrt the buffer.  It is similar to the last argument to
> > 'strtol', for example.
> 
> The `base' argument, you mean?

Sorry, meant the penultimate argument, ENDPTR.

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Pip Cet]

On Mon, Jun 17, 2019 at 3:58 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Mon, 17 Jun 2019 11:38:38 +0000
> > Cc: 36190@debbugs.gnu.org
> > > > > I wish we had a better alternative.
> > > >
> > > > (Such as not calling regular modification hooks for text property changes?)
> > >
> > > I thought about that, but I don't think this would be acceptable.
> >
> > It's certainly not something to be done on the spur of the moment, but
> > it is something I feel Emacs did wrongly, perhaps because XEmacs did
> > things differently, if I understand correctly. I'm not sure I'm aware
> > of even a single place where text properties are used for something
> > that's integrally part of buffer text.
>
> I don't think this i a part of the problem: applications that don't
> want the side effects of text properties can use overlays instead.

I've seen performance problems with overlays, and the subtle API
differences made switching, for me, difficult. You're right, though,
that this is certainly not part of this bug report, which I think the
latest patch I sent would fix for now (although I'd be happy to see it
fixed any other way, too).

> > when someone has time to test things properly, is to rewrite all
> > buffer-modifying functions to look like this:
> >
> >   Lisp_Object hooks = run_before_change_hooks (...);
> >   modify_buffer ();
> >   run_after_change_hooks (hooks, ...);
>
> I think that'd be a welcome refactoring, if indeed this paradigm
> doesn't break in some subtle use case (Emacs internals are frequently
> like that).

It would almost certainly be an observable change in behavior, but I'm
testing something and it doesn't seem to fall apart entirely, at
least.

> > > > It was out of genuine interest, because passing NULL to implicitly
> > > > specify a default argument is something that people advocate against,
> > >
> > > Not to specify the default, but to indicate that no action is needed
> > > at all wrt the buffer.  It is similar to the last argument to
> > > 'strtol', for example.
> >
> > The `base' argument, you mean?
>
> Sorry, meant the penultimate argument, ENDPTR.

Thanks again for explaining, I'll think about it more.

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 15 Jun 2019 19:27:30 +0000
> Cc: 36190@debbugs.gnu.org
> 
> > Looks better to me, but I'd suggest a comment explaining why we do
> > such an unusual thing.
> 
> Sounds good to me. The attached patch contains the comments and a hint
> in the documentation. (Maybe the comments should be a FIXME, because
> there's really no need to switch buffers at all when there are no
> applicable hooks?)

I'm okay with adding such a FIXME.

> Obviously, feel free to reword as you consider appropriate.

LGTM, with a couple of minor comments below.  Feel free to push after
fixing those.

> +  /* Ensure we run the modification hooks for the right buffer,
> +     without switching buffers twice (bug 36190). */
                                                   ^^
Here and elsewhere please keep 2 spaces between the period and "*/"

> +  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
> +    {
> +      ptrdiff_t count = SPECPDL_INDEX ();
> +      record_unwind_current_buffer ();
> +      set_buffer_internal (XBUFFER (object));
> +      return unbind_to (count, Fremove_list_of_text_properties (start, end,
> +								list_of_properties,
> +								object));
> +    }

The indentation here is too deep; suggest reformatting, e.g., by
inserting a newline before Fremove_list_of_text_properties, and then
reindenting the rest.

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Pip Cet]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On Sat, Jul 6, 2019 at 8:08 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > Sounds good to me. The attached patch contains the comments and a hint
> > in the documentation. (Maybe the comments should be a FIXME, because
> > there's really no need to switch buffers at all when there are no
> > applicable hooks?)
>
> I'm okay with adding such a FIXME.

Added.


>
> > Obviously, feel free to reword as you consider appropriate.
>
> LGTM, with a couple of minor comments below.  Feel free to push after
> fixing those.

I don't have push access, so I hope it's okay just to send the new patch.

Thanks for the review!

[-- Attachment #2: 0001-Update-current-buffer-when-changing-text-properties.patch --]
[-- Type: text/x-patch, Size: 5401 bytes --]

From 7e3b17ce5704b5327b977673382688f8e7f5b4c9 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sat, 6 Jul 2019 15:21:04 +0000
Subject: [PATCH] Update current buffer when changing text properties.

* src/textprop.c (add_text_properties_1, set_text_properties)
(set_text_properties_1, Fremove_text_properties): Switch buffer if
necessary.

* doc/lispref/text.texi (Examining Properties): Document performance
FIXME.
---
 doc/lispref/text.texi |  2 +-
 src/textprop.c        | 66 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/doc/lispref/text.texi b/doc/lispref/text.texi
index c4fc5247a1..ca0dd6642d 100644
--- a/doc/lispref/text.texi
+++ b/doc/lispref/text.texi
@@ -2800,7 +2800,7 @@ Examining Properties
 
   These functions handle both strings and buffers.  Keep in mind that
 positions in a string start from 0, whereas positions in a buffer start
-from 1.
+from 1.  Passing a buffer other than the current buffer may be slow.
 
 @defun get-text-property pos prop &optional object
 This function returns the value of the @var{prop} property of the
diff --git a/src/textprop.c b/src/textprop.c
index 9023f4efa0..44c333256a 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -1142,6 +1142,18 @@ DEFUN ("previous-single-property-change", Fprevious_single_property_change,
 add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 		       Lisp_Object properties, Lisp_Object object,
 		       enum property_set_type set_type) {
+  /* Ensure we run the modification hooks for the right buffer,
+     without switching buffers twice (bug 36190).  FIXME: Switching
+     buffers is slow and often unnecessary.  */
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+      return unbind_to (count, add_text_properties_1 (start, end, properties,
+						      object, set_type));
+    }
+
   INTERVAL i, unchanged;
   ptrdiff_t s, len;
   bool modified = false;
@@ -1343,6 +1355,19 @@ face(s) are retained.  This is done by setting the `face' property to
 set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
 		     Lisp_Object object, Lisp_Object coherent_change_p)
 {
+  /* Ensure we run the modification hooks for the right buffer,
+     without switching buffers twice (bug 36190).  FIXME: Switching
+     buffers is slow and often unnecessary.  */
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+      return unbind_to (count,
+			set_text_properties (start, end, properties,
+					     object, coherent_change_p));
+    }
+
   INTERVAL i;
   bool first_time = true;
 
@@ -1413,6 +1438,20 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
 set_text_properties_1 (Lisp_Object start, Lisp_Object end,
 		       Lisp_Object properties, Lisp_Object object, INTERVAL i)
 {
+  /* Ensure we run the modification hooks for the right buffer,
+     without switching buffers twice (bug 36190).  FIXME: Switching
+     buffers is slow and often unnecessary.  */
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+
+      set_text_properties_1 (start, end, properties, object, i);
+      unbind_to (count, Qnil);
+      return;
+    }
+
   INTERVAL prev_changed = NULL;
   ptrdiff_t s = XFIXNUM (start);
   ptrdiff_t len = XFIXNUM (end) - s;
@@ -1495,6 +1534,19 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 Use `set-text-properties' if you want to remove all text properties.  */)
   (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object)
 {
+  /* Ensure we run the modification hooks for the right buffer,
+     without switching buffers twice (bug 36190).  FIXME: Switching
+     buffers is slow and often unnecessary.  */
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+      return unbind_to (count,
+			Fremove_text_properties (start, end, properties,
+						 object));
+    }
+
   INTERVAL i, unchanged;
   ptrdiff_t s, len;
   bool modified = false;
@@ -1607,6 +1659,20 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 Return t if any property was actually removed, nil otherwise.  */)
   (Lisp_Object start, Lisp_Object end, Lisp_Object list_of_properties, Lisp_Object object)
 {
+  /* Ensure we run the modification hooks for the right buffer,
+     without switching buffers twice (bug 36190).  FIXME: Switching
+     buffers is slow and often unnecessary.  */
+  if (BUFFERP (object) && XBUFFER (object) != current_buffer)
+    {
+      ptrdiff_t count = SPECPDL_INDEX ();
+      record_unwind_current_buffer ();
+      set_buffer_internal (XBUFFER (object));
+      return unbind_to (count,
+			Fremove_list_of_text_properties (start, end,
+							 list_of_properties,
+							 object));
+    }
+
   INTERVAL i, unchanged;
   ptrdiff_t s, len;
   bool modified = false;
-- 
2.20.1

bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 6 Jul 2019 15:27:41 +0000
> Cc: 36190@debbugs.gnu.org
> 
> > LGTM, with a couple of minor comments below.  Feel free to push after
> > fixing those.
> 
> I don't have push access, so I hope it's okay just to send the new patch.

Thanks, pushed.  Please in the future mention the bug number in the
log message, and be sure NOT to end with a period the header line of
the log message, per CONTRIBUTE.