bug#68113: Wrong error message triggered in cl--generic-standard-method-combination

bug#68113: Wrong error message triggered in cl--generic-standard-method-combination

[Alan Mackenzie]

Hello, Emacs.

In my development version of Emacs (based on the master branch) I get a
backtrace with the error message being:

    Error: wrong-type-argument (symbolp mets-by-qual)

This occurs during the execution of cl-generic-combine-methods, whose
code starts:

(defun cl--generic-standard-method-combination (generic methods)
  (let ((mets-by-qual ()))
    (dolist (method methods)
      (let ((qualifiers (cl-method-qualifiers method)))
        (if (eq (car qualifiers) :extra) (setq qualifiers (cddr qualifiers)))
        (unless (member qualifiers '(() (:after) (:before) (:around)))
          (error "Unsupported qualifiers in function %S: %S"
                 (cl--generic-name generic) qualifiers))
        (push method (alist-get (car qualifiers) mets-by-qual))))

It is the last line that is signalling the error.  The pertinent line
from the backtrace which is the expansion of that last line reads:

    (let* ((k (car qualifiers)) (p (assq k mets-by-qual)) (v (cons method (cdr p)))) (progn (if p (setcdr p v) (progn (signal 'wrong-type-argument (list 'symbolp 'mets-by-qual)))) v))

..  The error is, in actual fact, the failure to find an entry for (car
qualifiers) in the alist mets-by-qual.  The error message given is
rubbish and more than a little misleading.  mets-by-qual is clearly a
symbol.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#68113: Wrong error message triggered in cl--generic-standard-method-combination

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

Hi Alan,

> In my development version of Emacs (based on the master branch) I get a
> backtrace with the error message being:
>
>     Error: wrong-type-argument (symbolp mets-by-qual)
>
> This occurs during the execution of cl-generic-combine-methods, whose
> code starts:
>
> (defun cl--generic-standard-method-combination (generic methods)
>   (let ((mets-by-qual ()))
>     (dolist (method methods)
>       (let ((qualifiers (cl-method-qualifiers method)))
>         (if (eq (car qualifiers) :extra) (setq qualifiers (cddr qualifiers)))
>         (unless (member qualifiers '(() (:after) (:before) (:around)))
>           (error "Unsupported qualifiers in function %S: %S"
>                  (cl--generic-name generic) qualifiers))
>         (push method (alist-get (car qualifiers) mets-by-qual))))
>
> It is the last line that is signalling the error.  The pertinent line
> from the backtrace which is the expansion of that last line reads:
>
>     (let* ((k (car qualifiers)) (p (assq k mets-by-qual)) (v (cons method (cdr p)))) (progn (if p (setcdr p v) (progn (signal 'wrong-type-argument (list 'symbolp 'mets-by-qual)))) v))
>
> ..  The error is, in actual fact, the failure to find an entry for (car
> qualifiers) in the alist mets-by-qual.  The error message given is
> rubbish and more than a little misleading.  mets-by-qual is clearly a
> symbol.

[ Side note: not finding an entry for (car qualifiers) in the above code
  is perfectly normal (it's the most common case, even).  The code only
  finds such an entry when there are several applicable methods (and
  they have the same set of qualifiers).  ]

Hmm... the error occurs during macroexpansion, because the
macroexpansion of the `push` above should be (and is, normally):

    ELISP> (macroexpand '(push method (alist-get (car qualifiers) mets-by-qual)))
    (let*
        ((k (car qualifiers)) (p (assq k mets-by-qual))
         (v (cons method (cdr p))))
      (progn
        (if p (setcdr p v)
          (setq mets-by-qual (cons (setq p (cons k v)) mets-by-qual)))
        v))
    
    ELISP>

I don't know why you're not getting that expansion, and I don't know
either why you're getting that

    (signal 'wrong-type-argument (list 'symbolp 'mets-by-qual))

AFAICT this weird code is likely generated by `gv-delay-error` but
according to `grep` it's only used in `map-elt` so I can't see why it's
showing up here.

I'd start debugging this with something like `M-x trace-function RET
gv-get RET` and `M-x debug-on-entry RET gv-delay-error RET`.
[ Tho, presumably you're seeing this during the bootstrap, so you'll
  probably want to add `message/debug` calls in the code instead.  ]


        Stefan

bug#68113: Wrong error message triggered in cl--generic-standard-method-combination

[Alan Mackenzie]

Hello, Stefan.

Thanks for the quick reply yesterday.

On Fri, Dec 29, 2023 at 12:24:36 -0500, Stefan Monnier wrote:
> Hi Alan,

> > In my development version of Emacs (based on the master branch) I get a
> > backtrace with the error message being:

> >     Error: wrong-type-argument (symbolp mets-by-qual)

> > This occurs during the execution of cl-generic-combine-methods, whose
> > code starts:

> > (defun cl--generic-standard-method-combination (generic methods)
> >   (let ((mets-by-qual ()))
> >     (dolist (method methods)
> >       (let ((qualifiers (cl-method-qualifiers method)))
> >         (if (eq (car qualifiers) :extra) (setq qualifiers (cddr qualifiers)))
> >         (unless (member qualifiers '(() (:after) (:before) (:around)))
> >           (error "Unsupported qualifiers in function %S: %S"
> >                  (cl--generic-name generic) qualifiers))
> >         (push method (alist-get (car qualifiers) mets-by-qual))))

> > It is the last line that is signalling the error.  The pertinent line
> > from the backtrace which is the expansion of that last line reads:

> >     (let* ((k (car qualifiers)) (p (assq k mets-by-qual)) (v (cons method (cdr p)))) (progn (if p (setcdr p v) (progn (signal 'wrong-type-argument (list 'symbolp 'mets-by-qual)))) v))

> > ..  The error is, in actual fact, the failure to find an entry for (car
> > qualifiers) in the alist mets-by-qual.  The error message given is
> > rubbish and more than a little misleading.  mets-by-qual is clearly a
> > symbol.

> [ Side note: not finding an entry for (car qualifiers) in the above code
>   is perfectly normal (it's the most common case, even).  The code only
>   finds such an entry when there are several applicable methods (and
>   they have the same set of qualifiers).  ]

> Hmm... the error occurs during macroexpansion, because the
> macroexpansion of the `push` above should be (and is, normally):

>     ELISP> (macroexpand '(push method (alist-get (car qualifiers) mets-by-qual)))
>     (let*
>         ((k (car qualifiers)) (p (assq k mets-by-qual))
>          (v (cons method (cdr p))))
>       (progn
>         (if p (setcdr p v)
>           (setq mets-by-qual (cons (setq p (cons k v)) mets-by-qual)))
>         v))
    
This was a good hint, thanks.  The most likely source of the (signal
.....) form would seem to be the clause handling `setq' in
macroexp--expand-all.  I suspected it might be caused, somehow, by
symbolp not recognising symbols with position as symbols.  But I
tightened up all the entry points, and disabled SWPs, and I still can't
get the code in macroexp--expand-all to printf a message for
mets-by-qual.

>     ELISP>

> I don't know why you're not getting that expansion, and I don't know
> either why you're getting that

>     (signal 'wrong-type-argument (list 'symbolp 'mets-by-qual))

I don't know, either.  :-(  As I say, I've tried instrumenting the `setq'
handling code in macroexp--expand-all, but haven't managed to get
anything pertinent output.

> AFAICT this weird code is likely generated by `gv-delay-error` but
> according to `grep` it's only used in `map-elt` so I can't see why it's
> showing up here.

> I'd start debugging this with something like `M-x trace-function RET
> gv-get RET` and `M-x debug-on-entry RET gv-delay-error RET`.
> [ Tho, presumably you're seeing this during the bootstrap, so you'll
>   probably want to add `message/debug` calls in the code instead.  ]

I am indeed seeing this in bootstrap, so it's message and a bit of prin1.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#68113: Wrong error message triggered in cl--generic-standard-method-combination

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

>> I don't know why you're not getting that expansion, and I don't know
>> either why you're getting that
>
>>     (signal 'wrong-type-argument (list 'symbolp 'mets-by-qual))
>
> I don't know, either.  :-(  As I say, I've tried instrumenting the `setq'
> handling code in macroexp--expand-all, but haven't managed to get
> anything pertinent output.

Ah, indeed instead of `gv-delay-error` it could also come from
`macroexp--expand-all`.
The `macroexp.el` hunk below would rule that out, tho.

>> AFAICT this weird code is likely generated by `gv-delay-error` but
>> according to `grep` it's only used in `map-elt` so I can't see why it's
>> showing up here.
>
>> I'd start debugging this with something like `M-x trace-function RET
>> gv-get RET` and `M-x debug-on-entry RET gv-delay-error RET`.
>> [ Tho, presumably you're seeing this during the bootstrap, so you'll
>>   probably want to add `message/debug` calls in the code instead.  ]
>
> I am indeed seeing this in bootstrap, so it's message and a bit of prin1.

Did you get to see the offending code in one of the outputs of `gv-get`?
Hpw 'bout a test that tries to see when that code is generated, as in
the `gv.el` patch below?


        Stefan


diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
index 78601c0648e..5c461206820 100644
--- a/lisp/emacs-lisp/macroexp.el
+++ b/lisp/emacs-lisp/macroexp.el
@@ -467,10 +467,10 @@ macroexp--expand-all
                                        ,var
                                      (signal 'setting-constant (list ',var))))
                                  ((symbolp var)
-                                  `(signal 'setting-constant (list ',var)))
+                                  (signal 'setting-constant (list var)))
                                  (t
-                                  `(signal 'wrong-type-argument
-                                           (list 'symbolp ',var))))
+                                  (signal 'wrong-type-argument
+                                          (list 'symbolp var))))
                                 nil 'compile-only var))))
                        (push assignment assignments))
                      (setq args (cddr args)))


diff --git a/lisp/emacs-lisp/gv.el b/lisp/emacs-lisp/gv.el
index 9f40c1f3c93..9cfd6d4b423 100644
--- a/lisp/emacs-lisp/gv.el
+++ b/lisp/emacs-lisp/gv.el
@@ -86,6 +86,8 @@ gv-get
 with a (not necessarily copyable) Elisp expression that returns the value to
 set it to.
 DO must return an Elisp expression."
+  (message "Entering gv-get for %S" place)
+  (let ((res
   (cond
    ((symbolp place)
     (let ((me (macroexpand-1 place macroexpand-all-environment)))
@@ -118,7 +120,13 @@ gv-get
                 (let* ((setter (gv-setter head)))
                   (gv--defsetter head (lambda (&rest args) `(,setter ,@args))
                                  do (cdr place))))
-            (gv-get me do))))))))
+            (gv-get me do)))))))
+  ))
+   (if (string-match-p "(list 'symbolp 'mets-by-qual)"
+        (prin1-to-string res))
+    (error "Gotcha!?"))
+   (message "Exiting gv-get for %S: %S" place res)
+   res))
 
 (defun gv-setter (name)
   ;; The name taken from Scheme's SRFI-17.  Actually, for SRFI-17, the argument

bug#68113: Wrong error message triggered in cl--generic-standard-method-combination

[Alan Mackenzie]

Hello, Stefan.

I think we've both been confused.

The code which signalled the error was

    (push method (alist-get (car qualifiers) mets-by-qual))

, and at the time of calling, qualifiers was nil.  So the call boiled
down to

    (push method (alist-get nil mets-by-qual))

, and there was no element of mets-by-qual with a car of nil.  So what
can the push macro do?  There's no list to push onto.  It can generate
code either
(i) to signal an error; or
(ii) to create a new element in the alist mets-by-qual with method being
the single element of its cdr.

In actual fact, it does (i), but (as reported in the bug report) gives
the wrong message.  It should say something like "void list" or
"non-existent list", not "mets-by-qual isn't a symbol", and it should
give further information to enable the working out of WHICH list doesn't
exist.

That line of code is poor.  It assumes that (alist-get (car qualifiers)
mets-by-qual) will always return a list element, and fails to make any
checks.  It fails to check that qualifiers is a non-empty list before
taking its car.

That's not to say I'm denying responsibility for the failure.  It's
almost certainly caused by my tentative alterations for bug #67455.  But
bug #68113, the current bug, is real - That wrong-argument-type error is
definitely erroneous.  It's possible it was also caused by my changes for
bug #67455, but I don't have the energy to look into that at the moment.

Would you please check the code that signaled that error (you wrote it, I
think), and let me know whether you find anything suspicious in it.
Thanks!

On Sun, Dec 31, 2023 at 15:01:05 -0500, Stefan Monnier wrote:
> >> I don't know why you're not getting that expansion, and I don't know
> >> either why you're getting that

> >>     (signal 'wrong-type-argument (list 'symbolp 'mets-by-qual))

> > I don't know, either.  :-(  As I say, I've tried instrumenting the `setq'
> > handling code in macroexp--expand-all, but haven't managed to get
> > anything pertinent output.

> Ah, indeed instead of `gv-delay-error` it could also come from
> `macroexp--expand-all`.
> The `macroexp.el` hunk below would rule that out, tho.

> >> AFAICT this weird code is likely generated by `gv-delay-error` but
> >> according to `grep` it's only used in `map-elt` so I can't see why it's
> >> showing up here.

> >> I'd start debugging this with something like `M-x trace-function RET
> >> gv-get RET` and `M-x debug-on-entry RET gv-delay-error RET`.
> >> [ Tho, presumably you're seeing this during the bootstrap, so you'll
> >>   probably want to add `message/debug` calls in the code instead.  ]

> > I am indeed seeing this in bootstrap, so it's message and a bit of prin1.

> Did you get to see the offending code in one of the outputs of `gv-get`?
> Hpw 'bout a test that tries to see when that code is generated, as in
> the `gv.el` patch below?


>         Stefan


> diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
> index 78601c0648e..5c461206820 100644
> --- a/lisp/emacs-lisp/macroexp.el
> +++ b/lisp/emacs-lisp/macroexp.el
> @@ -467,10 +467,10 @@ macroexp--expand-all
>                                         ,var
>                                       (signal 'setting-constant (list ',var))))
>                                   ((symbolp var)
> -                                  `(signal 'setting-constant (list ',var)))
> +                                  (signal 'setting-constant (list var)))
>                                   (t
> -                                  `(signal 'wrong-type-argument
> -                                           (list 'symbolp ',var))))
> +                                  (signal 'wrong-type-argument
> +                                          (list 'symbolp var))))
>                                  nil 'compile-only var))))
>                         (push assignment assignments))
>                       (setq args (cddr args)))


> diff --git a/lisp/emacs-lisp/gv.el b/lisp/emacs-lisp/gv.el
> index 9f40c1f3c93..9cfd6d4b423 100644
> --- a/lisp/emacs-lisp/gv.el
> +++ b/lisp/emacs-lisp/gv.el
> @@ -86,6 +86,8 @@ gv-get
>  with a (not necessarily copyable) Elisp expression that returns the value to
>  set it to.
>  DO must return an Elisp expression."
> +  (message "Entering gv-get for %S" place)
> +  (let ((res
>    (cond
>     ((symbolp place)
>      (let ((me (macroexpand-1 place macroexpand-all-environment)))
> @@ -118,7 +120,13 @@ gv-get
>                  (let* ((setter (gv-setter head)))
>                    (gv--defsetter head (lambda (&rest args) `(,setter ,@args))
>                                   do (cdr place))))
> -            (gv-get me do))))))))
> +            (gv-get me do)))))))
> +  ))
> +   (if (string-match-p "(list 'symbolp 'mets-by-qual)"
> +        (prin1-to-string res))
> +    (error "Gotcha!?"))
> +   (message "Exiting gv-get for %S: %S" place res)
> +   res))
 
