bug#53227: master: Wrong error message with M-: (funcall).

bug#53227: master: Wrong error message with M-: (funcall).

[Alan Mackenzie]

Hello, Emacs.

In the master branch, with emacs -Q, do

    M-: (funcall) RET

..  The error message one sees is

    (void-function nil)

..  This is wrong.  There is no function, nil or otherwise, supplied to
funcall.  The error message ought to be "Too few arguments", or
something like that.

The cause of the bug lies in Ffuncall in .../src/eval.c.  There, the
code uses args[0] without first checking that nargs > 0.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#53227: master: Wrong error message with M-: (funcall).

[Robert Pluim]

>>>>> On Thu, 13 Jan 2022 07:39:05 +0000, Alan Mackenzie <acm@muc.de> said:

    Alan> Hello, Emacs.
    Alan> In the master branch, with emacs -Q, do

    Alan>     M-: (funcall) RET

    Alan> ..  The error message one sees is

    Alan>     (void-function nil)

    Alan> ..  This is wrong.  There is no function, nil or otherwise, supplied to
    Alan> funcall.  The error message ought to be "Too few arguments", or
    Alan> something like that.

    Alan> The cause of the bug lies in Ffuncall in .../src/eval.c.  There, the
    Alan> code uses args[0] without first checking that nargs > 0.

Itʼs not Ffuncall's responsibility to check that, itʼs 'eval' that
should be doing it. Compare with eg M-: (format), which correctly
signals 'wrong-number-of-arguments.

Robert
-- 

bug#53227: master: Wrong error message with M-: (funcall).

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> Itʼs not Ffuncall's responsibility to check that, itʼs 'eval' that
> should be doing it. Compare with eg M-: (format), which correctly
> signals 'wrong-number-of-arguments.

Or rather macroexpand-all:

(macroexpand-all '(format))
=> (format)

(macroexpand-all '(funcall))
=> (funcall nil)

Perhaps Stefan has some insights into this; added to the CCs.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53227: master: Wrong error message with M-: (funcall).

[Andreas Schwab]

On Jan 13 2022, Lars Ingebrigtsen wrote:

> Robert Pluim <rpluim@gmail.com> writes:
>
>> Itʼs not Ffuncall's responsibility to check that, itʼs 'eval' that
>> should be doing it. Compare with eg M-: (format), which correctly
>> signals 'wrong-number-of-arguments.
>
> Or rather macroexpand-all:
>
> (macroexpand-all '(format))
> => (format)
>
> (macroexpand-all '(funcall))
> => (funcall nil)

Same as bug #46636.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#53227: master: Wrong error message with M-: (funcall).

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

> Same as bug #46636.

Actually, it's a different bug, tho its origin might be the same commit.


        Stefan

bug#53227: master: Wrong error message with M-: (funcall).

[Andreas Schwab]

On Jan 13 2022, Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" wrote:

>> Same as bug #46636.
>
> Actually, it's a different bug, tho its origin might be the same commit.

It's coming from the same funcall rewrite rule.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#53227: master: Wrong error message with M-: (funcall).

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

Andreas Schwab [2022-01-13 16:37:09] wrote:
> On Jan 13 2022, Stefan Monnier via "Bug reports for GNU Emacs, the Swiss
> army knife of text editors" wrote:
>>> Same as bug #46636.
>> Actually, it's a different bug, tho its origin might be the same commit.
> It's coming from the same funcall rewrite rule.

I see you agree ;-)

The patch should hopefully fix both cases, tho as you can see the two
cases are fixed separately (which is what I mean by them being two
different bugs).


        Stefan


diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
index b44917f7d56..33ce55a3de8 100644
--- a/lisp/emacs-lisp/macroexp.el
+++ b/lisp/emacs-lisp/macroexp.el
@@ -366,14 +366,18 @@ macroexp--expand-all
                              form)
 	   (macroexp--expand-all newform))))
 
