Subject: Re: bug#75845: mapconcat crashes for unreasonable self-modifications

From: Pip Cet <[email-masked]>
Date: Sun, 26 Jan 2025 12:07:50 +0000

"Stefan Monnier" <[email-masked]> writes:

>> Thanks, adding Stefan and Mattias to the discussion, in the hope that
>> they could review the patch and comment.
>
> The patch LGTM.

Thanks.  I'll wait for a decision to be made on which precise error to
throw in this extremely rare situation.

>
>>>  	  if (! CONSP (tail))
>>> -	    return i;
>>> +	    error ("list modified by mapping function");
>>
>> I think this error message is not explicit enough: it doesn't tell

FWIW, I started out by copying

    signal_error ("hash table test modifies table", obj);

since that's the only similar case I could think of right away, and I
thought this might not require additional thought.  Upon reflection, my
current suggestion is to define an error symbol vaguely equivalent to
Java's ConcurrentModificationException, but with a less puzzling name
(also, hopefully, this would happen much less often than that common
exception).

Having a single error symbol and documenting it once, for all cases,
with general advice for what might have gone wrong and how to fix it
(rewrite callbacks to be as pure as possible) doesn't seem harder than
arguing about this specific corner case, but does provide more benefits.

>>   string arg of mapcar or mapconcat clobbered by its mapping function arg

TBH, "string arg of ... mapconcat" sounds like it's referring to the
separator string, to me.

> +1 for "clobbered"

+1 from me, too.  "Concurrent modification exceptions" are puzzling to
programmers partly because the name doesn't make it clear who was at
fault.  "Clobbered" clearly indicates that the modifying code was
careless and is the part that needs to be fixed, and that we were lucky
to detect this was attempted or happened.

PUT_ERROR (Qdata_clobbered_error, "Lisp callback unexpectedly clobbered data");

on the assumption that xsignal2 is the right thing to use here, and that
the reference to the clobbering function would be the first and the data
clobbered would be the second argument.

No objections to reversing the string and arguments if that's clearer,
or any rewording that makes sense to you.

(There's a tiny white lie here: I don't think it's worth it to
distinguish cases in which the clobberer was successful from those in
which an illicit modification was prevented.  Even if it's a single
error for all cases, the combined probability isn't great enough to
warrant two error symbols).

As a tangent, I'd prefer providing format arguments in PUT_ERROR, on the
understanding that expanding them is optional: omitting the format
arguments would yield the current error message, and including them
would provide additional output in situations where the user desires
this.

Pip