Re: master f51f963: Fix some side-effecting uses of make-text-button

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Stefan Monnier]

>        (make-text-button name nil
>                          'type 'apropos-library
>                          'face 'apropos-symbol
> -                        'apropos-symbol name)
> -      name)))
> +                        'apropos-symbol name))))

So, IIUC we consider `make-text-button` to be functional rather than
side-effecting, right?

> @@ -202,7 +202,7 @@ The format has been repaired and the variable modified accordingly.
>  You can save the current value through the customize system by
>  either clicking or hitting return "
>            (make-text-button
> -           "here" nil
> +           (copy-sequence "here") nil
>             'face '(:weight bold :inherit button)
>             'mouse-face '(:weight normal :background "gray50" :inherit button)
>             'follow-link t

So, here why do we need to `copy-sequence`?


        Stefan

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Paul Eggert]

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

On 6/4/20 3:44 PM, Stefan Monnier wrote:
> So, IIUC we consider `make-text-button` to be functional rather than
> side-effecting, right?

It *ought* to be functional but it's currently not, because make-text-button has
a side effect on its argument string.

How about the attached patch? This would mean SLY would need the patch I sent in
earlier[1] since the attached patch is not 100% compatible with existing Emacs;
however, it does make make-text-button more functional and that's a good thing.

[1] https://lists.gnu.org/r/emacs-devel/2020-06/msg00152.html

