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

[Alan Mackenzie <acm@muc.de>]

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).