bogus change(s) in cl-macs.el

bogus change(s) in cl-macs.el

[Katsumi Yamaoka]

Hello,

There are many changes made in the version 1.46 of cl-macs.el.

revision 1.46
date: 2004/11/16 04:05:29;  author: monnier;  state: Exp;  lines: +219 -225
Use make-symbol rather than gensym.
(loop, cl-parse-loop-clause, defsetf): Use backquote.

First of all, I protest strongly that such a big change has not
been recorded on the ChangeLog file.  Second, the change at least
to the `labels' macro breaks emacs-w3m.  Emacs-w3m uses some cl
macros including `labels' in order to work together with the
external w3m command asynchronously.  The patch below should be
applied.  Third, I'm not sure whether a problem is limited only
to it.

*** cl-macs.el~	Tue Nov 16 21:54:14 2004
--- cl-macs.el	Fri Nov 19 01:30:39 2004
***************
*** 1314,1320 ****
  \(fn ((FUNC ARGLIST BODY...) ...) FORM...)"
    (let ((vars nil) (sets nil) (cl-macro-environment cl-macro-environment))
      (while bindings
!       (let ((var (make-symbol "--cl-var--")))
  	(push var vars)
  	(push (list 'function* (cons 'lambda (cdar bindings))) sets)
  	(push var sets)
--- 1314,1320 ----
  \(fn ((FUNC ARGLIST BODY...) ...) FORM...)"
    (let ((vars nil) (sets nil) (cl-macro-environment cl-macro-environment))
      (while bindings
!       (let ((var (gensym)))
  	(push var vars)
  	(push (list 'function* (cons 'lambda (cdar bindings))) sets)
  	(push var sets)

Regards,

Re: bogus change(s) in cl-macs.el

[Luc Teirlinck]

Katsumi Yamaoka wrote:

   First of all, I protest strongly that such a big change has not
   been recorded on the ChangeLog file.  Second, the change at least
   to the `labels' macro breaks emacs-w3m.  Emacs-w3m uses some cl
   macros including `labels' in order to work together with the
   external w3m command asynchronously.  The patch below should be
   applied.  Third, I'm not sure whether a problem is limited only
   to it.

It was not me who installed that change to cl-macs, but I believe that
it was made in response to problems with compiler warnings I reported.
I myself suggested the following alternate type solution, which I
believe would not have broken anything:

!       (let ((var (with-no-warnings (gensym))))

but after I saw Stefan's patch, I did not pursue (and did not double
check) this.  I could still do it given that Stefan's solution seems
to give problems.  It would eliminate the compiler warnings with no
side effects.  I guess that this should be done everywhere since if
the change breaks things in one place, it probably does in other
places too.  I could post a patch for review, if desired.

I do not immediately understand why Stefan's patch breaks things.  It
fails to adjoin a unique identifying number, like gensym does, but I
do not understand why that is necessary.  Distinct uninterned symbols
with the same name are still distinct.  I guess that if I knew the cl
package better, I would understand.

Sincerely,

Luc.

Re: bogus change(s) in cl-macs.el

[Miles Bader]

> I myself suggested the following alternate type solution, which I
> believe would not have broken anything:
> 
> !       (let ((var (with-no-warnings (gensym))))

As well as being unclean, that doesn't solve the problem of gensym
being undefined at runtime.

I think Stefan's change is correct.

-Miles

Re: bogus change(s) in cl-macs.el

[Luc Teirlinck]

   > I myself suggested the following alternate type solution, which I
   > believe would not have broken anything:
   > 
   > !       (let ((var (with-no-warnings (gensym))))

   As well as being unclean, that doesn't solve the problem of gensym
   being undefined at runtime.

`gensym' _only_ needs to be defined at runtime when gensym is going to
be _called_ at runtime.  I said that I still had to double check my
alternative patch should it be needed.  I did check in the `winner'
case that `gensym' was _not_ called at runtime.  I would strongly
guess that this is the case with _all_ these calls to `gensym'.
Clearly, I would have to _check_ that this is really the case.

All of this only matters if there are real problems with Stefan's patch.

Sincerely,

Luc.

Re: bogus change(s) in cl-macs.el

[Miles Bader]

> Second, the change at least
> to the `labels' macro breaks emacs-w3m.  Emacs-w3m uses some cl
> macros including `labels' in order to work together with the
> external w3m command asynchronously. 

Please describe this "breakage" in greater detail.

If you're outputing the result of macro-expansions to a file and
expecting to read them in later, you almost certainly should bind
`print-gensym' to t when doing so; otherwise the result is quite
likely to be wrong.

-Miles

Re: bogus change(s) in cl-macs.el

[Katsumi Yamaoka]

>>>>> In [emacs-w3m : No.07180] Miles Bader wrote:

>> Second, the change at least
>> to the `labels' macro breaks emacs-w3m.  Emacs-w3m uses some cl
>> macros including `labels' in order to work together with the
>> external w3m command asynchronously. 

> Please describe this "breakage" in greater detail.

>From yesterday I'm looking for a simpler way rather than to use
emacs-w3m to reproduce the bug.  However, it has not been
achieved so far.  I'll post it when I found it.

[emacs-w3m:07185] Re: bogus change(s) in cl-macs.el

[Katsumi Yamaoka]

>>>>>> In [emacs-w3m : No.07180] Miles Bader wrote:

>> Please describe this "breakage" in greater detail.

>>>>> In [emacs-w3m : No.07182] Katsumi Yamaoka wrote:

> From yesterday I'm looking for a simpler way rather than to use
> emacs-w3m to reproduce the bug.  However, it has not been
> achieved so far.  I'll post it when I found it.

I found the simplest way to explain the bug:

(macroexpand
 '(labels ((FOO nil FOO-BODY)
	   (BAR nil BAR-BODY))
    (FOO)
    (BAR)))

(let ((--cl---cl-var---- nil)
      (--cl---cl-var---- nil))
  (progn
    (progn
      (set '--cl---cl-var---- #'(lambda nil BAR-BODY))
      (set '--cl---cl-var---- #'(lambda nil FOO-BODY)))
    (funcall (symbol-value '--cl---cl-var----))
    (funcall (symbol-value '--cl---cl-var----))))

One of two functions FOO and BAR is disregarded as you see.
When the patch I posted is applied, it will be corrected as
follows:

(let ((--cl-G79813-- nil)
      (--cl-G79812-- nil))
  (progn
    (progn
      (set '--cl-G79813-- #'(lambda nil BAR-BODY))
      (set '--cl-G79812-- #'(lambda nil FOO-BODY)))
    (funcall (symbol-value '--cl-G79812--))
    (funcall (symbol-value '--cl-G79813--))))

Re: bogus change(s) in cl-macs.el

[Stefan Monnier]

> I found the simplest way to explain the bug:

> (macroexpand
>  '(labels ((FOO nil FOO-BODY)
> 	   (BAR nil BAR-BODY))
>     (FOO)
>     (BAR)))

> (let ((--cl---cl-var---- nil)
>       (--cl---cl-var---- nil))
>   (progn
>     (progn
>       (set '--cl---cl-var---- #'(lambda nil BAR-BODY))
>       (set '--cl---cl-var---- #'(lambda nil FOO-BODY)))
>     (funcall (symbol-value '--cl---cl-var----))
>     (funcall (symbol-value '--cl---cl-var----))))

> One of two functions FOO and BAR is disregarded as you see.

Well, actually we can't see it here because you haven't used print-gensym
to distinguish symbols with the same name.  But indeed:

  (labels ((foo nil 1) (bar nil 2)) (cons (foo) (bar)))

returns (1 . 1).  This is because cl's macro environment uses symbol names
rather than symbols for symbol-macros, so if two symbols have the same name
(i.e. `eq', not just `equal') and they both are symbol-macros, then
CL screws up.  I'm working on a patch for that.


        Stefan


PS: A symbol-macro is a macro `foo' that is not triggered by (foo bla bla)
but by a mere `foo'.
E.g. (symbol-macrolet ((x (hello))) (+ x 1)) => (+ (hello 1))

Re: bogus change(s) in cl-macs.el

[Richard Stallman]

    > (macroexpand
    >  '(labels ((FOO nil FOO-BODY)
    > 	   (BAR nil BAR-BODY))
    >     (FOO)
    >     (BAR)))

    > (let ((--cl---cl-var---- nil)
    >       (--cl---cl-var---- nil))
    >   (progn
    >     (progn
    >       (set '--cl---cl-var---- #'(lambda nil BAR-BODY))
    >       (set '--cl---cl-var---- #'(lambda nil FOO-BODY)))
    >     (funcall (symbol-value '--cl---cl-var----))
    >     (funcall (symbol-value '--cl---cl-var----))))

    > One of two functions FOO and BAR is disregarded as you see.

    Well, actually we can't see it here because you haven't used print-gensym
    to distinguish symbols with the same name.

Using print-gensym is the right way to print such code, but there is
some value in producing code that will run correctly (in most cases)
even if it is printed and read without using print-gensym.  Sure, if
you print it that way, there's a danger that a gensym might share the
name of some other symbol in the program, and cause incorrect
shadowing; but that will essentially never happen in practice.  It's
not an improvement when something that nearly always worked now nearly
always fails in an obscure way.

So I think it is better to continue using something that produces
different names, as gensym does.  It could be defined all the time in
Emacs, if that avoids some problems.

Re: bogus change(s) in cl-macs.el

[Katsumi Yamaoka]

>>>>> In [emacs-w3m : No.07189] Stefan Monnier wrote:

> Well, actually we can't see it here because you haven't used print-gensym
> to distinguish symbols with the same name.  But indeed:

>   (labels ((foo nil 1) (bar nil 2)) (cons (foo) (bar)))

> returns (1 . 1).  This is because cl's macro environment uses symbol names
> rather than symbols for symbol-macros, so if two symbols have the same name
> (i.e. `eq', not just `equal') and they both are symbol-macros, then
> CL screws up.  I'm working on a patch for that.

Excuse me for being so rude, but didn't you forget to commit the fix?

Re: bogus change(s) in cl-macs.el

[Stefan Monnier]

>> Well, actually we can't see it here because you haven't used print-gensym
>> to distinguish symbols with the same name.  But indeed:

>> (labels ((foo nil 1) (bar nil 2)) (cons (foo) (bar)))

>> returns (1 . 1).  This is because cl's macro environment uses symbol names
>> rather than symbols for symbol-macros, so if two symbols have the same name
>> (i.e. `eq', not just `equal') and they both are symbol-macros, then
>> CL screws up.  I'm working on a patch for that.

> Excuse me for being so rude, but didn't you forget to commit the fix?

Sorry, I haven't had time to do it yet,


        Stefan

Re: bogus change(s) in cl-macs.el

[Katsumi Yamaoka]

Hi all,

I got tired of waiting and made a substitution for the `labels'
macro in the emacs-w3m CVS trunk.  It will help you who are
being troubled with that bug.

>> Excuse me for being so rude, but didn't you forget to commit the fix?

>>>>> In [emacs-w3m : No.07213] Stefan Monnier wrote:

> Sorry, I haven't had time to do it yet,

Re: bogus change(s) in cl-macs.el

[Stefan Monnier]

> Well, actually we can't see it here because you haven't used print-gensym
> to distinguish symbols with the same name.  But indeed:

>   (labels ((foo nil 1) (bar nil 2)) (cons (foo) (bar)))

> returns (1 . 1).  This is because cl's macro environment uses symbol names
> rather than symbols for symbol-macros, so if two symbols have the same name
> (i.e. `eq', not just `equal') and they both are symbol-macros, then
> CL screws up.  I'm working on a patch for that.

OK, I've committed basically your fix (except it also needed to be applied
at one other place, and I added some comment explaining why we use `gensym'
rather than `make-symbol' there).


        Stefan

Re: bogus change(s) in cl-macs.el

[Stefan Monnier]

> First of all, I protest strongly that such a big change has not
> been recorded on the ChangeLog file.

The ChangeLog is somewhere, but as usual I forgot to commit it at the time
and now I'm working on another computer.  But trust me, it's not lost.

> *** cl-macs.el~	Tue Nov 16 21:54:14 2004
> --- cl-macs.el	Fri Nov 19 01:30:39 2004
> ***************
> *** 1314,1320 ****
>   \(fn ((FUNC ARGLIST BODY...) ...) FORM...)"
>     (let ((vars nil) (sets nil) (cl-macro-environment cl-macro-environment))
>       (while bindings
> !       (let ((var (make-symbol "--cl-var--")))
>   	(push var vars)
>   	(push (list 'function* (cons 'lambda (cdar bindings))) sets)
>   	(push var sets)
> --- 1314,1320 ----
>   \(fn ((FUNC ARGLIST BODY...) ...) FORM...)"
>     (let ((vars nil) (sets nil) (cl-macro-environment cl-macro-environment))
>       (while bindings
> !       (let ((var (gensym)))
>   	(push var vars)
>   	(push (list 'function* (cons 'lambda (cdar bindings))) sets)
>   	(push var sets)

It's odd that such a change would fix/break some code.
After all `gensym' is more or less doing (make-symbol (concat "G"
(random))), so other than the symbol's name, nothing is changed.
And since the symbol is uninterned, its name is mostly irrelevant, unless
some code later on uses `symbol-name' or does a print+read without using
print-gensym.

Maybe we'll indeed have to revert to gensym here, but I'd like to first
better understand the problem.

Could you give us a recipe to reproduce the problem, and a backtrace
if applicable.  Of course, ideally the recipe should not use emacs-w3m,
but as a first step it would still help to see a recipe and a description of
the resulting problem.


        Stefan