alter-var-root does not appear to change the variable's value

[ikappaki]

Hi,

I can't seem to get alter-var-root to alter any variable's root binding.

To reproduce, create a variable and alter its value. Even though the update happens, the variables root value remains the same as before the update

> basilisp repl
basilisp.user=> (def abc 7)
#'basilisp.user/abc

basilisp.user=> (alter-var-root #'abc (fn [x] (println :current x) (inc x)))
:current 7
nil

basilisp.user=> abc
7

The same works in clojure as expected

> clj
Clojure 1.12.0
user=> (def abc 7)
#'user/abc
user=> (alter-var-root #'abc (fn [x] (println :current x) (inc x)))
:current 7
8
user=>  abc
8

Thanks

[chrisrink10]

See #969

[chrisrink10]

I would accept a PR to change the docstring to include a warning about this behavior.

I would also consider a warning issued when calling alter-var-root on Vars not defined with ^:redef or ^:dynamic. I think trying to warn in every correct case may be prohibitively difficult since it is possible that not every namespace is compiled with the same compiler options, so I am a little more wary about accepting such a change since it would likely not be guaranteed to warn in all cases.

[ikappaki]

I would accept a PR to change the docstring to include a warning about this behavior.

Will do, I was also thinking of dropping a note in the differences with clojure section of the documentation.

I would also consider a warning issued when calling alter-var-root on Vars not defined with ^:redef or ^:dynamic. I think trying to warn in every correct case may be prohibitively difficult since it is possible that not every namespace is compiled with the same compiler options, so I am a little more wary about accepting such a change since it would likely not be guaranteed to warn in all cases.

I think documenting the behavior in the docstring should be enough, given the above concerns.

[chrisrink10]

Will do, I was also thinking of dropping a note in the differences with clojure section of the documentation.

Makes sense to me.

[ikappaki]

Hi @chrisrink10 , I'd also like to update alter-var-root to return the new value with this ticket, to make it the same with Clojure behavior please.

In Clojure, it returns the new value (even though it is not documented)

> clj
Clojure 1.12.0
user=> (def abc 7)
#'user/abc
user=> (alter-var-root #'abc (fn [x] (println :prev x) (inc x)))
:prev 7
8

Basilisp currenty returns nil

basilisp.user=> (def abc 7)
#'basilisp.user/abc

basilisp.user=> (alter-var-root #'abc (fn [x] (println :prev x) (inc x)))
:prev 7
nil

or let me know if you would like me to raise a separate issue for the same.

[chrisrink10]

@ikappaki you can fix it as part of the same ticket. Thanks!

[chrisrink10]

Fixed in #1171