Update metadata and dynamic flag on Var redef (#642)

Author: chrisrink10

CHANGELOG.md

@@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
  * Fixed a bug that allowed dynamic Vars to be `set!` even if they weren't thread-bound (#638)
  * Fixed a bug where it was impossible to specify negative CLI options for the compiler flags (#638)
  * Fixed a bug where it was impossible to use more than a single body expression in a `try` special form (#640)
+ * Fixed a bug where re-`def`ing a Var (regardless of `^:redef` metadata) would not update metadata or dynamic flag (#642)
 
 ### Removed
  * Removed Click as a dependency in favor of builtin `argparse` (#622, #624, #636)

src/basilisp/core.lpy

@@ -4193,7 +4193,9 @@
   "Set the binding of the Var. Must be thread-local."
   [v val]
   (if (thread-bound? v)
-    (set! (.-value v) val)
+    (do
+      (.set-value v val)
+      val)
     (throw
      (ex-info "Cannot set non-thread-bound Var binding" {:var v}))))
 
@@ -4239,7 +4241,7 @@
   ([ns name val]
    (let [ns (the-ns ns)
          v  (basilisp.lang.runtime/Var ns name)]
-     (set! (.-root v) val)
+     (.set-root v val)
      (.intern ns v))))
 
 (defn create-ns

src/basilisp/lang/compiler/generator.py

@@ -640,6 +640,7 @@ def _var_ns_as_python_sym(name: str) -> str:
 
 _NS_VAR_VALUE = f"{_NS_VAR}.value"
 
+_NS_VAR_VALUE_SETTER_FN_NAME = _load_attr(f"{_NS_VAR}.set_value")
 _NS_VAR_NAME = _load_attr(f"{_NS_VAR_VALUE}.name")
 _NEW_DECIMAL_FN_NAME = _load_attr(f"{_UTIL_ALIAS}.decimal_from_str")
 _NEW_FRACTION_FN_NAME = _load_attr(f"{_UTIL_ALIAS}.fraction")
@@ -2602,9 +2603,12 @@ def _require_to_py_ast(_: GeneratorContext, node: Require) -> GeneratedPyAST:
                 finalbody=[
                     # Restore the original namespace after each require to ensure that the
                     # following require starts with a clean slate
-                    ast.Assign(
-                        targets=[_load_attr(_NS_VAR_VALUE, ctx=ast.Store())],
-                        value=ast.Name(id=requiring_ns_name, ctx=ast.Load()),
+                    statementize(
+                        ast.Call(
+                            func=_NS_VAR_VALUE_SETTER_FN_NAME,
+                            args=[ast.Name(id=requiring_ns_name, ctx=ast.Load())],
+                            keywords=[],
+                        )
                     ),
                 ],
             )
@@ -2630,16 +2634,18 @@ def _set_bang_to_py_ast(ctx: GeneratorContext, node: SetBang) -> GeneratedPyAST:
         target, (HostField, Local, VarRef)
     ), f"invalid set! target type {type(target)}"
 
+    assign_ast: List[ast.AST]
     if isinstance(target, HostField):
         target_ast = _interop_prop_to_py_ast(ctx, target, is_assigning=True)
+        assign_ast = [ast.Assign(targets=[target_ast.node], value=val_ast.node)]
     elif isinstance(target, VarRef):
         # This is a bit of a hack to force the generator to generate code for accessing
         # a Var directly so we can store a temp reference to that Var rather than
         # performing a global Var find twice for the same single expression.
         temp_var_name = genname("var")
         var_ast = _var_sym_to_py_ast(ctx, target.assoc(return_var=True))
         target_ast = GeneratedPyAST(
-            node=_load_attr(f"{temp_var_name}.value", ctx=ast.Store()),
+            node=_load_attr(f"{temp_var_name}.set_value"),
             dependencies=list(
                 chain(
                     var_ast.dependencies,
@@ -2676,8 +2682,10 @@ def _set_bang_to_py_ast(ctx: GeneratorContext, node: SetBang) -> GeneratedPyAST:
                 )
             ),
         )
+        assign_ast = [ast.Call(func=target_ast.node, args=[val_ast.node], keywords=[])]
     elif isinstance(target, Local):
         target_ast = _local_sym_to_py_ast(ctx, target, is_assigning=True)
+        assign_ast = [ast.Assign(targets=[target_ast.node], value=val_ast.node)]
     else:  # pragma: no cover
         raise GeneratorException(
             f"invalid set! target type {type(target)}", lisp_ast=target
@@ -2697,7 +2705,7 @@ def _set_bang_to_py_ast(ctx: GeneratorContext, node: SetBang) -> GeneratedPyAST:
                         )
                     ],
                     target_ast.dependencies,
-                    [ast.Assign(targets=[target_ast.node], value=val_ast.node)],
+                    assign_ast,
                 )
             ),
         )
