bug#70188: [PATCH] Fix + ert for notification sending 'set unbound' when unbinding (7 of 9)

bug#70188: [PATCH] Fix + ert for notification sending 'set unbound' when unbinding (7 of 9)

[Robert Burks]

[-- Attachment #1.1: Type: text/plain, Size: 4845 bytes --]

(7 of 9) (Resistance is futile)

Bug#00006 (The mysterious unlet to 'unbound')
** Bug recreations are at the end

Once again the path from do_one_unbind leads to rare cases in
set_default_internal.  I have included a fix and ert.

The case is do_one_unbind calls set_default_internal (foo , Qunbound,
SET_INTERNAL_UNBIND)
-- We reach here with Qunbound as a value only when unletting if when
'letting'
   the buffer saw the void default/global value.
-- make_local_foo called for the first time within a let on a variable that
is globally void.
-- make_local_foo was called in some other buffer and a let takes place in
a buffer that
   still sees the default as void. (let_shadows_default)
-- The 'local_if_set' shadows default case cannot reach here with 'unbound'
as a value,
   even with something like this:
   (defvar foo)
   (make-variable-buffer-local 'foo)
   This will establish a default of 'nil'.
-- I don't think there are any other ways set_default_internal is being
called
   with Qunbound as value on localized variables.
-- My included test is the only test in data-tests.el(maybe all testing)
that
   hits the above conditions.

-- Currently it stores Qunbound to the cdr of blv->defcell and returns.
This seems correct.
-- I have only tested plain values.  Forward symbols like DEFVAR_LISP and
DEFVAR_BOOL
   (not DEFVAR_PER_BUFFER) could reach here with 'Qunbound' as 'new value'
if they start
   their life unbound.  If makunbound is used on one they get turned to
plain value
   by set_internal.  But if they are set unbound in 'C' who knows.
   Maybe we need to check for void and do blv->fwd.fwdptr = NULL? Will need
testing...

First example is 'make_local_foo' happened in some other buffer so the
'let' in this
buffer now shadows the default.

Second example is 'make_local_foo' happened in a 'let' making it 'unlet' to
default.

There still may exist other edge cases, I am working to uncover those.
Every bug
example I have sent has been fixed by the patches included in these seven
emails.

Additionally, the work I have submitted does not change the way Emacs
handles the
numerous scenarios around variable setting; it merely makes watcher
notification
reflect the already defined behavior.

Patch 0021:  Fix for this bug (one(1) place requires bug# update)

Patch 0022:  Make it so functions pass 'Qunbound' to 'notify' and do
conversion
             to 'Qnil' there.  This allows easier tracking in gdb, it was
hard
             to isolate prior.  Distinction may also be needed in the
future by
             watchers.

Patch 0023:  ert for this bug (four(4) places require bug# update)

Bug
Recreation------------------------------------------------------------------

This path can only be reached through the special path from do_one_unbind
that
leads to set_default_internal.
--------------------------------------------------------------------------------
(defvar test)
test

(defvar results nil)
results

(add-variable-watcher 'test (lambda (&rest args)
                              (push args results)))
nil

(with-temp-buffer
  (make-local-variable 'test)
  (let ((test 99))
    (set 'test 66)))
66

(let ((test 99))
  (set 'test 66))
66

results
((test unbound set nil) (test 66 set nil) (test 99 let nil) (test nil unlet
#<killed buffer>) (test 66 set #<killed buffer>) (test 99 let #<killed
buffer>))
;; notice it says 'unbound' vs 'nil', all other functions that notify use
'nil'
;; for the unbound case.  'test' is still unbound globally.  It is
returning a
;; potential legitimate symbol 'unbound' because it is sending the 'C'
string
;; definition for Qunbound;
;; Also, this should say 'unlet' (This was fixed with the bug#00004/5
solution)
--------------------------------------------------------------------------------
Extra broke case
--------------------------------------------------------------------------------
(defvar test)
test

(defvar results nil)
results

(add-variable-watcher 'test (lambda (&rest args)
                              (push args results)))
nil

(let ((test 99))
  (make-local-variable 'test)
  (set 'test 66))
66

test
66

results
((test unbound set nil) (test 66 set #<buffer *scratch*>) (test 99 let nil))
;; should be (test nil unlet nil) as it is now 66 in *scratch* but void
globally


Not a Bugged
Case****************************************************************
Notice this case works. One of the rare cases already tested for.
--------------------------------------------------------------------------------
(defvar test)
test

(defvar results nil)
results

(add-variable-watcher 'test (lambda (&rest args)
                              (push args results)))
nil

(make-local-variable 'test)
test

(let ((test 99))
  (set 'test 66))
66

results
((test nil unlet #<buffer *scratch*>) (test 66 set #<buffer *scratch*>)
(test 99 let #<buffer *scratch*>))

[-- Attachment #1.2: Type: text/html, Size: 5675 bytes --]

[-- Attachment #2: 0022-Functions-will-now-pass-Qunbound-to-notify_variable_.patch --]
[-- Type: application/x-patch, Size: 5477 bytes --]

[-- Attachment #3: 0021-Fix-set_default-from-notifying-watchers-with-unbound.patch --]
[-- Type: application/x-patch, Size: 1416 bytes --]

[-- Attachment #4: 0023-Add-ert-for-unbound-watcher-notification-BUG-00006.patch --]
[-- Type: application/x-patch, Size: 3522 bytes --]

bug#70188: [PATCH] Fix + ert for notification sending 'set unbound' when unbinding (7 of 9)

[Eli Zaretskii]

> From: Robert Burks <rburksdev@gmail.com>
> Date: Thu, 4 Apr 2024 04:46:47 -0400
> 
> (7 of 9) (Resistance is futile)
> 
> Bug#00006 (The mysterious unlet to 'unbound')
> ** Bug recreations are at the end
> 
> Once again the path from do_one_unbind leads to rare cases in
> set_default_internal.  I have included a fix and ert.
> 
> The case is do_one_unbind calls set_default_internal (foo , Qunbound, SET_INTERNAL_UNBIND)
> -- We reach here with Qunbound as a value only when unletting if when 'letting'
>    the buffer saw the void default/global value. 
> -- make_local_foo called for the first time within a let on a variable that is globally void.
> -- make_local_foo was called in some other buffer and a let takes place in a buffer that
>    still sees the default as void. (let_shadows_default)
> -- The 'local_if_set' shadows default case cannot reach here with 'unbound' as a value,
>    even with something like this:
>    (defvar foo)
>    (make-variable-buffer-local 'foo)
>    This will establish a default of 'nil'. 
> -- I don't think there are any other ways set_default_internal is being called
>    with Qunbound as value on localized variables.
> -- My included test is the only test in data-tests.el(maybe all testing) that
>    hits the above conditions.
> 
> -- Currently it stores Qunbound to the cdr of blv->defcell and returns.  This seems correct.
> -- I have only tested plain values.  Forward symbols like DEFVAR_LISP and DEFVAR_BOOL
>    (not DEFVAR_PER_BUFFER) could reach here with 'Qunbound' as 'new value' if they start
>    their life unbound.  If makunbound is used on one they get turned to plain value
>    by set_internal.  But if they are set unbound in 'C' who knows.
>    Maybe we need to check for void and do blv->fwd.fwdptr = NULL? Will need testing...
> 
> First example is 'make_local_foo' happened in some other buffer so the 'let' in this
> buffer now shadows the default.
> 
> Second example is 'make_local_foo' happened in a 'let' making it 'unlet' to default. 
> 
> There still may exist other edge cases, I am working to uncover those.  Every bug
> example I have sent has been fixed by the patches included in these seven emails.
> 
> Additionally, the work I have submitted does not change the way Emacs handles the
> numerous scenarios around variable setting; it merely makes watcher notification
> reflect the already defined behavior.
> 
> Patch 0021:  Fix for this bug (one(1) place requires bug# update)
> 
> Patch 0022:  Make it so functions pass 'Qunbound' to 'notify' and do conversion
>              to 'Qnil' there.  This allows easier tracking in gdb, it was hard
>              to isolate prior.  Distinction may also be needed in the future by
>              watchers.  
> 
> Patch 0023:  ert for this bug (four(4) places require bug# update)
> 
> Bug Recreation------------------------------------------------------------------
> 
> This path can only be reached through the special path from do_one_unbind that
> leads to set_default_internal.
> --------------------------------------------------------------------------------
> (defvar test)
> test
> 
> (defvar results nil)
> results
> 
> (add-variable-watcher 'test (lambda (&rest args)
>                               (push args results)))
> nil
> 
> (with-temp-buffer
>   (make-local-variable 'test)
>   (let ((test 99))
>     (set 'test 66)))
> 66
> 
> (let ((test 99))
>   (set 'test 66))
> 66
> 
> results
> ((test unbound set nil) (test 66 set nil) (test 99 let nil) (test nil unlet #<killed buffer>) (test 66 set #<killed
> buffer>) (test 99 let #<killed buffer>))
> ;; notice it says 'unbound' vs 'nil', all other functions that notify use 'nil'
> ;; for the unbound case.  'test' is still unbound globally.  It is returning a
> ;; potential legitimate symbol 'unbound' because it is sending the 'C' string
> ;; definition for Qunbound;
> ;; Also, this should say 'unlet' (This was fixed with the bug#00004/5 solution)
> --------------------------------------------------------------------------------
> Extra broke case
> --------------------------------------------------------------------------------
> (defvar test)
> test
> 
> (defvar results nil)
> results
> 
> (add-variable-watcher 'test (lambda (&rest args)
>                               (push args results)))
> nil
> 
> (let ((test 99))
>   (make-local-variable 'test)
>   (set 'test 66))
> 66
> 
> test
> 66
> 
> results
> ((test unbound set nil) (test 66 set #<buffer *scratch*>) (test 99 let nil))
> ;; should be (test nil unlet nil) as it is now 66 in *scratch* but void globally
> 
> Not a Bugged Case****************************************************************
> Notice this case works. One of the rare cases already tested for.
> --------------------------------------------------------------------------------
> (defvar test)
> test
> 
> (defvar results nil)
> results
> 
> (add-variable-watcher 'test (lambda (&rest args)
>                               (push args results)))
> nil
> 
> (make-local-variable 'test)
> test
> 
> (let ((test 99))
>   (set 'test 66))
> 66
> 
> results
> ((test nil unlet #<buffer *scratch*>) (test 66 set #<buffer *scratch*>) (test 99 let #<buffer *scratch*>))

This changes how the watchers are called, so it needs suitable changes
in NEWS in and in the ELisp manual.

Stefan, any comments?