Macro processing updates and fixes

[brandonwillard]

This PR changes the way macros are loaded and managed within modules/namespaces, their scope of evaluation, and how require statements are treated during the module bytecode loading process.

Alongside those changes, a general refactoring was started that moves toward a HyASTCompiler that can operate in two fashions: 1) compilation that occurs within a module and allows macros to interact with the compiler (as they are currently configured to do), and 2) compilation that does not require a concrete module context but does not allow true macro-compiler interaction. The latter case could drastically simplify some of the module importing work-arounds that currently exist in this PR (e.g. when interacting with runpy).

It addresses/closes the following issues:

• require doesn't compile to anything, which interacts badly with bytecode #1268
• require and macro dependencies #1650
• Consider moving macros to a __macros__ object in each module #1416

Example

Before

hy 0.15.0+39.gd2319dc using CPython(default) 3.6.6 on Linux
=> (with [file (open "test_mod.hy" "w")] (file.write "(require [hy.contrib.walk [let]])\n\n(defmacro test-macro [x]\n  `(let [y ~x] (print y)))\n"))
87
=> (with [file (open "test_mod.hy" "r")] (print (file.read)))
(require [hy.contrib.walk [let]])

(defmacro test-macro [x]
  `(let [y ~x] (print y)))

=> (require [test-mod [test-macro]])
=> (test-macro 1)
Traceback (most recent call last):
  File "/home/bwillard/projects/code/python/hy-master/hy/importer.py", line 141, in hy_eval
    return eval(ast_compile(expr, "<eval>", "eval"), namespace)
  File "<eval>", line 1, in <module>
NameError: name 'let' is not defined

After

hy 0.15.0+45.ga8d6a48 using CPython(default) 3.6.6 on Linux
=> (with [file (open "test_mod.hy" "w")] (file.write "(require [hy.contrib.walk [let]])\n\n(defmacro test-macro [x]\n  `(let [y ~x] (print y)))\n"))
87
=> (with [file (open "test_mod.hy" "r")] (print (file.read)))
(require [hy.contrib.walk [let]])