[-- Attachment #2: make-text-button.diff --]
[-- Type: text/x-patch, Size: 2549 bytes --]

diff --git a/etc/NEWS b/etc/NEWS
index ed4722b27f..5479831448 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -471,6 +471,11 @@ are 'eq'.  To compare contents, use 'compare-window-configurations'
 instead.  This change helps fix a bug in 'sxhash-equal', which returned
 incorrect hashes for window configurations and some other objects.
 
+** When its first argument is a string, 'make-text-button' no longer
+modifies the string's text properties; instead, it uses and returns a
+copy of the string.  This helps avoid trouble when strings are shared
+or constants.
+
 ---
 ** The obsolete function 'thread-alive-p' has been removed.
 
diff --git a/lisp/apropos.el b/lisp/apropos.el
index 22866cd2cc..2566d44dfc 100644
--- a/lisp/apropos.el
+++ b/lisp/apropos.el
@@ -661,7 +661,7 @@ apropos
 (defun apropos-library-button (sym)
   (if (null sym)
       "<nothing>"
-    (let ((name (copy-sequence (symbol-name sym))))
+    (let ((name (symbol-name sym)))
       (make-text-button name nil
                         'type 'apropos-library
                         'face 'apropos-symbol
diff --git a/lisp/button.el b/lisp/button.el
index 3a6a6de774..76b0e9102f 100644
--- a/lisp/button.el
+++ b/lisp/button.el
@@ -341,7 +341,7 @@ make-text-button
 as the argument for the `action' callback function instead of the
 default argument, which is the button itself.
 
-BEG can also be a string, in which case it is made into a button.
+BEG can also be a string, in which case a copy of it is made into a button.
 
 Also see `insert-text-button'."
   (let ((object nil)
@@ -349,7 +349,9 @@ make-text-button
 	 (or (plist-member properties 'type)
 	     (plist-member properties :type))))
     (when (stringp beg)
-      (setq object beg beg 0 end (length object)))
+      (setq object (copy-sequence beg))
+      (setq beg 0)
+      (setq end (length object)))
     ;; Disallow setting the `category' property directly.
     (when (plist-get properties 'category)
       (error "Button `category' property may not be set directly"))
diff --git a/lisp/ibuf-ext.el b/lisp/ibuf-ext.el
index c39000b488..bfb9787a96 100644
--- a/lisp/ibuf-ext.el
+++ b/lisp/ibuf-ext.el
@@ -202,7 +202,7 @@ ibuffer-old-saved-filters-warning
 You can save the current value through the customize system by
 either clicking or hitting return "
           (make-text-button
-           (copy-sequence "here") nil
+           "here" nil
            'face '(:weight bold :inherit button)
            'mouse-face '(:weight normal :background "gray50" :inherit button)
            'follow-link t

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Pip Cet]

Paul Eggert <eggert@cs.ucla.edu> writes:
> diff --git a/lisp/button.el b/lisp/button.el
> index 3a6a6de774..76b0e9102f 100644
> --- a/lisp/button.el
> +++ b/lisp/button.el
> @@ -341,7 +341,7 @@ make-text-button
>  as the argument for the `action' callback function instead of the
>  default argument, which is the button itself.
>  
> -BEG can also be a string, in which case it is made into a button.
> +BEG can also be a string, in which case a copy of it is made into a button.

"and returned", I think. People will need to use the return value now.

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Basil L. Contovounesios]

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

>>        (make-text-button name nil
>>                          'type 'apropos-library
>>                          'face 'apropos-symbol
>> -                        'apropos-symbol name)
>> -      name)))
>> +                        'apropos-symbol name))))
>
> So, IIUC we consider `make-text-button` to be functional rather than
> side-effecting, right?

As Paul said, not quite - it has historically modified its first
argument by placing properties on it.  If you look at the line preceding
this diff hunk you'll see a call to copy-sequence.

My reasons for making this particular change are:
0. Since Emacs 24.4, make-text-button has returned its modified first
   argument, so callers can use the value returned by make-text-button
   instead of calling it only for its side effects.
1. This has now been reverted again, but for the last month,
   make-text-button returned a modified _copy_ of its argument, which
   meant that its side effects could no longer be relied upon.

Either way, relying on its return value rather than its side effects
seems like the best style to stick with for now.

>> @@ -202,7 +202,7 @@ The format has been repaired and the variable modified accordingly.
>>  You can save the current value through the customize system by
>>  either clicking or hitting return "
>>            (make-text-button
>> -           "here" nil
>> +           (copy-sequence "here") nil
>>             'face '(:weight bold :inherit button)
>>             'mouse-face '(:weight normal :background "gray50" :inherit button)
>>             'follow-link t
>
> So, here why do we need to `copy-sequence`?

To avoid destructively modifying a string literal by placing properties
on it.

-- 
Basil

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Pip Cet]

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

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> @@ -202,7 +202,7 @@ The format has been repaired and the variable modified accordingly.
>>>  You can save the current value through the customize system by
>>>  either clicking or hitting return "
>>>            (make-text-button
>>> -           "here" nil
>>> +           (copy-sequence "here") nil
>>>             'face '(:weight bold :inherit button)
>>>             'mouse-face '(:weight normal :background "gray50" :inherit button)
>>>             'follow-link t
>>
>> So, here why do we need to `copy-sequence`?
>
> To avoid destructively modifying a string literal by placing properties
> on it.

I think adding a concept of mutability/constness/finality/... could be a
great extension of the ELisp language.

It would also be a very significant change of that language, perhaps
comparable to "true" multi-threading.

It would not be a quick bug fix for code that uses (propertize "string"
'a 'b).

In particular, I'm not convinced code like that is buggy at all. It's
true that it will fail under certain conditions (the string constant is
used again in the same function, the function is byte compiled, that
sort of thing), and it's true there are better ways of doing that, but
is that reason enough to off-handedly ban all such code?

I probably don't even know half of it, but there are so many overlapping
concepts ("const" in C, "constexpr" in C++, "final" in Java, "const" in
JavaScript, "frozen" objects in Python...) that I get the impression we
shouldn't discount the possibility that the current way of doing things
(after pure space) isn't so bad at all: all strings, vectors, and cons
cells are mutable to the same extent.

I think it's worth it to experiment with other concepts of mutability,
perhaps on a feature branch, but I don't think that's true for a concept
that, so far, appears to be "literal strings can't be given text
properties unless they already have at least one, in which case you can
alter their text properties but not remove them all".

I'll take all of that back if I actually see a bug that means Lisp code
can cause an Emacs crash (in the C sense of "crash") by mutating literal
strings, and that can't easily be fixed in C, and isn't actually a known
limitation of the byte compiler.

My guess is a concept of immutability won't be very useful if it's just
a single bit telling you "this object is immutable": we need to attach
more information to it, perhaps going as far as providing special ways
of mutating the object rather than simply signalling an information-free
error. (So, for example, a vector could be defined in Lisp to coerce all
of its entries to nil or t, so we wouldn't need bool vectors anymore).

Sorry this got long, but I don't understand the rush to actually commit
to what appears to me to be a simplistic model of mutability when we
haven't even removed the previous one, which I believe we've outgrown:
pure space.

(I think we should also get rid of the hash table mutability thing
again, but that's another discussion entirely).

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Basil L. Contovounesios]

Pip Cet <pipcet@gmail.com> writes:

[...]

> It would not be a quick bug fix for code that uses (propertize "string"
> 'a 'b).

propertize returns a copy of its argument, so it's not problematic in
this context.

> In particular, I'm not convinced code like that is buggy at all. It's
> true that it will fail under certain conditions (the string constant is
> used again in the same function, the function is byte compiled, that
> sort of thing), and it's true there are better ways of doing that, but
> is that reason enough to off-handedly ban all such code?

[...]

My only concern with destructively modifying string literals as in the
(make-text-button "here" ...) example is that it can "pollute" that
string literal for all of its users, which is both "rude" and
unnecessary in this case.  I simply find it cleaner, less intrusive, and
less surprising to modify only one's own copy of a string in such a
trivial case.  I'm not personally concerned with crashes, theoretical
purity or mutability, or anything like that; for that I defer to you,
Paul, and other experts.

-- 
Basil

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Pip Cet]

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

> Pip Cet <pipcet@gmail.com> writes:
>
> [...]
>
>> It would not be a quick bug fix for code that uses (propertize "string"
>> 'a 'b).
>
> propertize returns a copy of its argument, so it's not problematic in
> this context.

Thanks for pointing that out. I hope it was still clear what I meant.

>> In particular, I'm not convinced code like that is buggy at all. It's
>> true that it will fail under certain conditions (the string constant is
>> used again in the same function, the function is byte compiled, that
>> sort of thing), and it's true there are better ways of doing that, but
>> is that reason enough to off-handedly ban all such code?
>
> [...]
>
> My only concern with destructively modifying string literals as in the
> (make-text-button "here" ...) example is that it can "pollute" that
> string literal for all of its users, which is both "rude" and
> unnecessary in this case.  I simply find it cleaner, less intrusive, and
> less surprising to modify only one's own copy of a string in such a
> trivial case.  I'm not personally concerned with crashes, theoretical
> purity or mutability, or anything like that; for that I defer to you,
> Paul, and other experts.

I'd like to apologise. I made several mistakes in that email (including
using the word "off-handedly" when a less offensive term would have
done). Most importantly, I think, I was commenting on a change that, as
you correctly point out, made the code cleaner and less surprising, as
though it were a whole-hearted endorsement of breaking any existing code
that is more surprising or less clean. I see now that you said no such
thing.

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Basil L. Contovounesios]

Pip Cet <pipcet@gmail.com> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> Pip Cet <pipcet@gmail.com> writes:
>>
>> [...]
>>
>>> It would not be a quick bug fix for code that uses (propertize "string"
>>> 'a 'b).
>>
>> propertize returns a copy of its argument, so it's not problematic in
>> this context.
>
> Thanks for pointing that out. I hope it was still clear what I meant.

It rarely isn't. :)

>> My only concern with destructively modifying string literals as in the
>> (make-text-button "here" ...) example is that it can "pollute" that
>> string literal for all of its users, which is both "rude" and
>> unnecessary in this case.  I simply find it cleaner, less intrusive, and
>> less surprising to modify only one's own copy of a string in such a
>> trivial case.  I'm not personally concerned with crashes, theoretical
>> purity or mutability, or anything like that; for that I defer to you,
>> Paul, and other experts.
>
> I'd like to apologise. I made several mistakes in that email (including
> using the word "off-handedly" when a less offensive term would have
> done). Most importantly, I think, I was commenting on a change that, as
> you correctly point out, made the code cleaner and less surprising, as
> though it were a whole-hearted endorsement of breaking any existing code
> that is more surprising or less clean. I see now that you said no such
> thing.

No need to apologise.  I figured what you meant, don't take offence,
sympathise with your position, and am interested to follow this
discussion.  I just wanted to establish that this particular commit
should™ be pretty uncontroversial, which I hope I succeeded in doing. ;)

-- 
Basil

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Paul Eggert]

On 6/5/20 5:46 AM, Pip Cet wrote:
> I get the impression we
> shouldn't discount the possibility that the current way of doing things
> (after pure space) isn't so bad at all: all strings, vectors, and cons
> cells are mutable to the same extent.

That's not the current way of doing things, and although the area is murky there
have always been Emacs Lisp objects that are not mutable. For example:

(aset (symbol-name 'cons) 0 ?d)

This signals "Attempt to modify read-only object" error in Emacs 25, and makes
Emacs dump core in Emacs 27. And there are other cases like that.

Obviously we need to do better in the dumping-core area. When we do that, we
have an opportunity to simplify and/or document behavior in this area.

If we decide to simplify/document by saying "all strings are modifiable" then
we'll need significant work at both the C and Lisp level to do that. This will
hurt performance a bit since it will disable some optimizations.

If we decide to simplify/document by saying "an error is thrown if you try to
modify a string literal" then we'll need to add some code to do that. I have a
draft of something along those lines. It doesn't hurt performance significantly
in my standard benchmark of 'make compile-always'. Although it invalidates some
existing code, such code is quite rare and is already relying on undefined behavior.

If we decide to leave things alone, they will remain complicated and murky.

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Pip Cet]

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

> On 6/5/20 5:46 AM, Pip Cet wrote:
>> I get the impression we
>> shouldn't discount the possibility that the current way of doing things
>> (after pure space) isn't so bad at all: all strings, vectors, and cons
>> cells are mutable to the same extent.
>
> That's not the current way of doing things, and although the area is
> murky there
> have always been Emacs Lisp objects that are not mutable.

Lately, only pure ones, as far as I can tell?

> For example:

> (aset (symbol-name 'cons) 0 ?d)
>
> This signals "Attempt to modify read-only object" error in Emacs 25, and makes
> Emacs dump core in Emacs 27. And there are other cases like that.

Well, dumping core is bad. The problem here is how pdumper "changed"
pure space (actually, we're putting several megabytes of zeroes into
every Emacs binary as a result) and how make_pure_c_string tries so very
hard to save a few kilobytes of memory. Both problems, as I said, that
wouldn't exist if we simply removed pure space.

> Obviously we need to do better in the dumping-core area. When we do that, we
> have an opportunity to simplify and/or document behavior in this area.

Indeed. Simplify: remove pure space. Document: all strings, vectors, and
cons cells are mutable to the same extent.

> If we decide to simplify/document by saying "all strings are modifiable" then
> we'll need significant work at both the C and Lisp level to do that.

I don't see why. All strings are modifiable, but the byte compiler will
identify strings under certain circumstances. That doesn't violate the
simple rule that as far as the Emacs core is concerned, all strings are
equal.

> This will
> hurt performance a bit since it will disable some optimizations.

Which ones?

> If we decide to simplify/document by saying "an error is thrown if you try to
> modify a string literal" then we'll need to add some code to do that. I have a

So far, what you have proposed is "an error is thrown if you try to
modify the characters of a string literal, or if you add text
properties unless it already has some, or if you remove the last text
property".

> draft of something along those lines. It doesn't hurt performance
> significantly
> in my standard benchmark of 'make compile-always'.

(In general, I think that's probably not a good benchmark to optimize
Emacs for).

> Although it
> invalidates some
> existing code, such code is quite rare and is already relying on
> undefined behavior.

I'm not sure "undefined behavior" is a useful term when speaking about
Emacs Lisp, except for behavior which is explicitly documented to be
unreliable. There's a single implementation, and a lot of code is
written to conform not to what's documented but to what happens to
work.

> If we decide to leave things alone, they will remain complicated and murky.

But I'd call the behavior you suggest even more complicated.

I still think there's a significant risk that there will be ad-hoc
changes that essentially commit us to a simplistic model of
mutability. I don't think they're necessary or urgent, except for the
make_pure_c_string bug you describe.

For example, I think it might be very useful to have an immutable "view"
of a mutable object (as in C, where I can pass a char * to a function
expecting a const char *); that would mean storing the mutability flag
in the Lisp_Object, not in the struct Lisp_String.

RE: master f51f963: Fix some side-effecting uses of make-text-button

[Drew Adams]

> >> I get the impression we
> >> shouldn't discount the possibility that the current way of doing things
> >> (after pure space) isn't so bad at all: all strings, vectors, and cons
> >> cells are mutable to the same extent.
> >
> > That's not the current way of doing things, and although the area is
> > murky there have always been Emacs Lisp objects that are not mutable.
> 
> Lately, only pure ones, as far as I can tell?
> 
> > For example: (aset (symbol-name 'cons) 0 ?d)
> > This signals "Attempt to modify read-only object" error in Emacs 25, and
> > makes Emacs dump core in Emacs 27. And there are other cases like that.
> 
> Well, dumping core is bad. The problem here is how pdumper "changed"
> pure space (actually, we're putting several megabytes of zeroes into
> every Emacs binary as a result) and how make_pure_c_string tries so very
> hard to save a few kilobytes of memory. Both problems, as I said, that
> wouldn't exist if we simply removed pure space.
> 
> > Obviously we need to do better in the dumping-core area. When we do that,
> > we have an opportunity to simplify and/or document behavior in this area.
> 
> Indeed. Simplify: remove pure space. Document: all strings, vectors, and
> cons cells are mutable to the same extent.
> 
> > If we decide to simplify/document by saying "all strings are modifiable"
> > then we'll need significant work at both the C and Lisp level to do that.
> 
> I don't see why. All strings are modifiable, but the byte compiler will
> identify strings under certain circumstances. That doesn't violate the
> simple rule that as far as the Emacs core is concerned, all strings are
> equal.
> 
> > This will hurt performance a bit since it will disable some optimizations.
> 
> Which ones?
> 
> > If we decide to simplify/document by saying "an error is thrown if you try
> > to modify a string literal" then we'll need to add some code to do that. I
> > have a
> 
> So far, what you have proposed is "an error is thrown if you try to
> modify the characters of a string literal, or if you add text
> properties unless it already has some, or if you remove the last text
> property".
> 
> > draft of something along those lines. It doesn't hurt performance
> > significantly in my standard benchmark of 'make compile-always'.
> 
> (In general, I think that's probably not a good benchmark to optimize
> Emacs for).
> 
> > Although it invalidates some existing code, such code is quite
> > rare and is already relying on undefined behavior.
> 
> I'm not sure "undefined behavior" is a useful term when speaking about
> Emacs Lisp, except for behavior which is explicitly documented to be
> unreliable. There's a single implementation, and a lot of code is
> written to conform not to what's documented but to what happens to
> work.
> 
> > If we decide to leave things alone, they will remain complicated and murky.
> 
> But I'd call the behavior you suggest even more complicated.

Amen.

> I still think there's a significant risk that there will be ad-hoc
> changes that essentially commit us to a simplistic model of
> mutability. I don't think they're necessary or urgent, except for the
> make_pure_c_string bug you describe.
> 
> For example, I think it might be very useful to have an immutable "view"
> of a mutable object (as in C, where I can pass a char * to a function
> expecting a const char *); that would mean storing the mutability flag
> in the Lisp_Object, not in the struct Lisp_String.

+1.  All that Pip says here makes sense to me,
as far as I understand it.  (I can't speak to the
implementation matters, e.g. use of pure space.)

If someone has the cycles and will to improve
things by making the modification of strings,
including what look like literal strings in
code, easier and more flexible - in particular
text properties, great.

But going backwards, toward some perhaps unneeded
optimization, in the direction of systematically
raising an error when trying to modify text
properties of a string, is not a good idea, IMO.

Before even considering optimization there should
be some analysis of what would be gained and what
would be lost.

Yes, currently there is some mess and uncertainty.
But there are good and bad cures to mess and
uncertainty - different kinds of "cleanup".

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Stefan Monnier]

> But going backwards, toward some perhaps unneeded
> optimization, in the direction of systematically
> raising an error when trying to modify text
> properties of a string, is not a good idea, IMO.

I think there needs to be a clarification here: the issue is about
modifying data (here specifically strings, but the issue applies to all
other such data) that appears as literal in the code.

This issue is not one of optimization (preventing those modifications
would likely impose a slowdown, if anything) but one of detecting usage
that is usually a bug (one that leads to people being utterly
confused by the resulting behavior).


        Stefan

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Pip Cet]

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

>> But going backwards, toward some perhaps unneeded
>> optimization, in the direction of systematically
>> raising an error when trying to modify text
>> properties of a string, is not a good idea, IMO.
>
> I think there needs to be a clarification here: the issue is about
> modifying data (here specifically strings, but the issue applies to all
> other such data) that appears as literal in the code.

> This issue is not one of optimization (preventing those modifications
> would likely impose a slowdown, if anything) but one of detecting usage
> that is usually a bug (one that leads to people being utterly
> confused by the resulting behavior).

I don't see how literal data is special that way. If I expect a function
to look at a string argument, but it actually modifies its argument,
that's equally confusing. If I modify data that's been used in a hash
key, that's even more confusing. If I modify data in an image spec in a
Lisp callback from the image backend, Emacs will crash. These cases
deserve being thought about, too.

How many such bugs have appeared and been difficult to debug since pure
space essentially stopped existing when pdumper was introduced?

The cost of this isn't negligible; the single bit which I expect will be
kept for every string, cons cell, or vector isn't that significant, but
so far what's been proposed would be complicated to implement, explain,
and use. It would lead to some people developing a false sense of
security and others becoming insecure and copying everything needlessly
(and dangerously, for cyclic objects). And it would effectively prevent
any competing system of mutability, I fear.

For example, consider quasi-quoted literals: would those be immutable?
No matter what the answer to that question is, would people actually
remember?

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Paul Eggert]

On 6/6/20 12:00 PM, Pip Cet wrote:
> consider quasi-quoted literals: would those be immutable?

A string literal should not be modified, regardless of whether it's quasiquoted.

A subexpression of `X that contains no substitutions or splices yields conses
that should not be modified.

These two rules are reasonably simple and are already documented, and neither
rule should be that hard to remember for the few people whose code is affected
by this issue.

> It would lead to some people developing a false sense of security

"False sense" because programmers would start relying on Emacs to catch trivial
mistakes involving modifying string literals? Horrors! (Programmers should let
those mistakes persist into hard-to-debug complex programs, as this will give
them much more of a challenge when debugging. :-)

RE: master f51f963: Fix some side-effecting uses of make-text-button

[Drew Adams]

> > consider quasi-quoted literals: would those be immutable?
> 
> A string literal should not be modified, regardless
> of whether it's quasiquoted.

You keep repeating the mantra that a string literal
should not be modified, with no good supporting
argument.

Elisp strings, and hence its string "literals" are
not your textbook strings and string literals.  The
latter are not mutable objects, with text properties.

Emacs strings are, in general.  And more of them
could be - they could even all be, I expect.

> "False sense" because programmers would start relying
> on Emacs to catch trivial mistakes involving modifying
> string literals? Horrors! (Programmers should let
> those mistakes persist into hard-to-debug complex
> programs, as this will give them much more of a challenge
> when debugging. :-)

With that logic, we should outlaw list modification.
Some users will make mistakes, or get confused, and
get into trouble.

Heck, why not take it even further: outlaw dynamic
state altogether - complex, difficult to reason about,
difficult to analyze and optimize by program, difficult
to optimize and parallelize.

Just a lot of trouble and headache, right?  All such
arguments are 100% valid.  And there are languages
based on them.  Lisp isn't one of them.  And by design
Emacs isn't based on one of them.

FWIW, I love purely declarative, applicative, logic and
functional languages.  Always have.  I love formal logic,
algebraic specification of ADTs, theorem proving, etc.

But I also appreciate the dynamic, side-effecting
"messiness" of dirty old bit-fiddling Lisp - and
_especially_ for a user-programmer editor/env such
as that dirty old man, Emacs.

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Pip Cet]

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

>> consider quasi-quoted literals: would those be immutable?
>
> A string literal should not be modified, regardless of whether it's
> quasiquoted.
>
> A subexpression of `X that contains no substitutions or splices yields conses
> that should not be modified.
>
> These two rules are reasonably simple and are already documented, and neither
> rule should be that hard to remember for the few people whose code is affected
> by this issue.

Okay. It's a situation I was unsure about myself, but if these are the
rules, they are easy enough to remember.

>> It would lead to some people developing a false sense of security
>
> "False sense" because programmers would start relying on Emacs to
> catch trivial
> mistakes involving modifying string literals? Horrors! (Programmers should let
> those mistakes persist into hard-to-debug complex programs, as this will give
> them much more of a challenge when debugging. :-)

I'm not sure programmers would realize those errors are caught only at
run time, rather than being detected at compile time, which they could
be in many cases.

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Stefan Monnier]

> I don't see how literal data is special that way.

In code

    (let ((x "foo"))
      (cl-assert (eq (aref x 0) ?f))
      ...)

every programmer I know expects the assertion to be true, always, no
matter what.  Yet the assertion will fail if `...` does something like
(aset x 0 ?o).

That's what makes literals special.

This is not unique to Lisp [ tho Lisp experiences this much more because
of the presence of `quote` which makes it possible and common to have arbitrary
literals embedded in the code.  ]

IIRC, in C the standard says that modifying a string literal has
undefined behavior.

> If I expect a function to look at a string argument, but it actually
> modifies its argument, that's equally confusing.

I hate mutability, yes.  But mutability of literals amounts to
self-modifying code, which is yet a bit more evil.

> If I modify data that's been used in a hash key, that's even more
> confusing. If I modify data in an image spec in a Lisp callback from
> the image backend, Emacs will crash.  These cases deserve being
> thought about, too.

We agree, but I'm not sure what it is you're suggesting we should do.
We obviously can't make existing data types unilaterally immutable since
it would break way too much code.  Are you suggesting we add new
constructors for "immutable cons", "immutable string", ...?
Or a `set-immutable` function?

> The cost of this isn't negligible; the single bit which I expect will be
> kept for every string, cons cell, or vector isn't that significant, but
> so far what's been proposed would be complicated to implement, explain,
> and use.

I'm not exactly sure what has been suggested, to be honest.
Are you referring to the idea of making literal strings immutable?
I'm not sure what is the implementation plan for such a thing.
It seems at least not completely straightforward.

> It would lead to some people developing a false sense of security and
> others becoming insecure and copying everything needlessly (and
> dangerously, for cyclic objects). And it would effectively prevent any
> competing system of mutability, I fear.

That makes me think there's been a fairly concrete proposal that has
been made and which I missed (since otherwise, it seems unclear how
you'd get to these conclusions).  Can someone point me to it?


        Stefan

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Paul Eggert]

On 6/6/20 3:14 PM, Stefan Monnier wrote:
> That makes me think there's been a fairly concrete proposal that has
> been made and which I missed (since otherwise, it seems unclear how
> you'd get to these conclusions).  Can someone point me to it?

There's no concrete proposal yet, in terms of published code. That being said,
my idea is to change the Elisp interpreter to distinguish constant from mutable
strings, and to have a runtime check in the few primitives (notably aset) that
modify strings. String literals yield constant strings when evaluated.

I wrote draft code to do this (this was after the long kerfuffle about
mutability in the emacs-27 documentation), and the draft code has worked well so
far. 'make check' passes, it can compile all the .el files and interactive use
works fine for me. There is no runtime storage overhead. The CPU-time overhead
is insignificant (i.e., so close to 0 that I can't measure it) in my usual
benchmark of 'make compile-always' and from what I know about how Emacs and CPUs
work I would expect similar results in other benchmarks (except for programs
that mutate strings that they shouldn't :-).

In the draft code there are no new primitives to create constant strings, test
for mutability, or freeze strings. The only change in behavior visible to Lisp
is that some currently undefined behavior becomes defined behavior. That is,
when a program attempts to change a constant string an error is signaled, as
opposed to the current undefined behavior when sometimes an error is signaled,
sometimes Emacs dumps core, and sometimes Emacs behaves erratically afterwards.

I am still testing the draft code, and am gradually shaking out its changes into
master (you may have noticed some recent changes to alloc.c) that can be
installed independently of the main change. There's no rush and I have other,
more-urgent duties anyway. I am planning to publish the main change for
discussion before installing it.

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Stefan Monnier]

>> That makes me think there's been a fairly concrete proposal that has
>> been made and which I missed (since otherwise, it seems unclear how
>> you'd get to these conclusions).  Can someone point me to it?
> There's no concrete proposal yet, in terms of published code. That being said,
> my idea is to change the Elisp interpreter to distinguish constant from mutable
> strings, and to have a runtime check in the few primitives (notably aset) that
> modify strings.

Right, this loos like the easy part.

> String literals yield constant strings when evaluated.

This seems less obvious.  How do you do that?

[ I terms of semantics, I guess for strings it's fine but I can imagine
  some surprises if we try to apply the same idea to some
  other literals.  ]


        Stefan

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Paul Eggert]

On 6/7/20 8:24 AM, Stefan Monnier wrote:
>> String literals yield constant strings when evaluated.
> This seems less obvious.  How do you do that?

Well, perhaps my terminology wasn't the best.

The basic idea is that when 'read' scans a string literal, it yields a constant
string rather than a mutable one. Similarly for when you load string literal
from an .elc file, or restore a string from a dump where the string came from a
string literal, etc. So evaluating a string literal gives you a constant because
string literals are self-quoting and they are constants already.

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Pip Cet]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> If I expect a function to look at a string argument, but it actually
>> modifies its argument, that's equally confusing.
>
> I hate mutability, yes.  But mutability of literals amounts to
> self-modifying code, which is yet a bit more evil.

And, just once in a while, so very useful :-)

You're right, though, mutability of literals is a more serious concern
than mutability in general.

>> If I modify data that's been used in a hash key, that's even more
>> confusing. If I modify data in an image spec in a Lisp callback from
>> the image backend, Emacs will crash.  These cases deserve being
>> thought about, too.
>
> We agree, but I'm not sure what it is you're suggesting we should do.

Mostly: don't make it harder to experiment with mutability by pretending
we already have anything like it.

> We obviously can't make existing data types unilaterally immutable since
> it would break way too much code.  Are you suggesting we add new
> constructors for "immutable cons", "immutable string", ...?
> Or a `set-immutable` function?

Not at this point, no. I can describe the code I'm playing with, but
it's quite different and I'm not sure it's worth the considerable
performance cost...

>> The cost of this isn't negligible; the single bit which I expect will be
>> kept for every string, cons cell, or vector isn't that significant, but
>> so far what's been proposed would be complicated to implement, explain,
>> and use.
>
> I'm not exactly sure what has been suggested, to be honest.
> Are you referring to the idea of making literal strings immutable?
> I'm not sure what is the implementation plan for such a thing.
> It seems at least not completely straightforward.

I think the issue has been resolved: if I understand correctly, Paul is
probably going to propose an actual patch which makes strings immutable,
and we can discuss it then.

>> It would lead to some people developing a false sense of security and
>> others becoming insecure and copying everything needlessly (and
>> dangerously, for cyclic objects). And it would effectively prevent any
>> competing system of mutability, I fear.
>
> That makes me think there's been a fairly concrete proposal that has
> been made and which I missed (since otherwise, it seems unclear how
> you'd get to these conclusions).  Can someone point me to it?

You're right, we should wait for such a proposal.

RE: master f51f963: Fix some side-effecting uses of make-text-button

[Drew Adams]

> > But going backwards, toward some perhaps unneeded
> > optimization, in the direction of systematically
> > raising an error when trying to modify text
> > properties of a string, is not a good idea, IMO.
> 
> I think there needs to be a clarification here: the issue is about
> modifying data (here specifically strings, but the issue applies to all
> other such data) that appears as literal in the code.

Yes, I know.

> This issue is not one of optimization (preventing those modifications
> would likely impose a slowdown, if anything) but one of detecting usage
> that is usually a bug (one that leads to people being utterly
> confused by the resulting behavior).

The resulting behavior now is undefined/unpredictable
in the problematic cases.  That's the problem.

List-structure modification too can result in
application bugs and confusion.  That's not a
reason, in Lisp, to prevent such modification.
Lisp, including Elisp, gives you lots of rope
to hang yourself with.

A "literal" string occurrence in code is what
the implementation of Emacs defines it to be.

Strings can have text properties.  Those are not
visible as program text (code, viewed lexically).
Maybe no strings in Elisp should really be
considered "literal" in the usual sense.

Unless some sophisticated analysis takes place,
to determine that no modification of a given
string's properties will/can/might take place,
there are two "extreme" positions possible:

1. Disallow modification of a string's text props.
2. Allow modification of a string's text props.

Something in between is also possible.  We have
something in between now, but it's unclear or
undefined or accidental.

As I understand it, Paul proposes extreme #1.
I'm closer to extreme #2.

But I say that any backtracking from #2, and
especially any backtracking from what Emacs has
had so far (ad hoc, accidental, partial, or what
have you) - any considering a string ("literal"
or not) as needing an error to be raised if an
attempt is made to change its text properties
(including adding some when there are none) is
a loss.

And any such loss needs to have a good supporting
argument - as strong an argument as for raising
an error for list-structure modification where
today there's no such error-raising.

Elisp programmers need to be able to do both:
(1) program without structure modification (list,
string, whatever), and (2) program using such
modification.

And the "literal" case really shouldn't, I think,
be a special one.  I think that now, even for a
quoted list, but I won't argue that case.  (A
quoted list is clear just from the program text.
A propertized string is not clear textually, in
the general case.)

I don't expect others to hold the same "extreme"
position.  But that's the direction I'm thinking
in now, FWIW.  And so far I haven't seen any good
arguments in the other direction.

We heard arguments about important "optimization".
You've tossed that aside now, at least for literal
strings, saying there's no such optimization and
in fact there may be a performance cost.

Your argument is instead the value in "detecting
usage that is usually a bug (one that leads to
people being utterly confused by the resulting
behavior)".

That needs to be shown.  And not just by pointing
to the current all-bets-are-off situation in terms
of understanding.  Please make the argument in terms
of a situation where every string is considered to
have modifiable text properties (extreme #2).

When I see a good argument against #2, maybe I'll
pull back from it a bit. ;-)

Certainly, the general argument against this kind
of thing _is_ in terms of (1) simplicity of program
analysis and (2) performance.  It's the argument of
compilation vs interpretation, more or less.

I'd argue that Emacs Lisp is, among all the Lisps,
the one where use by end users of the result is the
most important, and interpretation (vs compilation)
is relatively more important than for other Lisps,
and performance and static (lexical) analysis is
relatively less important.

(And that's from someone who wishes that Elisp would
take more from Common Lisp, which is far from #2.)

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Paul Eggert]

On 6/6/20 1:18 AM, Pip Cet wrote:

>> there have always been Emacs Lisp objects that are not mutable.
> 
> Lately, only pure ones, as far as I can tell?

There are others. I haven't cataloged them but they include builtin symbol
names, empty strings, and constant variables (no, I didn't come up with that
last term :-). Also numbers of course. Constants have always been ubiquitous in
Emacs.

>> If we decide to simplify/document by saying "all strings are modifiable" then
>> we'll need significant work at both the C and Lisp level to do that.
> 
> I don't see why. All strings are modifiable, but the byte compiler will
> identify strings under certain circumstances.
That's not how Emacs works now, and it's not how Common Lisp or Scheme works. If
we insisted on making the change you're proposing, it would throw yet another
obstacle into the path of porting Emacs to other platforms such as Guile. That
would not be a good road to take.

It might be worth making such a significant change if modifiable string literals
were an important feature that Elisp programmers urgently needed. But they're
not: they're rarely used, partly because when they have been used their use
often caused subtle bugs (as we've seen with make-text-button). They're not a
feature worth fighting for, any more than mutable numbers would be.

>> This will
>> hurt performance a bit since it will disable some optimizations.
> 
> Which ones?

The ones Emacs is currently using, such as some strings are in read-only shared
memory, and some are coalesced. It would be unreasonable to coalesce strings if
they were mutable, since that would mean changing one would change the other.

> So far, what you have proposed is "an error is thrown if you try to
> modify the characters of a string literal, or if you add text
> properties unless it already has some, or if you remove the last text
> property".

There must be some confusion here, as I haven't proposed that. What I'm thinking
of proposing (though I haven't written it up yet, and this is just an
off-the-cuff first cut) is that Emacs signal an error if a program attempts to
change a string constant's characters or text properties. That's a simple
notion, and it's something that Emacs long did for preloaded strings so it's not
like this would be a giant revolution. To my mind it's a considerably
more-conservative change than the one you're suggesting.

> (In general, I think that's probably not a good benchmark to optimize
> Emacs for).

Admittedly it's crude but it is better than nothing and it is what we have
readily available. If you have another easy-to-use benchmark that would be
better, I'm all ears.

> I'm not sure "undefined behavior" is a useful term when speaking about
> Emacs Lisp, except for behavior which is explicitly documented to be
> unreliable. There's a single implementation, and a lot of code is
> written to conform not to what's documented but to what happens to
> work.

Of course, and there's a natural tension between trying to document every
unimportant implementation detail (which would be a mistake) and not documenting
useful behavior (which would also be a mistake). But that's not the issue here,
as the behavior in question is explicitly documented to be unreliable and we're
discussing what (if anything) to do about it.

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Pip Cet]

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

> On 6/6/20 1:18 AM, Pip Cet wrote:
>
>>> there have always been Emacs Lisp objects that are not mutable.
>> 
>> Lately, only pure ones, as far as I can tell?
>
> There are others. I haven't cataloged them but they include builtin symbol
> names

pure

> empty strings,

pure, and as modifiable as all other strings

> and constant variables (no, I didn't come up with that
> last term :-). Also numbers of course.

I don't think setting a symbol's values is comparable to mutating a
vector or string.  Neither are numbers.

> Constants have always been
> ubiquitous in
> Emacs.

I disagree, to me immutable objects appear to be the strange exception.

>>> If we decide to simplify/document by saying "all strings are
>>> modifiable" then
>>> we'll need significant work at both the C and Lisp level to do that.
>> 
>> I don't see why. All strings are modifiable, but the byte compiler will
>> identify strings under certain circumstances.
> That's not how Emacs works now,

Very close, though.

> and it's not how Common Lisp or Scheme works.

Neither is anything that has been proposed by you so far.

> If
> we insisted on making the change you're proposing, it would throw yet another
> obstacle into the path of porting Emacs to other platforms such as Guile. That
> would not be a good road to take.

"Yet another" is the important term here, I think. There are more
significant issues to sort out.

> It might be worth making such a significant change if modifiable
> string literals
> were an important feature that Elisp programmers urgently needed. But they're
> not: they're rarely used, partly because when they have been used their use
> often caused subtle bugs (as we've seen with make-text-button). They're not a
> feature worth fighting for, any more than mutable numbers would be.

I agree, actually. What I'm fighting against is a certain model of
immutability being installed into the Emacs source tree and effectively
preventing better ones from ever having a chance, as well as turning out
to be, as the vast majority of such models have, a problem rather than a
useful feature.

>
>>> This will
>>> hurt performance a bit since it will disable some optimizations.
>> 
>> Which ones?
>
> The ones Emacs is currently using, such as some strings are in
> read-only shared
> memory,

I don't think that's even an optimization. As I said, we're not
aggressively reducing the size of the Emacs binary, quite the opposite,
and read-only strings copied into pure space probably wouldn't be paged
in.

> and some are coalesced.

Just to be clear: nothing in Emacs "coalesces" two strings by making
them equal if they weren't before. The byte compiler generates new
strings, and might generate fewer than were put in, but a string always
has its own identity and keeps it.

> It would be unreasonable to coalesce
> strings if
> they were mutable, since that would mean changing one would change the
> other.

Strings are mutable, and we are "coalescing" them, if only in the weak
sense that the byte compiler does.

>
>> So far, what you have proposed is "an error is thrown if you try to
>> modify the characters of a string literal, or if you add text
>> properties unless it already has some, or if you remove the last text
>> property".
>
> There must be some confusion here, as I haven't proposed that.

I'm sorry for misunderstanding, then.

> What
> I'm thinking
> of proposing (though I haven't written it up yet, and this is just an
> off-the-cuff first cut) is that Emacs signal an error if a program attempts to
> change a string constant's characters or text properties.

Okay. I'm sorry I assumed you were just going to go ahead and commit
something without any prior discussion, and it would be unfair not to
wait for that proposal before criticizing it.

>> (In general, I think that's probably not a good benchmark to optimize
>> Emacs for).
>
> Admittedly it's crude but it is better than nothing and it is what we have
> readily available. If you have another easy-to-use benchmark that would be
> better, I'm all ears.

Let me second that, I would love to have a better benchmark.

>> I'm not sure "undefined behavior" is a useful term when speaking about
>> Emacs Lisp, except for behavior which is explicitly documented to be
>> unreliable. There's a single implementation, and a lot of code is
>> written to conform not to what's documented but to what happens to
>> work.
>
> Of course, and there's a natural tension between trying to document every
> unimportant implementation detail (which would be a mistake) and not
> documenting
> useful behavior (which would also be a mistake). But that's not the
> issue here,
> as the behavior in question is explicitly documented to be unreliable
> and we're
> discussing what (if anything) to do about it.

I'm not sure whether I've lost the right to comment on changes made all
of six weeks ago, because I must admit I hadn't been aware of those
documentation changes. I think you've essentially documented the changes
you're considering to propose as though they had already happened.

I think there are solutions here we'd both be happy with: we can easily
use the C preprocessor to generate the amount of mutability checking we
want, ignoring all or some of the information passed to the macros.

But if we want that C API to be flexible enough to allow unusual
applications (and isn't that what Emacs is all about?), it needs
something more than just the obvious CHECK_MUTABLE (obj) macro.

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Paul Eggert]

On 6/6/20 12:41 PM, Pip Cet wrote:

> What I'm fighting against is a certain model of
> immutability being installed into the Emacs source tree and effectively
> preventing better ones from ever having a chance, as well as turning out
> to be, as the vast majority of such models have, a problem rather than a
> useful feature.

I'm quite conscious of those dangers. What I had in mind was something far more
limited: just supporting runtime checking of attempts to modify strings that
either have undefined behavior if you mutate them now, or are close enough to
that category so that nobody will care about the difference (except to be happy
when Emacs catches unlikely glitches in their programs). In my drafts so far
Emacs requires no more storage than it does now, and the CPU performance does
not change significantly, so overall this change should be a win in practical use.

It'd be a more-drastic change to add mutability/immutability as a more-general
concept to Emacs, to add new functions that freeze or check the mutability
status of arbitrary objects, etc., etc.. I'm not ready to propose that now and I
don't know if it'd be a good idea. My goal right now is to prevent Emacs crashes
and and general bugginess, not to add general mutability features.

> I think you've essentially documented the changes
> you're considering to propose as though they had already happened.

No, I first discussed and wrote those changes in response to a bug report, and
only later looked into conservative ways of fixing some of the real problems
uncovered by that documentation effort.

> if we want that C API to be flexible enough to allow unusual
> applications (and isn't that what Emacs is all about?), it needs
> something more than just the obvious CHECK_MUTABLE (obj) macro.

It sounds like you're thinking ahead to the non-string case. I'm limiting myself
just to strings for now, as they're the most salient part of the problem (core
dumps and all). It should be OK to do that, and put off the more-general issues
until later (if we ever do that at all).

The obvious check_string_mutable function doesn't need to be used very often:
only in the places where CHECK_IMPURE is used on strings now. The only other
primitive I've found the need for at the C level is freeze_string (to mark an
already-constructed string as being a constant).

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Pip Cet]

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

> On 6/6/20 12:41 PM, Pip Cet wrote:
>
>> What I'm fighting against is a certain model of
>> immutability being installed into the Emacs source tree and effectively
>> preventing better ones from ever having a chance, as well as turning out
>> to be, as the vast majority of such models have, a problem rather than a
>> useful feature.
>
> I'm quite conscious of those dangers. What I had in mind was something
> far more
> limited: just supporting runtime checking of attempts to modify strings that
> either have undefined behavior if you mutate them now, or are close enough to
> that category so that nobody will care about the difference (except to
> be happy
> when Emacs catches unlikely glitches in their programs).

Okay, I'm looking forward to that proposal, and sorry for criticizing it
before I'd understood it clearly.

>> if we want that C API to be flexible enough to allow unusual
>> applications (and isn't that what Emacs is all about?), it needs
>> something more than just the obvious CHECK_MUTABLE (obj) macro.
>
> It sounds like you're thinking ahead to the non-string case.

I am.

> I'm
> limiting myself
> just to strings for now, as they're the most salient part of the problem (core
> dumps and all).

The core dumps definitely need to be fixed.

I still don't understand precisely how far you're planning to go in
protecting an immutable string's text properties, but you've convinced
me that it's a win in practice even if we just protect the characters.

> It should be OK to do that, and put off the
> more-general issues
> until later (if we ever do that at all).

So this would be only strings, not cons cells? That makes sense to me.

> The obvious check_string_mutable function doesn't need to be used very often:
> only in the places where CHECK_IMPURE is used on strings now. The only other
> primitive I've found the need for at the C level is freeze_string (to mark an
> already-constructed string as being a constant).

Just to be clear: there's no way to unfreeze a string, right? Because
that would add considerable complexity and not be worth it, IMHO.

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Paul Eggert]

On 6/7/20 2:21 AM, Pip Cet wrote:

> Just to be clear: there's no way to unfreeze a string, right?

Right.

RE: master f51f963: Fix some side-effecting uses of make-text-button

[Drew Adams]

> It might be worth making such a significant change if
> modifiable string literals were an important feature

They are, IMHO.  A wonderful feature.  Other Lisps
should be so lucky.

> that Elisp programmers urgently needed.

Urgently?  As in an emergency?  No.  That's a pretty
high bar.  Do your proposed wholesale changes in the
other direction handle an emergency?  Urgent?

> But they're not: they're rarely used,

Evidence?  But let's assume your guess is right.
Is frequency of use really an important criterion
here?

List modification in Lisp is infrequent, but it's
very important to Lisp - always has been, outside
the use of "pure Lisp" for some research purposes.

> partly because when they have been used their use
> often caused subtle bugs (as we've seen with
> make-text-button).

That's because there are bugs in the implementation.
And because there's not corresponding doc everywhere.

The same thing is true of list-structure modification.
(I hope your next crusade won't be to prevent that!)

> They're not a feature worth fighting for,

Your opposite "feature" isn't, IMO.

> any more than mutable numbers would be.

Wrong.  An Elisp string is an object, with properties.
When Elisp numbers get text properties your comparison
might make some sense.

> >> This will hurt performance a bit since it will
> >> disable some optimizations.
> >
> > Which ones?
> 
> The ones Emacs is currently using, such as some strings are in read-only
> shared memory, and some are coalesced. It would be unreasonable to coalesce strings
> if they were mutable, since that would mean changing one would change the other.

What's the cost in lost optimization?  Any plan to
fiddle with optimization should weigh the gain and
loss.

To you, there's apparently no loss, because you see
no value in modifying string properties (or at least
that's not important enough to keep - to "fight for").

> What I'm thinking of proposing ... is that Emacs signal
> an error if a program attempts to change a string constant's
> characters or text properties.

Very not-good.  What's next, impossibility to
modify list structure?

> That's a simple notion,

It sure is.  Too simple.  And unlispy.

> and it's something that Emacs long did for preloaded
> strings so it's not like this would be a giant revolution.

That some instances of a class of objects can be
immutable is very different from denying all such
objects the ability to change.  Yes, that's a
giant, and ill-conceived, revolution.

> To my mind it's a considerably more-conservative
> change than the one you're suggesting.

"Conservative" or ultraconservative?  It goes
backward IMO, tossing out an important feature of
Emacs Lisp.

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Stefan Monnier]

>> It might be worth making such a significant change if
>> modifiable string literals were an important feature
> They are, IMHO.  A wonderful feature.

I find it very hard to believe.
Either you're misunderstanding what we're talking about, or you do have
some really off programming habits.

Could you show some examples of code that rely of that "wonderful
feature"?  Extra points if such code is available in an existing
Elisp package.


        Stefan

RE: master f51f963: Fix some side-effecting uses of make-text-button

[Drew Adams]

> >> It might be worth making such a significant change if
> >> modifiable string literals were an important feature
> > They are, IMHO.  A wonderful feature.
> 
> I find it very hard to believe.

Me too.  But that's my current vision, and I'm
stickin to it, until I see some good arguments
to the contrary.

Show me why we need (ever) to treat Elisp strings
as immutable, at least wrt their text properties.

> Either you're misunderstanding what we're talking about, or you do have
> some really off programming habits.
> 
> Could you show some examples of code that rely of that "wonderful
> feature"?  Extra points if such code is available in an existing
> Elisp package.

I don't have an example.  I don't have any such
habit.  And we don't yet have consistently, clean
mutable literal strings.  Show me that we couldn't.

Elisp strings can have properties.  You don't see
the properties when looking at code.  Neither does
the Lisp reader etc.

I don't see why we would think of strings appearing
in Elisp code, i.e., literals, the way we think of
them in C, as fixed things, constant.  It's natural
that a programmer might think that way.  But I don't
see why it's important to the Elisp language that we
treat them that way.  Please explain why it is.

What's lost by treating them the same way we treat
a value returned by `make-string' or `copy-sequence'?

The `make-text-button' example is maybe a good example
of why we shouldn't need to bother to constantize them.
Dunno; I didn't really follow that discussion.

TBH, I haven't thought about this before this discussion.
But I'm wondering now why we ever try to treat Elisp
strings as constants - wrt their properties, at least -
rather than as mutable objects.

Sure, it's unusual to think this way.  But it's pretty
unusual for a language to have propertized strings in
the first place.

The case of strings is different from both conses and
symbols (with their properties), admittedly.  And?

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Stefan Monnier]

> As Paul said, not quite - it has historically modified its first
> argument by placing properties on it.  If you look at the line preceding
> this diff hunk you'll see a call to copy-sequence.
>
> My reasons for making this particular change are:
> 0. Since Emacs 24.4, make-text-button has returned its modified first
>    argument, so callers can use the value returned by make-text-button
>    instead of calling it only for its side effects.
> 1. This has now been reverted again, but for the last month,
>    make-text-button returned a modified _copy_ of its argument, which
>    meant that its side effects could no longer be relied upon.
>
> Either way, relying on its return value rather than its side effects
> seems like the best style to stick with for now.

So, IIUC `make-text-button` should ideally work functionally, but for
historical reasons it works by side-effect.  What's the long term plan?
Do we plan to live with the current side-effecting behavior, or do we
plan to move to the "pure" functional behavior?

If we could detect when a string-button is "used" (i.e. displayed or
inserted into a buffer), then we could detect the use of the old
side-effecting style (by checking if the string passed as argument had
already been displayed/inserted elsewhere) and emit a good warning.

>>> @@ -202,7 +202,7 @@ The format has been repaired and the variable modified accordingly.
>>>  You can save the current value through the customize system by
>>>  either clicking or hitting return "
>>>            (make-text-button
>>> -           "here" nil
>>> +           (copy-sequence "here") nil
>>>             'face '(:weight bold :inherit button)
>>>             'mouse-face '(:weight normal :background "gray50" :inherit button)
>>>             'follow-link t
>> So, here why do we need to `copy-sequence`?
> To avoid destructively modifying a string literal by placing properties
> on it.

Makes sense,


        Stefan

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Basil L. Contovounesios]

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

>> As Paul said, not quite - it has historically modified its first
>> argument by placing properties on it.  If you look at the line preceding
>> this diff hunk you'll see a call to copy-sequence.
>>
>> My reasons for making this particular change are:
>> 0. Since Emacs 24.4, make-text-button has returned its modified first
>>    argument, so callers can use the value returned by make-text-button
>>    instead of calling it only for its side effects.
>> 1. This has now been reverted again, but for the last month,
>>    make-text-button returned a modified _copy_ of its argument, which
>>    meant that its side effects could no longer be relied upon.
>>
>> Either way, relying on its return value rather than its side effects
>> seems like the best style to stick with for now.
>
> So, IIUC `make-text-button` should ideally work functionally, but for
> historical reasons it works by side-effect.  What's the long term plan?
> Do we plan to live with the current side-effecting behavior, or do we
> plan to move to the "pure" functional behavior?
>
> If we could detect when a string-button is "used" (i.e. displayed or
> inserted into a buffer), then we could detect the use of the old
> side-effecting style (by checking if the string passed as argument had
> already been displayed/inserted elsewhere) and emit a good warning.

AFAIK all relevant future plans are being discussed in this thread:
https://lists.gnu.org/archive/html/emacs-devel/2020-06/msg00117.html

For almost a month make-text-button was pure wrt strings, but that's now
been reverted again for backward compatibility and until a new decision
is reached.

-- 
Basil

Re: master f51f963: Fix some side-effecting uses of make-text-button

[Paul Eggert]

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

On 6/5/20 6:02 AM, Stefan Monnier wrote:
> So, IIUC `make-text-button` should ideally work functionally, but for
> historical reasons it works by side-effect.  What's the long term plan?
> Do we plan to live with the current side-effecting behavior, or do we
> plan to move to the "pure" functional behavior?

Given the comments in this thread it seems that there's consensus that it should
move to the "pure" functional behavior, as the side-effecting behavior is
confusing (and this is independent of whether string literals are constant). I
installed the attached patch, which is along the lines that I proposed a couple
of days ago; it has the doc fix that Pip Cet suggested.

[-- Attachment #2: 0001-make-text-button-no-longer-modifies-its-string-arg.patch --]
[-- Type: text/x-patch, Size: 3787 bytes --]

From 481e57cd310177452fe5d4e733370337eb35ba96 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 6 Jun 2020 12:05:10 -0700
Subject: [PATCH] make-text-button no longer modifies its string arg
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* etc/NEWS: Mention this.
* lisp/apropos.el (apropos-library-button):
* lisp/ibuf-ext.el (ibuffer-old-saved-filters-warning):
There’s no longer a need copy make-text-button’s string arg.
* lisp/button.el (make-text-button): Return a copy of a string arg.
Delay making the copy until after error-checking.
---
 etc/NEWS         | 5 +++++
 lisp/apropos.el  | 2 +-
 lisp/button.el   | 9 ++++++---
 lisp/ibuf-ext.el | 2 +-
 4 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 27e511047e..edad5b37d6 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -476,6 +476,11 @@ are 'eq'.  To compare contents, use 'compare-window-configurations'
 instead.  This change helps fix a bug in 'sxhash-equal', which returned
 incorrect hashes for window configurations and some other objects.
 
+** When its first argument is a string, 'make-text-button' no longer
+modifies the string's text properties; instead, it uses and returns
+a copy of the string.  This helps avoid trouble when strings are
+shared or constants.
+
 ---
 ** The obsolete function 'thread-alive-p' has been removed.
 
diff --git a/lisp/apropos.el b/lisp/apropos.el
index 22866cd2cc..2566d44dfc 100644
--- a/lisp/apropos.el
+++ b/lisp/apropos.el
@@ -661,7 +661,7 @@ apropos
 (defun apropos-library-button (sym)
   (if (null sym)
       "<nothing>"
-    (let ((name (copy-sequence (symbol-name sym))))
+    (let ((name (symbol-name sym)))
       (make-text-button name nil
                         'type 'apropos-library
                         'face 'apropos-symbol
diff --git a/lisp/button.el b/lisp/button.el
index 3a6a6de774..d9c36a0375 100644
--- a/lisp/button.el
+++ b/lisp/button.el
@@ -341,15 +341,14 @@ make-text-button
 as the argument for the `action' callback function instead of the
 default argument, which is the button itself.
 
-BEG can also be a string, in which case it is made into a button.
+BEG can also be a string, in which case a copy of it is made into
+a button and returned.
 
 Also see `insert-text-button'."
   (let ((object nil)
         (type-entry
 	 (or (plist-member properties 'type)
 	     (plist-member properties :type))))
-    (when (stringp beg)
-      (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"))
@@ -362,6 +361,10 @@ make-text-button
       (setcar type-entry 'category)
       (setcar (cdr type-entry)
               (button-category-symbol (cadr type-entry))))
+    (when (stringp beg)
+      (setq object (copy-sequence beg))
+      (setq beg 0)
+      (setq end (length object)))
     ;; Now add all the text properties at once.
     (add-text-properties beg end
                          ;; Each button should have a non-eq `button'
diff --git a/lisp/ibuf-ext.el b/lisp/ibuf-ext.el
index c39000b488..bfb9787a96 100644
--- a/lisp/ibuf-ext.el
+++ b/lisp/ibuf-ext.el
@@ -202,7 +202,7 @@ ibuffer-old-saved-filters-warning
 You can save the current value through the customize system by
 either clicking or hitting return "
           (make-text-button
-           (copy-sequence "here") nil
+           "here" nil
            'face '(:weight bold :inherit button)
            'mouse-face '(:weight normal :background "gray50" :inherit button)
            'follow-link t
-- 
2.17.1

RE: master f51f963: Fix some side-effecting uses of make-text-button

[Drew Adams]

> > So, IIUC `make-text-button` should ideally work functionally, but for
> > historical reasons it works by side-effect.  What's the long term plan?
> > Do we plan to live with the current side-effecting behavior, or do we
> > plan to move to the "pure" functional behavior?
> 
> Given the comments in this thread it seems that there's consensus that it
> should move to the "pure" functional behavior, as the side-effecting behavior is
> confusing (and this is independent of whether string literals are constant).

Please don't count me among the consenting, in
your consensus count.

> I installed the attached patch, which is along the lines that I proposed a
> couple of days ago; it has the doc fix that Pip Cet suggested.

What was the reason not to go the other direction,
and _always allow_ modification of that string arg?