Re: 31395511: "Don’t attempt to modify constant strings"

Re: 31395511: "Don’t attempt to modify constant strings"

[João Távora]

Hi Paul,

After a lengthy git bisect, I discovered that this commit is responsible
for breaking a very big part of my SLY extension, a Common Lisp IDE for
Emacs.  The reason is this change to make-text-button

-    (when (stringp beg)
-      (setq object beg beg 0 end (length object)))
+      (setq object (copy-sequence beg) beg 0 end (length object)))

I don't pretend to understand the reason for the change, but I know it
hasn't worked like this for a long time (SLY came about for Emacs 24.3).,

I didn't investigate much, but SLY has a lot of

   (insert (sly-make-action-button "[SOMEBUTTON]" ..))

and sly-make-action-button is

   (defun sly-make-action-button (label action &rest props)
     (apply #'sly--make-text-button
            label nil :type 'sly-action
            'action action
            'mouse-action action
            props)
     label)

and sly--make-text-button is

   (defun sly--make-text-button (beg end &rest properties)
     "Just like `make-text-button', but add sly-specifics."
     (apply #'make-text-button beg end
            'sly-connection (sly-current-connection)
            properties))

Not sure where the problem lies but every button inserted by SLY is now
just plain text.

Maybe you have an alternative formulation that I can apply in SLY,
otherwise I'd really appreciate that you could revert or find an
alternative to this change

Thanks in advance,
João

Re: 31395511: "Don’t attempt to modify constant strings"

[Paul Eggert]

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

On 6/3/20 2:52 PM, João Távora wrote:
> Not sure where the problem lies but every button inserted by SLY is now
> just plain text.

It's because I messed up in that part of the patch. Thanks for reporting the
bug. I installed the attached patch into master; please give it a try.

[-- Attachment #2: 0001-Fix-make-text-button-bug-with-string-copy.patch --]
[-- Type: text/x-patch, Size: 1315 bytes --]

From 6dad339f064df180e8f2c6257ffb53a6f341c4ec Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 3 Jun 2020 15:39:29 -0700
Subject: [PATCH] Fix make-text-button bug with string copy
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/button.el (make-text-button): Use the copy of BEG
uniformly, instead of in just one place.  This fixes a typo
introduced in 2020-05-17T05:23:28Z!eggert@cs.ucla.edu.
Problem reported by João Távora in:
https://lists.gnu.org/r/emacs-devel/2020-06/msg00117.html
---
 lisp/button.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/button.el b/lisp/button.el
index f969a03cb0..a91b0482ac 100644
--- a/lisp/button.el
+++ b/lisp/button.el
@@ -349,7 +349,8 @@ make-text-button
 	 (or (plist-member properties 'type)
 	     (plist-member properties :type))))
     (when (stringp beg)
-      (setq object (copy-sequence beg) beg 0 end (length object)))
+      (setq beg (copy-sequence beg)) ;; In case BEG is not mutable.
+      (setq object beg beg 0 end (length object)))
     ;; Disallow setting the `category' property directly.
     (when (plist-get properties 'category)
       (error "Button `category' property may not be set directly"))
-- 
2.17.1

Re: 31395511: "Don’t attempt to modify constant strings"

[Pip Cet]

On Wed, Jun 3, 2020 at 10:42 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 6/3/20 2:52 PM, João Távora wrote:
> > Not sure where the problem lies but every button inserted by SLY is now
> > just plain text.
>
> It's because I messed up in that part of the patch. Thanks for reporting the
> bug. I installed the attached patch into master; please give it a try.

I don't understand that change at all. All it does is replace a
six-argument setq by two successive setqs? Surely the symbol value is
irrelevant for the first, third, etc. argument to setq, so what
difference does it make?

Re: 31395511: "Don’t attempt to modify constant strings"

[Paul Eggert]

On 6/3/20 3:52 PM, Pip Cet wrote:
> All it does is replace a
> six-argument setq by two successive setqs? Surely the symbol value is
> irrelevant for the first, third, etc. argument to setq, so what
> difference does it make?

Oh, you're right, I misread the setqs.  (I sure do dislike setqs with more than
2 args....). So all my patch did was make the source code closer to emacs-27.

Instead, João should listen to the email you sent.

Re: 31395511: "Don’t attempt to modify constant strings"

[Basil L. Contovounesios]

Pip Cet <pipcet@gmail.com> writes:

> On Wed, Jun 3, 2020 at 10:42 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
>> On 6/3/20 2:52 PM, João Távora wrote:
>> > Not sure where the problem lies but every button inserted by SLY is now
>> > just plain text.
>>
>> It's because I messed up in that part of the patch. Thanks for reporting the
>> bug. I installed the attached patch into master; please give it a try.
>
> I don't understand that change at all. All it does is replace a
> six-argument setq by two successive setqs? Surely the symbol value is
> irrelevant for the first, third, etc. argument to setq, so what
> difference does it make?

Indeed the patch seems like a noop and shouldn't fix João's issue, which
is that until now make-text-button modified its argument.

-- 
Basil

Re: 31395511: "Don’t attempt to modify constant strings"

[Pip Cet]

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

João Távora <joaotavora@gmail.com> writes:

> Hi Paul,
>
> After a lengthy git bisect, I discovered that this commit is responsible
> for breaking a very big part of my SLY extension, a Common Lisp IDE for
> Emacs.  The reason is this change to make-text-button
>
> -    (when (stringp beg)
> -      (setq object beg beg 0 end (length object)))
> +      (setq object (copy-sequence beg) beg 0 end (length object)))
>
> I don't pretend to understand the reason for the change, but I know it
> hasn't worked like this for a long time (SLY came about for Emacs 24.3).,
>
> I didn't investigate much, but SLY has a lot of
>
>    (insert (sly-make-action-button "[SOMEBUTTON]" ..))
>
> and sly-make-action-button is
>
>    (defun sly-make-action-button (label action &rest props)
>      (apply #'sly--make-text-button
>             label nil :type 'sly-action
>             'action action
>             'mouse-action action
>             props)
>      label)

I think you want
(defun sly-make-action-button (label action &rest props)
  (apply #'sly--make-text-button
         label nil :type 'sly-action
         'action action
         'mouse-action action
         props))

instead, since the new function returns a copy of label rather than the
string passed in.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-adjust-to-Emacs-28-change.patch --]
[-- Type: text/x-diff, Size: 634 bytes --]

From d0e06fa8ae4c6d3156dccf922629f91985fd4822 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Wed, 3 Jun 2020 22:38:37 +0000
Subject: [PATCH] adjust to Emacs-28 change.

---
 lib/sly-buttons.el | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/sly-buttons.el b/lib/sly-buttons.el
index 8297ea74..8f393090 100644
--- a/lib/sly-buttons.el
+++ b/lib/sly-buttons.el
@@ -106,8 +106,7 @@
          label nil :type 'sly-action
          'action action
          'mouse-action action
