(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.