Update alter-var-root to return new val, updated docs

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()