bug#3467: 23.0.94; let + make-local-variable => let value made global

bug#3467: 23.0.94; let + make-local-variable => let value made global

[Lennart Borgman]

If you eval these lines

  (defvar w14 "global")
  (defvar w15 "global")
  (let ((w14 "let")
        (w15 "let"))
    (set (make-local-variable 'w14) "local")
    (message "w14 maybe let: in buffer=%S, global=%S" w14 (default-value 'w14))
    (message "w15 maybe let: in buffer=%S, global=%S" w15 (default-value 'w15)))
  (message "w14 top level: in buffer=%S, global=%S" w14 (default-value 'w14))
  (message "w15 top level: in buffer=%S, global=%S" w15 (default-value 'w15))

the output will be

  w14 maybe let: in buffer="local", global="let"
  w15 maybe let: in buffer="let", global="let"
  w14 top level: in buffer="global", global="let"
  w15 top level: in buffer="global", global="global"

All values here except w14 global value on next last line are arguably
correct. The last value of w14 should be "global", not "let".

It looks like perhaps the call to (make-local-variable w14) does not
mark the "global let" value of w14 as let bound (or removes that
mark).



In GNU Emacs 23.0.94.1 (i386-mingw-nt5.1.2600)
 of 2009-05-23
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (3.4) --no-opt --cflags
-Ic:/g/include -fno-crossjumping'

bug#3467: 23.0.94; let + make-local-variable => let value made global

[Lennart Borgman]

On Thu, Jun 4, 2009 at 11:14 PM, Lennart
Borgman<lennart.borgman@gmail.com> wrote:
> If you eval these lines
>
>  (defvar w14 "global")
>  (defvar w15 "global")
>  (let ((w14 "let")
>        (w15 "let"))
>    (set (make-local-variable 'w14) "local")
>    (message "w14 maybe let: in buffer=%S, global=%S" w14 (default-value 'w14))
>    (message "w15 maybe let: in buffer=%S, global=%S" w15 (default-value 'w15)))
>  (message "w14 top level: in buffer=%S, global=%S" w14 (default-value 'w14))
>  (message "w15 top level: in buffer=%S, global=%S" w15 (default-value 'w15))
>
> the output will be
>
>  w14 maybe let: in buffer="local", global="let"
>  w15 maybe let: in buffer="let", global="let"
>  w14 top level: in buffer="global", global="let"
>  w15 top level: in buffer="global", global="global"
>
> All values here except w14 global value on next last line are arguably
> correct. The last value of w14 should be "global", not "let".
>
> It looks like perhaps the call to (make-local-variable w14) does not
> mark the "global let" value of w14 as let bound (or removes that
> mark).


I think the semantics of let, make-variable-buffer-local and buffer
local variables must be examined a bit to clarify this:

I think let should preserve "buffer localness" otherwise it might do
something quite different from what a user probably expect.

Fortunately this as far as I can see requires only a change in Flet.
After Fprogn and before calling unbound_to the variable values must be
reset to those matching the buffer and frame localness before let.

spec_bind has saved the buffer and frame localness just before Fprogn
so I guess one can compare with what is saved there.


However no one would want me to write the C code for this... ;-) -
Someone who is better than me at that should preferrably do it.


(BTW: gmail does not automatically enter the reply address when I make
a reply to the original bug mail. Is that a bug in gmail?)

bug#3467: 23.0.94; let + make-local-variable => let value made global

[Stefan Monnier]

> If you eval these lines
>   (defvar w14 "global")
>   (defvar w15 "global")
>   (let ((w14 "let")
>         (w15 "let"))
>     (set (make-local-variable 'w14) "local")
>     (message "w14 maybe let: in buffer=%S, global=%S" w14 (default-value 'w14))
>     (message "w15 maybe let: in buffer=%S, global=%S" w15 (default-value 'w15)))
>   (message "w14 top level: in buffer=%S, global=%S" w14 (default-value 'w14))
>   (message "w15 top level: in buffer=%S, global=%S" w15 (default-value 'w15))

> the output will be

>   w14 maybe let: in buffer="local", global="let"
>   w15 maybe let: in buffer="let", global="let"
>   w14 top level: in buffer="global", global="let"
>   w15 top level: in buffer="global", global="global"

> All values here except w14 global value on next last line are arguably
> correct. The last value of w14 should be "global", not "let".

> It looks like perhaps the call to (make-local-variable w14) does not
> mark the "global let" value of w14 as let bound (or removes that
> mark).

Given the way let-binding and buffer-local bindings are currently
implemented, it's difficult to make it work correctly in all corner
cases, and even more so without slowing down the common case.
So don't hold your breath.


        Stefan

bug#3467: 23.0.94; let + make-local-variable => let value made global

[Lennart Borgman]

On Fri, Jun 5, 2009 at 4:36 PM, Stefan Monnier<monnier@iro.umontreal.ca> wrote:
>> If you eval these lines
>>   (defvar w14 "global")
>>   (defvar w15 "global")
>>   (let ((w14 "let")
>>         (w15 "let"))
>>     (set (make-local-variable 'w14) "local")
>>     (message "w14 maybe let: in buffer=%S, global=%S" w14 (default-value 'w14))
>>     (message "w15 maybe let: in buffer=%S, global=%S" w15 (default-value 'w15)))
>>   (message "w14 top level: in buffer=%S, global=%S" w14 (default-value 'w14))
>>   (message "w15 top level: in buffer=%S, global=%S" w15 (default-value 'w15))
>
>> the output will be
>
>>   w14 maybe let: in buffer="local", global="let"
>>   w15 maybe let: in buffer="let", global="let"
>>   w14 top level: in buffer="global", global="let"
>>   w15 top level: in buffer="global", global="global"
>
>> All values here except w14 global value on next last line are arguably
>> correct. The last value of w14 should be "global", not "let".
>
>> It looks like perhaps the call to (make-local-variable w14) does not
>> mark the "global let" value of w14 as let bound (or removes that
>> mark).
>
> Given the way let-binding and buffer-local bindings are currently
> implemented, it's difficult to make it work correctly in all corner
> cases, and even more so without slowing down the common case.
> So don't hold your breath.

I gave a suggestion in the next message for how to implement this:
Check buffer and frame localness before unbind_to in Flet. Would that
really be expensive?

bug#3467: 23.0.94; let + make-local-variable => let value made global

[Lennart Borgman]

On Fri, Jun 5, 2009 at 5:04 PM, Lennart
Borgman<lennart.borgman@gmail.com> wrote:
> On Fri, Jun 5, 2009 at 4:36 PM, Stefan Monnier<monnier@iro.umontreal.ca> wrote:
>>> If you eval these lines
>>>   (defvar w14 "global")
>>>   (defvar w15 "global")
>>>   (let ((w14 "let")
>>>         (w15 "let"))
>>>     (set (make-local-variable 'w14) "local")
>>>     (message "w14 maybe let: in buffer=%S, global=%S" w14 (default-value 'w14))
>>>     (message "w15 maybe let: in buffer=%S, global=%S" w15 (default-value 'w15)))
>>>   (message "w14 top level: in buffer=%S, global=%S" w14 (default-value 'w14))
>>>   (message "w15 top level: in buffer=%S, global=%S" w15 (default-value 'w15))
>>
>>> the output will be
>>
>>>   w14 maybe let: in buffer="local", global="let"
>>>   w15 maybe let: in buffer="let", global="let"
>>>   w14 top level: in buffer="global", global="let"
>>>   w15 top level: in buffer="global", global="global"
>>
>>> All values here except w14 global value on next last line are arguably
>>> correct. The last value of w14 should be "global", not "let".
>>
>>> It looks like perhaps the call to (make-local-variable w14) does not
>>> mark the "global let" value of w14 as let bound (or removes that
>>> mark).
>>
>> Given the way let-binding and buffer-local bindings are currently
>> implemented, it's difficult to make it work correctly in all corner
>> cases, and even more so without slowing down the common case.
>> So don't hold your breath.
>
> I gave a suggestion in the next message for how to implement this:
> Check buffer and frame localness before unbind_to in Flet. Would that
> really be expensive?


Sigh, and my suggestion was of course unnecessary stupid. What is
needed is of course to record values and frame+buffer localness and
dito values and reset them. Nothing less than this will ever work
correctly, or?

And does not this apply to all uses of specbind + unbind_to?

Can it be sufficient to just change specbind and unbind_to? Is there
anything else that will be affected by changes in the specbind stack?
Since info about buffer+frame is alwas needed should specbinding be
changed to the below form?

struct specbinding
  {
    Lisp_Object symbol;
    Lisp_Object old_value;
    Lisp_Object old_buffer_value;
    Lisp_Object old_frame_value;
    specbinding_func func;
    Lisp_Object unused;		/* Dividing by 16 is faster than by 12 */
    Lisp_Object unused;
    Lisp_Object unused;
  };

Is this structure used by other functions than specbind and unbind_to?

bug#3467: 23.0.94; let + make-local-variable => let value made global

[Lennart Borgman]

On Sat, Jun 6, 2009 at 12:48 AM, Lennart
Borgman<lennart.borgman@gmail.com> wrote:
>
> Sigh, and my suggestion was of course unnecessary stupid. What is
> needed is of course to record values and frame+buffer localness and
> dito values and reset them. Nothing less than this will ever work
> correctly, or?
>
> And does not this apply to all uses of specbind + unbind_to?
>
> Can it be sufficient to just change specbind and unbind_to? Is there
> anything else that will be affected by changes in the specbind stack?
> Since info about buffer+frame is alwas needed should specbinding be
> changed to the below form?
>
> struct specbinding
>  {
>    Lisp_Object symbol;
>    Lisp_Object old_value;
>    Lisp_Object old_buffer_value;
>    Lisp_Object old_frame_value;
>    specbinding_func func;
>    Lisp_Object unused;         /* Dividing by 16 is faster than by 12 */
>    Lisp_Object unused;
>    Lisp_Object unused;
>  };
>
> Is this structure used by other functions than specbind and unbind_to?

... and defvar ... but I do not think there are any more uses of
specbinding's structure. Or does some allocation need to change?

bug#3467: 23.0.94; let + make-local-variable => let value made global

[Lennart Borgman]

On Sat, Jun 6, 2009 at 1:08 AM, Lennart
Borgman<lennart.borgman@gmail.com> wrote:
>
> ... and defvar ... but I do not think there are any more uses of
> specbinding's structure. Or does some allocation need to change?

... and let_shadows_buffer_binding_p  which is a bit lonely without
its sister let_shadows_frame_binding_p (or is such a sister not of any
importance? I do not really know about frame local variables, but
specbind seems to know) ...

bug#3467: 23.0.94; let + make-local-variable => let value made global

[Richard Stallman]

I think this is a use case that should be avoided.
Maybe it can be made to work "right", but if that's difficult,
I'd rather tell people not to do it.

bug#3467: 23.0.94; let + make-local-variable => let value made global

[Lennart Borgman]

[-- Attachment #1: Type: text/plain, Size: 1555 bytes --]

On Sat, Jun 6, 2009 at 12:48 AM, Lennart
Borgman<lennart.borgman@gmail.com> wrote:
>
> Sigh, and my suggestion was of course unnecessary stupid. What is
> needed is of course to record values and frame+buffer localness and
> dito values and reset them. Nothing less than this will ever work
> correctly, or?
>
> And does not this apply to all uses of specbind + unbind_to?
>
> Can it be sufficient to just change specbind and unbind_to? Is there
> anything else that will be affected by changes in the specbind stack?
> Since info about buffer+frame is alwas needed should specbinding be
> changed to the below form?
>
> struct specbinding
>  {
>    Lisp_Object symbol;
>    Lisp_Object old_value;
>    Lisp_Object old_buffer_value;
>    Lisp_Object old_frame_value;
>    specbinding_func func;
>    Lisp_Object unused;         /* Dividing by 16 is faster than by 12 */
>    Lisp_Object unused;
>    Lisp_Object unused;
>  };

Here is a bit modified suggestion and some code where I try to
implement it. Please notice that I have not tested the code. It is
very possible that I have misunderstood something so please look at
the code.

struct specbinding
  {
    Lisp_Object symbol;
    Lisp_Object old_value;
    specbinding_func func;
    // total 8 fields
    Lisp_Object need_test_local;
    Lisp_Object old_buffer;
    Lisp_Object old_buffer_value;
    Lisp_Object old_frame;
    Lisp_Object old_frame_value;
    //Lisp_Object unused;		/* Dividing by 16 is faster than by 12 */
  };

[-- Attachment #2: eval.diff --]
[-- Type: application/octet-stream, Size: 7463 bytes --]

*** eval.c	2009-06-06 22:06:33.359375000 +0200
--- eval-orig.c	2009-03-27 09:21:26.453125000 +0100
***************
*** 3298,3304 ****
        specpdl_ptr->symbol = symbol;
        specpdl_ptr->old_value = valcontents;
        specpdl_ptr->func = NULL;
-       //specpdl_ptr->need_test_local = Qnil; // no need to set this
        ++specpdl_ptr;
        SET_SYMBOL_VALUE (symbol, value);
      }
--- 3298,3303 ----
***************
*** 3306,3317 ****
      {
        Lisp_Object ovalue = find_symbol_value (symbol);
        specpdl_ptr->func = 0;
!       //specpdl_ptr->old_value = ovalue;
!       /* Fix-me: Store symbol global value in old_value */
!       specpdl_ptr->old_value = do_symval_forwarding (valcontents);
!       specpdl_ptr->need_test_local = Qt;
!       specpdl_ptr->old_buffer= Qunbound;
!       specpdl_ptr->old_frame= Qunbound;
  
        valcontents = XSYMBOL (symbol)->value;
  
--- 3305,3311 ----
      {
        Lisp_Object ovalue = find_symbol_value (symbol);
        specpdl_ptr->func = 0;
!       specpdl_ptr->old_value = ovalue;
  
        valcontents = XSYMBOL (symbol)->value;
  
***************
*** 3324,3349 ****
  
  	  /* For a local variable, record both the symbol and which
  	     buffer's or frame's value we are saving.  */
!           /* Fix-me: This only saves the current used local binding,
!              not both as it should */
! 	  if (!NILP (Flocal_variable_p (symbol, Qnil))) {
  	    where = current_buffer;
!             specpdl_ptr->old_buffer = where;
!             specpdl_ptr->old_buffer_value = ovalue;
!           } else if (BUFFER_LOCAL_VALUEP (valcontents)
!                      && XBUFFER_LOCAL_VALUE (valcontents)->found_for_frame) {
  	    where = XBUFFER_LOCAL_VALUE (valcontents)->frame;
!             specpdl_ptr->old_frame = where;
!             specpdl_ptr->old_frame_value = ovalue;
! 	  } else {
  	    where = Qnil;
-           }
  
  	  /* We're not using the `unused' slot in the specbinding
  	     structure because this would mean we have to do more
  	     work for simple variables.  */
! 	  //specpdl_ptr->symbol = Fcons (symbol, Fcons (where, current_buffer));
!           specpdl_ptr->symbol = symbol;
  
  	  /* If SYMBOL is a per-buffer variable which doesn't have a
  	     buffer-local value here, make the `let' change the global
--- 3318,3335 ----
  
  	  /* For a local variable, record both the symbol and which
  	     buffer's or frame's value we are saving.  */
! 	  if (!NILP (Flocal_variable_p (symbol, Qnil)))
  	    where = current_buffer;
! 	  else if (BUFFER_LOCAL_VALUEP (valcontents)
! 		   && XBUFFER_LOCAL_VALUE (valcontents)->found_for_frame)
  	    where = XBUFFER_LOCAL_VALUE (valcontents)->frame;
! 	  else
  	    where = Qnil;
  
  	  /* We're not using the `unused' slot in the specbinding
  	     structure because this would mean we have to do more
  	     work for simple variables.  */
! 	  specpdl_ptr->symbol = Fcons (symbol, Fcons (where, current_buffer));
  
  	  /* If SYMBOL is a per-buffer variable which doesn't have a
  	     buffer-local value here, make the `let' change the global
***************
*** 3419,3479 ****
  	 binding.  WHERE nil means that the variable had the default
  	 value when it was bound.  CURRENT-BUFFER is the buffer that
           was current when the variable was bound.  */
!       /* else if (CONSP (this_binding.symbol)) */
!       /*   { */
!       /*     Lisp_Object symbol, where; */
! 
!       /*     symbol = XCAR (this_binding.symbol); */
!       /*     where = XCAR (XCDR (this_binding.symbol)); */
! 
!       /*     if (NILP (where)) */
!       /*       Fset_default (symbol, this_binding.old_value); */
!       /*     else if (BUFFERP (where)) */
!       /*       set_internal (symbol, this_binding.old_value, XBUFFER (where), 1); */
!       /*     else */
!       /*       set_internal (symbol, this_binding.old_value, NULL, 1); */
!       /*   } */
!       /* else */
!       /*   { */
!       /*     /\* If variable has a trivial value (no forwarding), we can */
!       /*        just set it.  No need to check for constant symbols here, */
!       /*        since that was already done by specbind.  *\/ */
!       /*     if (!MISCP (SYMBOL_VALUE (this_binding.symbol))) */
!       /*       SET_SYMBOL_VALUE (this_binding.symbol, this_binding.old_value); */
!       /*     else */
!       /*       set_internal (this_binding.symbol, this_binding.old_value, 0, 1); */
!       /*   } */
!       else {
!         /* Fix-me: Before restoring anything the buffer/frame
!            localness must be checked.  I am not sure how to do that,
!            but see below for my attempts.
!         */
!         /* Set global value */
          if (!MISCP (SYMBOL_VALUE (this_binding.symbol)))
            SET_SYMBOL_VALUE (this_binding.symbol, this_binding.old_value);
          else
!           set_default (this_binding.symbol, this_binding.old_value, 0, 1);
! 
!         {
!           register Lisp_Object valcontents = SYMBOL_VALUE (symbol);
!           int is_local_now = BUFFER_LOCAL_VALUEP (valcontents) || BUFFER_OBJFWDP (valcontents);
!           int is_buffer_local_now = is_local_now && Flocal_variable_p (this_binding.symbol, Qnil);
!           int is_frame_local_now = is_local_now && !is_buffer_local_now;
!           int was_buffer_local_before = this_binding.old_buffer /= Qunbound;
!           int was_local_before = (was_buffer_local_before ||
!                                   this_binding.old_frame /= Qunbound);
!           int was_frame_local_before = (was_local_before && ! was_buffer_local_before);
!           /* Fix me: restore localness */
!           if (was_buffer_local_before_specbind) {
!             if (!is_buffer_local_now) Fmake_local_variable (this_binding.symbol);
!             /* Set buffer local value */
!             set_internal (symbol, this_binding.old_buffer_value, XBUFFER (this_binding.old_buffer), 1);
!           } else if (was_frame_local_before_specbind) {
!             if (!is_frame_local_now) Fmake_variable_frame_local (this_binding.symbol);
!             /* Set frame local value */
!             set_internal (symbol, this_binding.old_frame_value, NULL, 1);
!           }
!         }
        }
      }
  
--- 3405,3433 ----
  	 binding.  WHERE nil means that the variable had the default
  	 value when it was bound.  CURRENT-BUFFER is the buffer that
           was current when the variable was bound.  */
!       else if (CONSP (this_binding.symbol))
! 	{
! 	  Lisp_Object symbol, where;
! 
! 	  symbol = XCAR (this_binding.symbol);
! 	  where = XCAR (XCDR (this_binding.symbol));
! 
! 	  if (NILP (where))
! 	    Fset_default (symbol, this_binding.old_value);
! 	  else if (BUFFERP (where))
! 	    set_internal (symbol, this_binding.old_value, XBUFFER (where), 1);
! 	  else
! 	    set_internal (symbol, this_binding.old_value, NULL, 1);
! 	}
!       else
! 	{
! 	  /* If variable has a trivial value (no forwarding), we can
! 	     just set it.  No need to check for constant symbols here,
! 	     since that was already done by specbind.  */
  	  if (!MISCP (SYMBOL_VALUE (this_binding.symbol)))
  	    SET_SYMBOL_VALUE (this_binding.symbol, this_binding.old_value);
  	  else
! 	    set_internal (this_binding.symbol, this_binding.old_value, 0, 1);
  	}
      }
  

bug#3467: 23.0.94; let + make-local-variable => let value made global

[Lennart Borgman]

On Sat, Jun 6, 2009 at 10:05 PM, Richard Stallman<rms@gnu.org> wrote:
> I think this is a use case that should be avoided.
> Maybe it can be made to work "right", but if that's difficult,
> I'd rather tell people not to do it.

I can't see how it can be avoided. Whenever make-local-variable or
kill-local-variable this problem can occur (if the variable happen to
be let bound of course).

bug#3467: 23.0.94; let + make-local-variable => let value made global

[Lennart Borgman]

Rethinking. Everything is wrong in my solution and suggestion. Coming back.


On Sat, Jun 6, 2009 at 10:11 PM, Lennart
Borgman<lennart.borgman@gmail.com> wrote:
> On Sat, Jun 6, 2009 at 12:48 AM, Lennart
> Borgman<lennart.borgman@gmail.com> wrote:
>>
>> Sigh, and my suggestion was of course unnecessary stupid. What is
>> needed is of course to record values and frame+buffer localness and
>> dito values and reset them. Nothing less than this will ever work
>> correctly, or?
>>
>> And does not this apply to all uses of specbind + unbind_to?
>>
>> Can it be sufficient to just change specbind and unbind_to? Is there
>> anything else that will be affected by changes in the specbind stack?
>> Since info about buffer+frame is alwas needed should specbinding be
>> changed to the below form?
>>
>> struct specbinding
>>  {
>>    Lisp_Object symbol;
>>    Lisp_Object old_value;
>>    Lisp_Object old_buffer_value;
>>    Lisp_Object old_frame_value;
>>    specbinding_func func;
>>    Lisp_Object unused;         /* Dividing by 16 is faster than by 12 */
>>    Lisp_Object unused;
>>    Lisp_Object unused;
>>  };
>
> Here is a bit modified suggestion and some code where I try to
> implement it. Please notice that I have not tested the code. It is
> very possible that I have misunderstood something so please look at
> the code.
>
> struct specbinding
>  {
>    Lisp_Object symbol;
>    Lisp_Object old_value;
>    specbinding_func func;
>    // total 8 fields
>    Lisp_Object need_test_local;
>    Lisp_Object old_buffer;
>    Lisp_Object old_buffer_value;
>    Lisp_Object old_frame;
>    Lisp_Object old_frame_value;
>    //Lisp_Object unused;               /* Dividing by 16 is faster than by 12 */
>  };
>

bug#3467: 23.0.94; let + make-local-variable => let value made global

[Lennart Borgman]

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]

On Sun, Jun 7, 2009 at 3:11 AM, Lennart
Borgman<lennart.borgman@gmail.com> wrote:
> Rethinking. Everything is wrong in my solution and suggestion. Coming back.


Ok, here is what I believe is a reasonable solution.

The first problem is that the semantics of let,
make-variable-buffer-local etc are not clearly defined. I suggest the
following semantics (instead of the one I proposed before):

- When (let ...) shadows a variable it is exactly this variable (ie
default or buffer local version) that should be revived after (let
...).

- No buffer local variables should be deleted or created by the
reviving of the shadowed variables after the (let ...) clause.

- More exactly: if the variable is buffer local in the current buffer
before (let ..) it is the buffer local variable in that buffer that
should be revived. If it does not exist after (let..) nothing should
be done.

- And if a shadowed variable have been made buffer local during the
let binding then it is still the default value that should be revived.
The buffer local value should in this case not be changed after the
(let ..) clause.

I think with this semantics everything will be nearly as fast as
before and there will be no such surprises as shown in this bug. What
do others think?


I have made a patch for this with some comments and questions. I have
included this in my patched version of Emacs+EmacsW32 and have been
using it myself for a couple of days without any problems.

However please review this carefully and especially what I have called
/* Case 1 in specbind */ in unbind_to.

[-- Attachment #2: eval-bug3467.diff --]
[-- Type: application/octet-stream, Size: 5510 bytes --]

Index: eval.c
===================================================================
RCS file: /sources/emacs/emacs/src/eval.c,v
retrieving revision 1.310
diff -u -b -r1.310 eval.c
--- eval.c	24 Mar 2009 16:37:25 -0000	1.310
+++ eval.c	12 Jun 2009 21:25:13 -0000
@@ -31,6 +31,8 @@
 #include "xterm.h"
 #endif
 
+#include <stdio.h>		/* For fprintf.  */
+
 /* This definition is duplicated in alloc.c and keyboard.c */
 /* Putting it in lisp.h makes cc bomb out! */
 
@@ -3295,6 +3297,7 @@
   valcontents = SYMBOL_VALUE (symbol);
   if (!MISCP (valcontents) && !SYMBOL_CONSTANT_P (symbol))
     {
+      /* Case 1 */
       specpdl_ptr->symbol = symbol;
       specpdl_ptr->old_value = valcontents;
       specpdl_ptr->func = NULL;
@@ -3304,7 +3307,7 @@
   else
     {
       Lisp_Object ovalue = find_symbol_value (symbol);
-      specpdl_ptr->func = 0;
+      specpdl_ptr->func = 0; /* Fix-me: Are NULL === 0? */
       specpdl_ptr->old_value = ovalue;
 
       valcontents = XSYMBOL (symbol)->value;
@@ -3319,11 +3322,14 @@
 	  /* For a local variable, record both the symbol and which
 	     buffer's or frame's value we are saving.  */
 	  if (!NILP (Flocal_variable_p (symbol, Qnil)))
+            /* Case 2 */
 	    where = current_buffer;
 	  else if (BUFFER_LOCAL_VALUEP (valcontents)
 		   && XBUFFER_LOCAL_VALUE (valcontents)->found_for_frame)
+            /* Case 3 */
 	    where = XBUFFER_LOCAL_VALUE (valcontents)->frame;
 	  else
+            /* Case 4 */
 	    where = Qnil;
 
 	  /* We're not using the `unused' slot in the specbinding
@@ -3397,6 +3403,7 @@
       this_binding = *--specpdl_ptr;
 
       if (this_binding.func != 0)
+        /* From record_unwind_protect */
 	(*this_binding.func) (this_binding.old_value);
       /* If the symbol is a list, it is really (SYMBOL WHERE
 	 . CURRENT-BUFFER) where WHERE is either nil, a buffer, or a
@@ -3413,21 +3420,72 @@
 	  where = XCAR (XCDR (this_binding.symbol));
 
 	  if (NILP (where))
+            {
+              /* Case 4 in specbind */
+              /* We should reset default value.  (Note: The default
+                 value is either let bound or global.) */
 	    Fset_default (symbol, this_binding.old_value);
+              //printf ("unbind_to: Fset_default where=nil %s\n", SDATA (SYMBOL_NAME (symbol)));
+            }
 	  else if (BUFFERP (where))
+            {
+              /* Case 2 in specbind */
+              /* We should reset buffer local binding, but only if it
+                 is buffer local now or will be when set. */
+              if (!NILP (Flocal_variable_p (symbol, where)) ||
+                  !NILP (Flocal_variable_if_set_p (symbol, where)))
+                {
 	    set_internal (symbol, this_binding.old_value, XBUFFER (where), 1);
+                  //printf ("unbind_to: set_internal in buffer %s\n", SDATA (SYMBOL_NAME (symbol)));
+                }
+            }
 	  else
+              /* Case 3 in specbind */
+              /* Fix-me: This is the frame local case.  I believe it
+                 is broken, but I do not know if it is worth fixing it
+                 since frame local variable are obsolete, or?
+
+                 Note: I do not understand NULL buf arg here, but I
+                 assume that NULL === 0 here because set_internal
+                 checks for 0 and does things like buf->value. */
 	    set_internal (symbol, this_binding.old_value, NULL, 1);
 	}
       else
 	{
+          /* Case 1 in specbind */
 	  /* If variable has a trivial value (no forwarding), we can
 	     just set it.  No need to check for constant symbols here,
 	     since that was already done by specbind.  */
 	  if (!MISCP (SYMBOL_VALUE (this_binding.symbol)))
+            {
+              /* Fix-me: I do not understand this case.  I assume that
+                 if we are here the variable had no buffer local
+                 binding in specbind so that we should set the default
+                 value.  Can SET_SYMBOL_VALUE be used even if the
+                 variable now have a buffer local value?  Doesn't it
+                 erase a buffer local binding that has been done after
+                 specbind?  Can Fset_default be used in that case?
+                 How can we test if make-variable-buffer-local has
+                 been done? */
 	    SET_SYMBOL_VALUE (this_binding.symbol, this_binding.old_value);
+              //printf ("unbind_to: SET_SYMBOL_VALUE %s\n", SDATA (SYMBOL_NAME (this_binding.symbol)));
+            }
 	  else
+            /* Fix-me: I do not understand why set_internal were used
+               here.  Could the variable have had a buffer local
+               binding in specbind if we are here?  Should not just
+               Fset_default be called here? */
+            if (0) //(NILP (Flocal_variable_p (this_binding.symbol, Qnil)))
+              {
 	    set_internal (this_binding.symbol, this_binding.old_value, 0, 1);
+                //printf ("unbind_to: set_internal 0 %s\n", SDATA (SYMBOL_NAME (this_binding.symbol)));
+              }
+            else
+              {
+                /* Set the default value since we had no buffer local binding before. */
+                Fset_default (this_binding.symbol, this_binding.old_value);
+                //printf ("unbind_to: Fset_default no need %s\n", SDATA (SYMBOL_NAME (this_binding.symbol)));
+              }
 	}
     }
 

bug#3467: 23.0.94; let + make-local-variable => let value made

[Stefan Monnier]

> The first problem is that the semantics of let,
> make-variable-buffer-local etc are not clearly defined. I suggest the
> following semantics (instead of the one I proposed before):

That's the intuitive semantics, indeed.

> I have made a patch for this with some comments and questions. I have
> included this in my patched version of Emacs+EmacsW32 and have been
> using it myself for a couple of days without any problems.

- NULL is equal to 0 by definition.
- Why do you check

             if (!NILP (Flocal_variable_p (symbol, where)) ||
                 !NILP (Flocal_variable_if_set_p (symbol, where)))
  i.s.o.
             if (!NILP (Flocal_variable_p (symbol, where)))
  That looks wrong since it would undo a kill-local-variable that was
  performed within the let.
- Regarding "Case 1 in specbind", I have (in my local changes)
  completely rewritten the variable-binding code so as not to use
  special MISCP values.  I find it makes the code clearer, tho maybe
  others will disagree.  I intend to install it in the trunk for 23.2
  and it of course conflicts completely with your patch ;-)
  If you want, I can try and extract the corresponding patch and send it
  to you.


        Stefan