functionp bug

functionp bug

[Katsumi Yamaoka]

Hi,

This change

2008-04-05  Stefan Monnier  <monnier@iro.umontreal.ca>

	* subr.el (functionp): Return nil for special forms.

makes the following form return nil:

(functionp 'or)

Because of this, the `file' mail source without the :path spec,
i.e. (setq mail-sources '((file))), for Gnus doesn't work as
follows:

(mail-source-value (cadr (assq :path (assq 'file mail-source-keyword-map))))
 => (or (getenv "MAIL")
        (expand-file-name (user-login-name) rmail-spool-directory))

But it should return the eval'd value of this form.

A workaround is to specify the :path argument explicitly in the
`file' mail source.  This bug seems to influence not only it but
also other programs, so I hope it is fixed as soon as possible.

Regards,

Re: functionp bug

[Stefan Monnier]

> 	* subr.el (functionp): Return nil for special forms.

> Because of this, the `file' mail source without the :path spec,
> i.e. (setq mail-sources '((file))), for Gnus doesn't work as
> follows:

> (mail-source-value (cadr (assq :path (assq 'file mail-source-keyword-map))))
>  => (or (getenv "MAIL")
>         (expand-file-name (user-login-name) rmail-spool-directory))

I've installed the patch below which should fix it.


        Stefan


--- mail-source.el.~1.38.~	2008-03-30 10:39:37.000000000 -0400
+++ mail-source.el	2008-04-07 12:02:53.000000000 -0400
@@ -500,8 +500,7 @@
    ((stringp value)
     value)
    ;; Function
-   ((and (listp value)
-	 (functionp (car value)))
+   ((and (listp value) (symbolp (car value)) (fboundp (car value)))
     (eval value))
    ;; Just return the value.
    (t

Re: functionp bug

[Romain Francoise]

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

> I've installed the patch below which should fix it.

> --- mail-source.el.~1.38.~	2008-03-30 10:39:37.000000000 -0400
> +++ mail-source.el	2008-04-07 12:02:53.000000000 -0400
> @@ -500,8 +500,7 @@
>     ((stringp value)
>      value)
>     ;; Function
> -   ((and (listp value)
> -	 (functionp (car value)))
> +   ((and (listp value) (symbolp (car value)) (fboundp (car value)))
>      (eval value))
>     ;; Just return the value.
>     (t

It will fix the problem in mail-source.el but the same issue exists
elsewhere, see for example Tassilo's report:

 http://permalink.gmane.org/gmane.emacs.gnus.general/66701

Re: functionp bug

[Reiner Steib]

On Mon, Apr 07 2008, Stefan Monnier wrote:

>> 	* subr.el (functionp): Return nil for special forms.
[...]
> I've installed the patch below which should fix it.

> --- mail-source.el.~1.38.~	2008-03-30 10:39:37.000000000 -0400
> +++ mail-source.el	2008-04-07 12:02:53.000000000 -0400
[...]
> -   ((and (listp value)
> -	 (functionp (car value)))
> +   ((and (listp value) (symbolp (car value)) (fboundp (car value)))

There are around 70 (functionp somevar) in Gnus.  I'd guess that many
of these expect `functionp' to return non-nil for special forms (or,
if, and, ...).  Maybe it is possible to fix them all, but is it really
the right thing to make such an incompatible change?  What is the
reason for this change?

See <http://thread.gmane.org/loom.20080407T080800-983@post.gmane.org>
for another problem reported today.

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: functionp bug

[Stefan Monnier]

> There are around 70 (functionp somevar) in Gnus.  I'd guess that many
> of these expect `functionp' to return non-nil for special forms (or,
> if, and, ...).

All the places that call `functionp' and expect it to return t for `or',
and `if', have a bug, because they should also accept `when' for which
`functionp' has been returning nil for a loooong time now.

> Maybe it is possible to fix them all, but is it really the right thing
> to make such an incompatible change?  What is the reason for
> this change?

That the strange definition of `functionp' was never what was really
needed: if special-forms should be accepted, than so should macros.


        Stefan

