Implement minimal macro namespacing and add tests

This commit adds just enough namespacing to resolve a macro first in the macro's
defining module's namespace (i.e. the module assigned to the `HyASTCompiler`),
then in the namespace/module it's evaluated in.  Namespacing is accomplished by
adding a `module` attribute to `HySymbol`, so that `HyExpression`s can be
checked for this definition namespace attribute and their car symbol resolved
per the above.

As well, a couple tests have been added that cover
- the loading of module-level macros
  - e.g. that only macros defined in the `require`d module are added
- the AST generated for `require`
  - using macros loaded from modules imported via bytecode
- the non-local macro namespace resolution described above
  - a `require`d macro that uses a macro `require` exclusively in its
    module-level namespace
- and that (second-degree `require`d) macros can reference variables within
  their module-level namespaces.

Closes hylang#1268, closes hylang#1650, closes hylang#1416.

hy/compiler.py

@@ -1744,7 +1744,8 @@ def compile_dict(self, m):
         return ret + asty.Dict(m, keys=keyvalues[::2], values=keyvalues[1::2])
 
 
-def hy_compile(tree, module, root=ast.Module, get_expr=False, include_require=True):
+def hy_compile(tree, module, root=ast.Module, get_expr=False,
+               include_require=True):
     """
     Compile a Hy tree into a Python AST tree.
 
@@ -1774,9 +1775,9 @@ def hy_compile(tree, module, root=ast.Module, get_expr=False, include_require=Tr
             module = types.ModuleType(module)
         else:
             module = importlib.import_module(ast_str(module, piecewise=True))
-    elif not inspect.ismodule(module):
-        raise TypeError('Invalid module type: {}'.format(type(module)))
 
+    if not inspect.ismodule(module):
+        raise TypeError('Invalid module type: {}'.format(type(module)))
 
     tree = wrap_value(tree)
     if not isinstance(tree, HyObject):

hy/importer.py

@@ -18,6 +18,7 @@
 
 from functools import partial
 from contextlib import contextmanager
+from warnings import warn
 
 from hy.errors import HyTypeError
 from hy.compiler import hy_compile, ast_str
@@ -419,6 +420,7 @@ def load_module(self, fullname=None):
                     if self.file:
                         self.file.close()
 
+            mod.__cached__ = cache_from_source(self.filename)
             mod.__loader__ = self
             return mod
 
@@ -453,8 +455,8 @@ def byte_compile_hy(self, fullname=None):
             if not sys.dont_write_bytecode:
                 try:
                     hyc_compile(code, module=fullname)
-                except IOError:
-                    pass
+                except IOError as e:
+                    warn('Could not write Hy bytecode: {}'.format(e))
             return code
 
         def get_code(self, fullname=None):

hy/macros.py

@@ -111,11 +111,11 @@ def require(source_module, target_module, assignments, prefix=""):
         prefix += "."
 
     if assignments == "ALL":
-        # Only add macros/tags created in/by the source module?
-        name_assigns = [(n, n) for n, f in source_macros.items()]
-                        # if inspect.getmodule(f) == source_module]
-        name_assigns += [(n, n) for n, f in source_tags.items()]
-                         # if inspect.getmodule(f) == source_module]
+        # Only add macros/tags created in/by the source module.
+        name_assigns = [(n, n) for n, f in source_macros.items()
+                        if inspect.getmodule(f) == source_module]
+        name_assigns += [(n, n) for n, f in source_tags.items()
+                         if inspect.getmodule(f) == source_module]
     else:
         # If one specifically requests a macro/tag not created in the source
         # module, I guess we allow it?
@@ -150,7 +150,8 @@ def load_macros(module):
     for builtin_mod_name in builtin_macros:
         builtin_mod = importlib.import_module(builtin_mod_name)
 
-        # Make sure we don't overwrite macros in the module
+        # Make sure we don't overwrite macros in the module.
+        # TODO: Is this the desired behavior?
         if hasattr(builtin_mod, '__macros__'):
             module_macros.update({k: v
                                   for k, v in builtin_mod.__macros__.items()
@@ -182,10 +183,39 @@ def empty_fn(*args, **kwargs):
 
 
 def macroexpand(tree, module, compiler=None, once=False):
-    """Expand the toplevel macros for the `tree`.
+    """Expand the toplevel macros for the given Hy AST tree.
 
-    Load the macros from the given `module`, then expand the
-    (top-level) macros in `tree` until we no longer can.
+    Load the macros from the given `module`, then expand the (top-level) macros
+    in `tree` until we no longer can.
+
+    `HyExpression` resulting from macro expansions are assigned the module in
+    which the macro function is defined (determined using `inspect.getmodule`).
+    If the resulting `HyExpression` is itself macro expanded, then the
+    namespace of the assigned module is checked first for a macro corresponding
+    to the expression's head/car symbol.  If the head/car symbol of such a
+    `HyExpression` is not found among the macros of its assigned module's
+    namespace, the outer-most namespace--e.g.  the one given by the `module`
+    parameter--is used as a fallback.
+
+
+    Parameters
+    ----------
+    tree: HyObject or list
+        Hy AST tree.
+
+    module: str or types.ModuleType
+        Module used to determine the local namespace for macros.
+
+    compiler: HyASTCompiler, optional
+        The compiler object passed to expanded macros.
+
+    once: boolean, optional
+        Only expand the first macro in `tree`.
+
+    Returns
+    ------
+    out: HyObject
+        Returns a mutated tree with macros expanded.
     """
     if not inspect.ismodule(module):
         module = importlib.import_module(module)