>  (defun gv-setter (name)
>    ;; The name taken from Scheme's SRFI-17.  Actually, for SRFI-17, the argument

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#68113: Wrong error message triggered in cl--generic-standard-method-combination

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

> , and at the time of calling, qualifiers was nil.  So the call boiled
> down to
>
>     (push method (alist-get nil mets-by-qual))
>
> , and there was no element of mets-by-qual with a car of nil.  So what
> can the push macro do?  There's no list to push onto.  It can generate
> code either
> (i) to signal an error; or
> (ii) to create a new element in the alist mets-by-qual with method being
> the single element of its cdr.
>
> In actual fact, it does (i), but (as reported in the bug report) gives
> the wrong message.

But as I pointed out, the normal macroexpansion does (ii), so the
problem *is* in the wrong macroexpansion.

> That line of code is poor.  It assumes that (alist-get (car qualifiers)
> mets-by-qual) will always return a list element,

No, it doesn't.  The `gv-expander` for `alist-get` handles that case
just fine (tho for some reason not in your case, obviously).

> and fails to make any checks.  It fails to check that qualifiers is
> a non-empty list before taking its car.

It's on purpose.  The "nil" case is the common case, actually.
This has been working fine since Emacs-25, remember.

> Would you please check the code that signaled that error (you wrote it, I
> think), and let me know whether you find anything suspicious in it.

I already did.

Did you try the patch I sent?


        Stefan

bug#68113: Wrong error message triggered in cl--generic-standard-method-combination

[Alan Mackenzie]

Hello, Stefan.

On Mon, Jan 01, 2024 at 23:39:57 -0500, Stefan Monnier wrote:
> > , and at the time of calling, qualifiers was nil.  So the call boiled
> > down to

> >     (push method (alist-get nil mets-by-qual))

> > , and there was no element of mets-by-qual with a car of nil.  So what
> > can the push macro do?  There's no list to push onto.  It can generate
> > code either
> > (i) to signal an error; or
> > (ii) to create a new element in the alist mets-by-qual with method being
> > the single element of its cdr.

> > In actual fact, it does (i), but (as reported in the bug report) gives
> > the wrong message.

> But as I pointed out, the normal macroexpansion does (ii), so the
> problem *is* in the wrong macroexpansion.

Yes indeed.

> > That line of code is poor.  It assumes that (alist-get (car qualifiers)
> > mets-by-qual) will always return a list element,

> No, it doesn't.  The `gv-expander` for `alist-get` handles that case
> just fine (tho for some reason not in your case, obviously).

> > and fails to make any checks.  It fails to check that qualifiers is
> > a non-empty list before taking its car.

> It's on purpose.  The "nil" case is the common case, actually.
> This has been working fine since Emacs-25, remember.

Yes.

> > Would you please check the code that signaled that error (you wrote it, I
> > think), and let me know whether you find anything suspicious in it.

> I already did.

> Did you try the patch I sent?

Yes I did, thanks.  It showed up the real problem.

I had modified the macroexpansion system in an attempt to use the
framework in macroexp--expand-all but without the "base case", by
fset'ing macroexp-macroexpand temporarily to an identity function.  This
was an attempt to strip symbols with position of their positions for bug
#67455, getting lambda positions into the doc strings.  In the end this
attempt was hopeless, but I spent quite some time on it.

I hadn't realised that macro expansion was central to gv.el.  Bug #68113
might be a real bug, but I somehow doubt it.  I'll close it as not a
bug.

Sorry to have wasted your time in the process.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#68113: Wrong error message triggered in cl--generic-standard-method-combination

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

> I had modified the macroexpansion system in an attempt to use the
> framework in macroexp--expand-all but without the "base case", by
> fset'ing macroexp-macroexpand temporarily to an identity function.  This
> was an attempt to strip symbols with position of their positions for bug
> #67455, getting lambda positions into the doc strings.  In the end this
> attempt was hopeless, but I spent quite some time on it.

Makes sense.

> I hadn't realised that macro expansion was central to gv.el.
> Bug #68113 might be a real bug, but I somehow doubt it.  I'll close it
> as not a bug.

Great, thanks.  Glad I could help,


        Stefan