-         props)
-  label)
+         props))
 
 (defface sly-action-face
   `((t (:inherit warning)))
-- 
2.27.0.rc0

Re: 31395511: "Don’t attempt to modify constant strings"

[Basil L. Contovounesios]

Pip Cet <pipcet@gmail.com> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>>    (defun sly-make-action-button (label action &rest props)
>>      (apply #'sly--make-text-button
>>             label nil :type 'sly-action
>>             'action action
>>             'mouse-action action
>>             props)
>>      label)
>
> I think you want
> (defun sly-make-action-button (label action &rest props)
>   (apply #'sly--make-text-button
>          label nil :type 'sly-action
>          'action action
>          'mouse-action action
>          props))
>
> instead, since the new function returns a copy of label rather than the
> string passed in.

Note that doing that would break compatibility with Emacs < 24.3, which
is when make-text-button started returning the modified string object,
in case that's important to you.

-- 
Basil

Re: 31395511: "Don’t attempt to modify constant strings"

[Basil L. Contovounesios]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Note that doing that would break compatibility with Emacs < 24.3, which
                                                              ^^^^
                                                              24.4
> is when make-text-button started returning the modified string object,
> in case that's important to you.

-- 
Basil

Re: 31395511: "Don’t attempt to modify constant strings"

[João Távora]

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

On Wed, Jun 3, 2020 at 11:42 PM Pip Cet <pipcet@gmail.com> wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> I think you want
> (defun sly-make-action-button (label action &rest props)
>   (apply #'sly--make-text-button
>          label nil :type 'sly-action
>          'action action
>          'mouse-action action
>          props))
>
> instead, since the new function returns a copy of label rather than the
> string passed in.


By itself, that doesn't work.  I have the same problem.

I think I'd rather this previous behavior were retained, or at least
achievable by request. I haven't touched this code in a long
I don't know what else might depend on it.

I don't care about < 24.4 compatibility, if that helps

João

[-- Attachment #2: Type: text/html, Size: 1326 bytes --]

Re: 31395511: "Don’t attempt to modify constant strings"

[Paul Eggert]

On 6/3/20 4:48 PM, João Távora wrote:
> I think I'd rather this previous behavior were retained, or at least
> achievable by request.

It's tricky, as make-text-button in emacs-27 (and earlier) modifies its string
argument, which is buggy because string constants are not always unique. For
example:

(defun example-bug ()
  (concat "1. " (make-text-button
                 "example" nil
                 'action (lambda (_) (message "action 1")))
          "2. " (make-text-button
                 "example" nil
                 'action (lambda (_) (message "action 2")))))

If you byte-compile this in emacs-27, both buttons message "action 2" because
there's there's really just one instance of the string constant "example", and
so there's just one button and the second action overwrites the first.

Does SLY always pass mutable strings to make-text-button? I.e., strings built
from 'concat' etc. (not string constants)? If so, I could change
make-string-button to copy its string argument only if it's a constant, and that
should fix the compatibility issue without needing to make any changes to SLY.

Re: 31395511: "Don’t attempt to modify constant strings"

[Paul Eggert]

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

On 6/3/20 5:43 PM, Paul Eggert wrote:
> On 6/3/20 4:48 PM, João Távora wrote:
>> I think I'd rather this previous behavior were retained, or at least
>> achievable by request.
> 
> It's tricky, as make-text-button in emacs-27 (and earlier) modifies its string
> argument, which is buggy because string constants are not always unique.

On second thought, I'll work on coming up with a better workaround for the
problem along the lines I suggested in my previous message: make-text-button can
copy the button label string only if the string's not mutable. I hope this helps
with SLY (as I hope SLY is not trying to modify string literals...). In the
meantime I've reverted the change by installing the attached patch; this means
the bug I mentioned in my previous message remains unfixed.

[-- Attachment #2: 0001-Revert-make-text-button-string-copy.patch --]
[-- Type: text/x-patch, Size: 1295 bytes --]

From dd3484bedb7f0207c64ed391d1d7e699741e9917 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 3 Jun 2020 18:15:54 -0700
Subject: [PATCH] Revert make-text-button string copy
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/button.el (make-text-button): Don’t make a copy of
a button’s string label.  This reverts the change made in
2020-05-17T05:23:28Z!eggert@cs.ucla.edu, which broke SLY.
Problem reported by João Távora in:
https://lists.gnu.org/r/emacs-devel/2020-06/msg00117.html
However, we’ll need a better fix for this once string
literals become contents, if SLY uses string constants
for text button labels.
---
 lisp/button.el | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lisp/button.el b/lisp/button.el
index a91b0482ac..3a6a6de774 100644
--- a/lisp/button.el
+++ b/lisp/button.el
@@ -349,7 +349,6 @@ make-text-button
 	 (or (plist-member properties 'type)
 	     (plist-member properties :type))))
     (when (stringp beg)
-      (setq beg (copy-sequence beg)) ;; In case BEG is not mutable.
       (setq object beg beg 0 end (length object)))
     ;; Disallow setting the `category' property directly.
     (when (plist-get properties 'category)
-- 
2.25.4

Re: 31395511: "Don’t attempt to modify constant strings"

[Pip Cet]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 6/3/20 5:43 PM, Paul Eggert wrote:
>> On 6/3/20 4:48 PM, João Távora wrote:
>>> I think I'd rather this previous behavior were retained, or at least
>>> achievable by request.
>> It's tricky, as make-text-button in emacs-27 (and earlier) modifies
>> its string
>> argument, which is buggy because string constants are not always unique.
> On second thought, I'll work on coming up with a better workaround for the
> problem along the lines I suggested in my previous message:
> make-text-button can
> copy the button label string only if the string's not mutable. I hope
> this helps
> with SLY (as I hope SLY is not trying to modify string literals...).

It is. I'm not sure the copy-sequence-unless-mutable semantics really
make sense, though, as that might make bugs such as this one even harder
to find.

I think we should add a new function with clean semantics, and throw an
error in the old function if the string isn't "mutable", whatever that
means in this context. (I guess I can't modify the string contents or
add text properties, but can I modify existing properties?  What about
cons cells deep within the properties? If they're recursively immutable,
what about markers and other objects that change state behind your
back?)

> In the
> meantime I've reverted the change by installing the attached patch; this means
> the bug I mentioned in my previous message remains unfixed.

I'm still surprised my patch fixed the problem here (for some buttons,
at least, for others there are a few more places that do the same
thing...) but not for João. Maybe we're looking at a more complicated
bug, or maybe we haven't tested with the same kind of button (I entered
an unbound symbol and got the restart options, and those were buttons
with my patch but plain text without).

Re: 31395511: "Don’t attempt to modify constant strings"

[Basil L. Contovounesios]

Pip Cet <pipcet@gmail.com> writes:

> Paul Eggert <eggert@cs.ucla.edu> writes:
>
>> On second thought, I'll work on coming up with a better workaround for the
>> problem along the lines I suggested in my previous message:
>> make-text-button can
>> copy the button label string only if the string's not mutable. I hope
>> this helps
>> with SLY (as I hope SLY is not trying to modify string literals...).
>
> It is. I'm not sure the copy-sequence-unless-mutable semantics really
> make sense, though, as that might make bugs such as this one even harder
> to find.
>
> I think we should add a new function with clean semantics, and throw an
> error in the old function if the string isn't "mutable", whatever that
> means in this context. (I guess I can't modify the string contents or
> add text properties, but can I modify existing properties?  What about
> cons cells deep within the properties? If they're recursively immutable,
> what about markers and other objects that change state behind your
> back?)

How would make-text-button detect whether its first argument is mutable?
Would it not suffice to clarify in its documentation that it modifies
its argument, in the same way that we warn about passing immutable lists
to nconc?

-- 
Basil

Re: 31395511: "Don’t attempt to modify constant strings"

[Paul Eggert]

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

On 6/4/20 4:11 AM, Basil L. Contovounesios wrote:

> How would make-text-button detect whether its first argument is mutable?

It could try to mutate the string, and catch the error that is thrown when it's
not mutable. No such error is thrown now and Emacs can crash or worse - but I
plan to arrange for one to be thrown.

> Would it not suffice to clarify in its documentation that it modifies
> its argument, in the same way that we warn about passing immutable lists
> to nconc?

We could do that, yes. Some code passes string literals to make-text-button,
though, and we'd need to change it. The first example I found was ibuf-ext.el's
ibuffer-old-saved-filters-warning, which calls (make-text-button "here" ...).
Such code is already "broken" in some sense, so we'll need to fix it anyway somehow.

On 6/4/20 12:26 AM, Pip Cet wrote:

> I'm not sure the copy-sequence-unless-mutable semantics really
> make sense, though, as that might make bugs such as this one even harder
> to find.

True.

> I think we should add a new function with clean semantics, and throw an
> error in the old function if the string isn't "mutable", whatever that
> means in this context.

Throwing an error matches Basil's suggestion. What sort of clean semantics did
you have in mind?

> (I guess I can't modify the string contents or
> add text properties, but can I modify existing properties?  What about
> cons cells deep within the properties? If they're recursively immutable,
> what about markers and other objects that change state behind your
> back?)

The test I was thinking of is pretty simple: you can't modify the string object
itself, but you can modify the objects it points at. We could come up with
fancier tests later involving immutable property lists, but one thing at a time
and maybe this one thing is good enough (at least it should avoid the undefined
behavior).

> I'm still surprised my patch fixed the problem here (for some buttons,
> at least, for others there are a few more places that do the same
> thing...) but not for João.