@@ -197,12 +227,23 @@ def macroexpand(tree, module, compiler=None, once=False):
         if not isinstance(tree, HyExpression) or tree == []:
             break
 
-        fn = tree[0]
-        if fn in ("quote", "quasiquote") or not isinstance(fn, HySymbol):
+        fn_symbol = tree[0]
+        if (fn_symbol in ("quote", "quasiquote") or
+                not isinstance(fn_symbol, HySymbol)):
             break
 
-        fn = mangle(fn)
-        m = module.__macros__.get(fn, None)
+        fn_name = mangle(fn_symbol)
+
+        # NOTE: We're using a list here just in case we want/need to stack
+        # macro namespaces.
+        expr_modules = [] if not hasattr(tree, 'module') else [tree.module]
+        expr_modules += [module]
+
+        # Choose the first namespace with the macro.
+        m = next((mod.__macros__[fn_name]
+                  for mod in expr_modules
+                  if fn_name in mod.__macros__),
+                 None)
         if not m:
             break
 
@@ -230,6 +271,15 @@ def macroexpand(tree, module, compiler=None, once=False):
         except Exception as e:
             msg = "expanding `" + str(tree[0]) + "': " + repr(e)
             raise HyMacroExpansionError(tree, msg)
+
+        if isinstance(obj, HyExpression):
+            macro_module = inspect.getmodule(m)
+            # TODO: We could also consider [temporarily] expanding the compiler
+            # module's namespace by including macros and/or tags from the
+            # expression's namespace (without overwriting, so that local scope
+            # is always preferred).
+            obj.module = macro_module
+
         tree = replace_hy_obj(obj, tree)
 
         if once:
@@ -245,15 +295,31 @@ def macroexpand_1(tree, module, compiler=None):
     return macroexpand(tree, module, compiler, once=True)
 
 
-def tag_macroexpand(tag, tree, module):
+def tag_macroexpand(tag_symbol, tree, module):
     """Expand the tag macro `tag` with argument `tree`."""
     if not inspect.ismodule(module):
         module = importlib.import_module(module)
 
-    tag_macro = module.__tags__.get(tag, None)
+    # NOTE: We're using a list here just in case we want/need to stack
+    # macro namespaces.
+    expr_modules = [] if not hasattr(tree, 'module') else [tree.module]
+    expr_modules += [module]
+
+    # Choose the first namespace with the macro.
+    tag_macro = next((mod.__tags__[tag_symbol]
+                      for mod in expr_modules
+                      if tag_symbol in mod.__tags__),
+                     None)
 
     if tag_macro is None:
-        raise HyTypeError(tag, "'{0}' is not a defined tag macro.".format(tag))
+        raise HyTypeError(
+            tag_symbol,
+            "'{0}' is not a defined tag macro.".format(tag_symbol))
 
     expr = tag_macro(tree)
