Disallow private Vars to be resolved within macros during macroexpansion (#648)

* Disallow private Vars to be resolved within macros during macroexpansion

* Spit fire

Author: chrisrink10

CHANGELOG.md

@@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
  * 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)
+ * Fixed a bug where private Vars could be resolved from the source namespace of a public macro during macroexpansion (#648)
 
 ### Removed
  * Removed Click as a dependency in favor of builtin `argparse` (#622, #624, #636)

Makefile

@@ -29,7 +29,7 @@ test:
 lispcore.py:
 	@BASILISP_DO_NOT_CACHE_NAMESPACES=true \
 		poetry run basilisp run -c \
-		'(with [f (python/open "lispcore.py" "w")] (.write f basilisp.core/*generated-python*))'
+			'(spit "lispcore.py" @#'"'"'basilisp.core/*generated-python*)'
 	@poetry run black lispcore.py
 
 
@@ -44,4 +44,4 @@ pypy-shell:
 		--mount src=`pwd`,target=/usr/src/app,type=bind \
 		--workdir /usr/src/app \
 		pypy:3.6-7.3-slim-buster \
-		/bin/sh -c 'pip install -e . && basilisp repl'
+		/bin/sh -c 'pip install -e . && basilisp repl'

src/basilisp/core.lpy

@@ -4720,7 +4720,7 @@
       (refer-lib current-ns libspec))
     nil))
 
-(defonce ^:dynamic ^:private *loaded-libs*
+(defonce ^:dynamic *loaded-libs*
   (atom #{}))
 
 (defn loaded-libs
@@ -5794,7 +5794,7 @@
               (alter-var-root (var ~protocol-name) merge ~protocol-name)))
        ~protocol-name)))
 
-(defn ^:private protocol?
+(defn protocol?
   "Return true if x is a Protocol."
   [x]
   (boolean (and (map? x) (:interface x))))
@@ -6174,21 +6174,21 @@
   [v]
   (instance? basilisp.lang.interfaces/IRecord v))
 
-(defmulti ^:private evolve
-  (fn [v & _]
-    (cond
-      (record? v) :record
-      (map? v)    :map)))
+(defmulti evolve
+  "Implementation detail of `defrecord`."
+  (fn [v & _] (type v)))
 
-(defmethod evolve :map
+(defmethod evolve basilisp.lang.interfaces/IPersistentMap
   [m & kwargs]
   (apply assoc m kwargs))
 
-(defmethod evolve :record
+(defmethod evolve basilisp.lang.interfaces/IRecord
   [rec & kwargs]
   (->> (apply hash-map kwargs)
        (apply-kw attr/evolve rec)))
 
+(prefer-method evolve basilisp.lang.interfaces/IRecord basilisp.lang.interfaces/IPersistentMap)
+
 (defn ^:private validate-record-fields
   "Validate that record fields do not contain any reserved entries,
   are not declared as mutable, and do not declare any default values."

src/basilisp/lang/compiler/analyzer.py

@@ -307,7 +307,6 @@ class AnalyzerContext:
         "_filename",
         "_func_ctx",
         "_is_quoted",
-        "_macro_ns",
         "_opts",
         "_recur_points",
         "_should_macroexpand",
@@ -326,7 +325,6 @@ def __init__(
         self._filename = Maybe(filename).or_else_get(DEFAULT_COMPILER_FILE_PATH)
         self._func_ctx: Deque[FunctionContext] = collections.deque([])
         self._is_quoted: Deque[bool] = collections.deque([])
-        self._macro_ns: Deque[Optional[runtime.Namespace]] = collections.deque([])
         self._opts = (
             Maybe(opts).map(lmap.map).or_else_get(lmap.PersistentMap.empty())  # type: ignore
         )
@@ -383,35 +381,6 @@ def quoted(self):
         yield
         self._is_quoted.pop()
 
-    @property
-    def current_macro_ns(self) -> Optional[runtime.Namespace]:
-        """Return the current transient namespace available during macroexpansion.
-
-        If None, the analyzer should only use the current namespace for symbol
-        resolution."""
-        try:
-            return self._macro_ns[-1]
-        except IndexError:
-            return None
-
-    @contextlib.contextmanager
-    def macro_ns(self, ns: Optional[runtime.Namespace]):
-        """Set the transient namespace which is available to the analyzer during a
-        macroexpansion phase.
-
-        If set to None, prohibit the analyzer from using another namespace for symbol
-        resolution.
-
-        During macroexpansion, new forms referenced from the macro namespace would
-        be unavailable to the namespace containing the original macro invocation.
-        The macro namespace is a temporary override pointing to the namespace of the
-        macro definition which can be used to resolve these transient references."""
-        self._macro_ns.append(ns)
-        try:
-            yield
-        finally:
-            self._macro_ns.pop()
-
     @property
     def should_allow_unresolved_symbols(self) -> bool:
         """If True, the analyzer will allow unresolved symbols. This is primarily
@@ -2285,9 +2254,7 @@ def _invoke_ast(form: Union[llist.PersistentList, ISeq], ctx: AnalyzerContext) -
                         expanded = expanded.with_meta(
                             old_meta.cons(form.meta) if old_meta else form.meta
                         )
-                    with ctx.macro_ns(
-                        fn.var.ns if fn.var.ns is not ctx.current_ns else None
-                    ), ctx.expr_pos():
+                    with ctx.expr_pos():
                         expanded_ast = _analyze_form(expanded, ctx)
 
                     # Verify that macroexpanded code also does not have any
@@ -3117,15 +3084,7 @@ def __resolve_namespaced_symbol(  # pylint: disable=too-many-branches  # noqa: M
     v = Var.find(form)
     if v is not None:
         # Disallow global references to Vars defined with :private metadata
-        #
-        # Global references to private Vars are allowed in macroexpanded code
-        # as long as the macro referencing the private Var is in the same
-        # Namespace as the private Var (via `ctx.current_macro_ns`)
-        if (
-            v.ns != ctx.current_macro_ns
-            and v.meta is not None
-            and v.meta.val_at(SYM_PRIVATE_META_KEY, False)
-        ):
+        if v.meta is not None and v.meta.val_at(SYM_PRIVATE_META_KEY, False):
             raise AnalyzerException(
                 f"cannot resolve private Var {form.name} from namespace {form.ns}",
                 form=form,
@@ -3219,16 +3178,6 @@ def __resolve_bare_symbol(
             env=ctx.get_node_env(pos=ctx.syntax_position),
         )
 
-    # Look up the symbol in the current macro namespace, if one
-    if ctx.current_macro_ns is not None:
-        v = ctx.current_macro_ns.find(form)
-        if v is not None:
-            return VarRef(
-                form=form,
-                var=v,
-                env=ctx.get_node_env(pos=ctx.syntax_position),
-            )
-
     if "." in form.name:
         raise AnalyzerException(
             "symbol names may not contain the '.' operator", form=form