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

Previous Next

To reply to this bug, email your comments to 70184 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Robert Burks <rburksdev <at> gmail.com>
To: GNU BUGS <bug-gnu-emacs <at> gnu.org>
Subject: [PATCH] Fix + ert for improper alias notification and setting. Also
 triple notification bug. (3 of 9)
Date: Thu, 4 Apr 2024 04:45:09 -0400

[Message part 1 (text/plain, inline)]

(3 of 9)

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.

[Message part 2 (text/html, inline)]

[0005-Fix-ert-improper-setting-and-notification-in-defvara.patch (application/x-patch, attachment)]

Message #8 received at 70184 <at> debbugs.gnu.org (full text, mbox):

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: Re: 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.