(defmacro test-macro [x]
  `(let [y ~x] (print y)))

=> (require [test-mod [test-macro]])
True
=> (test-macro 1)
1
=> (import [pprint [pprint]])
=> (pprint __macros__)
{'_hyx_XgreaterHthan_signX': <function _hy_anon_var_14 at 0x7fbc2fef0a60>,
 '_hyx_XgreaterHthan_signXXgreaterHthan_signX': <function _hy_anon_var_17 at 0x7fbc2fef0b70>,
 'assoc': <function _hy_anon_var_3 at 0x7fbc2fef07b8>,
 'comment': <function _hy_anon_var_29 at 0x7fbc2fef31e0>,
 'cond': <function _hy_anon_var_13 at 0x7fbc2fef09d8>,
 'defmacro': <function _hy_anon_var_3 at 0x7fbc2fee70d0>,
 'defmain': <function _hy_anon_var_27 at 0x7fbc2fef30d0>,
 'defn': <function _hy_anon_var_11 at 0x7fbc2fee7378>,
 'deftag': <function _hy_anon_var_7 at 0x7fbc2fee7268>,
 'doc': <function _hy_anon_var_30 at 0x7fbc2fef3268>,
 'doto': <function _hy_anon_var_16 at 0x7fbc2fef0ae8>,
 'hyx_as_XgreaterHthan_signX': <function _hy_anon_var_1 at 0x7fbc2fee7598>,
 'hyx_defmacroXexclamation_markX': <function _hy_anon_var_26 at 0x7fbc2fef3048>,
 'hyx_defmacroXsolidusXgXexclamation_markX': <function _hy_anon_var_24 at 0x7fbc2fef0f28>,
 'hyx_defnXsolidusXa': <function _hy_anon_var_14 at 0x7fbc2fee7400>,
 'hyx_if': <function _hy_anon_var_4 at 0x7fbc2fee71e0>,
 'hyx_with': <function _hy_anon_var_7 at 0x7fbc2fef08c8>,
 'hyx_withXsolidusXa': <function _hy_anon_var_8 at 0x7fbc2fef0950>,
 'ideas': <function ideas_macro at 0x7fbc302698c8>,
 'if_not': <function _hy_anon_var_18 at 0x7fbc2fef0bf8>,
 'if_python2': <function _hy_anon_var_15 at 0x7fbc2fee7488>,
 'koan': <function koan_macro at 0x7fbc30269840>,
 'lif': <function _hy_anon_var_19 at 0x7fbc2fef0c80>,
 'lif_not': <function _hy_anon_var_20 at 0x7fbc2fef0d08>,
 'macro_error': <function _hy_anon_var_8 at 0x7fbc2fee72f0>,
 'test_macro': <function <lambda> at 0x7fbc2fd9f598>,
 'unless': <function _hy_anon_var_22 at 0x7fbc2fef0e18>,
 'when': <function _hy_anon_var_21 at 0x7fbc2fef0d90>,
 'with_gensyms': <function _hy_anon_var_23 at 0x7fbc2fef0ea0>}

[Kodiologist]

Sounds pretty exciting. Want to add some tests?

[brandonwillard]

Doing just that — plus further necessary additions. I'm working somewhat incrementally on this one, so you folks can see/be involved in the process.

[brandonwillard]

The changes introduced here might also help address the following issues:

• A macro can't expand in the same module it's defined in if it uses a global from it. #1414
• .pyc files don't get recreated when they use a macro from a file that was changed #1324
• Namespaced symbols #277
  • The macro-only "namespacing" for symbols implemented in this PR is a light-weight, behind-the-scenes version of this; however, full namespacing should probably be introduced (e.g. as part of the Hy syntax) and used instead.

(Moved from initial comment)

[Kodiologist]

At a high level, how does calling test-macro-module in a file other than the file it was defined in work? How does Hy now know that let refers to the let that was required in the module where test-macro-module was defined? How would this interact with any new macro named let in the calling file?

[brandonwillard]

When test-macro-module is macroexpanded (in, say, __main__), we get its definition namespace (i.e. module tests.resources.macro_with_require) from the macro function object, then we assign the definition namespace to the HyExpression resulting from the macro's expansion via the new HyObject.module field. (Currently, this is only done for HyExpressions objects resulting from a macro expansion.)

When that HyExpression is macroexpanded, we first check the compiler namespace's __macros__ dictionary (e.g. __main__), then, if that fails, the definition namespace's via HyExpression.module. So the test example should use a let defined in the calling file (or module __main__) ahead of the let defined in the macro's definition namespace (i.e. tests.resources.macro_with_require).

This form of namespace/module resolution is intentionally restrictive; for instance, it doesn't propagate into sub-expressions or apply to anything else other than macro expansion. This can easily be changed; however, I wanted to start by implementing just enough to make non-trivial macro work a little more tolerable.

[Kodiologist]

Interesting; thanks for the explanation. As the system gets more mature, we'll probably also want to change the order of checks so that the macro's defining module is checked first. Otherwise, a macro user can inadvertently shadow one of the macro's subroutines.

[brandonwillard]

That's a simple change to make, so I'll do it now before it causes confusion down the line.

[brandonwillard]

Macro expansion now checks the macro's module's namespace first, then the "local" namespace.
Also, requireing all will only add the macros and tags defined in the module being required (and not its required dependencies).

Finally, I rewrote the tests so that they use test.resources macros and modules exclusively. There's also a simple test in there confirming that required macros (even requirements of required macros) can reference variables in their module namespaces and that their expansions only bind to symbols in the namespace they're expanded (e.g. locally). (Just to be safe.)

[Kodiologist]

Nice. Are you done with this PR or are you planning more changes?

[brandonwillard]

Not yet; just noticed that the tag macros don't resolve in the correct order (didn't update the code in that place), so I need to fix that and add another test.

Otherwise, I have some AST/source line numbering fixes to make. For example, hy_eval strips line numbering and doesn't compile with the correct filename, so anywhere that's used — like eval-and-compile and whatever uses it (e.g. the defmacro macro) — the resulting bytecode produces unreadable source in the debugger and trace output.

On a related note, hy_eval and hy_parse and ast_compile should all probably be in compiler.py; currently, there's a weird back and forth between those modules that simply isn't necessary. Also, it seems like we shouldn't have to create a whole new HyAstCompiler during compilation of eval-and-compile, which is what's currently happening through our use of hy_eval, so I would like to refactor that, too.

I think the two aforementioned changes could/should have their own PRs, though, so I'll end this one here (unless something more related to macros comes up).

[brandonwillard]

All right, updated the last commit with the correct macro resolution order (for tags) and corresponding tests.

[Kodiologist]

Otherwise, I have some AST/source line numbering fixes to make.

Are these regressions introduced by this PR, or preexisting issues?

[brandonwillard]

Those are pre-existing issues.

[Kodiologist]

I tried the test I suggested at the top of #1268 with Python 3.6.6. Running hy foo.hy once crashes:

 Traceback (most recent call last):
   File "/home/hippo/Desktop/py36-env/bin/hy", line 12, in <module>
     sys.exit(hy_main())
   File "/home/hippo/Desktop/hyenv/hy/hy/cmdline.py", line 382, in hy_main
     sys.exit(cmdline_handler("hy", sys.argv))
   File "/home/hippo/Desktop/hyenv/hy/hy/cmdline.py", line 368, in cmdline_handler
     runhy.run_path(filename, run_name='__main__')
   File "/usr/lib/python3.6/runpy.py", line 263, in run_path
     pkg_name=pkg_name, script_name=fname)
   File "/usr/lib/python3.6/runpy.py", line 96, in _run_module_code
     mod_name, mod_spec, pkg_name, script_name)
   File "/usr/lib/python3.6/runpy.py", line 85, in _run_code
     exec(code, run_globals)
   File "/tmp/foo.hy", line 1, in <module>
     (require [hy.extra.anaphoric [ap-if]])
   File "/home/hippo/Desktop/hyenv/hy/hy/macros.py", line 106, in require
     target_module = importlib.import_module(target_module)
   File "/home/hippo/Desktop/py36-env/lib/python3.6/importlib/__init__.py", line 126, in import_module
     return _bootstrap._gcd_import(name[level:], package, level)
   File "<frozen importlib._bootstrap>", line 994, in _gcd_import
   File "<frozen importlib._bootstrap>", line 971, in _find_and_load
   File "<frozen importlib._bootstrap>", line 955, in _find_and_load_unlocked
   File "<frozen importlib._bootstrap>", line 665, in _load_unlocked
   File "<frozen importlib._bootstrap_external>", line 678, in exec_module
   File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
   File "/tmp/foo.hy", line 2, in <module>
     (print (eval '(ap-if (+ "a" "b") (+ it "c"))))
   File "/home/hippo/Desktop/hyenv/hy/hy/importer.py", line 145, in hy_eval
     return eval(ast_compile(expr, "<eval>", "eval"), globals, locals)
   File "<eval>", line 1, in <module>
 NameError: name 'ap_if' is not defined

Likewise on master (in fact, that code I wrote hasn't worked, as written, since several releases ago). On master, running it again makes no difference, but with your PR, running it again prints:

abc
abc

Only one abc is expected.

So I'm not sure that your PR is fault given that the code was already broken—all your PR has done is make it a bit less broken—but perhaps it's something to think about?

[brandonwillard]

The example in #1268 should work, and it does in Python 2.7, but not Python 3.x. I'm looking at it now.

[Kodiologist]

The substantive changes here are fantastic and long overdue, but let's separate out some of the other stuff.

[Kodiologist]

There have been several attempts. I think #1166 is the latest.

[brandonwillard]

@Kodiologist, anything else needed for this one?

[Kodiologist]

Sorry, I'd assumed you were working on writing require as a macro and were either going to change this PR a lot or replace it. I'll look at it again tomorrow.

[brandonwillard]

Turning require into a macro would be cool, but I would rather get these fixes through first. I have a few other things in the pipeline that would benefit greatly from the way macros and require work in this PR.

[Kodiologist]

It looks like you've undone the separation of deleting get_arity and moving the other functions. Is there a reason you want to keep those changes in one commit?

[brandonwillard]

An explanation for those changes is given here. Otherwise, get_arity doesn't warrant any special distinction (it isn't even used) and the changes are easily summarized by only a single commit.

More importantly, why would we want to be so granular? Is there something special about get_arity that I'm not seeing? Is there a more general convention behind this: e.g. we want separate commits for each addition and removal of a function?

[Kodiologist]

Putting the functions in hy.macros instead of hy._compat is fine.

More importantly, why would we want to be so granular?

Because the two things being accomplished in the combined commit are independent. One is deleting dead code. Another is moving code from one place to another, changing only what's necessary to support the move. With each change as its own commit, it's clear what's going on. On the other hand, putting both the move and deletion into one commit makes it not so obvious that one function doesn't end up surviving the move.

This is the same reason why indentation changes to a large body of code should in a separate commit from functional changes to a small portion of that code. The changes are independent, and the reader will know to look at the functional-change commit much more closely than the indentation commit.

[brandonwillard]

Because the two things being accomplished in the combined commit are independent.

get_arity's removal occurs as a direct result of hy.inspect's removal (i.e. the focus of the commit), so those two are not independent. Likewise, removal of hy.inspect necessitates the relocation of its functions used within hy.macros (to hy.macros), so those are also not independent.
As a result, all the actions comprising the commit are clearly centered around the removal of hy.inspect and perfectly suited for one commit on that topic.

[Kodiologist]

removal of hy.inspect necessitates the relocation of its functions used within hy.macros (to hy.macros), so those are also not independent.

I'm with you there, to be clear.

get_arity's removal occurs as a direct result of hy.inspect's removal (i.e. the focus of the commit), so those two are not independent.

I don't see this. You could as easily delete get_arity in a separate PR, which could be merged before or after any PR in which the other functions are moved, and all the individual changes would still make sense. That seems like independence par excellence.

The only reason that I deleted get_arity here, in the same PR as I was moving those functions, is that I noticed get_arity was never actually used while I was moving the functions.

[brandonwillard]

I don't see this. You could as easily delete get_arity in a separate PR, which could be merged before or after any PR in which the other functions are moved, and all the individual changes would still make sense. That seems like independence par excellence.

Because you could delete get_arity independent of hy.inspect (e.g. in a separate PR) does not negate the fact that deleting hy.inspect necessarily deletes get_arity. Whether or not the changes can be made independently is not relevant, since, for that matter, every character change could justify its own commit on such "independence".

The only reason that I deleted get_arity here, in the same PR as I was moving those functions, is that I noticed get_arity was never actually used while I was moving the functions.

I don't see how its irrelevance is a reason for an unnecessary commit. The opposite seems much more appropriate: because the function isn't used, there's no reason to emphasise its removal in the Git commit history!

Also, decoupling get_arity's removal from the rest of the changes serves no apparent functional purpose. If, for instance, one wanted to revert the change and re-introduce get_arity in isolation, a separate commit that first removes or relocates it from hy.inspect wouldn't be helpful, since the former would require the reversion of hy.inspect's removal—and further justify their coupling—and the latter would require another superfluous commit that subsequently deletes get_arity after relocating it.

If the purpose of the separate commit is simply logging, and nothing particularly functional, then it's best to mention the removal explicitly in the commit message. Still, I don't see why get_arity deserves this much attention.

[Kodiologist]

deleting hy.inspect necessarily deletes get_arity

On the contrary, you could keep the function while getting rid of its original file, as I did originally.

Whether or not the changes can be made independently is not relevant, since, for that matter, every character change could justify its own commit on such "independence".

I don't see that, either. Suppose you spun off the first change of a single character in this PR into its own commit, making it the new first commit of the PR. I'd ask you to squash it into the following commit, because on its own, the one-character change would make no sense. The change only makes sense as part of the larger change in the commit it was split off from.

I don't see how its irrelevance is a reason for an unnecessary commit.

Commits exist to group related changes together. If two changes are irrelevant to each other, then in general, they should be in different commits.

Also, decoupling get_arity's removal from the rest of the changes serves no apparent functional purpose.

The purpose is not function but the legibility of history. Many sequences of commits can lead to the same final code, and are hence functionally identical, but some are more intelligible than others.

If the purpose of the separate commit is simply logging, and nothing particularly functional, then it's best to mention the removal explicitly in the commit message.

It's better to indicate a sequence of changes in the code itself, with a sequence of commits, than in prose, with the commit message.

[brandonwillard]

Just to be clear, you're arguing for situation A over B, no?

For a repository with the following file in its index:

;; some_file.hy

(defn func-1 [] 
  (print 1))

(defn func-2 [] 
  (print 2))

(defn func-3 [] 
  (print 3))

Situation A

Commit 1

--- a/some_file.hy
+++ b/some_file.hy
@@ -1,8 +1,5 @@
 ;; some_file.hy
 
-(defn func-1 [] 
-  (print 1))
-

Commit 2

--- a/some_file.hy
+++ b/some_file.hy
@@ -1,7 +1,4 @@
 ;; some_file.hy
 
-(defn func-2 [] 
-  (print 2))
-

Commit 3

deleted file mode 100644
index c2ff364..0000000
--- a/some_file.hy
+++ /dev/null
@@ -1,4 +0,0 @@
-;; some_file.hy
-
-(defn func-3 [] 
-  (print 3))

Situation B

Commit 1

deleted file mode 100644
index 14ce434..0000000
--- a/some_file.hy
+++ /dev/null
@@ -1,10 +0,0 @@
-;; some_file.hy
-
-(defn func-1 [] 
-  (print 1))
-
-(defn func-2 [] 
-  (print 2))
-
-(defn func-3 [] 
-  (print 3))

[Kodiologist]

Maybe in some cases? I guess it depends on the actual situation. If an entire file is dead code, for example, then I could certainly imagine it making sense to delete the whole file in one commit. #1189 is an example I did.

[Kodiologist]

Similarly, in this case, if we were just deleting hy.inspect instead of moving some of its code, it would presumably make sense to delete all three functions in the same commit.

[brandonwillard]

All right, I may not understand it, but I've recreated that squashed commit and added it back.

[Kodiologist]

I guess the core problem is that Git doesn't have a notion of moving code. It can only track additions and deletions. So when you move code, make sure that in the commit where moving happens, only moving happens. When you want to make less superficial changes to that moved code (which is common, because you probably moved it because you're making related changes), do it in another commit, whether before or after the move.

[Kodiologist]

Woohoo!