Subject: bug#70184: [PATCH] Fix + ert for improper alias notification and setting. Also triple notification bug. (3 of 9)

From: Robert Burks <[email-masked]>
Date: Thu, 4 Apr 2024 04:45:09 -0400

(3 of 9)

Bug00002a (Improper notification when a new alias is made for an unbound
variable)
Bug00002b (Triple notification when using set-default on the an alias of an
unbound)
** Bug recreation is at the end

  I have attached a commit that provides a fix and (close to)full coverage
ert for the portion of code where it resided.  I also added some notes to
better clarify the meaning of 'new_alias' and 'base_variable'.
(Please note the BUG# placeholder in seven(7) places will need to be
updated.)

  There are notes and a link above the code that clearly spelled out what
should be done.  However, the code below did not reflect that; it does
now.  There seemed to be confusion with the flag variable's enumeration,
as the set functions use 'SET_INTERNAL_BIND' for let binding not for
variable instantiating.

  I said "close to" above in regards to testing;  because, both 'new_alias'
and 'base_variable' can be alias chains and that is not tested for.  While
'Fboundp', 'find_symbol_value', and 'set_internal' follow redirection and
everything currently works, there still should be regression testing to
enforce/reflect defined behaviour.

  Variable watchers are only fully functional with interned symbols.
However,
a watcher can work with uninterned symbols being used as variable aliases
so long as the aliases are added after the watcher is added to the base
variable.  This is because the trapped write flag is copied at the time
of alias definition and does not require "harmonization" across the obarray.
The test I have included use uninternaed symbols as aliases, I have
alternate
versions that use global variables as aliases.  Using uninterned symbols
allows for the creation of cleanly written and repeatable tests.
(A bit of foreshadowing) Currently defvaralias allows uninterned symbols as
an alias or base (even though broken cases exist, i.e. watchers added after
an uninterned alias).

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

emacs -Q---------------------------------------------------------------

(defvar results nil)
results

(defvar test)
test

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

(defvaralias 'testalias 'test)
test

results
((test nil let nil))
;; There was no let binding here!!!!!!!!!!
;; It shouldn't be calling `set' either in this case, as both are unbound
;; and we are simply assigning a newly created alias to an unbound variable.
;; The base variable is being neither set nor let bound!!!!
;; If defining an old variable obsolete we shouldn't be telling
;; the new variable's watchers that it is being let bound.
;; The confusion in the code came from the naming in the enum.

emacs -Q---------------------------------------------------------------
(defvar results nil)
results

(defvar test)
test

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

(defvaralias 'testalias 'test)
test

(set-default 'testalias 100)
100

results
((test 100 set nil) (test 100 set nil) (test nil let nil))
;; By combining with the bug prior we get triple notification when
;; there should only be one 'set' to 100.
;; My bug fix prior reduces it to two and this fix corrects this
completely.

emacs -Q---------------------------------------------------------------
(defvar results nil)
results

(defvar test)
test

(defvar testalias 50)
testalias

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

(defvaralias 'testalias 'test)
test

(set-default 'testalias 100)
100

results
((test 100 set nil) (test 100 set nil) (test 50 let nil))
;; This example shows improper 'let' that should read set
;; and double notification that arises from the previous bug.
;; This example should have a 'set' to 50 and then a 'set' to 100.