bug#70185: [PATCH] Fix + ert for numerous bugs involving re-aliasing, un

(4 of 9)

Bug#00003a (Bypass watcher by adding an uninterned symbol as an alias before watcher.)
Bug#00003b (re-aliasing the middle of a chain to a constant)
Bug#00003c (re-aliasing the middle of a chain from an unwatched to a watched
variable then writing from the end bypasses watching.)
Bug#00003d (re-aliasing notifies the base variable watcher that it is becoming an alias)

** Bug recreations are at the end

My patches for the three bugs prior essentially fix these bugs. The
included patches are basically some clean up work and closing gaps when working
with constants. That is, uninterned symbols would have worked as a side effect
of the previous submissions. But some cleanup, flag management, and some small
fixes for constants were left over.

The last two bugs happen because 'trapped_write' is only "harmonized" across
the obarray in the event of watcher adding and removing and not in the event
of re-aliasing. I will show in testing that I was able to close up these bugs
and also remove the need for "harmonizing". My previous patches fix the
setting of the constant and the included patches enforce re-aliasing behavior.

Oddly, existing testing calls 'defvar' on the alias prior to 'defvaralias' to
seemingly side step the fact that they do not work with uninterned symbols.
Maybe things have changed? As near as I can tell, the evaluation process
(such as 'eval-sexp-add-defvars') seems to see that 'defvaralias' starts with
'def' and interns its arguments as part of evaluation. For now I was able to
feed 'defvaralias' uninterned symbols using 'gensym' and 'make-symbol'.

Should uninterned symbols be allowed to be a variable alias? Currently it is
allowed and could be easily prevented in 'defvaralias'. Currently, the base
variable being uninterned is also allowed, though as long as it is the only one
and last in the chain the current system will work. Regardless of uninterned
symbols being allowed the included patches apply as they remove the need to
"harmonize" even for interned symbols (this "harmonizing" wasn't even occurring
at all required times anyways).

In regards to bug '3d' from above, if an alias of a watched variable gets
re-aliased there should be no notification, the warning of losing value is all
that should happen if the value is different. Currently, notification not only
happens, it happens wrong (see example below).

Consider a variable with a watcher function designed only to watch it. Then an
alias is added and then re-aliased, suddenly the watcher function would receive
notifications about symbols other than that which it was designed for. A
watcher function should be able to assume that any notification if receives is
for the operation applying to the base variable of any given alias chain. A
base variable that has watchers and then becomes an alias is no longer a base
variable. The only time a watcher should receive notifications for multiple
symbols is if it is explicitly assigned to multiple variables/chains. That is a
watcher function shouldn't gain additional symbols names being sent to it from a
'defvaralias' assignment happening somewhere along its alias chain. This allows
one to write simple watchers for the general case without the need to anticipate
future outside actions.

Additionally, once a variable becomes an alias if it was watched those watchers
should be removed (after notification that it is about to become an alias, which
is questionable as to its actual value as a feature). We shouldn't save the
watchers to fallback on in the event of re-aliasing as once a variable becomes
an alias those watchers were made inaccessible (i.e. the user cannot add or
remove watchers as those functions will follow the redirection).

Notification should always be based on the 'base' variable only. When a watcher is
assigned to an alias the assignment follows redirection and is applied to the
base variable. Notifications when working properly use the base variable symbol
name (this is existing behaviour and will continue). Ideally if one adds
a watcher to a variable that will rely on the name of the symbol it should use
'Findirect_variable' on said variable to discover if it points elsewhere.
Also, I have a missing feature(Fvaralias-p) ready to add after these nine emails.

----------------------------------------------------------------------------

Patch 0006: These functions were the only ones that checked for constant
after the redirection loop, however, they were still using the original variable.
I moved it to the top to make all these functions look alike and made them
checking for constants along every step of redirection. Additionally, it
eliminates an extra conversion that was occurring in the most common case.

Patch 0007: makunbound was making an unneeded check for constant, it already
happens after Fset called set_internal. Additionally, this check didn't
follow redirection and the one in Fset will. Also, the very first thing
set_internal does is call CHECK_SYMBOL. I eliminated the extra function call,
call set_internal directly and let it do all the work.

Patch 0008: Eliminated the usage of an enumeration as a boolean.

Patch 0009: Eliminated the usage of an enumeration as a boolean. Additionally,
the commentary had no mention of thread switching. This section of code and
commentary mostly predates the addition of "SET_INTERNAL_THREAD_SWITCH" to the
enumeration. Which points out a downside of using an enumeration as a boolean,
someone can add to it or rearrange it and overlook its boolean usages.

