bug#70183: [PATCH] Fix + ert for multiple watcher notifications (2 of 9)

[Eli Zaretskii]

> From: Robert Burks <rburksdev@gmail.com>
> Date: Thu, 4 Apr 2024 04:44:41 -0400
> 
> Bug#00001a (Using set-default with an alias double notifies)
> Bug#00001a (Using set-default on the alias of an unbound forwarded symbol)
> Bug#00001b (Setting a let bound forwarded symbol in buffer with no blv)
> 
> ** Bug recreation is at the end
> (These are extremely specific paths derived from digging at the c code
> to make bugs happen, they are in no way intentional execution paths.)
> 
> I am calling all of these the same bug because they arise from the
> functions set_internal and set_default_internal each calling the other
> along specific paths.  There exists a way to achieve triple notification;
> however, that involves combining with another bug that I will cover later.
> 
> Attached are patches to fix and testing for the bugs shown below.
> (Please note the BUG# placeholder in twelve(12) places will need to be
> updated.)
> 
> I divided the fix into a commit for each function.  These functions are
> tangled together so both are required to fix the problems. I added the
> testing as a separate commit.
> 
> Also, there is reasoning behind checking for the constant at every
> step of redirection that will play out along the rest of my submissions.
> It isn't so much about checking at every step as much as it is about the
> end of the chain. At the top in the goto start loop is the easiest place to
> make that happen.
> 
>   Additionally, I moved the XSETSYMBOL to the top from inside the
> localized to point out that while redirection was important, the notes
> there didn't express how important it was to convert the symbol to bare.
> If handled only during cases of redirection there will occur
> times when byte compiling in which set_internal will be passed a symbol
> with position and then store it as a blv. Then kill-buffer will choke
> on encountering a symbol with position in a blv cons cell when it is
> accessed with positions disabled.
> 
> e.g. auto-composition-mode was one I encountered.  Simply moving the
> XSETSYMBOL to only happen after variable alias redirection caused an issue.
> That variable is a DEFVAR_LISP but is made buffer local in an .el file. 
> Everything would build fine but byte compilation would die on that file
> because kill-buffer would pull all blv's and encounter that one put on
> with 'position' while not being in a 'positions-enabled' environment. 
> 
> Bug Recreations -------------------------------------------------------
> 
> Bug#00001a (Using set-default with an alias double notifies)
> emacs -Q---------------------------------------------------------------
> 
> (defvar test 5)
> test
> 
> (defvar result nil)
> result
> 
> (defvaralias 'test-alias 'test)
> test
> 
> (add-variable-watcher 'test (lambda (&rest args)
>                                 (push args result)))
> nil
> 
> (set-default 'test-alias 100)
> 100
> 
> result
> ((test 100 set nil) (test 100 set nil))
> ;; there should only be one result here!!!!!!!!!!
> ;; This bug arises from set_default_internal making notification 
> ;; one time for the alias and another time when it calls set_internal.
> 
> Bug#00001a (Using set-default on the alias of an unbound forwarded symbol)
> emacs -Q---------------------------------------------------------------
> 
> (defvar results nil)
> results
> 
> (add-variable-watcher 'right-margin-width (lambda (&rest args) (push args 
> results)))
> nil
> 
> (defvaralias 'testa 'right-margin-width)
> right-margin-width
> 
> (makunbound 'right-margin-width)
> right-margin-width
> 
> (set 'results nil)
> nil
> 
> (set-default 'testa 2000)
> 2000
> 
> results
> ((right-margin-width 2000 set nil) (right-margin-width 2000 set nil))
> ;; only one set occurred!!!!!!!!
> ;; Calling set-default with the alias of an unbound forwarded symbol
> ;; causes watchers to be notified once in set_default_internal and another
> ;; time in set_internal.
> ;; This is the same bug as the previous example and occurs because 
> set_internal converts
> ;; forwarded symbol into a plain value when it is unbound.
> 
> Bug#00001b (Setting a let bound forwarded symbol in a buffer with no blv)
> emacs -Q---------------------------------------------------------------
> 
> (defvar results nil)
> results
> 
> (add-variable-watcher 'right-margin-width (lambda (&rest args) (push args 
> results)))
> nil
> 
> (let ((right-margin-width 150))
>   (set 'right-margin-width 2000))
> 2000
> 
> results
> ((right-margin-width 0 set nil) (right-margin-width 2000 set nil) 
> (right-margin-width 2000 set #<buffer
> *scratch*>) (right-margin-width 150 set nil))
> ;;                                                   ^                        
>           ^
> ;; These are double 
> _________________________________|__________________________________|
> ;; Notification is being sent once in set_internal and again in
> ;; set_default_internal as a result of being a let bound forwarded symbol.
> ;; Also, it is not sending the proper 'operation'.  While these should be
> ;; acting on the default value just as a normal blv that is shadowing default
> ;; does, with blv the notification still reflects the proper operation used.
> ;; That being said, in this scenario they should not have a buffer name as it 
> is
> ;; acting globally but it should still have the 'let' and 'unlet' like a blv 
> would.
> ;; The (right-margin-width 2000 set #<buffer *scratch*>) being sent by
> ;; set_internal should not be there in this scenario.
> 
> **********************************************************************
> ;; I spent way to much time in gdb finding these last two paths.  I knew
> ;; they existed just by looking at the code but couldn't trigger them
> ;; in lisp for the longest time.
> 
> -----an example that looks like my testing------
> ;; As long as the uninterned alias is added after the watcher uninterned 
> symbols
> ;; work because the trapped write flag is copied and does not need to be 
> "harmonized".
> ;; I will cover this in more depth later.  This makes for clean and 
> repeatable tests.
> ;; I have an alternate version of most of the tests without uninterned 
> aliases.
> (let* ((r1 nil)
>        (a1 (gensym))
>        (v1 (gensym))
>        (f1 (lambda (&rest args) (push args r1))))
>   (set v1 5)
>   (add-variable-watcher v1 f1)
>   (defvaralias a1 v1)
>   (set-default a1 100)
>   r1)
> ((g1 100 set nil) (g1 100 set nil))

Stefan, any comments about this?  I'm not sure what should watchers
do with aliases.  Also, please look at the patches (which seem to have
been updated in bug#70189, sigh...)