GNU bug report logs - #70184
[PATCH] Fix + ert for improper alias notification and setting. Also triple notification bug. (3 of 9)

Previous Next

Full log

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: 70184 <at> debbugs.gnu.org
Subject: bug#70184: [PATCH] Fix + ert for improper alias notification and setting. Also triple notification bug. (3 of 9)
Date: Sat, 06 Apr 2024 10:31:45 +0300

> From: Robert Burks <rburksdev <at> gmail.com>
> Date: Thu, 4 Apr 2024 04:45:09 -0400
> 
> 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.

Stefan, WDYT about this issue and the patch (note that the patch was
updated in bug#70189)?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.