From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs 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 Message-ID: <86o7an3pv0.fsf@gnu.org> References: Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="39735"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 70185@debbugs.gnu.org To: Robert Burks , Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Apr 06 09:37:12 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rt0bv-000A5v-1w for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 06 Apr 2024 09:37:11 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rt0bj-0005kG-Ug; Sat, 06 Apr 2024 03:36:59 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rt0bi-0005k6-N2 for bug-gnu-emacs@gnu.org; Sat, 06 Apr 2024 03:36:58 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rt0bh-0001w5-Lb for bug-gnu-emacs@gnu.org; Sat, 06 Apr 2024 03:36:58 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rt0bn-0005vu-K7 for bug-gnu-emacs@gnu.org; Sat, 06 Apr 2024 03:37:03 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 06 Apr 2024 07:37:03 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 70185 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 70185-submit@debbugs.gnu.org id=B70185.171238896422475 (code B ref 70185); Sat, 06 Apr 2024 07:37:03 +0000 Original-Received: (at 70185) by debbugs.gnu.org; 6 Apr 2024 07:36:04 +0000 Original-Received: from localhost ([127.0.0.1]:38088 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rt0ap-0005qR-Hk for submit@debbugs.gnu.org; Sat, 06 Apr 2024 03:36:04 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:44384) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rt0am-0005pG-LG for 70185@debbugs.gnu.org; Sat, 06 Apr 2024 03:36:01 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rt0ab-0001pn-9c; Sat, 06 Apr 2024 03:35:49 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=JTkDzVmEymGrXm8YEl3ws5YKZjF//rIjM6ku/y08Vq8=; b=qCccJ8EzEW65 01ZdnyJUlEixV02mWpB0ggnF+cx6WhYSAgHSHoxdSgdo3PCXGKN58AVbxZVpummVWZuwRnhPmQoy4 8bfIBXGu2mxYB0CNCngx2lguK7tSXF+qpkh64McYCrPsd7RJnIWj5oUAFHuZ/HxI7pky4iJIg22Jj tE2OwmQjqgYM/nONtYmEPJwNnjaGWnQufIHj3Hi/0aPJuRADIjHFNovZkDRiXPzs0LK7hjH6zbhtu CvGBwBPihIvl7VoQ40XFYHTLwB/0uVQnGX39XTv099WgbLAJgal0yNnRXAoWtUxXsiLA3LWds/QcT PzNuegM7sQsKXPJ0jzI74A==; In-Reply-To: (message from Robert Burks on Thu, 4 Apr 2024 04:45:40 -0400) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:282750 Archived-At: > From: Robert Burks > 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?