Fix symbol warnings (#312)

* Fix symbol warnings

* Test coverage for uncovered lines

Author: chrisrink10

src/basilisp/cli.py

@@ -74,7 +74,7 @@ def bootstrap_repl(which_ns: str) -> types.ModuleType:
 )
 @click.option(
     "--warn-on-shadowed-var",
-    default=True,
+    default=False,
     is_flag=True,
     envvar="BASILISP_WARN_ON_SHADOWED_VAR",
     help="if provided, emit warnings if a Var name is shadowed by a local name",
@@ -162,7 +162,7 @@ def repl(
 )
 @click.option(
     "--warn-on-shadowed-var",
-    default=True,
+    default=False,
     is_flag=True,
     envvar="BASILISP_WARN_ON_SHADOWED_VAR",
     help="if provided, emit warnings if a Var name is shadowed by a local name",

src/basilisp/core/__init__.lpy

@@ -418,39 +418,6 @@
                       {:first (first clauses)})))
           (cons 'basilisp.core/cond (nthrest clauses 2)))))
 
-(defmacro condp
-  "Take a predicate and an expression and a series of clauses, call
-  (pred test expr) on the first expression for each clause. The result
-  expression from first the set of clauses for which this expression
-  returns a truthy value will be returned from the condp expression.
-
-  Clauses can take two forms:
-
-  - test-expr result-expr
-  - test-expr :>> result-fn, where :>> is a keyword literal
-
-  For the ternary expression clause, the unary result-fn will be called
-  with the result of the predicate."
-  [pred expr & clauses]
-  (when (seq clauses)
-    (let [test-expr (first clauses)
-          remaining (rest clauses)]
-      (if (seq remaining)
-        (let [result    (first remaining)
-              remaining (rest remaining)]
-          (cond
-            (= result :>>) `(let [res# ~(list pred test-expr expr)]
-                              (if res#
-                                (~(first remaining) res#)
-                                (condp ~pred ~expr ~@(rest remaining))))
-            result         `(if ~(list pred test-expr expr)
-                              ~result
-                              (condp ~pred ~expr ~@remaining))
-            :else          (throw
-                            (ex-info "expected result expression"
-                                     {:test test-expr}))))
-        test-expr))))
-
 (defn not
   "Return the logical negation of expr."
   [expr]
@@ -1089,6 +1056,39 @@
   [& forms]
   nil)
 
+(defmacro condp
+  "Take a predicate and an expression and a series of clauses, call
+  (pred test expr) on the first expression for each clause. The result
+  expression from first the set of clauses for which this expression
+  returns a truthy value will be returned from the condp expression.
+
+  Clauses can take two forms:
+
+  - test-expr result-expr
+  - test-expr :>> result-fn, where :>> is a keyword literal
+
+  For the ternary expression clause, the unary result-fn will be called
+  with the result of the predicate."
+  [pred expr & clauses]
+  (when (seq clauses)
+    (let [test-expr (first clauses)
+          remaining (rest clauses)]
+      (if (seq remaining)
+        (let [result    (first remaining)
+              remaining (rest remaining)]
+          (cond
+            (= result :>>) `(let [res# ~(list pred test-expr expr)]
+                              (if res#
+                                (~(first remaining) res#)
+                                (condp ~pred ~expr ~@(rest remaining))))
+            result         `(if ~(list pred test-expr expr)
+                              ~result
+                              (condp ~pred ~expr ~@remaining))
+            :else          (throw
+                            (ex-info "expected result expression"
+                                     {:test test-expr}))))
+        test-expr))))
+
 (defmacro new
   "Create a new instance of class with args.
 
@@ -1621,7 +1621,7 @@
       ;; Add refers
       (cond
         (= :all (:refer opts))
-        (.refer-all which-ns new-ns)
+        (.refer-all current-ns new-ns)
 
         (:refer opts)
         (add-refers current-ns new-ns (:refer opts))

src/basilisp/lang/compiler.py

@@ -212,7 +212,7 @@ def warn_on_shadowed_var(self) -> bool:
         Implied by warn_on_shadowed_name. The value of warn_on_shadowed_name
         supersedes the value of this flag."""
         return self.warn_on_shadowed_name or self._opts.entry(
-            WARN_ON_SHADOWED_VAR, True
+            WARN_ON_SHADOWED_VAR, False
         )
 
     @property
@@ -1842,6 +1842,8 @@ def _resolve_sym_var(ctx: CompilerContext, v: Var) -> Optional[str]:
             safe_ns = munge(v.ns.name)
             return f"{safe_ns}.{safe_name}"
 
+    if ctx.warn_on_var_indirection:
+        logger.warning(f"could not resolve a direct link to Var '{v.name}'")
     return None
 
 
@@ -1880,12 +1882,8 @@ def _resolve_sym(ctx: CompilerContext, form: sym.Symbol) -> Optional[str]:  # no
 
             # Otherwise, try to direct-link it like a Python variable
             safe_ns = munge(form.ns)
-            try:
-                ns_module = sys.modules[safe_ns]
-            except KeyError:
-                # This should never happen. A module listed in the namespace
-                # imports should always be imported already.
-                raise CompilerException(f"Module '{safe_ns}' is not imported")
+            assert safe_ns in sys.modules, f"Module '{safe_ns}' is not imported"
+            ns_module = sys.modules[safe_ns]
 
             # Try without allowing builtins first
             safe_name = munge(form.name)
@@ -1898,12 +1896,18 @@ def _resolve_sym(ctx: CompilerContext, form: sym.Symbol) -> Optional[str]:  # no
                 return f"{safe_ns}.{safe_name}"
 
             # If neither resolve, then defer to a Var.find
+            if ctx.warn_on_var_indirection:
+                logger.warning(
+                    f"could not resolve a direct link to Python variable '{form}'"
+                )
             return None
         elif ns_sym in ctx.current_ns.aliases:
             aliased_ns: runtime.Namespace = ctx.current_ns.aliases[ns_sym]
             v = Var.find(sym.symbol(form.name, ns=aliased_ns.name))
             if v is not None:
                 return _resolve_sym_var(ctx, v)
+            if ctx.warn_on_var_indirection:
+                logger.warning(f"could not resolve a direct link to Var '{form}'")
             return None
 
     # Look up the symbol in the namespace mapping of the current namespace.
@@ -1914,6 +1918,8 @@ def _resolve_sym(ctx: CompilerContext, form: sym.Symbol) -> Optional[str]:  # no
     if v is not None:
         return _resolve_sym_var(ctx, v)
 
+    if ctx.warn_on_var_indirection:
+        logger.warning(f"could not resolve a direct link to Var '{form}'")
     return None
 
 
@@ -1974,9 +1980,6 @@ def _sym_ast(ctx: CompilerContext, form: sym.Symbol) -> ASTStream:
         return
 
     # If we couldn't find the symbol anywhere else, generate a Var.find call
-    # and issue a warning if warn_on_var_indirection is active.
-    if ctx.warn_on_var_indirection:
-        logger.warning(f"could not resolve a direct link to Var '{form}'")
     yield _node(
         ast.Attribute(
             value=ast.Call(func=_FIND_VAR_FN_NAME, args=[base_sym], keywords=[]),

src/basilisp/lang/runtime.py

@@ -34,6 +34,10 @@
 _GENERATED_PYTHON_VAR_NAME = "*generated-python*"
 _PRINT_GENERATED_PY_VAR_NAME = "*print-generated-python*"
 
+_DYNAMIC_META_KEY = kw.keyword("dynamic")
+_PRIVATE_META_KEY = kw.keyword("private")
+_REDEF_META_KEY = kw.keyword("redef")
+
 _CATCH = sym.symbol("catch")
 _DEF = sym.symbol("def")
 _DO = sym.symbol("do")
@@ -97,6 +101,15 @@ def __init__(
         self._tl = threading.local()
         self._meta = meta
 
+        if dynamic:
+            # If this var was created with the dynamic keyword argument, then the
+            # Var metadata should also specify that the Var is dynamic.
+            if isinstance(self._meta, lmap.Map):
+                if not self._meta.entry(_DYNAMIC_META_KEY):
+                    self._meta = self._meta.assoc(_DYNAMIC_META_KEY, True)
+            else:
+                self._meta = lmap.map({_DYNAMIC_META_KEY: True})
+
     def __repr__(self):
         return f"#'{self.ns.name}/{self.name}"
 
@@ -127,7 +140,7 @@ def dynamic(self, is_dynamic: bool):
     @property
     def is_private(self) -> Optional[bool]:
         try:
-            return self.meta.entry(kw.keyword("private"))
+            return self.meta.entry(_PRIVATE_META_KEY)
         except AttributeError:
             return False
 
@@ -175,9 +188,8 @@ def intern(
     ) -> "Var":
         """Intern the value bound to the symbol `name` in namespace `ns`."""
         var_ns = Namespace.get_or_create(ns)
-        var = var_ns.intern(name, Var(var_ns, name, dynamic=dynamic))
+        var = var_ns.intern(name, Var(var_ns, name, dynamic=dynamic, meta=meta))
         var.root = val
-        var.meta = meta
         return var
 
     @staticmethod
@@ -186,9 +198,7 @@ def intern_unbound(
     ) -> "Var":
         """Create a new unbound `Var` instance to the symbol `name` in namespace `ns`."""
         var_ns = Namespace.get_or_create(ns)
-        var = var_ns.intern(name, Var(var_ns, name, dynamic=dynamic))
-        var.meta = meta
-        return var
+        return var_ns.intern(name, Var(var_ns, name, dynamic=dynamic, meta=meta))
 
     @staticmethod
     def find_in_ns(ns_sym: sym.Symbol, name_sym: sym.Symbol) -> "Optional[Var]":
@@ -929,7 +939,7 @@ def add_generated_python(
             sym.symbol(var_name),
             "",
             dynamic=True,
-            meta=lmap.map({kw.keyword("private"): True}),
+            meta=lmap.map({_PRIVATE_META_KEY: True}),
         )
     )
     v.value = v.value + generated_python
@@ -973,19 +983,26 @@ def in_ns(s: sym.Symbol):
 
     Var.intern_unbound(core_ns_sym, sym.symbol("unquote"))
     Var.intern_unbound(core_ns_sym, sym.symbol("unquote-splicing"))
-    Var.intern(core_ns_sym, sym.symbol("set!"), set_BANG_)
-    Var.intern(core_ns_sym, sym.symbol("in-ns"), in_ns)
+    Var.intern(
+        core_ns_sym,
+        sym.symbol("set!"),
+        set_BANG_,
+        meta=lmap.map({_REDEF_META_KEY: True}),
+    )
+    Var.intern(
+        core_ns_sym, sym.symbol("in-ns"), in_ns, meta=lmap.map({_REDEF_META_KEY: True})
+    )
     Var.intern(
         core_ns_sym,
         sym.symbol(_PRINT_GENERATED_PY_VAR_NAME),
         False,
         dynamic=True,
-        meta=lmap.map({kw.keyword("private"): True}),
+        meta=lmap.map({_PRIVATE_META_KEY: True}),
     )
     Var.intern(
         core_ns_sym,
         sym.symbol(_GENERATED_PYTHON_VAR_NAME),
         "",
         dynamic=True,
-        meta=lmap.map({kw.keyword("private"): True}),
+        meta=lmap.map({_PRIVATE_META_KEY: True}),
     )

tests/basilisp/compiler_test.py

@@ -329,25 +329,27 @@ def test_fn_warn_on_shadow_var(ns: runtime.Namespace):
         logger.warning.assert_not_called()
 
     with mock.patch("basilisp.lang.compiler.logger") as logger:
-        lcompile(
-            """
+        code = """
         (def unique-kuieeid :a)
         (fn [unique-kuieeid] unique-kuieeid)
         """
+        lcompile(
+            code, ctx=compiler.CompilerContext({compiler.WARN_ON_SHADOWED_VAR: True})
         )
 
         logger.warning.assert_called_once_with(
             "name 'unique-kuieeid' shadows def'ed Var from outer scope"
         )
 
     with mock.patch("basilisp.lang.compiler.logger") as logger:
-        lcompile(
-            """
+        code = """
         (def unique-peuudcdf :a)
         (fn
           ([] :b)
           ([unique-peuudcdf] unique-peuudcdf))
         """
+        lcompile(
+            code, ctx=compiler.CompilerContext({compiler.WARN_ON_SHADOWED_VAR: True})
         )
 
         logger.warning.assert_called_once_with(
@@ -654,11 +656,12 @@ def test_let_warn_on_shadow_var(ns: runtime.Namespace):
         logger.warning.assert_not_called()
 
     with mock.patch("basilisp.lang.compiler.logger") as logger:
-        lcompile(
-            """
+        code = """
         (def unique-uoieyqq :a)
         (let [unique-uoieyqq 3] unique-uoieyqq)
         """
+        lcompile(
+            code, ctx=compiler.CompilerContext({compiler.WARN_ON_SHADOWED_VAR: True})
         )
         logger.warning.assert_called_once_with(
             "name 'unique-uoieyqq' shadows def'ed Var from outer scope"
@@ -991,6 +994,53 @@ def test_aliased_macro_symbol_resolution(ns: runtime.Namespace):
         runtime.Namespace.remove(other_ns_name)
 
 
+def test_warn_on_var_indirection_cross_ns(ns: runtime.Namespace):
+    current_ns: runtime.Namespace = ns
+    other_ns_name = sym.symbol("other.ns")
+    try:
+        other_ns = runtime.Namespace.get_or_create(other_ns_name)
+        current_ns.add_alias(other_ns_name, other_ns)
+        current_ns.add_alias(sym.symbol("other"), other_ns)
+
+        with runtime.ns_bindings(current_ns.name):
+            with mock.patch("basilisp.lang.compiler.logger") as logger:
+                lcompile(
+                    "(fn [] (other.ns/m :z))",
+                    ctx=compiler.CompilerContext({compiler.WARN_ON_SHADOWED_VAR: True}),
+                )
+
+                logger.warning.assert_called_once_with(
+                    "could not resolve a direct link to Var 'other.ns/m'"
+                )
+
+            with mock.patch("basilisp.lang.compiler.logger") as logger:
+                lcompile(
+                    "(fn [] (other/m :z))",
+                    ctx=compiler.CompilerContext({compiler.WARN_ON_SHADOWED_VAR: True}),
+                )
+
+                logger.warning.assert_called_once_with(
+                    "could not resolve a direct link to Var 'other/m'"
+                )
+    finally:
+        runtime.Namespace.remove(other_ns_name)
+
+
+def test_warn_on_var_indirection_on_import(ns: runtime.Namespace):
+    ns.add_import(sym.symbol("string"), __import__("string"))
+
+    with runtime.ns_bindings(ns.name):
+        with mock.patch("basilisp.lang.compiler.logger") as logger:
+            lcompile(
+                "(fn [] (string/m :z))",
+                ctx=compiler.CompilerContext({compiler.WARN_ON_SHADOWED_VAR: True}),
+            )
+
+            logger.warning.assert_called_once_with(
+                "could not resolve a direct link to Python variable 'string/m'"
+            )
+
+
 def test_var(ns: runtime.Namespace):
     code = """
     (def some-var "a value")