@@ -2708,7 +2716,7 @@ def _set_bang_to_py_ast(ctx: GeneratorContext, node: SetBang) -> GeneratedPyAST:
                 chain(
                     val_ast.dependencies,
                     target_ast.dependencies,
-                    [ast.Assign(targets=[target_ast.node], value=val_ast.node)],
+                    assign_ast,
                 )
             ),
         )

src/basilisp/lang/interfaces.py

@@ -116,11 +116,15 @@ class IReference(IMeta):
     __slots__ = ()
 
     @abstractmethod
-    def alter_meta(self, f: Callable[..., "IPersistentMap"], *args) -> "IPersistentMap":
+    def alter_meta(
+        self, f: Callable[..., Optional["IPersistentMap"]], *args
+    ) -> Optional["IPersistentMap"]:
         raise NotImplementedError()
 
     @abstractmethod
-    def reset_meta(self, meta: "IPersistentMap") -> "IPersistentMap":
+    def reset_meta(
+        self, meta: Optional["IPersistentMap"]
+    ) -> Optional["IPersistentMap"]:
         raise NotImplementedError()
 
 

src/basilisp/lang/reference.py

@@ -18,7 +18,7 @@
 try:
     from typing import Protocol
 except ImportError:
-    AlterMeta = Callable[..., IPersistentMap]
+    AlterMeta = Callable[..., Optional[IPersistentMap]]
 else:
 
     class AlterMeta(Protocol):  # type: ignore [no-redef]
@@ -45,12 +45,12 @@ def meta(self) -> Optional[IPersistentMap]:
         with self._rlock:
             return self._meta
 
-    def alter_meta(self, f: AlterMeta, *args) -> IPersistentMap:
+    def alter_meta(self, f: AlterMeta, *args) -> Optional[IPersistentMap]:
         with self._wlock:
             self._meta = f(self._meta, *args)
             return self._meta
 
-    def reset_meta(self, meta: IPersistentMap) -> IPersistentMap:
+    def reset_meta(self, meta: Optional[IPersistentMap]) -> Optional[IPersistentMap]:
         with self._wlock:
             self._meta = meta
             return meta

src/basilisp/lang/runtime.py

@@ -277,6 +277,14 @@ def name(self) -> sym.Symbol:
     def dynamic(self) -> bool:
         return self._dynamic
 
+    def set_dynamic(self, dynamic: bool) -> None:
+        with self._wlock:
+            if dynamic == self._dynamic:
+                return
+
+            self._dynamic = dynamic
+            self._tl = _VarBindings() if dynamic else None
+
     @property
     def is_private(self) -> Optional[bool]:
         if self._meta is not None:
@@ -301,23 +309,25 @@ def root(self):
         with self._rlock:
             return self._root
 
-    @root.setter
-    def root(self, val):
+    def bind_root(self, val) -> None:
+        """Atomically update the root binding of this Var to val."""
         with self._wlock:
             self._set_root(val)
 