+
+    if isinstance(expr, HyExpression):
+        macro_module = inspect.getmodule(tag_macro)
+        expr.module = macro_module
+
     return replace_hy_obj(expr, tree)

hy/models.py

@@ -32,11 +32,13 @@ class HyObject(object):
     Generic Hy Object model. This is helpful to inject things into all the
     Hy lexing Objects at once.
     """
+    # TODO: Use slots?
+    __properties__ = ["module", "start_line", "end_line", "start_column",
+                      "end_column"]
 
     def replace(self, other, recursive=False):
         if isinstance(other, HyObject):
-            for attr in ["start_line", "end_line",
-                         "start_column", "end_column"]:
+            for attr in self.__properties__:
                 if not hasattr(self, attr) and hasattr(other, attr):
                     setattr(self, attr, getattr(other, attr))
         else:
@@ -45,7 +47,8 @@ def replace(self, other, recursive=False):
         return self
 
     def __repr__(self):
-        return "%s(%s)" % (self.__class__.__name__, super(HyObject, self).__repr__())
+        return "%s(%s)" % (self.__class__.__name__,
+                           super(HyObject, self).__repr__())
 
 
 _wrappers = {}
@@ -247,7 +250,7 @@ def replace(self, other, recursive=True):
         if recursive:
             for x in self:
                 replace_hy_obj(x, other)
-        HyObject.replace(self, other)
+        super(HySequence, self).replace(other)
         return self
 
     def __add__(self, other):

tests/native_tests/native_macros.hy

@@ -2,7 +2,8 @@
 ;; This file is part of Hy, which is free software licensed under the Expat
 ;; license. See the LICENSE.
 
-(import [hy.errors [HyTypeError]])
+(import pytest
+        [hy.errors [HyTypeError]])
 
 (defmacro rev [&rest body]
   "Execute the `body` statements in reverse"
@@ -329,3 +330,124 @@
     (except [e SystemExit]
       (assert (= (str e) "42"))))
   (setv --name-- oldname))
+
+(defn test-macro-namespace-resolution []
+  "Confirm that local versions of macro-macro dependencies do not shadow the
+versions from the macro's own module, but do resolve unbound macro references
+in expansions."
+
+  ;; `nonlocal-test-macro` is a macro used within
+  ;; `tests.resources.macro-with-require.test-module-macro`.
+  ;; Here, we introduce an equivalently named version in local scope that, when
+  ;; used, will expand to a different output string.
+  (defmacro nonlocal-test-macro [x]
+    (print "this is the local version of `nonlocal-test-macro`!"))
+
+  ;; Was the above macro created properly?
+  (assert (in "nonlocal_test_macro" __macros__))
+
+  (setv nonlocal-test-macro (get __macros__ "nonlocal_test_macro"))
+
+  (require [tests.resources.macro-with-require [*]])
+
+  ;; Make sure our local version wasn't overwritten by a faulty `require` of the
+  ;; one in tests.resources.macro-with-require.
+  (assert (= nonlocal-test-macro (get __macros__ "nonlocal_test_macro")))
+
+  (setv module-name-var "tests.native_tests.native_macros.test-macro-namespace-resolution")
+  (assert (= (+ "This macro was created in tests.resources.macros, "
+                "expanded in tests.native_tests.native_macros.test-macro-namespace-resolution "
+                "and passed the value 2.")
+             (test-module-macro 2)))
+  (assert (= (+ "This macro was created in tests.resources.macros, "
+                "expanded in tests.native_tests.native_macros.test-macro-namespace-resolution "
+                "and passed the value 2.")
+             #test-module-tag 2))
+
+  ;; Now, let's use a `require`d macro that depends on another macro defined only
+  ;; in this scope.
+  (defmacro local-test-macro [x]
+    (.format "This is the local version of `nonlocal-test-macro` returning {}!" x))
+
+  (assert (= "This is the local version of `nonlocal-test-macro` returning 3!"
+             (test-module-macro-2 3)))
+  (assert (= "This is the local version of `nonlocal-test-macro` returning 3!"
+             #test-module-tag-2 3)))
+
+(defn test-macro-from-module []
+  "Macros loaded from an external module, which itself `require`s macros, should
+ work without having to `require` the module's macro dependencies (due to
+ [minimal] macro namespace resolution).
+
+ In doing so we also confirm that a module's `__macros__` attribute is correctly
+ loaded and used.
+
+ Additionally, we confirm that `require` statements are executed via loaded bytecode."
+
+  (import os sys marshal types)
+  (import [hy.importer [cache-from-source]])
+
+  (setv pyc-file (cache-from-source
+                   (os.path.realpath
+                     (os.path.join
+                       "tests" "resources" "macro_with_require.hy"))))
+
+  ;; Remove any cached byte-code, so that this runs from source and
+  ;; gets evaluated in this module.
+  (when (os.path.isfile pyc-file)
+    (os.unlink pyc-file)
+    (.clear sys.path_importer_cache)
+    (when (in  "tests.resources.macro_with_require" sys.modules)
+      (del (get sys.modules "tests.resources.macro_with_require"))
+      (__macros__.clear)
+      (__tags__.clear)))
+
+  ;; Ensure that bytecode isn't present when we require this module.
+  (assert (not (os.path.isfile pyc-file)))
+
+  (defn test-requires-and-macros []
+    (require [tests.resources.macro-with-require
+              [test-module-macro]])
+
+    ;; Make sure that `require` didn't add any of its `require`s
+    (assert (not (in "nonlocal-test-macro" __macros__)))
+    ;; and that it didn't add its tags.
+    (assert (not (in "test_module_tag" __tags__)))
+
+    ;; Now, require everything.
+    (require [tests.resources.macro-with-require [*]])
+
+    ;; Again, make sure it didn't add its required macros and/or tags.
+    (assert (not (in "nonlocal-test-macro" __macros__)))
+
+    ;; Its tag(s) should be here now.
+    (assert (in "test_module_tag" __tags__))
+
+    ;; The test macro expands to include this symbol.
+    (setv module-name-var "tests.native_tests.native_macros")
+    (assert (= (+ "This macro was created in tests.resources.macros, "
+                  "expanded in tests.native_tests.native_macros "
+                  "and passed the value 1.")
+               (test-module-macro 1)))
+
+    (assert (= (+ "This macro was created in tests.resources.macros, "
+                  "expanded in tests.native_tests.native_macros "
+                  "and passed the value 1.")
+               #test-module-tag 1)))
+
+  (test-requires-and-macros)
+
+  ;; Now that bytecode is present, reload the module, clear the `require`d
+  ;; macros and tags, and rerun the tests.
+  (assert (os.path.isfile pyc-file))
+
+  ;; Reload the module and clear the local macro context.
+  (.clear sys.path_importer_cache)
+  (del (get sys.modules "tests.resources.macro_with_require"))
+  (.clear __macros__)
+  (.clear __tags__)
+
+  ;; XXX: There doesn't seem to be a way--via standard import mechanisms--to
+  ;; ensure that an imported module used the cached bytecode.  We'll simply have
+  ;; to trust that the .pyc loading convention was followed.
+  (test-requires-and-macros))

tests/resources/macro_with_require.hy

@@ -0,0 +1,25 @@
+;; Require all the macros and make sure they don't pollute namespaces/modules
+;; that require `*` from this.
+(require [tests.resources.macros [*]])
+
+(defmacro test-module-macro [a]
+  "The variable `macro-level-var' here should not bind to the same-named symbol
+in the expansion of `nonlocal-test-macro'."
+  (setv macro-level-var "tests.resources.macros.macro-with-require")
+  `(nonlocal-test-macro ~a))
+
+(deftag test-module-tag [a]
+  "The variable `macro-level-var' here should not bind to the same-named symbol
+in the expansion of `nonlocal-test-macro'."
+  (setv macro-level-var "tests.resources.macros.macro-with-require")
+  `(nonlocal-test-macro ~a))
+
+(defmacro test-module-macro-2 [a]
+  "The macro `local-test-macro` isn't in this module's namespace, so it better
+ be in the expansion's!"
+  `(local-test-macro ~a))
+
+(deftag test-module-tag-2 [a]
+  "The macro `local-test-macro` isn't in this module's namespace, so it better
+ be in the expansion's!"
+  `(local-test-macro ~a))