There are several instances of the same problem in SLY. I found the ones in the
attached patch, and I expect there are others. So perhaps João was running into
one of the other problems.

[-- Attachment #2: sly.diff --]
[-- Type: text/x-patch, Size: 5359 bytes --]

diff --git a/contrib/sly-mrepl.el b/contrib/sly-mrepl.el
index f569bc48..265d752b 100644
--- a/contrib/sly-mrepl.el
+++ b/contrib/sly-mrepl.el
@@ -539,8 +539,7 @@ BEFORE and AFTER as in `sly-mrepl--save-and-copy-for-repl'"
                          'part-args (list (cadr result) idx)
                          'part-label (format "REPL Result")
                          'sly-mrepl--result result
-                         'sly-button-search-id (sly-button-next-search-id))
-  (car result))
+                         'sly-button-search-id (sly-button-next-search-id)))
 
 (defun sly-mrepl--insert-results (results)
   (let* ((comint-preoutput-filter-functions nil))
diff --git a/contrib/sly-stickers.el b/contrib/sly-stickers.el
index bd4bda06..7fcb4cc5 100644
--- a/contrib/sly-stickers.el
+++ b/contrib/sly-stickers.el
@@ -353,8 +353,7 @@ render the underlying text unreadable."
          :type 'sly-stickers--recording-part
          'part-args (list sticker-id recording vindex)
          'part-label "Recorded value"
-         props)
-  label)
+         props))
 
 (cl-defun sly-stickers--describe-recording-values (recording &key
                                                              (indent 0)
diff --git a/contrib/sly-trace-dialog.el b/contrib/sly-trace-dialog.el
index 44759e01..ec36a895 100644
--- a/contrib/sly-trace-dialog.el
+++ b/contrib/sly-trace-dialog.el
@@ -363,8 +363,7 @@ inspecting details of traced functions. Invoke this dialog with C-c T."
                          'part-label (format "%s %s"
                                              (capitalize
                                               (substring (symbol-name type) 1))
-                                             part-id))
-  part-text)
+                                             part-id)))
 
 (define-button-type 'sly-trace-dialog-spec :supertype 'sly-part
   'action 'sly-button-show-source
@@ -401,8 +400,7 @@ inspecting details of traced functions. Invoke this dialog with C-c T."
            'part-args (list id
                             (cdr (sly-trace-dialog--trace-spec trace)))
            'part-label (format "Trace entry: %s" id)
-           props))
-  label)
+           props)))
 
 (defun sly-trace-dialog--draw-tree-lines (start offset direction)
   (save-excursion
diff --git a/lib/sly-buttons.el b/lib/sly-buttons.el
index 8297ea74..81d63e7c 100644
--- a/lib/sly-buttons.el
+++ b/lib/sly-buttons.el
@@ -106,8 +106,7 @@
          label nil :type 'sly-action
          'action action
          'mouse-action action
-         props)
-  label)
+         props))
 
 (defface sly-action-face
   `((t (:inherit warning)))
diff --git a/sly.el b/sly.el
index 0ff8c0e0..d6ae2e79 100644
--- a/sly.el
+++ b/sly.el
@@ -3877,8 +3877,7 @@ SEARCH-FN is either the symbol `search-forward' or `search-backward'."
 (defun sly--compilation-note-group-button  (label notes)
   "Pepare notes as a `sly-compilation-note' button.
 For insertion in the `compilation-mode' buffer"
-  (sly--make-text-button label nil :type 'sly-compilation-note-group 'sly-notes-group notes)
-  label)
+  (sly--make-text-button label nil :type 'sly-compilation-note-group 'sly-notes-group notes))
 
 \f
 ;;;; Basic arglisting
@@ -4568,12 +4567,12 @@ TODO"
           (car designator)))
 
 (defun sly-apropos-insert-symbol (designator item bounds package-designator-searched-p)
-  (let ((label (sly-apropos-designator-string designator)))
-    (sly--make-text-button label nil
+  (let ((label
+	 (sly--make-text-button (sly-apropos-designator-string designator) nil
 				'face 'sly-apropos-symbol
 				'part-args (list item nil)
 				'part-label "Symbol"
-                           :type 'sly-apropos-symbol)
+				:type 'sly-apropos-symbol)))
     (cl-loop
      with offset = (if package-designator-searched-p
                        0
@@ -4728,8 +4727,7 @@ The most important commands:
   (sly--make-text-button label nil
                          :type 'sly-xref
                          'part-args (list location)
-                         'part-label "Location")
-  label)
+                         'part-label "Location"))
 
 (defun sly-insert-xrefs (xref-alist)
   "Insert XREF-ALIST in the current-buffer.
@@ -5742,8 +5740,7 @@ If MORE is non-nil, more frames are on the Lisp stack."
          'part-args (list (car frame)
                           (sly-db--guess-frame-function frame))
          'part-label (format "Frame %d" (car frame))
-         props)
-  label)
+         props))
 
 (defun sly-db-frame-number-at-point ()
   (let ((button (sly-db-frame-button-near-point)))
@@ -5851,8 +5848,7 @@ If MORE is non-nil, more frames are on the Lisp stack."
   (apply #'sly--make-text-button label nil
          :type 'sly-db-local-variable
          'part-args (list frame-number var-id)
-         'part-label (format "Local Variable %d" var-id) props)
-  label)
+         'part-label (format "Local Variable %d" var-id) props))
 
 (defun sly-db-frame-details-region (frame-button)
   "Get (BEG END) for FRAME-BUTTON's details, or nil if hidden"
@@ -6584,8 +6580,7 @@ was called originally."
          :type 'sly-inspector-part
          'part-args (list id)
          'part-label "Inspector Object"
-         props)
-  label)
+         props))
 
 (defmacro sly-inspector-fontify (face string)
   `(sly-add-face ',(intern (format "sly-inspector-%s-face" face)) ,string))

Re: 31395511: "Don’t attempt to modify constant strings"

[João Távora]

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

On Thu, Jun 4, 2020 at 8:46 PM Paul Eggert <eggert@cs.ucla.edu> wrote:

>
>
> > I'm still surprised my patch fixed the problem here (for some buttons,
> > at least, for others there are a few more places that do the same
> > thing...) but not for João.
>
> There are several instances of the same problem in SLY. I found the ones
> in the
> attached patch, and I expect there are others. So perhaps João was running
> into
> one of the other problems.
>

Yes, sorry, that was the case.  There was more code doing the same pattern.

I'm OK with changing to the new pattern, and now that this
has come up I do seem to remember being annoyed that I
make-text-button didn't return the string it added the properties
to.

The only problem is that this will break Emacs 24.4 support, unless
I do some version-checking thing.

João


-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 1371 bytes --]

Re: 31395511: "Don’t attempt to modify constant strings"

[Paul Eggert]

On 6/4/20 1:25 PM, João Távora wrote:
> On Thu, Jun 4, 2020 at 8:46 PM Paul Eggert <eggert@cs.ucla.edu> wrote:

> I'm OK with changing to the new pattern, and now that this
> has come up I do seem to remember being annoyed that I
> make-text-button didn't return the string it added the properties
> to.

In that case perhaps all that's needed is to document the issue with
make-text-button, at least unless someone comes up with a cleaner API.

RE: 31395511: "Don’t attempt to modify constant strings"

[Drew Adams]

> In that case perhaps all that's needed is to document the issue with
> make-text-button, at least unless someone comes up with a cleaner API.

Apologies for not following this thread.

I have code that uses `make-text-button'.
What is it, exactly that must not be done
by a user?
___

1. In help-fns+.el I pass a string as first arg
that results from `copy-sequence' of a key 
description.

2. In finder+.el I pass a `help-echo' value
that's a literal string.

3. In facemenu+.el I pass a `mouse-face' value
that's a list with a `:foreground' value
that's a literal string.

I'm guessing that you're talking only about
passing a literal string as the first arg
(so only #1 above).  Is that right?  If so,
is my #1 problematic?

Re: 31395511: "Don’t attempt to modify constant strings"

[Pip Cet]

Paul Eggert <eggert@cs.ucla.edu> writes:
> On 6/4/20 4:11 AM, Basil L. Contovounesios wrote:
>> How would make-text-button detect whether its first argument is mutable?
>
> It could try to mutate the string, and catch the error that is thrown
> when it's
> not mutable.

To be honest, I'd prefer a mutablep predicate, with a strong warning not
to use it in the way that was suggested:

(if (mutablep object)
    (do-something object)
  (do-something (copy object)))

> No such error is thrown now and Emacs can crash or worse - but I
> plan to arrange for one to be thrown.

Have those plans been discussed anywhere? I get the impression it would
help me to understand what you're planning to do.

>> Would it not suffice to clarify in its documentation that it modifies
>> its argument, in the same way that we warn about passing immutable lists
>> to nconc?
>
> We could do that, yes. Some code passes string literals to make-text-button,
> though, and we'd need to change it. The first example I found was
> ibuf-ext.el's
> ibuffer-old-saved-filters-warning, which calls (make-text-button "here" ...).

> Such code is already "broken" in some sense, so we'll need to fix it
> anyway somehow.

I fail to see how that code is broken: it uses an ephemeral string
literal, just once, and gives it text properties. I don't think this is
the best way of doing things, but it's a far cry from "Emacs can crash
or worse".  Am I missing something?


>
> On 6/4/20 12:26 AM, Pip Cet wrote:
>
>> I'm not sure the copy-sequence-unless-mutable semantics really
>> make sense, though, as that might make bugs such as this one even harder
>> to find.
>
> True.
>
>> I think we should add a new function with clean semantics, and throw an
>> error in the old function if the string isn't "mutable", whatever that
>> means in this context.
>
> Throwing an error matches Basil's suggestion. What sort of clean semantics did
> you have in mind?

Well, a documented return value would be a good start.  The "BEG can be
a string, in which case it's really the object, and we'll return it"
thing is confusing, I think.

I would suggest two functions, one which propertizes a string to be a
button when inserted, and returns the propertized string; and one which
adds text properties to make a range of an object (string or buffer)
into a button, and doesn't return anything useful.

>> (I guess I can't modify the string contents or
>> add text properties, but can I modify existing properties?  What about
>> cons cells deep within the properties? If they're recursively immutable,
>> what about markers and other objects that change state behind your
>> back?)
>
> The test I was thinking of is pretty simple: you can't modify the
> string object
> itself, but you can modify the objects it points at.

I think I can kind of decrypt that, but I'm not sure: keep in mind that
currently, for example, (text-properties-at N STRING) returns the
string's actual plist, so you can mutate it, which seems useless and
potentially dangerous to me. (Please, let's change that?)

Would you consider (text-properties-at N STRING) to be part of the
string object itself, or an object it points at?

> We could come up with
> fancier tests later involving immutable property lists, but one thing
> at a time
> and maybe this one thing is good enough (at least it should avoid the
> undefined
> behavior).

Which undefined behavior is that, precisely? It seems to me it would be
pretty easy to define current behavior, though it wouldn't be very
useful.

>> I'm still surprised my patch fixed the problem here (for some buttons,
>> at least, for others there are a few more places that do the same
>> thing...) but not for João.
>
> There are several instances of the same problem in SLY. I found the
> ones in the
> attached patch, and I expect there are others. So perhaps João was
> running into
> one of the other problems.

I think that was what was happening, yes.

Re: 31395511: "Don’t attempt to modify constant strings"

[Stefan Monnier]

> To be honest, I'd prefer a mutablep predicate, with a strong warning not
> to use it in the way that was suggested:
>
> (if (mutablep object)
>     (do-something object)
>   (do-something (copy object)))

Aka  (do-something (if (mutablep object) object (copy object)))


        Stefan

Re: 31395511: "Don’t attempt to modify constant strings"

[Pip Cet]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> To be honest, I'd prefer a mutablep predicate, with a strong warning not
>> to use it in the way that was suggested:
>>
>> (if (mutablep object)
>>     (do-something object)
>>   (do-something (copy object)))
>
> Aka  (do-something (if (mutablep object) object (copy object)))

All the more reason to avoid my bad-example code!

Re: 31395511: "Don’t attempt to modify constant strings"

[Paul Eggert]

On 6/4/20 1:43 PM, Pip Cet wrote:

> I'd prefer a mutablep predicate, with a strong warning not
> to use it

I'd rather not not build/support/advertise predicates that shouldn't be used....

>> No such error is thrown now and Emacs can crash or worse - but I
>> plan to arrange for one to be thrown.
> 
> Have those plans been discussed anywhere? I get the impression it would
> help me to understand what you're planning to do.

A few weeks ago, a bit. The idea I have is pretty simple: the Emacs interpreter
throws an error if you attempt to modify a string constant. Although the
interpreter done this for years, (a) its test for whether a string is a constant
has always been spotty and (b) the test has gone downhill recently.

> I fail to see how that code is broken: it uses an ephemeral string
> literal

String literals are not ephemeral; they can be coalesced, or retained, or put
into read-only memory. And when Emacs does that your program's behavior becomes
squirrelly.

> Well, a documented return value would be a good start.  The "BEG can be
> a string, in which case it's really the object, and we'll return it"
> thing is confusing, I think.

Yup.

> I would suggest two functions, one which propertizes a string to be a
> button when inserted, and returns the propertized string; and one which
> adds text properties to make a range of an object (string or buffer)
> into a button, and doesn't return anything useful.

I'm no expert on make-text-button etc. so I'll let the experts comment on that one.

> (text-properties-at N STRING) returns the
> string's actual plist, so you can mutate it, which seems useless and
> potentially dangerous to me. (Please, let's change that?)

We could do something along those lines eventually. The immediate problem that
I'm looking at is just the string itself.

> Would you consider (text-properties-at N STRING) to be part of the
> string object itself, or an object it points at?

My earlier email was assuming the current implementation, which is the latter.
However, I don't think this matters much, since string literals shouldn't have
text properties.

> Which undefined behavior is that, precisely?

I was referring to code that modifies literal strings' contents or properties.
We don't really define how that works, and in practice it doesn't work the way
people might naively expect since strings might be coalesced and their contents
might be in read-only memory.

Re: 31395511: "Don’t attempt to modify constant strings"

[Clément Pit-Claudel]

On 04/06/2020 19.10, Paul Eggert wrote:
>  I don't think this matters much, since string literals shouldn't have
> text properties.

Really? I've used the reader syntax for propertized strings a few times — it's pretty convenient.

Re: 31395511: "Don’t attempt to modify constant strings"

[Paul Eggert]

On 6/4/20 7:09 PM, Clément Pit-Claudel wrote:
> On 04/06/2020 19.10, Paul Eggert wrote:
>>  I don't think this matters much, since string literals shouldn't have
>> text properties.
> 
> Really? I've used the reader syntax for propertized strings a few times — it's pretty convenient.

Oh, you're right. That's a special case for the same reason that the reader
syntax for ordinary string literals is a special case; these strings are constants.

In the implementation that I have in mind, reading the syntax for a propertized
string gives you a string constant, in that the interpreter signals an error if
you try to change the string's characters or its text properties slot - but the
implementation does not prevent you from using setcar or setcdr on the text
properties list. Perhaps some day later we can add further checking to prevent
that sort of funny business on string constants' properties, but one thing at a
time.

If memory serves, it wasn't that long ago that the Elisp interpreter prevented
you from doing that sort of funny business on propertized string constants (at
least when they were in pure space), but we've fallen back a bit on our runtime
checking.

Re: 31395511: "Don’t attempt to modify constant strings"

[Stefan Monnier]

> If memory serves, it wasn't that long ago that the Elisp interpreter prevented
> you from doing that sort of funny business on propertized string constants (at
> least when they were in pure space),

AFAIK there aren't any propertized strings in purespace, because
`purecopy` doesn't copy the properties:

    static Lisp_Object
    purecopy (Lisp_Object obj)
    {
      if (FIXNUMP (obj)
          || (! SYMBOLP (obj) && PURE_P (XPNTR (obj)))
          || SUBRP (obj))
        return obj;    /* Already pure.  */
    
      if (STRINGP (obj) && XSTRING (obj)->u.s.intervals)
        message_with_string ("Dropping text-properties while making string `%s' pure",
                             obj, true);


-- Stefan

RE: 31395511: "Don’t attempt to modify constant strings"

[Drew Adams]

> > I don't think this matters much, since string
> > literals shouldn't have text properties.
> 
> Really? I've used the reader syntax for propertized
> strings a few times — it's pretty convenient.

+1.

And not just using reader syntax, i.e., not just
#("whatever" ...).  

"String literals shouldn't have text properties"
is an unhelpful and unlispy mantra.  It, in
effect, marginalizes the use of propertized
strings. And to what end - what's the gain that
offsets the loss?

"String literals appearing in code shouldn't be
implemented as constants" is a better mantra,
if we need a blanket point of view - hopefully
we don't.

Privileging byte-compiler optimizations such as
treating literal strings in code as constants,
over Lisp flexibility, is counter-productive
and not worth it.  What's lost is greater than
what's gained (presumably some space or speed).

Just as a (global) symbol is an _object_, with
properties, so can an Elisp string be.  That's
a powerful construct (missing in other Lisps).

Expecting users to use `make-string' or some
such, to avoid constant-string modification -
a la using `cons' or `list' to avoid modifying
constant list structure (e.g. quoted lists),
isn't very lispy, helpful, or reasonable.

This is true whether or not this has long been
problematic (accidentally or intentionally).

Users should be able to propertize a string
written literally in code, and change the
properties dynamically, over and over, without
inviting trouble.  They already know to use
`copy-sequence' when they need to ensure a new
string and not bother an existing one.  That
should be about all they ever need to do.

We should favor use of Elisp by Emacs users.
In general, we  shouldn't toss out flexibility
and convenience in an attempt to achieve
general-programming language features such as
high  performance.

It's OK to offer high performance if there's
no cost in convenience or flexibility, or if
that cost is really worth it.  And it's OK to
offer it optionally, where the sacrifice is an
intentional trade-off (by a user).

If you go the other way on this, then we at
least need to provide a simple way to
manipulate strings with properties - something
simpler than fiddling with `make-string' and
`copy-sequence' as often as we need to use
`cons', `list', and their backquote-comma
equivalents to avoid the gotcha of modifying
a quoted list.

If I use `copy-sequence' once, to ensure that
I don't bother an existing string, I should
be able to modify my new string over and over,
especially its text properties.

A quoted list that gets modified is bad enough
as a gotcha.  Many users never modify list
structure, and the doc makes clear when some
function does that.  And we try to document
that modify-quoted-list gotcha explicitly.
Not perfect, but we do try to help users with
that gotcha.

Modifying _symbol_ properties isn't a problem
because there is only ever one symbol with a
given name interned in a given obarray, and
the properties are separate from the obarray.
The obarray just holds the symbol identifier.

Modification of string properties needs to be
dealt with and documented reasonably, somehow.
Currently we can't rely on something like an
obarray, as we do with symbols.  And probably
more users will modify string properties than
list structure, so we need a solution that is
at least as good as the way we handle the list
gotcha.

Modifying the _chars_ in a string might be a
different story.  It's string properties I'm
mostly concerned about here.

We should look for a reasonable solution that
helps Emacs users, and not just favor compiled
code performance.

I know that more than the compiler is involved.
There's also the reader.  I'm asking for a
solution that makes modifying string properties
less, not more, problematic.

And barring any advance in the direction I'm
suggesting, let's at least try to give users
good, clear doc about whatever gotchas do exist
for modifying string text properties.  That's
not the best solution, but it's a necessary
fallback if nothing is improved in this regard
in the code.

Just one opinion.  +1 for propertized strings,
a wonderful Emacs-Lisp feature.

Re: 31395511: "Don’t attempt to modify constant strings"

[Pip Cet]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 6/4/20 1:43 PM, Pip Cet wrote:
>
>> I'd prefer a mutablep predicate, with a strong warning not
>> to use it
>
> I'd rather not not build/support/advertise predicates that shouldn't
> be used....

It's perfectly usable in most situations, it's just that you shouldn't
use it to decide whether your function has side effects or not.

>>> No such error is thrown now and Emacs can crash or worse - but I
>>> plan to arrange for one to be thrown.
>> 
>> Have those plans been discussed anywhere? I get the impression it would
>> help me to understand what you're planning to do.
>
> A few weeks ago, a bit. The idea I have is pretty simple: the Emacs
> interpreter
> throws an error if you attempt to modify a string constant. Although the
> interpreter done this for years, (a) its test for whether a string is
> a constant
> has always been spotty and (b) the test has gone downhill recently.

I think there was only CHECK_IMPURE, which relies on PURE_P, which is
effectively a nop in post-dump binaries. (I still think we should remove
pure space entirely, but even if we don't we should stop wasting so much
binary size on zeroes. But let's wait for Emacs 27 first, as Eli
suggested).

>> I fail to see how that code is broken: it uses an ephemeral string
>> literal
>
> String literals are not ephemeral;

I still believe this one is. It's used in a top-level form in a defvar.

> they can be coalesced, or retained, or put into read-only memory.

Really? Is there code in Emacs (other than purecopy, which isn't the
problem here) that does any of that today?

> And when Emacs does that your program's behavior becomes squirrelly.

If Emacs were to, a lot of code would break, yes. IMHO, that's a good
reason to leave things as they are for now, deal with the pure space
issues first, and then decide whether immutable objects are worth it at
all...

>> (text-properties-at N STRING) returns the
>> string's actual plist, so you can mutate it, which seems useless and
>> potentially dangerous to me. (Please, let's change that?)
>
> We could do something along those lines eventually. The immediate problem that
> I'm looking at is just the string itself.
>
>> Would you consider (text-properties-at N STRING) to be part of the
>> string object itself, or an object it points at?
>
> My earlier email was assuming the current implementation, which is the latter.
> However, I don't think this matters much, since string literals shouldn't have
> text properties.

But if text properties aren't part of "the string itself", they can be
given text properties.

>> Which undefined behavior is that, precisely?
>
> I was referring to code that modifies literal strings' contents or properties.
> We don't really define how that works, and in practice it doesn't work the way
> people might naively expect since strings might be coalesced and their
> contents
> might be in read-only memory.

You're saying "in practice ... their contents might be in read-only
memory"? How?

Re: 31395511: "Don’t attempt to modify constant strings"

[Paul Eggert]

On 6/5/20 2:48 AM, Pip Cet wrote:

>> they can be coalesced, or retained, or put into read-only memory.
> 
> Really? Is there code in Emacs (other than purecopy, which isn't the
> problem here) that does any of that today?

Sure, the byte-compiler coalesces strings with identical contents. If you change
one you change them all. And some built-in strings are put into read-only memory
now.

> if text properties aren't part of "the string itself", they can be
> given text properties.

I was thinking of just mimicking the traditional Elisp behavior that you can't
give modify the text properties of readonly strings. Of course if we want to
allow that sort of thing we could - but it sounds to me like a feature that's
more trouble than it's worth. I doubt there's a much legacy code out there that
depends on modifying text properties of string constants, since it wasn't
allowed until quite recently and was allowed more as an accident than anything else.

Re: 31395511: "Don’t attempt to modify constant strings"

[Basil L. Contovounesios]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 6/4/20 4:11 AM, Basil L. Contovounesios wrote:
>
>> Would it not suffice to clarify in its documentation that it modifies
>> its argument, in the same way that we warn about passing immutable lists
>> to nconc?
>
> We could do that, yes. Some code passes string literals to make-text-button,
> though, and we'd need to change it. The first example I found was ibuf-ext.el's
> ibuffer-old-saved-filters-warning, which calls (make-text-button "here" ...).
> Such code is already "broken" in some sense, so we'll need to fix it anyway somehow.

Thanks, that should be fixed now:

Fix some side-effecting uses of make-text-button
f51f963478 2020-06-04 23:30:34 +0100
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=f51f9634788323b3bf2dde59d0d20a8ca8fbfeaf

-- 
Basil

Re: 31395511: "Don’t attempt to modify constant strings"

[João Távora]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 6/3/20 4:48 PM, João Távora wrote:
>> I think I'd rather this previous behavior were retained, or at least
>> achievable by request.
>
> It's tricky, as make-text-button in emacs-27 (and earlier) modifies its string
> argument, which is buggy because string constants are not always unique. For
> example:
>
> (defun example-bug ()
>   (concat "1. " (make-text-button
>                  "example" nil
>                  'action (lambda (_) (message "action 1")))
>           "2. " (make-text-button
>                  "example" nil
>                  'action (lambda (_) (message "action 2")))))
>
> If you byte-compile this in emacs-27, both buttons message "action 2" because
> there's there's really just one instance of the string constant "example", and
> so there's just one button and the second action overwrites the first.

But if you evaluate it, that doesn't happen, which is probably even
worse.

And this is even stranger, IMO:

    (defun example-bug2 ()
      (eq (make-text-button
                     "example" nil
                     'action (lambda (_) (message "action 1")))
          (make-text-button
                     "example" nil
                     'action (lambda (_) (message "action 2")))))
     
    (defun example-bug3 ()
      (eq "example" "example"))
     
    (defun example-bug4 ()
      (let ((str1 "example")
            (str2 "example"))
        (eq str1 str2)))
     
    (list (example-bug2) (example-bug3) (example-bug4))

  when compiled, last form returns (t nil t)   in emacs 27,
  when compiled, last form returns (nil nil t) in emacs 28.
  when evaluated, last form returns (nil nil nil) in emacs 28.

For comparison, example-bug4 is valid Common Lisp and will return nil in
every Common Lisp implementation I know (I tested with ACL and SBCL),
regardless of whether compiled or evaluated.  I'm reasonably confident
there's somewhere in the Hyperspec where that behaviour may be specified
(I trust some CL pope will find it for me ;-) )

Elisp is its own Lisp of course, and I suppose these things allow for
performance/space optimizations under the hood, but is all that strange
behaviour worth it?

> Does SLY always pass mutable strings to make-text-button? I.e., strings built
> from 'concat' etc. (not string constants)? If so, I could change
> make-string-button to copy its string argument only if it's a constant, and that
> should fix the compatibility issue without needing to make any changes
> to SLY.

No it doesn't, as someone else has already reported.

João

Re: 31395511: "Don’t attempt to modify constant strings"

[Dmitry Gutov]

On 05.06.2020 18:25, João Távora wrote:
> But if you evaluate it, that doesn't happen, which is probably even
> worse.
> 
> And this is even stranger, IMO:
> 
>      (defun example-bug2 ()
>        (eq (make-text-button
>                       "example" nil
>                       'action (lambda (_) (message "action 1")))
>            (make-text-button
>                       "example" nil
>                       'action (lambda (_) (message "action 2")))))
>       
>      (defun example-bug3 ()
>        (eq "example" "example"))
>       
>      (defun example-bug4 ()
>        (let ((str1 "example")
>              (str2 "example"))
>          (eq str1 str2)))
>       
>      (list (example-bug2) (example-bug3) (example-bug4))
> 
>    when compiled, last form returns (t nil t)   in emacs 27,
>    when compiled, last form returns (nil nil t) in emacs 28.
>    when evaluated, last form returns (nil nil nil) in emacs 28.
> 
> For comparison, example-bug4 is valid Common Lisp and will return nil in
> every Common Lisp implementation I know (I tested with ACL and SBCL),
> regardless of whether compiled or evaluated.  I'm reasonably confident
> there's somewhere in the Hyperspec where that behaviour may be specified
> (I trust some CL pope will find it for me;-)  )

FWIW, it's a non-intuitive limitation for me as well.

But you can give bug#40671 a read, so see some context you might be missing.

Re: 31395511: "Don’t attempt to modify constant strings"

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> FWIW, it's a non-intuitive limitation for me as well.
>
> But you can give bug#40671 a read, so see some context you might be missing.

Thanks, that's a pretty long read.  There is indeed a relation, but that
bug (the first parts) is about modifying literal objects and this
particular strangeness seems bigger than that. I totally agree it is
undefined behaviour to change structure of literals (quoted or
self-evaluating objects), also in Common Lisp, because compilers are
probably allowed to reuse parts of the internal structure of such
objects.  But that's a far cry from having two different manifestations
of `equal` such objects _be_ the same object, but only for compiled
code.  Emacs doesn't behave that way for quoted lists (fortunately), so
I don't think it should behave that way for strings either.

An "easy" solution would be to say: in Elisp, there are no string
literals, period, because properties.  But that's likely expensive...
unless some clever copy-on-write semantics operate under the hood.  But
I'm talking out of my elbow, I don't really know what's under the hood
here.

João

Re: 31395511: "Don’t attempt to modify constant strings"

[Dmitry Gutov]

On 06.06.2020 02:19, João Távora wrote:
> There is indeed a relation, but that
> bug (the first parts) is about modifying literal objects and this
> particular strangeness seems bigger than that.

Later it goes deeper than that.

> But that's a far cry from having two different manifestations
> of `equal` such objects_be_  the same object, but only for compiled
> code.
That looks suboptimal to me too.  I mean, it sounds like a sound 
optimization (to an extent), but I really wonder if it really buys us 
much in practice. And we're paying the price of not exactly obvious 
behavior.

FW: 31395511: "Don’t attempt to modify constant strings"

[Drew Adams]

(Guess I forgot to use Reply All.  Re-sending.)

> > [bug#40671] is about modifying literal objects and this
> > particular strangeness seems bigger than that.
> 
> Later it goes deeper than that.
> 
> > But that's a far cry from having two different manifestations
> > of `equal` such objects _be_  the same object, but only for
> > compiled code.
>
> That looks suboptimal to me too.  I mean, it sounds like a sound
> optimization (to an extent), but I really wonder if it really buys us
> much in practice. And we're paying the price of not exactly obvious
> behavior.

Exactly the question - is it worth anything?  Is it
worth the cost of unclear and less useful behavior?

RE: 31395511: "Don’t attempt to modify constant strings"

[Drew Adams]

> > But you can give bug#40671 a read, so see some context you might be
> missing.
> 
> Thanks, that's a pretty long read.  There is indeed a relation, but that
> bug (the first parts) is about modifying literal objects and this
> particular strangeness seems bigger than that.
>
> I totally agree it is undefined behaviour to change structure
> of literals (quoted or self-evaluating objects), also in
> Common Lisp, because compilers are probably allowed to reuse
> parts of the internal structure of such objects.

Yes.  But Common Lisp is a general-programming
language.  Elisp is for use with Emacs.  And it
has strings that have properties, like symbols.

> But that's a far cry from having two different manifestations
> of `equal` such objects _be_ the same object, but only for compiled
> code.  Emacs doesn't behave that way for quoted lists (fortunately), so
> I don't think it should behave that way for strings either.

+1.

> An "easy" solution would be to say: in Elisp, there
> are no string literals, period, because properties.

+1.  Clean, flexible (dunno how "easy").

> But that's likely expensive...

Do we have an idea how expensive?  Lots of things
are expensive.  And some of them are worth it.

OK, some Elisp code might make heavy, repeated use
of long strings, and maybe some such uses would pay
a penalty.  Not sure though that the penalty would
be large, or "too large".

And most Elisp code won't be like that, I expect.

Maybe we could have a Boolean variable (which could
be made file-local or buffer-local on demand).

You could turn it on in some scope or for some
extent, to enable some string optimization at the
cost of either an occasional gotcha or systematic
error-raising when you try to modify etc.

Maybe that decision would need to be made at
byte-compile time, or maybe the compiled code could
(at the cost of being larger) respect it.

> unless some clever copy-on-write semantics operate
> under the hood.  But I'm talking out of my elbow,
> I don't really know what's under the hood here.

Nor do I, obviously.  I just have a hunch that,
in general, string optimization isn't worth it for
most Lisp code used in Emacs - not worth the loss
of being able to comfortably and generally modify
string properties.

Re: 31395511: "Don’t attempt to modify constant strings"

[Paul Eggert]

On 6/5/20 4:19 PM, João Távora wrote:
> I totally agree it is
> undefined behaviour to change structure of literals (quoted or
> self-evaluating objects), also in Common Lisp, because compilers are
> probably allowed to reuse parts of the internal structure of such
> objects.  But that's a far cry from having two different manifestations
> of `equal` such objects _be_ the same object, but only for compiled
> code.

I don't understand this remark, as the idea that "compilers are allowed to reuse
parts" necessarily implies that (eq "a" "a") can be t if the compiler decides to
reuse the string. Certainly in Common Lisp (eq "Foo" "Foo") might be true or
false (this specific example is called out in CLtL 6.3).

Anyway, Elisp has behaved compatibly with Common Lisp for some time, and it
works well in practice. I doubt whether it'd be a good idea to try to change
Elisp to require each string literal "unique", whatever that turns out to mean.

Re: 31395511: "Don’t attempt to modify constant strings"

[Richard Stallman]

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

  > Anyway, Elisp has behaved compatibly with Common Lisp for some time, and it
  > works well in practice. I doubt whether it'd be a good idea to try to change
  > Elisp to require each string literal "unique", whatever that turns out to mean.

I agree.

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

Re: 31395511: "Don’t attempt to modify constant strings"

[João Távora]

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

On Sat, Jun 6, 2020 at 2:43 AM Paul Eggert <eggert@cs.ucla.edu> wrote:

> On 6/5/20 4:19 PM, João Távora wrote:
> > I totally agree it is
> > undefined behaviour to change structure of literals (quoted or
> > self-evaluating objects), also in Common Lisp, because compilers are
> > probably allowed to reuse parts of the internal structure of such
> > objects.  But that's a far cry from having two different manifestations
> > of `equal` such objects _be_ the same object, but only for compiled
> > code.
>
> I don't understand this remark, as the idea that "compilers are allowed to
> reuse
> parts" necessarily implies that (eq "a" "a") can be t if the compiler
> decides to
> reuse the string.


Depending on the implementation of sequences, it could reuse only the
later parts of the sequences to maintain uniqueness and still have


> Certainly in Common Lisp (eq "Foo" "Foo") might be true or
> false (this specific example is called out in CLtL 6.3).
>

I stand corrected.  I was simply mistaken :-)

João

[-- Attachment #2: Type: text/html, Size: 1624 bytes --]

Re: 31395511: "Don’t attempt to modify constant strings"

[João Távora]

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

On Sat, Jun 6, 2020 at 12:41 PM João Távora <joaotavora@gmail.com> wrote:

> On Sat, Jun 6, 2020 at 2:43 AM Paul Eggert <eggert@cs.ucla.edu> wrote:
>
>> On 6/5/20 4:19 PM, João Távora wrote:
>> > I totally agree it is
>> > undefined behaviour to change structure of literals (quoted or
>> > self-evaluating objects), also in Common Lisp, because compilers are
>> > probably allowed to reuse parts of the internal structure of such
>> > objects.  But that's a far cry from having two different manifestations
>> > of `equal` such objects _be_ the same object, but only for compiled
>> > code.
>>
>> I don't understand this remark, as the idea that "compilers are allowed
>> to reuse
>> parts" necessarily implies that (eq "a" "a") can be t if the compiler
>> decides to
>> reuse the string.
>
>
> Depending on the implementation of sequences, it could reuse only the
> later parts of the sequences to maintain uniqueness and still have
>

I forgot to finish the sentence: "and still have some some reuse".

João

[-- Attachment #2: Type: text/html, Size: 1749 bytes --]

Re: 31395511: "Don’t attempt to modify constant strings"

[Pip Cet]

On Wed, Jun 3, 2020 at 11:48 PM João Távora <joaotavora@gmail.com> wrote:
> On Wed, Jun 3, 2020 at 11:42 PM Pip Cet <pipcet@gmail.com> wrote:
>> I think you want
>> (defun sly-make-action-button (label action &rest props)
>>   (apply #'sly--make-text-button
>>          label nil :type 'sly-action
>>          'action action
>>          'mouse-action action
>>          props))
>>
>> instead, since the new function returns a copy of label rather than the
>> string passed in.

> By itself, that doesn't work.  I have the same problem.

Strange, I'd tried it locally and it appeared to work.

> I think I'd rather this previous behavior were retained, or at least
> achievable by request. I haven't touched this code in a long
> I don't know what else might depend on it.

But the previous behavior was buggy. Things like

(defun sly-inspector-insert-more-button (index previous)
  (insert (sly-make-action-button
           (if previous " [--more--]\n" " [--more--]")
           #'sly-inspector-fetch-more
           'range-args (list index previous))))

worked only by accident, before.

Re: 31395511: "Don’t attempt to modify constant strings"

[João Távora]

Pip Cet <pipcet@gmail.com> writes:

> On Wed, Jun 3, 2020 at 11:48 PM João Távora <joaotavora@gmail.com> wrote:
>> On Wed, Jun 3, 2020 at 11:42 PM Pip Cet <pipcet@gmail.com> wrote:
>> By itself, that doesn't work.  I have the same problem.
>
> Strange, I'd tried it locally and it appeared to work.

Yep, I didn't dig in, but it seems now that some buttons works and
other's don't  work.  So in a REPL typing "123"

  CL-USER> 123
  123 (7 bits, #x7b, #o173, #b1111011)

doesn't work (the second line should be interactive and it's not). But
now I did dig in and that's because other code is using the same
paradigm that sly-make-action-button was using.  Everything should work
if I track such users and change them.

I think I will lose compatibility to ~24.4, which is where I developed
this code.  Oh well.

>> I think I'd rather this previous behavior were retained, or at least
>> achievable by request. I haven't touched this code in a long
>> I don't know what else might depend on it.
>
> But the previous behavior was buggy. Things like
>
> (defun sly-inspector-insert-more-button (index previous)
>   (insert (sly-make-action-button
>            (if previous " [--more--]\n" " [--more--]")
>            #'sly-inspector-fetch-more
>            'range-args (list index previous))))
>
> worked only by accident, before.
  
Accident/spec/divine providence: they worked, and for a long time.
make-text-button never warned me about mutability/immutability, constant
strings or non-constant strings.  So effectively we're changing the sepc
implicit or explicit, in a backwards-incompatible way, and that breaks
programs, case in point.

João

PS: I admit I didn't check the recent "incompatible lisp changes" NEWS
for recent Emacs versions, maybe there's something in there that could
have given me a heads up.