Re: functionp bug

[Stefan Monnier]

>> --- mail-source.el.~1.38.~	2008-03-30 10:39:37.000000000 -0400
>> +++ mail-source.el	2008-04-07 12:02:53.000000000 -0400
>> @@ -500,8 +500,7 @@
>> ((stringp value)
>> value)
>> ;; Function
>> -   ((and (listp value)
>> -	 (functionp (car value)))
>> +   ((and (listp value) (symbolp (car value)) (fboundp (car value)))
>> (eval value))
>> ;; Just return the value.
>> (t

> It will fix the problem in mail-source.el but the same issue exists
> elsewhere, see for example Tassilo's report:

>  http://permalink.gmane.org/gmane.emacs.gnus.general/66701

He must not have up-to-date code, because gnus-win.el does not use
`functionp' any more in the latest Emacs's trunk.


        Stefan

Re: functionp bug

[Manoj Srivastava]

On Mon, 07 Apr 2008 15:26:17 -0400, Stefan Monnier
<monnier@iro.umontreal.ca> said:  

>>> --- mail-source.el.~1.38.~ 2008-03-30 10:39:37.000000000 -0400
>>> +++ mail-source.el 2008-04-07 12:02:53.000000000 -0400
>>> @@ -500,8 +500,7 @@ ((stringp value) value)
>>> ;; Function
>>> - ((and (listp value)
>>> - (functionp (car value)))
>>> + ((and (listp value) (symbolp (car value)) (fboundp (car value)))
>>> (eval value))
>>> ;; Just return the value.
>>> (t

>> It will fix the problem in mail-source.el but the same issue exists
>> elsewhere, see for example Tassilo's report:

>> http://permalink.gmane.org/gmane.emacs.gnus.general/66701

> He must not have up-to-date code, because gnus-win.el does not use
> `functionp' any more in the latest Emacs's trunk.

        I think the latest CVS HEAD in gnus still does. I had a problem
 with my CVS gnus breaking today, and had to back out the CVS gnus.

        manoj

-- 
A little experience often upsets a lot of theory.
Manoj Srivastava <srivasta@acm.org> <http://www.golden-gryphon.com/>  
1024D/BF24424C print 4966 F272 D093 B493 410B  924B 21BA DABB BF24 424C

Re: functionp bug

[Reiner Steib]

On Mon, Apr 07 2008, Manoj Srivastava wrote:

> Stefan Monnier <monnier@iro.umontreal.ca> said:  
>> He must not have up-to-date code, because gnus-win.el does not use
>> `functionp' any more in the latest Emacs's trunk.
>
>         I think the latest CVS HEAD in gnus still does. 

It will merge from Emacs with Miles' next sync.

>  I had a problem with my CVS gnus breaking today, and had to back
>  out the CVS gnus.

In such situations, I'd suggest to remove No Gnus from your load path,
since the code in the bundled Gnus 5.13 is more recent.

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: functionp bug

[Katsumi Yamaoka]

>>>>> Reiner Steib wrote:
>> Maybe it is possible to fix them all, but is it really the right thing
>> to make such an incompatible change?  What is the reason for
>> this change?

>>>>> Stefan Monnier wrote:
> That the strange definition of `functionp' was never what was really
> needed: if special-forms should be accepted, than so should macros.

I don't want `functionp' to accept macros because macros cannot
be funcall'd.  I had been using `functionp' in order to mainly
examine whether the object is a function symbol or a lambda form.
For such a usage, there's no problem with the present definition
if only the function that actually does something, e.g. the one
applicable to `gnus-article-x-face-command', is assumed.

OTHO, Gnus used it even to examine whether the Lisp form in which
the first item might be a special form is able to be eval'd.  `if'
and `or' are not functions?  Yes, it's a sound argument and Gnus'
way might have been a wrong approach even though `functionp' was
easy to use.  However, I'm not sure such an incompatible change
sticking to semantics is really necessary.  While programing
ELisp, to consider that whatever is placed right after the open
parenthesis is a function generally causes no problem, if the
form is assumed to be able to be evaluated, doesn't it?  The use
of `functionp' was handy for such a case.  Neither of examining
whether the object is not a special form and examining whether
the object is a special form will probably be done rarely.

If making the change revert is not acceptable, what we have to
do toward the Gnus 5.10.10 release and the No Gnus v 0.8 release
will be:

1. Merge Stefan's changes:

2008-04-07  Stefan Monnier  <monnier@iro.umontreal.ca>

	* mail-source.el (mail-source-value):
	Prefer fboundp to functionp so it works with macros as well.

2008-04-03  Stefan Monnier  <monnier@iro.umontreal.ca>

	* gnus-win.el (gnus-configure-frame, gnus-all-windows-visible-p):
	Fix last change in case the element is not even a symbol.

2008-03-20  Stefan Monnier  <monnier@iro.umontreal.ca>

	* gnus-win.el (gnus-configure-frame, gnus-all-windows-visible-p):
	Prefer fboundp to functionp so it works with macros as well.

   I'm not sure that's all, though.

2. Post a notice to let people who use the Emacs trunk use this
   workaround:

(or (functionp 'if)
    (defadvice functionp (around workaround-bug (object) activate)
      "Workaround bug."
      (or ad-do-it
	  (setq ad-return-value (and (symbolp object) (fboundp object))))))

   It only conceals the problem, though.

3. Replace all `functionp' with `gmm-functionp' which is the same
   as the one in Emacs 22.2.50.  It only conceals the problem, too.

I vote to 1.

Regards,

Re: functionp bug

[Andreas Schwab]

Katsumi Yamaoka <yamaoka@jpl.org> writes:

> OTHO, Gnus used it even to examine whether the Lisp form in which
> the first item might be a special form is able to be eval'd.  `if'
> and `or' are not functions?

You can eval macro calls as well.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: functionp bug

[Richard Stallman]

I think that change in functionp should be reverted.  Any change in
such things tends to cause problems, so it is better to leave them alone
unless there is an important problem to be fixed.

Theoretical arguments trying to show that "this behavior couldn't
possibly be right for any caller" tend to be unreliable.  Whatever
functionp does, code will have come to depend on it.

Re: functionp bug

[Stefan Monnier]

> I think that change in functionp should be reverted.  Any change in
> such things tends to cause problems, so it is better to leave them alone
> unless there is an important problem to be fixed.

The old definition lead to bugs in *all* uses I've seen.  All the uses
I've seen fall in the following 2 categories:
1 - (if (functionp <foo>) (funcall <foo> ...))
    with the old definition, this signalled an error if <foo> was
    a special form.
2 - the kind of use that is discussed in this thread, where the code
    wants to check "can <foo> be the `car' of an valid Elisp
    expression", where using `functionp' made the test reject uses of
    `when' while it accepted uses of `if'.

> Theoretical arguments trying to show that "this behavior couldn't
> possibly be right for any caller" tend to be unreliable.  Whatever
> functionp does, code will have come to depend on it.

Yes, the change is an incompatible one.  But it will get rid of bugs.


        Stefan

Re: functionp bug

[Stefan Monnier]

> 1. Merge Stefan's changes:

Miles's sync between Gnus's CVS and Emacs's CVS will do that anyway.
Maybe you want to ask Miles to do a round of sync'ing (he seems to have
been a bit busy with other things recently).

>    I'm not sure that's all, though.

Feel free to check all other uses.  There are many, but the check is
fairly easy: if the `functionp' is immediately followed by `funcall',
you can be pretty sure that the code is correct.


        Stefan

Re: functionp bug

[Johan Bockgård]

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

> All the places that call `functionp' and expect it to return t for
> `or', and `if', have a bug, because they should also accept `when' for
> which `functionp' has been returning nil for a loooong time now.

Where "loooong" means "since Emacs 22".

-- 
Johan Bockgård

Re: functionp bug

[Katsumi Yamaoka]

>>>>> Stefan Monnier wrote:
>> 1. Merge Stefan's changes:

> Miles's sync between Gnus's CVS and Emacs's CVS will do that anyway.
> Maybe you want to ask Miles to do a round of sync'ing (he seems to have
> been a bit busy with other things recently).

