bug#20153: 24.4.91; destructive add-face-text-property and string deep copying

bug#20153: 24.4.91; destructive add-face-text-property and string deep copying

[Oleh Krehel]

Here's an example of what I'm trying to do:

(setq test-str-1
      #(";; This `is' a test"
        0 3 (fontified nil face font-lock-comment-delimiter-face)
        3 9 (fontified nil face font-lock-comment-face)
        9 11 (fontified nil face (font-lock-constant-face font-lock-comment-face))
        11 19 (fontified nil face font-lock-comment-face)))
(setq test-str-2 (concat test-str-1))
(add-face-text-property 0 (length test-str-2) 'foobar t test-str-2)

I would like to modify `test-str-2' without touching `test-str-1'.
For that, I either need `add-face-text-property' to be non-destructive,
either w.r.t. the string or at least w.r.t. the properties of the
string, or a way to deep-copy a string.

Here's the result of evaling the code above:

test-str-2
;; =>
#(";; This `is' a test" 0 3 (fontified nil face (font-lock-comment-delimiter-face foobar))
  3 9 (fontified nil face (font-lock-comment-face foobar))
  9 11 (fontified nil face (font-lock-constant-face font-lock-comment-face foobar))
  11 19 (fontified nil face (font-lock-comment-face foobar)))
test-str-1
;; =>
#(";; This `is' a test" 0 3 (face font-lock-comment-delimiter-face fontified nil)
  3 9 (face font-lock-comment-face fontified nil)
  9 11 (face (font-lock-constant-face font-lock-comment-face foobar) ; <= foobar is here
        fontified nil)
  11 19 (face font-lock-comment-face fontified nil))

As you see, `test-str-1' was modified.

I'd like to know a straightforward solution to the problem itself, I'm
fine with either way of implementation.

Oleh

bug#20153: 24.4.91; destructive add-face-text-property and string deep copying

[Lars Ingebrigtsen]

Oleh Krehel <ohwoeowho@gmail.com> writes:

> Here's an example of what I'm trying to do:
>
> (setq test-str-1
>       #(";; This `is' a test"
>         0 3 (fontified nil face font-lock-comment-delimiter-face)
>         3 9 (fontified nil face font-lock-comment-face)
>         9 11 (fontified nil face (font-lock-constant-face font-lock-comment-face))
>         11 19 (fontified nil face font-lock-comment-face)))
> (setq test-str-2 (concat test-str-1))
> (add-face-text-property 0 (length test-str-2) 'foobar t test-str-2)
>
> I would like to modify `test-str-2' without touching `test-str-1'.
> For that, I either need `add-face-text-property' to be non-destructive,
> either w.r.t. the string or at least w.r.t. the properties of the
> string, or a way to deep-copy a string.

It's this code (in add_properties):

		/* The previous value is a list, so prepend (or
		   append) the new value to this list. */
		if (set_type == TEXT_PROPERTY_PREPEND)
		  Fsetcar (this_cdr, Fcons (val1, Fcar (this_cdr)));
		else
		  nconc2 (Fcar (this_cdr), list1 (val1));

But changing that would have pretty far-reaching consequences.  I wonder
whether instead Fadd_face_text_property should just (when called with a
string parameter) just copy the `face' property list first?  That should
avoid the problem and not lead to any general slowdown.

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

bug#20153: 24.4.91; destructive add-face-text-property and string deep copying

[Lars Ingebrigtsen]

I've never really worked with the interval internals before, so I
thought this was going to be easy to fix.  :-/  But the problem is that
copy_intervals doesn't do a "deep" copy of the text properties, so this
has no effect, really.

(Patch included for reference.)

Instead I've now changed add_properties (and add_text_properties_1) to
take a bool parameter to say whether they're allowed to be destructive
or not, and make the add-face-text-property call that with false as the
parameter if the object is a string.  This fixes the test case for me
and should hopefully have no measurable performance impact.

diff --git a/src/textprop.c b/src/textprop.c
index d36b9e14a6..dcd3284209 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -1334,6 +1334,18 @@ face(s) are retained.  This is done by setting the `face' property to
    Lisp_Object append, Lisp_Object object)
 {
   AUTO_LIST2 (properties, Qface, face);
+
+  /* If we're adding face properties to a string, and the face
+     property is already a list, then copy the list first to avoid
+     destructively altering it. */
+  if (STRINGP (object))
+    {
+      INTERVAL copy = copy_intervals (string_intervals (object),
+				      0, SCHARS (object));
+      set_interval_object (copy, object);
+      set_string_intervals (object, copy);
+    }
+
   add_text_properties_1 (start, end, properties, object,
 			 (NILP (append)
 			  ? TEXT_PROPERTY_PREPEND

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

bug#20153: 24.4.91; destructive add-face-text-property and string deep copying

[Oleh Krehel]

Thanks for your work, Lars.

  Oleh

bug#20153: 24.4.91; destructive add-face-text-property and string deep copying

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Wed, 09 Oct 2019 05:04:03 +0200
> Cc: 20153@debbugs.gnu.org
> 
> Instead I've now changed add_properties (and add_text_properties_1) to
> take a bool parameter to say whether they're allowed to be destructive
> or not, and make the add-face-text-property call that with false as the
> parameter if the object is a string.  This fixes the test case for me
> and should hopefully have no measurable performance impact.

Isn't that a backward-incompatible change?

bug#20153: 24.4.91; destructive add-face-text-property and string deep copying

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Instead I've now changed add_properties (and add_text_properties_1) to
>> take a bool parameter to say whether they're allowed to be destructive
>> or not, and make the add-face-text-property call that with false as the
>> parameter if the object is a string.  This fixes the test case for me
>> and should hopefully have no measurable performance impact.
>
> Isn't that a backward-incompatible change?

It is, but the previous behaviour was a bug.  If you have a copy of a
string and modify the copy, the original string shouldn't change.

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

bug#20153: 24.4.91; destructive add-face-text-property and string deep copying

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: ohwoeowho@gmail.com,  20153@debbugs.gnu.org
> Date: Wed, 09 Oct 2019 19:45:54 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Instead I've now changed add_properties (and add_text_properties_1) to
> >> take a bool parameter to say whether they're allowed to be destructive
> >> or not, and make the add-face-text-property call that with false as the
> >> parameter if the object is a string.  This fixes the test case for me
> >> and should hopefully have no measurable performance impact.
> >
> > Isn't that a backward-incompatible change?
> 
> It is, but the previous behaviour was a bug.  If you have a copy of a
> string and modify the copy, the original string shouldn't change.

AFAIU, the string didn't change, only its plist did.  Right?

Please bring this up on emacs-devel, and if no one objects to the
change, we should at least mention it in NEWS.

Thanks.

bug#20153: 24.4.91; destructive add-face-text-property and string deep copying

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> AFAIU, the string didn't change, only its plist did.  Right?

Yes.

> Please bring this up on emacs-devel, and if no one objects to the
> change, we should at least mention it in NEWS.

Will do.

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