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

[Paul Eggert <eggert@cs.ucla.edu>]

[-- 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))