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?