The release of No Gnus v0.8 and Gnus v5.10.10 is just around the
corner so merging is stopped.  It's why the problems concerning
the functionp change happen here and there.

Re: functionp bug

[Stefan Monnier]

>>> 1. Merge Stefan's changes:

>> Miles's sync between Gnus's CVS and Emacs's CVS will do that anyway.
>> Maybe you want to ask Miles to do a round of sync'ing (he seems to have
>> been a bit busy with other things recently).

> The release of No Gnus v0.8 and Gnus v5.10.10 is just around the
> corner so merging is stopped.  It's why the problems concerning
> the functionp change happen here and there.

Sorry, I missed this part.  Yes, this change should clearly be merged.
Actually, sync'ing shouldn't be stopped, really.  Emacs's version of
Gnus should simply follow the same "bug fix only" policy.


        Stefan

Re: functionp bug

[Katsumi Yamaoka]

>>>>> Stefan Monnier wrote:
>>>> 1. Merge Stefan's changes:

>>> Miles's sync between Gnus's CVS and Emacs's CVS will do that anyway.
>>> Maybe you want to ask Miles to do a round of sync'ing (he seems to have
>>> been a bit busy with other things recently).

>> The release of No Gnus v0.8 and Gnus v5.10.10 is just around the
>> corner so merging is stopped.  It's why the problems concerning
>> the functionp change happen here and there.

