Update alter-var-root to return new val, updated docs (#1171)

Hi,

can you please review change to have have `alter-var-root` to return the
new value. It addresses #1166.

I've also updated various part of the documentation to note the effect
of direct linking on this fn. Please feel free to adjust partially or
completely.

I've also added test cases for the `alter-var-root` for the new change
plus to demo the direct linking behavior.

Thanks

---------

Co-authored-by: ikappaki <[email protected]>
Co-authored-by: Chris Rink <[email protected]>

Author: ikappaki

CHANGELOG.md

@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+ * `alter-var-root` now returns the new value to align with Clojure behavior. Updated the docstring to highlight side effects of direct linking optimization (#1166)
+
 ## [v0.3.4]
 ### Added
  * Added support for the optional `attr-map?` on the `ns` macro (#1159)

docs/compiler.rst

@@ -131,7 +131,7 @@ You can suppress those warnings locally by attaching the ``^:no-warn-on-var-indi
 
 .. note::
 
-   Changes to Vars which were direct linked will not be propagated to any code that used the direct link, rather than Var indirection.
+   Changes to Vars (such as with :lpy:fn:`alter-var-root`) which were direct linked will not be propagated to any code that used the direct link, rather than Var indirection.
 
 .. note::
 

docs/differencesfromclojure.rst

@@ -159,6 +159,8 @@ basilisp.core
 
 - :lpy:fn:`basilisp.core/float` coerces its argument to a floating-point number. When given a string input, Basilisp will try to parse it as a floating-point number, whereas Clojure will raise an error if the input is a character or a string.
 
+- :lpy:fn:`basilisp.core/alter-var-root`: updates to a Var’s root via this function may not reflect in code that directly references the Var unless the Var is marked with ``^:redef`` metadata or declared as a dynamic variable. This is due to the :ref:`Direct Linking Optimization <direct_linking>` and differs with Clojure where such changes are always visible.
+
 .. _refs_and_transactions_differences:
 
 Refs and Transactions

src/basilisp/core.lpy

@@ -4735,7 +4735,14 @@
 
 (defn alter-var-root
   "Atomically alter the Var root by calling ``(apply f root args)`` and setting the root
-  as the result."
+  as the result. Returns the new value.
+
+  .. note::
+  
+     Due to Basilisp's :ref:`Direct Linking Optimization <direct_linking>`, changes to
+     a Var's root value may not be reflected in the code unless the Var is dynamic.
+     To ensure updates propagate, set the Var's `^:redef` metadata, which disables
+     direct linking."
   [v f & args]
   (apply-method v alter-root f args))
 

src/basilisp/lang/runtime.py

@@ -309,6 +309,7 @@ def _set_root(self, newval) -> None:
         self._is_bound = True
         self._root = newval
         self._notify_watches(oldval, newval)
+        return newval
 
     @property
     def root(self):
@@ -322,9 +323,10 @@ def bind_root(self, val) -> None:
 
     def alter_root(self, f, *args) -> None:
         """Atomically alter the root binding of this Var to the result of calling
-        f with the existing root value and any additional arguments."""
+        f with the existing root value and any additional arguments. Returns the
+        new value."""
         with self._lock:
-            self._set_root(f(self._root, *args))
+            return self._set_root(f(self._root, *args))
 
     def push_bindings(self, val):
         if not self._dynamic or self._tl is None:

tests/basilisp/test_core_fns.lpy

@@ -1565,14 +1565,16 @@
       (is (= true (realized? fut)))))
 
   (testing "timed deref of future"
-    (let [fut (future (time/sleep 3))
+    (let [pre (time/perf-counter)
+          fut (future (time/sleep 3))
           start (time/perf-counter)
           ;; block for 100ms
           ret (deref fut 100 :timed-out)
           end (time/perf-counter)
           diff (- end start)]
       (is (= :timed-out ret))
-      (is (< 0.1 diff 0.5) (str "deref timeout outside of [100ms, 500ms] range: " diff "ms"))
+      (is (< 0.1 diff 0.5) (str "deref timeout outside of [100ms, 500ms] range: " diff "ms"
+                              " -- :pre " pre " :start " start " :end " end))
       (is (= false (future-cancelled? fut)))
       (is (= false (future-done? fut)))
       ;; can't always cancel a sleep-ed Future
@@ -1849,6 +1851,37 @@
   (testing "calling vars"
     (is (= '(1 2) (#'basilisp.core/list 1 2)))))
 
+;;;;;;;;;;;;;;;;;;;
+;; Var Utilities ;;
+;;;;;;;;;;;;;;;;;;;
+
+(def test-var-unadorned 7)
+(def ^:redef test-var-redef 19)
+(def ^:dynamic test-var-dynamic 23)
+
+(deftest test-alter-var-root
+  (testing "on unadorned var"
+    (is (= 8 (alter-var-root #'test-var-unadorned inc)))
+    ;; Basilisp Direct Linking Optimization side effect: altering the
+    ;; root var on variables is not reflected back to the code ...
+    (is (= 7 test-var-unadorned))
+    ;; ... unless the var is referenced explicitly.
+    (is (= 8 @#'test-var-unadorned)))
+
+  (testing "on ^:redef var"
+    (is (= 20 (alter-var-root #'test-var-redef inc)))
+    ;; altering the root is reflected in the code because the variable
+    ;; was defined with ^:redef.
+    (is (= 20 test-var-redef))
+    (is (= 20 @#'test-var-redef)))
+
+  (testing "on dynamic var"
+    (is (= 24 (alter-var-root #'test-var-dynamic inc)))
+    ;; altering the root is reflected in the code because the variable
+    ;; is dynamic.
+    (is (= 24 test-var-dynamic))
+    (is (= 24 @#'test-var-dynamic))))
+
 ;;;;;;;;;;;;;;;;;
 ;; Hierarchies ;;
 ;;;;;;;;;;;;;;;;;

tests/basilisp/var_test.py

@@ -392,8 +392,9 @@ def alter_root(root, *args):
         assert alter_args == args
         return new_root
 
-    v.alter_root(alter_root, *alter_args)
+    return_value = v.alter_root(alter_root, *alter_args)
 
+    assert new_root == return_value
     assert new_root == v.root
     assert new_root == v.value
     assert new_root == v.deref()