Use a single lock in Namespace instances rather than Atoms (#494)

chrisrink10 · web-flow · commit fbbc1c14b1e2 · 2020-03-05T22:05:36.000-05:00
* Use a single lock in Namespace instances rather than Atoms

* Change the Namespace.add_alias signature

* Fix the core, obviously
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,12 +13,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
  * Added `defn-`, `declare`, and `defonce` macros (#480)
  * Added EDN reader in the `basilisp.edn` namespace (#477)
  * Added line, column, and file information to reader `SyntaxError`s (#488)
+ * Added context information to the `CompilerException` string output (#493)
 
 ### Changed
  * Change the default user namespace to `basilisp.user` (#466)
  * Changed multi-methods to use a `threading.Lock` internally rather than an Atom (#478)
  * Changed the Basilisp module type from `types.ModuleType` to a custom subtype with support for custom attributes (#482)
  * Basilisp's runtime function `Namespace.get_or_create` no longer refers `basilisp.core` by default, which allows callers to exclude `basilisp.core` names in the `ns` macro (#481)
+ * Namespaces now use a single internal lock rather than putting each property inside of an Atom (#494)
 
 ### Fixed
  * Fixed a reader bug where no exception was being thrown splicing reader conditional forms appeared outside of valid splicing contexts (#470)
diff --git a/src/basilisp/cli.py b/src/basilisp/cli.py
@@ -61,7 +61,7 @@ def bootstrap_repl(which_ns: str) -> types.ModuleType:
     assert core_ns is not None
     ns.refer_all(core_ns)
     repl_module = importlib.import_module(REPL_NS)
-    ns.add_alias(sym.symbol(REPL_NS), repl_ns)
+    ns.add_alias(repl_ns, sym.symbol(REPL_NS))
     ns.refer_all(repl_ns)
     return repl_module
 
diff --git a/src/basilisp/core.lpy b/src/basilisp/core.lpy
@@ -3471,7 +3471,7 @@
     (importlib/import-module (name ns-sym))
     (let [new-ns (the-ns ns-sym)]
       ;; Add the namespace alias
-      (.add-alias current-ns (or (:as opts) ns-sym) new-ns)
+      (.add-alias current-ns new-ns (or (:as opts) ns-sym))
 
       ;; Add refers
       (cond
diff --git a/src/basilisp/edn.lpy b/src/basilisp/edn.lpy
@@ -212,7 +212,7 @@
                                                    (update m k inc)
                                                    (assoc m k 1)))
                                                {})
-                                       (filter (fn [[k n]] (> n 1)))
+                                       (filter (fn [[_ n]] (> n 1)))
                                        (ffirst))}))
                 coll-set)))]
     (read-coll reader opts set-if-valid "}" "set")))
diff --git a/src/basilisp/lang/runtime.py b/src/basilisp/lang/runtime.py
@@ -425,20 +425,18 @@ def __init__(self, name: sym.Symbol, module: BasilispModule = None) -> None:
         self._meta: Optional[IPersistentMap] = None
         self._lock = threading.Lock()
 