> Sorry, I missed this part.  Yes, this change should clearly be merged.

I left it to the release maintainer.  In relation to this, I hope
anyone who changes Gnus in the Emacs trunk subscribe to the ding
list or the gmane.emacs.gnus.general newsgroup.

> Actually, sync'ing shouldn't be stopped, really.  Emacs's version of
> Gnus should simply follow the same "bug fix only" policy.

Please note that all the changes have to be verified also with
Emacs 21 and 22 and XEmacs 21.4 and 21.5.  That's my routine.

Regards,

Re: functionp bug

[Reiner Steib]

On Thu, Apr 10 2008, Katsumi Yamaoka wrote:

>>>>>> Stefan Monnier wrote:
>>>>> 1. Merge Stefan's changes:
>
>>>> Miles's sync between Gnus's CVS and Emacs's CVS will do that anyway.
>>>> Maybe you want to ask Miles to do a round of sync'ing (he seems to have
>>>> been a bit busy with other things recently).
>
>>> The release of No Gnus v0.8 and Gnus v5.10.10 is just around the
>>> corner so merging is stopped.  

It's more coincidence the Miles it too busy to sync.

>>> It's why the problems concerning the functionp change happen here
>>> and there.
>
>> Sorry, I missed this part.  Yes, this change should clearly be merged.
>
> I left it to the release maintainer.  
[ Quoting re-ordered ]
> Please note that all the changes have to be verified also with
> Emacs 21 and 22 and XEmacs 21.4 and 21.5.  That's my routine.

Did anyone verify that Stefan's changes are safe also for Emacs 21/22
and XEmacs 21.4/21.5?

>> Actually, sync'ing shouldn't be stopped, really.  Emacs's version of
>> Gnus should simply follow the same "bug fix only" policy.

As No Gnus 0.8 is just a development snapshot, I don't want to invest
too much time to enforce such a policy.  (And I didn't know when the
release will be done, because I didn't hear from Lars for some weeks.
Maybe he was on holidays.)

As pointed out in <news:v98wzmon51.fsf@marauder.physik.uni-ulm.de> /
<http://thread.gmane.org/gmane.emacs.gnus.general/66553/focus=66746>,
it's not very important to have the Gnus 5.10.10/No Gnus 0.8 work with
Emacs trunk:

- It's important that Gnus 5.10.10 works well with Emacs 21 and XEmacs
  21.4/21.5.  Users of Emacs 22 should use Gnus 5.11 bundled with
  Emacs 22.2.  Users of Emacs 23 (trunk), shouldn't use Gnus 5.10 at
  all.

- It's important that No Gnus 0.8 works well with Emacs 21/22 and
  XEmacs 21.4/21.5.  Users of Emacs 23 (trunk), should use either the
  bundled Gnus 5.13 or Gnus trunk (when Gnus trunk is newer).

> In relation to this, I hope anyone who changes Gnus in the Emacs
> trunk subscribe to the ding list or the gmane.emacs.gnus.general
> newsgroup.

Or at least cc ding@gnus.org when doing non-trivial changes in
lisp/gnus/*.el or changes that are likely to be relevant for Gnus.

Maybe it would have been better to send
<http://thread.gmane.org/v9y786slel.fsf@marauder.physik.uni-ulm.de> to
emacs-devel as well.

Bye, Reiner.

[1] Here's the relevant diff between Emacs trunk Gnus trunk:

--8<---------------cut here---------------start------------->8---
--- emacs/cvs-HEAD/emacs/lisp/gnus/gnus-win.el	2008-04-06 20:15:08.000000000 +0200
+++ plain_No/lisp/gnus-win.el	2008-01-20 06:23:57.000000000 +0100
@@ -317,7 +317,7 @@
     ;; The SPLIT might be something that is to be evaled to
     ;; return a new SPLIT.
     (while (and (not (assq (car split) gnus-window-to-buffer))
-		(symbolp (car split)) (fboundp (car split)))
+		(functionp (car split)))
       (setq split (eval split)))
     (let* ((type (car split))
 	   (subs (cddr split))
@@ -380,7 +380,7 @@
 	  (while subs
 	    (setq sub (append (pop subs) nil))
 	    (while (and (not (assq (car sub) gnus-window-to-buffer))
-			(symbolp (car sub)) (fboundp (car sub)))
+			(functionp (car sub)))
 	      (setq sub (eval sub)))
 	    (when sub
 	      (push sub comp-subs)
@@ -520,7 +520,7 @@
       ;; The SPLIT might be something that is to be evaled to
       ;; return a new SPLIT.
       (while (and (not (assq (car split) gnus-window-to-buffer))
-		  (symbolp (car split)) (fboundp (car split)))
+		  (functionp (car split)))
 	(setq split (eval split)))
 
       (setq type (elt split 0))

-++ emacs/cvs-HEAD/emacs/lisp/gnus/mail-source.el	2008-04-10 12:10:43.000000000 +0200
+-- plain_No/lisp/mail-source.el	2008-03-14 14:43:32.000000000 +0100
@@ -500,7 +500,8 @@
    ((stringp value)
     value)
    ;; Function
-   ((and (listp value) (symbolp (car value)) (fboundp (car value)))
+   ((and (listp value)
+	 (functionp (car value)))
     (eval value))
    ;; Just return the value.
    (t
--8<---------------cut here---------------end--------------->8---
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: functionp bug

[Katsumi Yamaoka]

>>>>> Reiner Steib wrote:
> Did anyone verify that Stefan's changes are safe also for Emacs 21/22
> and XEmacs 21.4/21.5?

I've already verified it in the condition where
`gnus-buffer-configuration' is the default value and the condition
where it has been modified by message-multiple-frames.el. ---[1]
I guess those who customized the window configuration are few.

For the `file' mail source, it surely works without the :path
argument.  It is hard to imagine the customization that makes
any mail source malfunction.

Regards,

[1] ftp://ftp.jpl.org/pub/elisp/

Re: functionp bug

[Reiner Steib]

On Thu, Apr 10 2008, Katsumi Yamaoka wrote:

>>>>>> Reiner Steib wrote:
>> Did anyone verify that Stefan's changes are safe also for Emacs 21/22
>> and XEmacs 21.4/21.5?
>
> I've already verified it in the condition where
> `gnus-buffer-configuration' is the default value and the condition
> where it has been modified by message-multiple-frames.el. ---[1]
> I guess those who customized the window configuration are few.
>
> For the `file' mail source, it surely works without the :path
> argument.  It is hard to imagine the customization that makes
> any mail source malfunction.

Thanks for testing.  If you want to install it in the Gnus trunk now,
I don't object.

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: functionp bug

[Katsumi Yamaoka]

>>>>> Reiner Steib wrote:
> On Thu, Apr 10 2008, Katsumi Yamaoka wrote:
>> I've already verified it in the condition where

[...]

> Thanks for testing.  If you want to install it in the Gnus trunk now,
> I don't object.

Done.  Maybe no one uses Gnus v5.10.10 with the Emacs trunk so
applying the changes also to the v5-10 branch seems to be
unnecessary.

Regards,

Re: functionp bug

[Stefan Monnier]

> it's not very important to have the Gnus 5.10.10/No Gnus 0.8 work with
> Emacs trunk:

Oh, I missed that part: the imminent release is on the "release branch"
(the one synced with EMACS_22_BASE), not on the branch synced with the
Emacs trunk.
Now I understand better.

>> In relation to this, I hope anyone who changes Gnus in the Emacs
>> trunk subscribe to the ding list or the gmane.emacs.gnus.general
>> newsgroup.

I do not have time to read this.


        Stefan

Re: functionp bug

[Reiner Steib]

On Thu, Apr 10 2008, Stefan Monnier wrote:

>> it's not very important to have the Gnus 5.10.10/No Gnus 0.8 work with
>> Emacs trunk:
>
> Oh, I missed that part: the imminent release is on the "release branch"
> (the one synced with EMACS_22_BASE), not on the branch synced with the
> Emacs trunk.
> Now I understand better.

Not exactly, we will release both:

- A new stable version "Gnus 5.10.10" (from the v5-10 branch,
  corresponding to EMACS_22_BASE).

- A new development (snapshot) release "No Gnus 0.8" (from the Gnus
  trunk, corresponding to Emacs trunk).

But as mentioned previously, Emacs 23 users are not supposed to use
these releases, so WRT the releases, there's no problem.

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: functionp bug

[Stefan Monnier]

>>> it's not very important to have the Gnus 5.10.10/No Gnus 0.8 work with
>>> Emacs trunk:
>> 
>> Oh, I missed that part: the imminent release is on the "release branch"
>> (the one synced with EMACS_22_BASE), not on the branch synced with the
>> Emacs trunk.
>> Now I understand better.

> Not exactly, we will release both:

> - A new stable version "Gnus 5.10.10" (from the v5-10 branch,
>   corresponding to EMACS_22_BASE).

> - A new development (snapshot) release "No Gnus 0.8" (from the Gnus
>   trunk, corresponding to Emacs trunk).

> But as mentioned previously, Emacs 23 users are not supposed to use
> these releases, so WRT the releases, there's no problem.

OK, so we still have to be careful to only install bug-fix patches into
Emacs's trunk's gnus because it's getting ready for a release.


        Stefan

Re: functionp bug

[Reiner Steib]

On Thu, Apr 10 2008, Stefan Monnier wrote:

>> - A new stable version "Gnus 5.10.10" (from the v5-10 branch,
>>   corresponding to EMACS_22_BASE).
>
>> - A new development (snapshot) release "No Gnus 0.8" (from the Gnus
>>   trunk, corresponding to Emacs trunk).
>
>> But as mentioned previously, Emacs 23 users are not supposed to use
>> these releases, so WRT the releases, there's no problem.
>
> OK, so we still have to be careful to only install bug-fix patches into
> Emacs's trunk's gnus because it's getting ready for a release.

I have already prepared the tar-balls for both releases.  Unless
something unexpected happens, Lars will put them on gnus.org and sent
out the announcements on Friday morning (CEST).

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/