From 2d12fc0ee5a94570307773aa2aa27c1898fce51d Mon Sep 17 00:00:00 2001 From: "Brandon T. Willard" Date: Tue, 25 Sep 2018 16:12:54 -0500 Subject: [PATCH] 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/hy#1268, closes hylang/hy#1650, closes hylang/hy#1416. --- hy/compiler.py | 7 +- hy/importer.py | 6 +- hy/macros.py | 98 ++++++++++++++++---- hy/models.py | 11 ++- tests/native_tests/native_macros.hy | 124 +++++++++++++++++++++++++- tests/resources/macro_with_require.hy | 25 ++++++ tests/resources/macros.hy | 13 +++ 7 files changed, 258 insertions(+), 26 deletions(-) create mode 100644 tests/resources/macro_with_require.hy diff --git a/hy/compiler.py b/hy/compiler.py index 942598948..c0f62f756 100755 --- a/hy/compiler.py +++ b/hy/compiler.py @@ -1742,7 +1742,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. @@ -1772,9 +1773,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): diff --git a/hy/importer.py b/hy/importer.py index a9fefa124..64e9ef035 100644 --- a/hy/importer.py +++ b/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 @@ -410,6 +411,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 @@ -444,8 +446,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): diff --git a/hy/macros.py b/hy/macros.py index 396334e32..04c20afea 100644 --- a/hy/macros.py +++ b/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) diff --git a/hy/models.py b/hy/models.py index 943458e03..141d8a1fe 100644 --- a/hy/models.py +++ b/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): diff --git a/tests/native_tests/native_macros.hy b/tests/native_tests/native_macros.hy index ef6ae2676..73b433bb5 100644 --- a/tests/native_tests/native_macros.hy +++ b/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)) diff --git a/tests/resources/macro_with_require.hy b/tests/resources/macro_with_require.hy new file mode 100644 index 000000000..ef25018b5 --- /dev/null +++ b/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)) diff --git a/tests/resources/macros.hy b/tests/resources/macros.hy index 300a790d1..c3f4fb169 100644 --- a/tests/resources/macros.hy +++ b/tests/resources/macros.hy @@ -1,3 +1,5 @@ +(setv module-name-var "tests.resources.macros") + (defmacro thread-set-ab [] (defn f [&rest args] (.join "" (+ (, "a") args))) (setv variable (HySymbol (-> "b" (f)))) @@ -10,3 +12,14 @@ (defmacro test-macro [] '(setv blah 1)) + +(defmacro nonlocal-test-macro [x] + "When called from `macro-with-require`'s macro(s), the first instance of +`module-name-var` should resolve to the value in the module where this is +defined, then the expansion namespace/module" + `(.format (+ "This macro was created in {}, expanded in {} " + "and passed the value {}.") + ~module-name-var module-name-var ~x)) + +;; TODO For when we're ready to address hylang/hy#1414. +;; (setv module-expanded-macro (nonlocal-test-macro 1))