Package: emacs;
Reported by: Robert Burks <rburksdev <at> gmail.com>
Date: Thu, 4 Apr 2024 09:18:05 UTC
Severity: normal
Tags: patch
View this message in rfc822 format
From: Eli Zaretskii <eliz <at> gnu.org> To: Robert Burks <rburksdev <at> gmail.com>, Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: 70185 <at> debbugs.gnu.org Subject: bug#70185: [PATCH] Fix + ert for numerous bugs involving re-aliasing, uninterned symbols, and watchers (4 of 9) Date: Sat, 06 Apr 2024 10:35:47 +0300
> From: Robert Burks <rburksdev <at> gmail.com> > Date: Thu, 4 Apr 2024 04:45:40 -0400 > > (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. It sounds like some of the patches here depend on others. If that is the case, the mutually-dependent parts should be in a single patch, to facilitate the review. Stefan, WDYT about these issues?
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.