Patch 0010: Added commentary regarding input sanitation. Findirect_variable
converts a Lisp_Object to Lisp_Symbol and then back to a Lisp_Object even when
there is no alias redirection. I almost added brackets to make it go right
through when there is no alias. However, I realized it had the added benefit
that a calling function can use it to make sure their parameter is a bare symbol.

Patch 0011: With all previous changes only 'TRAPPED_NOWRITE' will be
copied to an alias that is explicitly aliased to a constant (or to another
alias that was explicitly aliased to a constant). Other functions check for constant
as they follow redirection all the way to the end. An alias set as constant
cannot be re-aliased. I added checking in 'defvaralias' that will prevent any
alias in the upstream of an alias that got re-aliased to a constant from being
re-aliased itself (maybe we can allow them to be re-aliased, since they were
not explicitly set? Though until then they are still perceived as constant to
everything including 'defvaralias'). Because of previous bug fixes the other
values of the flag along the chain are essentially ignored, so this is really
just making the flag have a consistent value (i.e. SYMBOL_TRAPPED_WRITE only
applies to non-aliases) and fixing gaps in re-aliasing.

Patch 0012: This applies on top of the previous and is just to get
rid of a needless conversion call.

Patch 0013: Harmonizing is no longer required at all due to previous bug fixes.
My testing attached along with all other regression tests passing proves this.

Patch 0014: This is a full test showing that aliases and watchers now
work with uninterned symbols (do we allow them is secondary). Using them
allowed this test of aliases to not clutter up the global symbol space
and have the test simply be garbage collected away (it's also repeatable
interactively). Now that they work, maybe someone (me lol, I have testing
applications for aliases themselves that could use this functionality) will
find them to be a useful tool. But at least now they cannot be accidentally
used and circumvent a watcher.
(Please note the BUG# placeholder in three(3) places will need to be updated.)

Patch 0015: Test for the specific chain breaking and re-aliasing bugs
below. I placed this in data-tests.el because it involves watchers.
(Please note the BUG# placeholder in seven(7) places will need to be updated.)

Interestingly enough the uninterned symbols test actually stems from the very
first test I attempted to write for the first bug. I often use uninterned
symbols while testing as they make it easy to make tests that can be repeated
without mucking up the symbol space.

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

This occurs because adding an uninterned alias before watchers are added
makes them never get their 'trapped_write' flag set as they are never
"harmonized".

Bug#00003a (Bypass watcher by adding an uninterned symbol as alias before watcher)
emacs -Q-------------------------------------------------------------------------
(defvar results nil)
results

(defvar fake (gensym))
fake

(defvar notsafe "I should be watched")
notsafe

(defvaralias fake 'notsafe)
notsafe

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

(set fake "bypassed")
"bypassed"

notsafe
"bypassed"

results
nil
;; There should be watch results

Bug#00003a (Bypass watcher by adding an uninterned symbol as alias before watcher)
emacs -Q-------------------------------------------------------------------------
(defvar results nil)
results

(defvar fake (make-symbol "dummy"))
fake

(defvar notsafe "I should be watched")
notsafe

(defvaralias fake 'notsafe)
notsafe

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

(set fake "bypassed")
"bypassed"

notsafe
"bypassed"

results
nil

Bug#00003b (re-aliasing the base/middle of a chain to a constant)
emacs - Q------------------------------------------------------------------------

(defvar test 5)
test

(defvaralias 'testa1 'test)
test

(defvaralias 'testa2 'testa1)
testa1

(defvaralias 'testa1 'nil)
nil

(set 'testa2 5) ;; makunbound works also
5

nil
5
;; We just set a constant

Bug#00003c (re-aliasing the middle of a chain from an unwatched to a watched
variable then writing from the end bypasses watching.)
emacs -Q-------------------------------------------------------------------------

(defvar results nil)
results

;; watched
(defvar test 5)
test

;; unwatched
(defvar test2 100)
test2

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

(defvaralias 'testa1 'test2)
test2

(defvaralias 'testa2 'testa1)
testa1

(defvaralias 'testa1 'test)
test

(set 'testa2 500)
500

test
500

results
nil
;; There should be watch results

Bug#00003d (re-aliasing notifies the base variable watcher that it is becoming an alias)
emacs -Q-------------------------------------------------------------------------
(defvar results nil)
results

(defvar test 5)
test

(defvar test2 10)
test2

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

(defvaralias 'testa 'test)
test

(defvaralias 'testa 'test2)
test2

results
((test test2 defvaralias nil))

test
5

testa
10

;; 'test' is not becoming an alias of 'test2', 'testa' was re-aliased to 'test2'.
;; No notification should be made in this instance, the warning is sufficient.

From:	Robert Burks
Subject:	bug#70185: [PATCH] Fix + ert for numerous bugs involving re-aliasing, uninterned symbols, and watchers (4 of 9)
Date:	Thu, 4 Apr 2024 04:45:40 -0400