-        self._aliases: Atom[NamespaceMap] = Atom(lmap.Map.empty())
-        self._imports: Atom[ModuleMap] = Atom(
-            lmap.map(
-                dict(
-                    map(
-                        lambda s: (s, importlib.import_module(s.name)),
-                        Namespace.DEFAULT_IMPORTS.deref(),
-                    )
+        self._aliases: NamespaceMap = lmap.Map.empty()
+        self._imports: ModuleMap = lmap.map(
+            dict(
+                map(
+                    lambda s: (s, importlib.import_module(s.name)),
+                    Namespace.DEFAULT_IMPORTS.deref(),
                 )
             )
         )
-        self._import_aliases: Atom[AliasMap] = Atom(lmap.Map.empty())
-        self._interns: Atom[VarMap] = Atom(lmap.Map.empty())
-        self._refers: Atom[VarMap] = Atom(lmap.Map.empty())
+        self._import_aliases: AliasMap = lmap.Map.empty()
+        self._interns: VarMap = lmap.Map.empty()
+        self._refers: VarMap = lmap.Map.empty()
 
     @classmethod
     def add_default_import(cls, module: str):
@@ -470,129 +468,133 @@ def module(self, m: BasilispModule):
     def aliases(self) -> NamespaceMap:
         """A mapping between a symbolic alias and another Namespace. The
         fully qualified name of a namespace is also an alias for itself."""
-        return self._aliases.deref()
+        with self._lock:
+            return self._aliases
 
     @property
     def imports(self) -> ModuleMap:
         """A mapping of names to Python modules imported into the current
         namespace."""
-        return self._imports.deref()
+        with self._lock:
+            return self._imports
 
     @property
     def import_aliases(self) -> AliasMap:
         """A mapping of a symbolic alias and a Python module name."""
-        return self._import_aliases.deref()
+        with self._lock:
+            return self._import_aliases
 
     @property
     def interns(self) -> VarMap:
         """A mapping between a symbolic name and a Var. The Var may point to
         code, data, or nothing, if it is unbound. Vars in `interns` are
         interned in _this_ namespace."""
-        return self._interns.deref()
+        with self._lock:
+            return self._interns
 
     @property
     def refers(self) -> VarMap:
         """A mapping between a symbolic name and a Var. Vars in refers are
         interned in another namespace and are only referred to without an
         alias in this namespace."""
-        return self._refers.deref()
+        with self._lock:
+            return self._refers
 
     def __repr__(self):
         return f"{self._name}"
 
     def __hash__(self):
         return hash(self._name)
 
-    def add_alias(self, alias: sym.Symbol, namespace: "Namespace") -> None:
-        """Add a Symbol alias for the given Namespace."""
-        self._aliases.swap(lambda m: m.assoc(alias, namespace))
+    def add_alias(self, namespace: "Namespace", *aliases: sym.Symbol) -> None:
+        """Add Symbol aliases for the given Namespace."""
+        with self._lock:
+            new_m = self._aliases
+            for alias in aliases:
+                new_m = new_m.assoc(alias, namespace)
+            self._aliases = new_m
 
     def get_alias(self, alias: sym.Symbol) -> "Optional[Namespace]":
         """Get the Namespace aliased by Symbol or None if it does not exist."""
-        return self.aliases.val_at(alias, None)
+        with self._lock:
+            return self._aliases.val_at(alias, None)
 
     def remove_alias(self, alias: sym.Symbol) -> None:
         """Remove the Namespace aliased by Symbol. Return None."""
-        self._aliases.swap(lambda m: m.dissoc(alias))
+        with self._lock:
+            self._aliases = self._aliases.dissoc(alias)
 
     def intern(self, sym: sym.Symbol, var: Var, force: bool = False) -> Var:
         """Intern the Var given in this namespace mapped by the given Symbol.
         If the Symbol already maps to a Var, this method _will not overwrite_
         the existing Var mapping unless the force keyword argument is given
         and is True."""
-        m: lmap.Map = self._interns.swap(Namespace._intern, sym, var, force=force)
-        return m.val_at(sym)
-
-    @staticmethod
-    def _intern(
-        m: lmap.Map, sym: sym.Symbol, new_var: Var, force: bool = False
-    ) -> lmap.Map:
-        """Swap function used by intern to atomically intern a new variable in
-        the symbol mapping for this Namespace."""
-        var = m.val_at(sym, None)
-        if var is None or force:
-            return m.assoc(sym, new_var)
-        return m
+        with self._lock:
+            old_var = self._interns.val_at(sym, None)
+            if old_var is None or force:
+                self._interns = self._interns.assoc(sym, var)
+            return self._interns.val_at(sym)
 
     def unmap(self, sym: sym.Symbol) -> None:
-        self._interns.swap(lambda m: m.dissoc(sym))
+        with self._lock:
+            self._interns = self._interns.dissoc(sym)
 
     def find(self, sym: sym.Symbol) -> Optional[Var]:
         """Find Vars mapped by the given Symbol input or None if no Vars are
         mapped by that Symbol."""
-        v = self.interns.val_at(sym, None)
-        if v is None:
-            return self.refers.val_at(sym, None)
-        return v
+        with self._lock:
+            v = self._interns.val_at(sym, None)
+            if v is None:
+                return self._refers.val_at(sym, None)
+            return v
 
     def add_import(self, sym: sym.Symbol, module: Module, *aliases: sym.Symbol) -> None:
         """Add the Symbol as an imported Symbol in this Namespace. If aliases are given,
         the aliases will be applied to the """
-        self._imports.swap(lambda m: m.assoc(sym, module))
-        if aliases:
-            self._import_aliases.swap(
-                lambda m: m.assoc(
-                    *itertools.chain.from_iterable([(alias, sym) for alias in aliases])
-                )
-            )
+        with self._lock:
+            self._imports = self._imports.assoc(sym, module)
+            if aliases:
+                m = self._import_aliases
+                for alias in aliases:
+                    m = m.assoc(alias, sym)
+                self._import_aliases = m
 
     def get_import(self, sym: sym.Symbol) -> Optional[BasilispModule]:
         """Return the module if a moduled named by sym has been imported into
         this Namespace, None otherwise.
 
         First try to resolve a module directly with the given name. If no module
         can be resolved, attempt to resolve the module using import aliases."""
-        mod = self.imports.val_at(sym, None)
-        if mod is None:
-            alias = self.import_aliases.get(sym, None)
-            if alias is None:
-                return None
-            return self.imports.val_at(alias, None)
-        return mod
+        with self._lock:
+            mod = self._imports.val_at(sym, None)
+            if mod is None:
+                alias = self._import_aliases.get(sym, None)
+                if alias is None:
+                    return None
+                return self._imports.val_at(alias, None)
+            return mod
 
     def add_refer(self, sym: sym.Symbol, var: Var) -> None:
         """Refer var in this namespace under the name sym."""
         if not var.is_private:
-            self._refers.swap(lambda s: s.assoc(sym, var))
+            with self._lock:
+                self._refers = self._refers.assoc(sym, var)
 
     def get_refer(self, sym: sym.Symbol) -> Optional[Var]:
         """Get the Var referred by Symbol or None if it does not exist."""
-        return self.refers.val_at(sym, None)
+        with self._lock:
+            return self._refers.val_at(sym, None)
 
-    @classmethod
-    def __refer_all(cls, refers: lmap.Map, other_ns_interns: lmap.Map) -> lmap.Map:
-        """Refer all _public_ interns from another namespace."""
-        final_refers = refers
-        for entry in other_ns_interns:
-            s: sym.Symbol = entry.key
-            var: Var = entry.value
-            if not var.is_private:
-                final_refers = final_refers.assoc(s, var)
-        return final_refers
-
-    def refer_all(self, other_ns: "Namespace"):
+    def refer_all(self, other_ns: "Namespace") -> None:
         """Refer all the Vars in the other namespace."""
-        self._refers.swap(Namespace.__refer_all, other_ns.interns)
+        with self._lock:
+            final_refers = self._refers
+            for entry in other_ns.interns:
+                s: sym.Symbol = entry.key
+                var: Var = entry.value
+                if not var.is_private:
+                    final_refers = final_refers.assoc(s, var)
+            self._refers = final_refers
 
     @classmethod
     def ns_cache(cls) -> lmap.Map:
diff --git a/src/basilisp/testrunner.py b/src/basilisp/testrunner.py
@@ -13,13 +13,16 @@
 from basilisp.lang.obj import lrepr
 from basilisp.util import Maybe
 
-basilisp.init()
-importlib.import_module("basilisp.test")
-
 _COLLECTED_TESTS_SYM = sym.symbol("collected-tests", ns="basilisp.test")
 _CURRENT_NS_SYM = sym.symbol("current-ns", ns="basilisp.test")
 
 
+# pylint: disable=unused-argument
+def pytest_configure(config):
+    basilisp.init()
+    importlib.import_module("basilisp.test")
+
+
 def pytest_collect_file(parent, path):
     """Primary PyTest hook to identify Basilisp test files."""
     if path.ext == ".lpy":
diff --git a/tests/basilisp/compiler_test.py b/tests/basilisp/compiler_test.py
@@ -3123,8 +3123,8 @@ def test_aliased_var_does_not_resolve(
         other_ns_name = sym.symbol("other.ns")
         try:
             other_ns = get_or_create_ns(other_ns_name)
-            current_ns.add_alias(other_ns_name, other_ns)
-            current_ns.add_alias(sym.symbol("other"), other_ns)
+            current_ns.add_alias(other_ns, other_ns_name)
+            current_ns.add_alias(other_ns, sym.symbol("other"))
 
             with pytest.raises(compiler.CompilerException):
                 lcompile("(other/m :arg)")
@@ -3138,8 +3138,8 @@ def test_aliased_macro_symbol_resolution(
         other_ns_name = sym.symbol("other.ns")
         try:
             other_ns = get_or_create_ns(other_ns_name)
-            current_ns.add_alias(other_ns_name, other_ns)
-            current_ns.add_alias(sym.symbol("other"), other_ns)
+            current_ns.add_alias(other_ns, other_ns_name)
+            current_ns.add_alias(other_ns, sym.symbol("other"))
 
             with runtime.ns_bindings(other_ns_name.name):
                 lcompile("(def ^:macro m (fn* [&env &form v] v))")
@@ -3204,10 +3204,10 @@ def test_cross_ns_macro_symbol_resolution(
         third_ns_name = sym.symbol("third.ns")
         try:
             other_ns = get_or_create_ns(other_ns_name)
-            current_ns.add_alias(other_ns_name, other_ns)
+            current_ns.add_alias(other_ns, other_ns_name)
 
             third_ns = get_or_create_ns(third_ns_name)
-            other_ns.add_alias(third_ns_name, third_ns)
+            other_ns.add_alias(third_ns, third_ns_name)
 
             with runtime.ns_bindings(third_ns_name.name):
                 lcompile(
@@ -3241,10 +3241,10 @@ def test_cross_ns_macro_symbol_resolution_with_aliases(
         third_ns_name = sym.symbol("third.ns")
         try:
             other_ns = get_or_create_ns(other_ns_name)
-            current_ns.add_alias(other_ns_name, other_ns)
+            current_ns.add_alias(other_ns, other_ns_name)
 
             third_ns = get_or_create_ns(third_ns_name)
-            other_ns.add_alias(sym.symbol("third"), third_ns)
+            other_ns.add_alias(third_ns, sym.symbol("third"))
 
             with runtime.ns_bindings(third_ns_name.name):
                 lcompile(
@@ -3278,7 +3278,7 @@ def test_cross_ns_macro_symbol_resolution_with_refers(
         third_ns_name = sym.symbol("third.ns")
         try:
             other_ns = get_or_create_ns(other_ns_name)
-            current_ns.add_alias(other_ns_name, other_ns)
+            current_ns.add_alias(other_ns, other_ns_name)
 
             third_ns = get_or_create_ns(third_ns_name)
 
@@ -3313,8 +3313,8 @@ def other_ns(self, lcompile: CompileFn, ns: runtime.Namespace):
         try:
             other_ns = get_or_create_ns(other_ns_name)
             Var.intern(other_ns_name, sym.symbol("m"), lambda x: x)
-            current_ns.add_alias(other_ns_name, other_ns)
-            current_ns.add_alias(sym.symbol("other"), other_ns)
+            current_ns.add_alias(other_ns, other_ns_name)
+            current_ns.add_alias(other_ns, sym.symbol("other"))
 
             with runtime.ns_bindings(current_ns.name):
                 yield
diff --git a/tests/basilisp/namespace_test.py b/tests/basilisp/namespace_test.py
@@ -277,7 +277,7 @@ def test_alias(ns_cache: atom.Atom[NamespaceMap]):
     ns1 = get_or_create_ns(sym.symbol("ns1"))
     ns2 = get_or_create_ns(sym.symbol("ns2"))
 
-    ns1.add_alias(sym.symbol("n2"), ns2)
+    ns1.add_alias(ns2, sym.symbol("n2"))
 
     assert None is ns1.get_alias(sym.symbol("ns2"))
     assert ns2 is ns1.get_alias(sym.symbol("n2"))
@@ -301,10 +301,10 @@ def ns(self) -> Namespace:
         str_ns.intern(
             chars_sym, Var(ns, chars_sym, meta=lmap.map({kw.keyword("private"): True}))
         )
-        ns.add_alias(str_ns_alias, str_ns)
+        ns.add_alias(str_ns, str_ns_alias)
 
         str_alias = sym.symbol("str")
-        ns.add_alias(str_alias, Namespace(str_alias))
+        ns.add_alias(Namespace(str_alias), str_alias)
 
         str_sym = sym.symbol("str")
         ns.intern(str_sym, Var(ns, str_sym))
diff --git a/tests/basilisp/runtime_test.py b/tests/basilisp/runtime_test.py
@@ -420,7 +420,7 @@ def test_resolve_alias(self, core_ns):
 
             foo_ns_sym = sym.symbol("zux.bar.foo")
             foo_ns = get_or_create_ns(foo_ns_sym)
-            ns.add_alias(sym.symbol("foo"), foo_ns)
+            ns.add_alias(foo_ns, sym.symbol("foo"))
             assert sym.symbol(
                 "aliased-var", ns=foo_ns_sym.name
             ) == runtime.resolve_alias(sym.symbol("aliased-var", ns="foo"), ns=ns)