-    def alter_root(self, f, *args):
+    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."""
         with self._wlock:
             self._set_root(f(self._root, *args))
 
     def push_bindings(self, val):
-        if self._tl is None:
+        if not self._dynamic or self._tl is None:
             raise RuntimeException("Can only push bindings to dynamic Vars")
         self._validate(val)
         self._tl.bindings.append(val)
 
     def pop_bindings(self):
-        if self._tl is None:
+        if not self._dynamic or self._tl is None:
             raise RuntimeException("Can only pop bindings from dynamic Vars")
         return self._tl.bindings.pop()
 
@@ -330,15 +340,22 @@ def is_thread_bound(self):
 
     @property
     def value(self):
+        """Return the current value of the Var visible in the current thread.
+
+        For non-dynamic Vars, this will just be the root. For dynamic Vars, this will
+        be any thread-local binding if one is defined. Otherwise, the root value."""
         with self._rlock:
             if self._dynamic:
                 assert self._tl is not None
                 if len(self._tl.bindings) > 0:
                     return self._tl.bindings[-1]
             return self._root
 
-    @value.setter
-    def value(self, v):
+    def set_value(self, v) -> None:
+        """Set the current value of the Var.
+
+        If the Var is not dynamic, this is equivalent to binding the root value. If the
+        Var is dynamic, this will set the thread-local bindings for the Var."""
         with self._wlock:
             if self._dynamic:
                 assert self._tl is not None
@@ -350,32 +367,46 @@ def value(self, v):
                 return
             self._set_root(v)
 
-    @staticmethod
-    def intern(
+    __UNBOUND_SENTINEL = object()
+
+    @classmethod
+    def intern(  # pylint: disable=too-many-arguments
+        cls,
         ns: Union["Namespace", sym.Symbol],
         name: sym.Symbol,
         val,
         dynamic: bool = False,
-        meta=None,
+        meta: Optional[IPersistentMap] = None,
     ) -> "Var":
-        """Intern the value bound to the symbol `name` in namespace `ns`."""
+        """Intern the value bound to the symbol `name` in namespace `ns`.
+
+        If the Var already exists, it will have its root value updated to `val`,
+        its meta will be reset to match the provided `meta`, and the dynamic flag
+        will be updated to match the provided `dynamic` argument."""
         if isinstance(ns, sym.Symbol):
             ns = Namespace.get_or_create(ns)
-        var = ns.intern(name, Var(ns, name, dynamic=dynamic, meta=meta))
-        var.root = val
+        new_var = cls(ns, name, dynamic=dynamic, meta=meta)
+        var = ns.intern(name, new_var)
+        if val is not cls.__UNBOUND_SENTINEL:
+            var.bind_root(val)
+        # Namespace.intern will return an existing interned Var for the same name
+        # if one exists. We only want to set the meta and/or dynamic flag if the
+        # Var is not new.
+        if var is not new_var:
+            var.reset_meta(meta)
+            var.set_dynamic(dynamic)
         return var
 
-    @staticmethod
+    @classmethod
     def intern_unbound(
+        cls,
         ns: Union["Namespace", sym.Symbol],
         name: sym.Symbol,
         dynamic: bool = False,
-        meta=None,
+        meta: Optional[IPersistentMap] = None,
     ) -> "Var":
         """Create a new unbound `Var` instance to the symbol `name` in namespace `ns`."""
-        if isinstance(ns, sym.Symbol):
-            ns = Namespace.get_or_create(ns)
-        return ns.intern(name, Var(ns, name, dynamic=dynamic, meta=meta))
+        return cls.intern(ns, name, cls.__UNBOUND_SENTINEL, dynamic=dynamic, meta=meta)
 
     @staticmethod
     def find_in_ns(
@@ -390,8 +421,8 @@ def find_in_ns(
             return ns.find(name_sym)
         return None
 
-    @staticmethod
-    def find(ns_qualified_sym: sym.Symbol) -> "Optional[Var]":
+    @classmethod
+    def find(cls, ns_qualified_sym: sym.Symbol) -> "Optional[Var]":
         """Return the value currently bound to the name in the namespace specified
         by `ns_qualified_sym`."""
         ns = Maybe(ns_qualified_sym.ns).or_else_raise(
@@ -401,16 +432,16 @@ def find(ns_qualified_sym: sym.Symbol) -> "Optional[Var]":
         )
         ns_sym = sym.symbol(ns)
         name_sym = sym.symbol(ns_qualified_sym.name)
-        return Var.find_in_ns(ns_sym, name_sym)
+        return cls.find_in_ns(ns_sym, name_sym)
 
-    @staticmethod
-    def find_safe(ns_qualified_sym: sym.Symbol) -> "Var":
+    @classmethod
+    def find_safe(cls, ns_qualified_sym: sym.Symbol) -> "Var":
         """Return the Var currently bound to the name in the namespace specified
         by `ns_qualified_sym`. If no Var is bound to that name, raise an exception.
 
         This is a utility method to return useful debugging information when code
         refers to an invalid symbol at runtime."""
-        v = Var.find(ns_qualified_sym)
+        v = cls.find(ns_qualified_sym)
         if v is None:
             raise RuntimeException(
                 f"Unable to resolve symbol {ns_qualified_sym} in this context"
@@ -1888,7 +1919,7 @@ def bootstrap_core(compiler_opts: CompilerOpts) -> None:
 
     def in_ns(s: sym.Symbol):
         ns = Namespace.get_or_create(s)
-        _NS.value = ns
+        _NS.set_value(ns)
         return ns
 
     # Vars used in bootstrapping the runtime

tests/basilisp/compiler_test.py

@@ -5360,7 +5360,7 @@ def test_private_aliased_var_does_not_resolve(
             private_var = Var(
                 other_ns, private_var_sym, meta=lmap.map({SYM_PRIVATE_META_KEY: True})
             )
-            private_var.value = kw.keyword("private-var")
+            private_var.set_value(kw.keyword("private-var"))
             other_ns.intern(private_var_sym, private_var)
 
             with pytest.raises(compiler.CompilerException):
@@ -5401,14 +5401,14 @@ def test_fully_namespaced_sym_resolves(self, lcompile: CompileFn):
 
             # Intern a public symbol in `other.ns`
             public_var = Var(other_ns, public_var_sym)
-            public_var.value = kw.keyword("public-var")
+            public_var.set_value(kw.keyword("public-var"))
             other_ns.intern(public_var_sym, public_var)
 
             # Intern a private symbol in `other.ns`
             private_var = Var(
                 other_ns, private_var_sym, meta=lmap.map({SYM_PRIVATE_META_KEY: True})
             )
-            private_var.value = kw.keyword("private-var")
+            private_var.set_value(kw.keyword("private-var"))
             other_ns.intern(private_var_sym, private_var)
 
             with runtime.ns_bindings(third_ns_name.name):