bug#38872: 27.0.50; Keywords can be let-bound

[Stefan Kangas]

Thibault Polge <thibault@thb.lt> writes:

> This sexp:
>
>    (let ((:k 1)) :k)
>
> evals to 1 in Emacs 25.3.1, 26.3, and HEAD
> (d36adb544d984b91c70f6194da01344e4b2b6fc9) if and only if
> `lexical-binding` is t.
>
> If lexical-binding is nil, it raises an error, as I'd expect it to.
>
> This is in contradiction with documentation at (info "Constant variables
> (elisp)"). The issue also appears if the symbol's been interned with
> (intern ":k")

Thanks, I can reproduce this on current master.

I'm very new to Emacs internals, but decided to dig in.

The problem is that a keyword ":a" has that:

 (a1)   XSYMBOL (tem)->u.s.declared_special == false
 (a2)   XSYMBOL (tem)->u.s.trapped_write == SYMBOL_NOWRITE

However, in eval.c line 972, where "let" is defined, we lexically bind
a symbol under the above conditions:

      if (!NILP (lexenv) && SYMBOLP (var)
          && !XSYMBOL (var)->u.s.declared_special
          && NILP (Fmemq (var, Vinternal_interpreter_environment)))
        /* Lexically bind VAR by adding it to the lexenv alist.  */
        lexenv = Fcons (Fcons (var, tem), lexenv);

This is what causes the bug.

In contrast, "nil" and "t" has that:

 (b1)   XSYMBOL (tem)->u.s.declared_special == true

This makes the above check fail, and we defer to the dynamic binding
code which refuses to bind them.

1. One possible fix is to add a check for (b), and refuse to bind it
   if it's true.  The attached patch does that.

2. I'm not sure, but it seems to me that keywords *should* perhaps
   have declared_special == TRUE.  That should be set for anything
   that should never be lexically bound -- right?  If this is correct,
   then I guess we should make sure that it gets set properly,
   probably already in read1?

In plain language, either we (1) change the test in "let" or we (2)
change how keyword symbols gets initialized to pass the existing
test.  Or maybe we should do both..?

Please, if someone could check my findings or point me in the right
direction here, that would be very helpful.  Or even provide a proper
fix if mine is completely wrong.  :-)

Best regards,
Stefan Kangas

>From 1070ee4579bc907bfdd1b96edc80b4f4d271d57d Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Thu, 16 Jan 2020 15:33:08 +0100
Subject: [PATCH] Don't let-bind keywords when lexical-binding is t

* src/eval.c (Flet): Signal an error if trying to bind a keyword
symbol when lexical-binding is t.  This is consistent with the manual
section "(elisp)Constant variables".  (Bug#38872)

* test/src/eval-tests.el (eval-tests-let): Add rudimentary tests for
the let-form.
---
 src/eval.c             |  7 +++++--
 test/src/eval-tests.el | 12 ++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/src/eval.c b/src/eval.c
index 4559a0e1f6..9a3f703f40 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -972,8 +972,11 @@ DEFUN ("let", Flet, Slet, 1, UNEVALLED, 0,
       if (!NILP (lexenv) && SYMBOLP (var)
          && !XSYMBOL (var)->u.s.declared_special
          && NILP (Fmemq (var, Vinternal_interpreter_environment)))
-       /* Lexically bind VAR by adding it to the lexenv alist.  */
-       lexenv = Fcons (Fcons (var, tem), lexenv);
+       if (XSYMBOL (var)->u.s.trapped_write == SYMBOL_NOWRITE)
+         xsignal1 (Qsetting_constant, var);
+       else
+         /* Lexically bind VAR by adding it to the lexenv alist.  */
+         lexenv = Fcons (Fcons (var, tem), lexenv);
       else
        /* Dynamically bind VAR.  */
        specbind (var, tem);
diff --git a/test/src/eval-tests.el b/test/src/eval-tests.el
index 074f5be1ef..702bd25040 100644
--- a/test/src/eval-tests.el
+++ b/test/src/eval-tests.el
@@ -28,6 +28,18 @@
 (require 'ert)
 (eval-when-compile (require 'cl-lib))
 
+(ert-deftest eval-tests-let ()
+  "Test let binding using dynamic and lexical scope."
+  (dolist (nil-or-t '(nil t))
+    (with-temp-buffer
+      (setq lexical-binding nil-or-t)
+      (should (equal (let ((x 1)) x) 1))
+      (should-error (let ((1 2)) x) :type '(wrong-type-argument))
+      ;; Behave consistently with (info "(elisp)Constant variables")
+      (should-error (let ((t 1)) t) :type '(setting-constant))
+      (should-error (let ((nil 1)) nil) :type '(setting-constant))
+      (should-error (let ((:a 1)) :a) :type '(setting-constant)))))
+
 (ert-deftest eval-tests--bug24673 ()
   "Check that Bug#24673 has been fixed."
   ;; This should not crash.
-- 
2.20.1