[PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Naoya Yamashita]

[-- Attachment #1: Type: Text/Plain, Size: 802 bytes --]

Hi!

I found src/eval.c (let) has redundant conditions, that compares
the length of the list with the current index and also checks if
the current list is cons.

I remove latter check, and it compiled fine and passed all the
tests I added.

However, in such cases, Lisper prefers to compare that the
current list is cons, rather than comparing indices.  Taking a
`cdr` and checking that it is cons is a Lisp idiom in situations
where `mapc` and `dolist` are not available.

The concern is it may be faster to check the index than CONSP.
This would be fast enough because CONSP in C is a macro, which is
eventually converted to a bitwise operation.  The code is
considered to be easier to read and less prone to bugs than the
current code that includes variable reuse and reassignment.

Regards,
Naoya.

[-- Attachment #2: 0001-src-eval.c-Stop-checking-for-nvars-and-use-only-CONS.patch --]
[-- Type: Text/X-Patch, Size: 2481 bytes --]

From 622c96bdb41bda2727242f8a8078bb8114ef667f Mon Sep 17 00:00:00 2001
From: Naoya Yamashita <conao3@gmail.com>
Date: Tue, 2 Mar 2021 10:17:29 +0900
Subject: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

* src/eval.c (let): Remove checking nvars (length of arglist),
and use only CONSP check.
---
 src/eval.c             |  6 ++----
 test/src/eval-tests.el | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/src/eval.c b/src/eval.c
index 542d7f686e..30783f204a 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1001,11 +1001,10 @@ DEFUN ("let", Flet, Slet, 1, UNEVALLED, 0,
   /* Make space to hold the values to give the bound variables.  */
   EMACS_INT varlist_len = list_length (varlist);
   SAFE_ALLOCA_LISP (temps, varlist_len);
-  ptrdiff_t nvars = varlist_len;
 
   /* Compute the values and store them in `temps'.  */
 
-  for (argnum = 0; argnum < nvars && CONSP (varlist); argnum++)
+  for (argnum = 0; CONSP (varlist); argnum++)
     {
       maybe_quit ();
       elt = XCAR (varlist);
@@ -1017,12 +1016,11 @@ DEFUN ("let", Flet, Slet, 1, UNEVALLED, 0,
       else
 	temps[argnum] = eval_sub (Fcar (Fcdr (elt)));
     }
-  nvars = argnum;
 
   lexenv = Vinternal_interpreter_environment;
 
   varlist = XCAR (args);
-  for (argnum = 0; argnum < nvars && CONSP (varlist); argnum++)
+  for (argnum = 0; CONSP (varlist); argnum++)
     {
       Lisp_Object var;
 
diff --git a/test/src/eval-tests.el b/test/src/eval-tests.el
index b2b7dfefda..3576254d7d 100644
--- a/test/src/eval-tests.el
+++ b/test/src/eval-tests.el
@@ -226,4 +226,31 @@ eval-tests/backtrace-in-batch-mode/demoted-errors
       (should (equal (string-trim (buffer-string))
                      "Error: (error \"Boo\")")))))
 
+(ert-deftest eval-tests/let ()
+  (should (equal (let (a)
+                   a)
+                 nil))
+
+  (should (equal (let (a b)
+                   (list a b))
+                 '(nil nil)))
+
+  (should (equal (let ((a 1))
+                   a)
+                 1))
+
+  (should (equal (let ((a 1) b)
+                   (list a b))
+                 '(1 nil)))
+
+  ;; (error "`let' bindings can have only one value-form" a 1 2)
+  (should-error (let ((a 1 2))
+                  a)
+                :type 'error)
+
+  ;; (wrong-type-argument symbolp (a))
+  (should-error (let (((a) 1))
+                  a)
+                :type 'wrong-type-argument))
+
 ;;; eval-tests.el ends here
-- 
2.30.1

Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Stefan Monnier]

> I found src/eval.c (let) has redundant conditions, that compares
> the length of the list with the current index and also checks if
> the current list is cons.

The patch looks fine, thank you.

Just one detail about the tests: the change you made affects the
evaluation of `let` in the case where the code is interpreted but it
is not used when the code is byte-compiled.  So you might like your
tests to use things like

    (eval '(let ...) t)

to avoid the compiler getting in the way.

> +  ;; (error "`let' bindings can have only one value-form" a 1 2)
> +  (should-error (let ((a 1 2))
> +                  a)
> +                :type 'error)

Also, while this `let` is indeed invalid code, I don't think we
guarantee that it will signal an error, and especially not at runtime
(it's more likely to signal an error at macroexpansion or compile
time).

I think the compiler (or `macroexpand-all`) should make an effort to
detect and diagnose those problems, but I don't think it's important to
catch those problems in the interpreter.


        Stefan

Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Naoya Yamashita]

[-- Attachment #1: Type: Text/Plain, Size: 1269 bytes --]

> The patch looks fine, thank you.

Thanks!


> Just one detail about the tests: the change you made affects the
> evaluation of `let` in the case where the code is interpreted but it
> is not used when the code is byte-compiled.  So you might like your
> tests to use things like
> 
>     (eval '(let ...) t)
> 
> to avoid the compiler getting in the way.

Thanks, I didn't notice this point!  I fix testcases.


> Also, while this `let` is indeed invalid code, I don't think we
> guarantee that it will signal an error, and especially not at runtime
> (it's more likely to signal an error at macroexpansion or compile
> time).
> 
> I think the compiler (or `macroexpand-all`) should make an effort to
> detect and diagnose those problems, but I don't think it's important to
> catch those problems in the interpreter.

That testcase comes from this code (src/eval.c:L1014) which we already had.

      else if (! NILP (Fcdr (Fcdr (elt))))
	signal_error ("`let' bindings can have only one value-form", elt);

I tried to remove this, my temp Emacs works like this in *scratch* buffer.

    (let ((a 1 2))
       a)                       ; Type C-j
    1

This is very strange I think.  I still think it's important for
Emacs, even as an interpreter, to produce errors.

[-- Attachment #2: 0001-src-eval.c-Stop-checking-for-nvars-and-use-only-CONS.patch --]
[-- Type: Text/X-Patch, Size: 2731 bytes --]

From 8e65fea2d95bf7118ecd2b816f3b49292353a431 Mon Sep 17 00:00:00 2001
From: Naoya Yamashita <conao3@gmail.com>
Date: Tue, 2 Mar 2021 10:17:29 +0900
Subject: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

* src/eval.c (let): Remove checking nvars (length of arglist),
and use only CONSP check.
---
 src/eval.c             |  6 ++----
 test/src/eval-tests.el | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/src/eval.c b/src/eval.c
index 542d7f686e..30783f204a 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1001,11 +1001,10 @@ DEFUN ("let", Flet, Slet, 1, UNEVALLED, 0,
   /* Make space to hold the values to give the bound variables.  */
   EMACS_INT varlist_len = list_length (varlist);
   SAFE_ALLOCA_LISP (temps, varlist_len);
-  ptrdiff_t nvars = varlist_len;
 
   /* Compute the values and store them in `temps'.  */
 
-  for (argnum = 0; argnum < nvars && CONSP (varlist); argnum++)
+  for (argnum = 0; CONSP (varlist); argnum++)
     {
       maybe_quit ();
       elt = XCAR (varlist);
@@ -1017,12 +1016,11 @@ DEFUN ("let", Flet, Slet, 1, UNEVALLED, 0,
       else
 	temps[argnum] = eval_sub (Fcar (Fcdr (elt)));
     }
-  nvars = argnum;
 
   lexenv = Vinternal_interpreter_environment;
 
   varlist = XCAR (args);
-  for (argnum = 0; argnum < nvars && CONSP (varlist); argnum++)
+  for (argnum = 0; CONSP (varlist); argnum++)
     {
       Lisp_Object var;
 
diff --git a/test/src/eval-tests.el b/test/src/eval-tests.el
index b2b7dfefda..85dc2a53ae 100644
--- a/test/src/eval-tests.el
+++ b/test/src/eval-tests.el
@@ -226,4 +226,37 @@ eval-tests/backtrace-in-batch-mode/demoted-errors
       (should (equal (string-trim (buffer-string))
                      "Error: (error \"Boo\")")))))
 
+(ert-deftest eval-tests/let ()
+  (should (equal (eval '(let (a)
+                          a)
+                       t)
+                 nil))
+
+  (should (equal (eval '(let (a b)
+                          (list a b))
+                       t)
+                 '(nil nil)))
+
+  (should (equal (eval '(let ((a 1))
+                          a)
+                       t)
+                 1))
+
+  (should (equal (eval '(let ((a 1) b)
+                          (list a b))
+                       t)
+                 '(1 nil)))
+
+  ;; (error "`let' bindings can have only one value-form" a 1 2)
+  (should-error (eval '(let ((a 1 2))
+                         a)
+                      t)
+                :type 'error)
+
+  ;; (wrong-type-argument symbolp (a))
+  (should-error (eval '(let (((a) 1))
+                         a)
+                      t)
+                :type 'wrong-type-argument))
+
 ;;; eval-tests.el ends here
-- 
2.30.1

Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Stefan Monnier]

> That testcase comes from this code (src/eval.c:L1014) which we already had.
>
>       else if (! NILP (Fcdr (Fcdr (elt))))
> 	signal_error ("`let' bindings can have only one value-form", elt);

Yes, some people disagree with my me ;-)

> I tried to remove this, my temp Emacs works like this in *scratch* buffer.
>
>     (let ((a 1 2))
>        a)                       ; Type C-j
>     1
>
> This is very strange I think.  I still think it's important for
> Emacs, even as an interpreter, to produce errors.

I view the ELisp interpreter as a crutch to bootstrap the system.
AFAIK the only really good reason why we still have it is for the
benefit of debugging: debugging byte-compiled code (whether with Edebug
or with the backtrace debugger) is a lot more constraining than
debugging interpreted code, mostly because it loses the information
about the currently bound lexical variables.  All other uses of it could
be replaced by something like

    (defun eval (exp) (funcall (byte-compile `(lambda () ,exp))))


        Stefan

Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Pip Cet]

On Tue, Mar 2, 2021 at 2:32 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> I view the ELisp interpreter as a crutch to bootstrap the system.

Well, I view the bytecode compiler as a crutch to get to LIMPLE for
native compilation ;-)

I'm all for a revolution, but it might be a bit early to chop off this
particular king's head...

> AFAIK the only really good reason why we still have it is for the
> benefit of debugging: debugging byte-compiled code (whether with Edebug
> or with the backtrace debugger) is a lot more constraining than
> debugging interpreted code, mostly because it loses the information
> about the currently bound lexical variables.
> All other uses of it could
> be replaced by something like
>
>     (defun eval (exp) (funcall (byte-compile `(lambda () ,exp))))

Except for the ones that can't. ELC is still limited to 64K constants
in the vector, for example, isn't it? It's a bit late in the century
for 16-bit limits...

But as for the original question, do we have to have Flet? I just
defined a macro called let which builds a closure and calls it, and it
crashed my Emacs. But if it would work, could we get rid of this
entire (C) function?

Oh, right, I assumed lexical scoping...

Pip

Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Stefan Monnier]

>> I view the ELisp interpreter as a crutch to bootstrap the system.
> Well, I view the bytecode compiler as a crutch to get to LIMPLE for
> native compilation ;-)

I'll get there.

> I'm all for a revolution, but it might be a bit early to chop off this
> particular king's head...

Until we have good debugging support for byte-compiled code, the
interpreter isn't going anywhere, indeed.

But error reporting from the interpreter is very secondary because in my
view that interpreter should only be used for bootstrap and for
debugging, so all the code it executes should *also* be byte-compiled
(so we can rely on the byte-compiler for error reporting).

>>     (defun eval (exp) (funcall (byte-compile `(lambda () ,exp))))
> Except for the ones that can't.  ELC is still limited to 64K constants
> in the vector, for example, isn't it?

I believe so, yes.  If/when we bump into this limit we can push it
further (or finally replace our bytecode language with a new one ;-).

> But as for the original question, do we have to have Flet?

I have not seen this question asked in this thread.


        Stefan

Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Pip Cet]

On Tue, Mar 2, 2021 at 3:48 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> > I'm all for a revolution, but it might be a bit early to chop off this
> > particular king's head...
> Until we have good debugging support for byte-compiled code, the
> interpreter isn't going anywhere, indeed.

We could rewrite it in ELisp, though :-)

> But error reporting from the interpreter is very secondary because in my
> view that interpreter should only be used for bootstrap and for
> debugging, so all the code it executes should *also* be byte-compiled
> (so we can rely on the byte-compiler for error reporting).

Agreed. But having to choose between compile-time error reporting and
run-time debugging can be very inconvenient, and now we're about to
add the third choice of "debug the natively-compiled code in gdb",
everything's going to get even more confusing...

> >>     (defun eval (exp) (funcall (byte-compile `(lambda () ,exp))))
> > Except for the ones that can't.  ELC is still limited to 64K constants
> > in the vector, for example, isn't it?
>
> I believe so, yes.  If/when we bump into this limit we can push it
> further (or finally replace our bytecode language with a new one ;-).

I hear the hot new thing is to use leb128 integers for everything.

> > But as for the original question, do we have to have Flet?
>
> I have not seen this question asked in this thread.

The question was "can we remove this code from Flet?", right? I think
"yes, all of it. No, all of Flet." is a valid answer to that question
:-)

(I dislike the "let" family, mostly for the fact that it is a family.
It leads naturally to cl-loop.)

Pip

Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Stefan Monnier]

>> > I'm all for a revolution, but it might be a bit early to chop off this
>> > particular king's head...
>> Until we have good debugging support for byte-compiled code, the
>> interpreter isn't going anywhere, indeed.
> We could rewrite it in ELisp, though :-)

That would mean just replacing the `Flet` in C with another in ELisp, so
it would largely just move the question (which is about diagnosing
invalid code).

>> >>     (defun eval (exp) (funcall (byte-compile `(lambda () ,exp))))
>> > Except for the ones that can't.  ELC is still limited to 64K constants
>> > in the vector, for example, isn't it?
>> I believe so, yes.  If/when we bump into this limit we can push it
>> further (or finally replace our bytecode language with a new one ;-).
> I hear the hot new thing is to use leb128 integers for everything.

I see I'm fashionable, then:

    (defconst bindat-test--LEB128
      (bindat-type
       letrec ((loop
                (struct :pack-var n
                        (head u8
                              :pack-val (+ (logand n 127) (if (> n 127) 128 0)))
                        (tail if (< head 128) (unit 0) loop
                              :pack-val (ash n -7))
                        :unpack-val (+ (logand head 127) (ash tail 7)))))
       loop))

;-)

>> > But as for the original question, do we have to have Flet?
>> I have not seen this question asked in this thread.
> The question was "can we remove this code from Flet?", right? I think
> "yes, all of it. No, all of Flet." is a valid answer to that question :-)

Point taken.

> (I dislike the "let" family, mostly for the fact that it is a family.
> It leads naturally to cl-loop.)

Removing its special-form status wouldn't remove it from the
language, tho :-(

[ Personally, I do find `let` important.
  Partly because of my let-polymorphism upbringing ;-)  ]


        Stefan

Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Pip Cet]

On Tue, Mar 2, 2021 at 5:44 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> >> > I'm all for a revolution, but it might be a bit early to chop off this
> >> > particular king's head...
> >> Until we have good debugging support for byte-compiled code, the
> >> interpreter isn't going anywhere, indeed.
> > We could rewrite it in ELisp, though :-)
>
> That would mean just replacing the `Flet` in C with another in ELisp, so
> it would largely just move the question (which is about diagnosing
> invalid code).

Two advantages: A macro can be redefined and have its redefinition
apply to compiled code; and, if let were implemented as a macro, other
letlikes would have a starting point.

For example, I'd love

(traced-let ((a 1) (b 2) (c (throw 'out nil)) (d 3)) ...)

to work (and print "a = 1 b = 2 c = <interrupted>"), but every time I
need it it seems more effort to write than just to debug things the
old-fashioned way (did I mention it's my non-integral birthday?)...

I think this applies to debugging invalid code, too.

> > (I dislike the "let" family, mostly for the fact that it is a family.
> > It leads naturally to cl-loop.)
>
> Removing its special-form status wouldn't remove it from the
> language, tho :-(

It would make it live at the same level as cl-loop or pcase.

> [ Personally, I do find `let` important.
>   Partly because of my let-polymorphism upbringing ;-)  ]

I do wonder why other languages have moved to the equivalent of

(defun f (a) (letq x 1) (+ x a))

or

(defun f (a) (+ (letq x 1) a x))

or even

(defun f (a) (if (> a 3) (letq x 1) (letq x 2)) (+ x a x))

while Lisps insist you have to be up front and list all your variables
when it creates the stack frame. And

(letq a 1)
(message "a is %S" a)
(letq b 2)

is easier to read than

(let* ((a 1)
      (_ (message "a is %S" a))
      (b 2))

What seems particularly problematic to me is that other languages can
emulate "let" easily, but implementing "letq" in ELisp is...an
interesting exercise (i.e. I tried and failed).

So those are the emacs-devel-relevant questions: Can you implement
letq in ELisp? And why does it feel so wrong in ELisp when it's how
most other languages do this? For bonus points, make

(defun f ()
  (letq g (lambda () (letq-in g g-counter 0) (letq-in f f-counter 0)
(incf g-counter) (incf f-counter)))
  (cons g (lambda () f-counter))

work.

Pip

Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Stefan Monnier]

>> That would mean just replacing the `Flet` in C with another in ELisp, so
>> it would largely just move the question (which is about diagnosing
>> invalid code).
> Two advantages: A macro can be redefined and have its redefinition
> apply to compiled code; and, if let were implemented as a macro, other
> letlikes would have a starting point.

I'd be happy to have a "low-level" `let` and then macros on top, yes.
But some kind of let-like thingy at the lowest level is largely
unavoidable for practical reasons (falling back on funcall+lambda is
cute for the theory, but to get acceptable performance you need to
treat that combination specially).

> For example, I'd love
>
> (traced-let ((a 1) (b 2) (c (throw 'out nil)) (d 3)) ...)
>
> to work (and print "a = 1 b = 2 c = <interrupted>"), but every time I
> need it it seems more effort to write than just to debug things the
> old-fashioned way (did I mention it's my non-integral birthday?)...

I can't see any trouble defining such a macro nor in which way it's
influenced by whether `let` itself is a macro or a special form.

> I think this applies to debugging invalid code, too.

What do you mean by "debugging invalid code"?

> I do wonder why other languages have moved to the equivalent of
>
> (defun f (a) (letq x 1) (+ x a))
>
> or
>
> (defun f (a) (+ (letq x 1) a x))

I find this horrible, like Scheme's `define`.
The semantics of such things tends to be quite intricate.

I think what I find ugly about it is that it means an expression affects
the set of variables bound in the surrounding context.

> What seems particularly problematic to me is that other languages can
> emulate "let" easily, but implementing "letq" in ELisp is...an
> interesting exercise (i.e. I tried and failed).

You can emulate it with a new macro `progn-with-lets` which looks for
`letq` in its body.  It's an interesting exercise, indeed, and its
complexity is a good part of why I dislike such features.

> So those are the emacs-devel-relevant questions: Can you implement
> letq in ELisp?

You can, but by its very nature it can't be implemented "as is": it has
to be done within the context of some other element (like
`progn-with-lets`).

> And why does it feel so wrong in ELisp when it's how
> most other languages do this?

Basically those other languages define their functions (and other forms)
to take bodies implicitly wrapped in `progn-with-lets`.

> For bonus points, make
>
> (defun f ()
>   (letq g (lambda () (letq-in g g-counter 0) (letq-in f f-counter 0)
> (incf g-counter) (incf f-counter)))
>   (cons g (lambda () f-counter))

I'm afraid I have no idea what this code means.
[ I must admit, I'm not familiar with your "most other languages" ;-)  ]


        Stefan

Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Pip Cet]

On Tue, Mar 2, 2021 at 2:19 AM Naoya Yamashita <conao3@gmail.com> wrote:
> I found src/eval.c (let) has redundant conditions, that compares
> the length of the list with the current index and also checks if
> the current list is cons.

Technically, those aren't redundant. We're calling eval_sub in between
the checks, and that might modify the list of arguments.

That's not something that is supported, but it is something that
shouldn't segfault, just throw an error.

IIUC, the following will likely segfault with your patch:

(let ((cons-cell '((a 2) (b 3))))
  (eval `(let ((x (setcdr ',cons-cell nil))
           . ,cons-cell)
       (message "foo"))))

Again, that's not code that should work. It isn't quite nasty enough
to justify a segfault, though, IMHO.

Pip

Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Eli Zaretskii]

> Date: Tue, 02 Mar 2021 11:10:43 +0900 (JST)
> From: Naoya Yamashita <conao3@gmail.com>
> 
> I found src/eval.c (let) has redundant conditions, that compares
> the length of the list with the current index and also checks if
> the current list is cons.

They aren't redundant, they were added to avoid crashes in certain
rare use cases.  See commit 93511e9 and the emacs-devel discussion
which the log message refers to.  If the problems described there are
no longer pertinent, can you explain why?  TIA.

P.S. When I see code in Emacs that looks wrong/redundant/unclear, I
find it useful to look at the VC history of that code.  This is how I
discovered where this particular "redundancy" came from, just 3.5
years ago.

Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Naoya Yamashita]

Thanks, eli.

> They aren't redundant, they were added to avoid crashes in certain
> rare use cases.  See commit 93511e9 and the emacs-devel discussion
> which the log message refers to.  If the problems described there are
> no longer pertinent, can you explain why?  TIA.

Thanks for mentioning 93511e9.

I compared the two and found that the problem of crashing on
(if . "abc") crash problem was fixed in the former.

However

    (let ((vars (list 'v))))
      (setcdr vars vars)
      (eval (list let vars))))

and

    (let ((clauses (list '((progn (setcdr clauses "ouch")) nil)))))
      (eval (cons 'cond clauses))))

did not crash.  The circular list example became an infinite loop
in an older version of b3a3ed5 (even older changes to eval.c).
My change removed the list length checker, which is vulnerable to
list changes.  Furthermore, Emacs with my patch passes these test
cases.

My patched Emacs does not segfault the Pip example either.

    (let ((cons-cell '((a 2) (b 3))))
      (eval `(let ((x (setcdr ',cons-cell nil))
               . ,cons-cell)
           (message "foo"))))
    ;;=> foo

Please let me know if there are any test cases I'm missing.

> P.S. When I see code in Emacs that looks wrong/redundant/unclear, I
> find it useful to look at the VC history of that code.  This is how I
> discovered where this particular "redundancy" came from, just 3.5
> years ago.

Thanks, I find `M-x vc-history-region` and this is very useful!
I should use before write a patch.

Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Pip Cet]

On Tue, Mar 2, 2021 at 7:15 AM Naoya Yamashita <conao3@gmail.com> wrote:
> My patched Emacs does not segfault the Pip example either.
>
>     (let ((cons-cell '((a 2) (b 3))))
>       (eval `(let ((x (setcdr ',cons-cell nil))
>                . ,cons-cell)
>            (message "foo"))))
>     ;;=> foo

Sorry, I had misunderstood what you were changing about the code. This crashes:

(let ((cell '((a 3))))
  (eval `(let ((a (setcdr ',cell ',cell)) . ,cell)
           (message "foo"))))

(Please keep in mind such crashes are hard to predict; if I wrote my
example the way I intended, it should exhaust the stack on pretty much
all machines, but it might still work, by sheer accident, on yours).

Pip

Re: [PATCH] * src/eval.c: Stop checking for nvars, and use only CONSP

[Eli Zaretskii]

> Date: Tue, 02 Mar 2021 16:14:18 +0900 (JST)
> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> From: Naoya Yamashita <conao3@gmail.com>
> 
> I compared the two and found that the problem of crashing on
> (if . "abc") crash problem was fixed in the former.
> 
> However
> 
>     (let ((vars (list 'v))))
>       (setcdr vars vars)
>       (eval (list let vars))))
> 
> and
> 
>     (let ((clauses (list '((progn (setcdr clauses "ouch")) nil)))))
>       (eval (cons 'cond clauses))))
> 
> did not crash.  The circular list example became an infinite loop
> in an older version of b3a3ed5 (even older changes to eval.c).
> My change removed the list length checker, which is vulnerable to
> list changes.  Furthermore, Emacs with my patch passes these test
> cases.
> 
> My patched Emacs does not segfault the Pip example either.
> 
>     (let ((cons-cell '((a 2) (b 3))))
>       (eval `(let ((x (setcdr ',cons-cell nil))
>                . ,cons-cell)
>            (message "foo"))))
>     ;;=> foo
> 
> Please let me know if there are any test cases I'm missing.

I think the problem is a general one, and it's impossible to prove it
doesn't exist by providing a small number of examples.  This code was
explicitly added to make the code safer in the face of self-modifying
expressions.

So I think removing it is not TRT.

> > P.S. When I see code in Emacs that looks wrong/redundant/unclear, I
> > find it useful to look at the VC history of that code.  This is how I
> > discovered where this particular "redundancy" came from, just 3.5
> > years ago.
> 
> Thanks, I find `M-x vc-history-region` and this is very useful!
> I should use before write a patch.

Thanks.