bug#59710: Wrong type argument when editing a multisession variable

bug#59710: Wrong type argument when editing a multisession variable

[Juanma Barranquero]

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

Package: emacs
Version: 29.0.60
Severity: normal
X-Debbugs-Cc: "Lars Ingebrigtsen" <larsi@gnus.org>


;;; init.el
(require 'multisession)
(define-multisession-variable test-var nil "Test var" :package "test")
(setf (multisession-value test-var) t)
;;; end of init.el

emacs
M-x list-multisession-values <RET>
e

Debugger entered--Lisp error: (wrong-type-argument symbolp (intern (cdr
id)))
  multisession-edit-value(("test" . "test-var"))
  funcall-interactively(multisession-edit-value ("test" . "test-var"))
  command-execute(multisession-edit-value)


The problem comes from this change

  commit bd586121ac21e046f60f75eeb0200866c38d6f9f
  Author: Lars Ingebrigtsen <larsi@gnus.org>
  Date:   2022-01-22 11:56:13 +0100

      Make the test for existing multisession variables more sensible

      * lisp/emacs-lisp/multisession.el (multisession-edit-value):
      Unconfuse the code.

  diff --git a/lisp/emacs-lisp/multisession.el
b/lisp/emacs-lisp/multisession.el
  index 4a293796a8..25307594c6 100644
  --- a/lisp/emacs-lisp/multisession.el
  +++ b/lisp/emacs-lisp/multisession.el
  @@ -437,8 +437,8 @@ multisession-edit-value
     (let* ((object (or
                     ;; If the multisession variable already exists, use
                     ;; it (so that we update it).
  -                  (and (boundp (intern-soft (cdr id)))
  -                       (symbol-value (intern (cdr id))))
  +                  (and (intern-soft (cdr id))
  +                       (bound-and-true-p (intern (cdr id))))
                     ;; Create a new object.
                     (make-multisession
                      :package (car id)


because `bound-and-true-p' is a macro that requires as argument a symbol,
which (intern ...) is not.

ELISP> (bound-and-true-p (intern "whatever"))
*** Eval error ***  Wrong type argument: symbolp, (intern "whatever")

so I'm afraid this change was never tested.

The fix is reverting the change, doing perhaps this

diff --git a/lisp/emacs-lisp/multisession.el
b/lisp/emacs-lisp/multisession.el
index 9d6e8c0d88..78d4137317 100644
--- a/lisp/emacs-lisp/multisession.el
+++ b/lisp/emacs-lisp/multisession.el
@@ -447,8 +447,9 @@ multisession-edit-value
   (let* ((object (or
                   ;; If the multisession variable already exists, use
                   ;; it (so that we update it).
-                  (and (intern-soft (cdr id))
-                       (bound-and-true-p (intern (cdr id))))
+                  (if-let (sym (intern-soft (cdr id)))
+                      (and (boundp sym) (symbol-value sym))
+                    nil)
                   ;; Create a new object.
                   (make-multisession
                    :package (car id)

[-- Attachment #2: Type: text/html, Size: 4644 bytes --]

bug#59710: Wrong type argument when editing a multisession variable

[Eli Zaretskii]

> Cc: "lars ingebrigtsen" <larsi@gnus.org>
> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 30 Nov 2022 02:49:15 +0100
> 
> Debugger entered--Lisp error: (wrong-type-argument symbolp (intern (cdr id)))
>   multisession-edit-value(("test" . "test-var"))
>   funcall-interactively(multisession-edit-value ("test" . "test-var"))
>   command-execute(multisession-edit-value)
> 
> The problem comes from this change
> 
>   commit bd586121ac21e046f60f75eeb0200866c38d6f9f
>   Author: Lars Ingebrigtsen <larsi@gnus.org>
>   Date:   2022-01-22 11:56:13 +0100
> 
>       Make the test for existing multisession variables more sensible
> 
>       * lisp/emacs-lisp/multisession.el (multisession-edit-value):
>       Unconfuse the code.
> 
>   diff --git a/lisp/emacs-lisp/multisession.el b/lisp/emacs-lisp/multisession.el
>   index 4a293796a8..25307594c6 100644
>   --- a/lisp/emacs-lisp/multisession.el
>   +++ b/lisp/emacs-lisp/multisession.el
>   @@ -437,8 +437,8 @@ multisession-edit-value
>      (let* ((object (or
>                      ;; If the multisession variable already exists, use
>                      ;; it (so that we update it).
>   -                  (and (boundp (intern-soft (cdr id)))
>   -                       (symbol-value (intern (cdr id))))
>   +                  (and (intern-soft (cdr id))
>   +                       (bound-and-true-p (intern (cdr id))))
>                      ;; Create a new object.
>                      (make-multisession
>                       :package (car id)
> 
> because `bound-and-true-p' is a macro that requires as argument a symbol, which (intern ...) is not.
> 
> ELISP> (bound-and-true-p (intern "whatever"))
> *** Eval error ***  Wrong type argument: symbolp, (intern "whatever")
> 
> so I'm afraid this change was never tested.
> 
> The fix is reverting the change, doing perhaps this
> 
> diff --git a/lisp/emacs-lisp/multisession.el b/lisp/emacs-lisp/multisession.el
> index 9d6e8c0d88..78d4137317 100644
> --- a/lisp/emacs-lisp/multisession.el
> +++ b/lisp/emacs-lisp/multisession.el
> @@ -447,8 +447,9 @@ multisession-edit-value
>    (let* ((object (or
>                    ;; If the multisession variable already exists, use
>                    ;; it (so that we update it).
> -                  (and (intern-soft (cdr id))
> -                       (bound-and-true-p (intern (cdr id))))
> +                  (if-let (sym (intern-soft (cdr id)))
> +                      (and (boundp sym) (symbol-value sym))
> +                    nil)
>                    ;; Create a new object.
>                    (make-multisession
>                     :package (car id)

This makes sense to me, so please go ahead and install (assuming that the
multisession tests still all pass after this change).

Thanks.

bug#59710: Wrong type argument when editing a multisession variable

[Juanma Barranquero]

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

On Fri, Dec 2, 2022 at 2:07 PM Eli Zaretskii <eliz@gnu.org> wrote:

> This makes sense to me, so please go ahead and install (assuming that the
> multisession tests still all pass after this change).

Yes, they pass. Installed in commit e5b0141b0d of 2022-12-02.

BTW, I wonder if it would make sense to make bound-and-true-p to check that
it gets a symbol:

diff --git i/lisp/bindings.el w/lisp/bindings.el
index c1ad5f7520..6ee730af58 100644
--- i/lisp/bindings.el
+++ w/lisp/bindings.el
@@ -671,4 +671,6 @@ bound-and-true-p
 Note that if `lexical-binding' is in effect, this function isn't
 meaningful if it refers to a lexically bound variable."
+  (unless (symbolp var)
+    (error "Wrong type argument: symbolp, %S" var))
   `(and (boundp (quote ,var)) ,var))

[-- Attachment #2: Type: text/html, Size: 1246 bytes --]

bug#59710: Wrong type argument when editing a multisession variable

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Fri, 2 Dec 2022 14:30:52 +0100
> Cc: larsi@gnus.org, 59710-done@debbugs.gnu.org
> 
> BTW, I wonder if it would make sense to make bound-and-true-p to check that it gets a symbol:
> 
> diff --git i/lisp/bindings.el w/lisp/bindings.el
> index c1ad5f7520..6ee730af58 100644
> --- i/lisp/bindings.el
> +++ w/lisp/bindings.el
> @@ -671,4 +671,6 @@ bound-and-true-p
>  Note that if `lexical-binding' is in effect, this function isn't
>  meaningful if it refers to a lexically bound variable."
> +  (unless (symbolp var)
> +    (error "Wrong type argument: symbolp, %S" var))
>    `(and (boundp (quote ,var)) ,var))

I have no opinion on that.  Lars, Stefan: WDYT?

bug#59710: Wrong type argument when editing a multisession variable

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> @@ -671,4 +671,6 @@ bound-and-true-p
>>  Note that if `lexical-binding' is in effect, this function isn't
>>  meaningful if it refers to a lexically bound variable."
>> +  (unless (symbolp var)
>> +    (error "Wrong type argument: symbolp, %S" var))
>>    `(and (boundp (quote ,var)) ,var))
>
> I have no opinion on that.  Lars, Stefan: WDYT?

I wish Someone™ implemented something like
[Fortifying macros](https://dl.acm.org/doi/10.1145/1863543.1863577)
to solve this in a more general way :-)

I have no objections against such a change.


        Stefan

bug#59710: Wrong type argument when editing a multisession variable

[Juanma Barranquero]

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

In fact, I think it would be marginally better

(unless (symbolp var)
  (signal 'wrong-type-argument (list 'symbolp var)))

Assuming ok to install... where? emacs-29 or master?

[-- Attachment #2: Type: text/html, Size: 433 bytes --]

bug#59710: Wrong type argument when editing a multisession variable

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Juanma Barranquero [2022-12-03 01:25:13] wrote:

> In fact, I think it would be marginally better
>
> (unless (symbolp var)
>   (signal 'wrong-type-argument (list 'symbolp var)))
>
> Assuming ok to install... where? emacs-29 or master?

master


        Tefan