-      (`(funcall . ,(or `(,exp . ,args) pcase--dontcare))
+      (`(funcall ,exp . ,args)
        (let ((eexp (macroexp--expand-all exp))
              (eargs (macroexp--all-forms args)))
          ;; Rewrite (funcall #'foo bar) to (foo bar), in case `foo'
          ;; has a compiler-macro, or to unfold it.
          (pcase eexp
-           (`#',f (macroexp--expand-all `(,f . ,eargs)))
+           ((and `#',f
+                 (guard (not (or (special-form-p f) (macrop f)))));; bug#46636
+            (macroexp--expand-all `(,f . ,eargs)))
            (_ `(funcall ,eexp . ,eargs)))))
+      (`(funcall . ,_) form)            ;bug#53227
+
       (`(,func . ,_)
        (let ((handler (function-get func 'compiler-macro))
              (funargs (function-get func 'funarg-positions)))

bug#53227: master: Wrong error message with M-: (funcall).

[Andreas Schwab]

On Jan 13 2022, Stefan Monnier wrote:

> Andreas Schwab [2022-01-13 16:37:09] wrote:
>> On Jan 13 2022, Stefan Monnier via "Bug reports for GNU Emacs, the Swiss
>> army knife of text editors" wrote:
>>>> Same as bug #46636.
>>> Actually, it's a different bug, tho its origin might be the same commit.
>> It's coming from the same funcall rewrite rule.
>
> I see you agree ;-)
>
> The patch should hopefully fix both cases, tho as you can see the two
> cases are fixed separately (which is what I mean by them being two
> different bugs).

Are they?  Why do you need the second pattern?

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#53227: master: Wrong error message with M-: (funcall).

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

> Why do you need the second pattern?

The second pattern should let `pcase` generate better code.


        Stefan

bug#53227: master: Wrong error message with M-: (funcall).

[Andreas Schwab]

On Jan 13 2022, Stefan Monnier wrote:

>> Why do you need the second pattern?
>
> The second pattern should let `pcase` generate better code.

But it is not needed for this bug.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#53227: master: Wrong error message with M-: (funcall).

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

Andreas Schwab [2022-01-14 01:04:47] wrote:
> On Jan 13 2022, Stefan Monnier wrote:
>>> Why do you need the second pattern?
>> The second pattern should let `pcase` generate better code.
> But it is not needed for this bug.

The actual fix is in the change from

    (`(funcall . ,(or `(,exp . ,args) pcase--dontcare))

to

    (`(funcall ,exp . ,args)

which makes the pattern not match `(funcall . ,_) any more.
The explicit

    (`(funcall . ,_) form)            ;bug#53227

is then present to make sure the first change doesn't worsen the
rest of the resulting macroexpanded code.


        Stefan

bug#53227: master: Wrong error message with M-: (funcall).

[Andreas Schwab]

On Jan 13 2022, Stefan Monnier wrote:

> The explicit
>
>     (`(funcall . ,_) form)            ;bug#53227
>
> is then present to make sure the first change doesn't worsen the
> rest of the resulting macroexpanded code.

I don't understand.  How much better than (funcall) can it get?

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#53227: master: Wrong error message with M-: (funcall).

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

>>     (`(funcall . ,_) form)            ;bug#53227
>>
>> is then present to make sure the first change doesn't worsen the
>> rest of the resulting macroexpanded code.
>
> I don't understand.  How much better than (funcall) can it get?

I'm not talking about the quality of the code output by
`macroexp--expand-all` but about the quality of the code *of*
`macroexp--expand-all`.


        Stefan

bug#53227: master: Wrong error message with M-: (funcall).

[Andreas Schwab]

On Jan 13 2022, Stefan Monnier wrote:

>>>     (`(funcall . ,_) form)            ;bug#53227
>>>
>>> is then present to make sure the first change doesn't worsen the
>>> rest of the resulting macroexpanded code.
>>
>> I don't understand.  How much better than (funcall) can it get?
>
> I'm not talking about the quality of the code output by
> `macroexp--expand-all` but about the quality of the code *of*
> `macroexp--expand-all`.

You mean this?

@@ -534,16 +534,14 @@ byte code for macroexp--expand-all:
 431	stack-ref 3
 432	call	  2
 433	return	  
-434:28	stack-ref 2
-435	stack-ref 5
-436	stack-ref 3
-437	call	  2
-438	return	  
-439:29	stack-ref 1
-440	stack-ref 4
-441	stack-ref 2
-442	call	  2
-443	return	  
-444:30	stack-ref 2
-445	car-safe  
-446	return	  
+434:28	stack-ref 4
+435	car-safe  
+436	return	  
+437:29	stack-ref 1
+438	stack-ref 4
+439	stack-ref 2
+440	call	  2
+441	return	  
+442:30	stack-ref 2
+443	car-safe  
+444	return	  

Hardly worth optimizing for a case that never occurs, IMHO.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#53227: master: Wrong error message with M-: (funcall).

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

> You mean this?
>
> @@ -534,16 +534,14 @@ byte code for macroexp--expand-all:
>  431	stack-ref 3
>  432	call	  2
>  433	return	  
> -434:28	stack-ref 2
> -435	stack-ref 5
> -436	stack-ref 3
> -437	call	  2
> -438	return	  
> -439:29	stack-ref 1
> -440	stack-ref 4
> -441	stack-ref 2
> -442	call	  2
> -443	return	  
> -444:30	stack-ref 2
> -445	car-safe  
> -446	return	  
> +434:28	stack-ref 4
> +435	car-safe  
> +436	return	  
> +437:29	stack-ref 1
> +438	stack-ref 4
> +439	stack-ref 2
> +440	call	  2
> +441	return	  
> +442:30	stack-ref 2
> +443	car-safe  
> +444	return	  

Hmm... indeed a few of the other patterns, such as:

      (`(,(and fun `(lambda . ,_)) . ,args)

already prevent the optimization I was aiming for :-(


        Stefan

bug#53227: master: Wrong error message with M-: (funcall).

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> The patch should hopefully fix both cases, tho as you can see the two
> cases are fixed separately (which is what I mean by them being two
> different bugs).

I can confirm that this fixes the reported problem (and it doesn't lead
to any test suite regressions), so please do go ahead and push it.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53227: master: Wrong error message with M-: (funcall).

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

>> The patch should hopefully fix both cases, tho as you can see the two
>> cases are fixed separately (which is what I mean by them being two
>> different bugs).
> I can confirm that this fixes the reported problem (and it doesn't lead
> to any test suite regressions), so please do go ahead and push it.

Done, thanks,


        Stefan

bug#53227: master: Wrong error message with M-: (funcall).

[Alan Mackenzie]

Hello, Lars.

On Thu, Jan 13, 2022 at 10:32:44 +0100, Lars Ingebrigtsen wrote:
> Robert Pluim <rpluim@gmail.com> writes:

> > Itʼs not Ffuncall's responsibility to check that, itʼs 'eval' that
> > should be doing it. Compare with eg M-: (format), which correctly
> > signals 'wrong-number-of-arguments.

> Or rather macroexpand-all:

> (macroexpand-all '(format))
> => (format)

> (macroexpand-all '(funcall))
> => (funcall nil)

Yes, it is the macro expansion causing the problem, thanks.

The following fixes it (or, at least, "fixes" it; I'm not clear exactly
what the pcase--dontcare clause is there for).

diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
index 663856a8fb..eb5af9ece6 100644
--- a/lisp/emacs-lisp/macroexp.el
+++ b/lisp/emacs-lisp/macroexp.el
@@ -441,8 +373,9 @@ macroexp--expand-all
                              (macroexp--all-forms args)
                              form)
 	   (macroexp--expand-all newform))))
-
-      (`(funcall . ,(or `(,exp . ,args) pcase--dontcare))
+      (`(funcall)
+       form)
+      (`(funcall . (,exp . ,args))
        (let ((eexp (macroexp--expand-all exp))
              (eargs (macroexp--all-forms args)))
          ;; Rewrite (funcall #'foo bar) to (foo bar), in case `foo'

> Perhaps Stefan has some insights into this; added to the CCs.

> -- 
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no

-- 
Alan Mackenzie (Nuremberg, Germany).