[CLJ-865] Macroexpansion discards &form metadata Created: 26/Oct/11 Updated: 06/Jun/12
|Attachments:||0001-Add-test-for-macroexpansion-metadata-preservation.patch 0002-Preserve-form-metadata-on-macroexpanded-forms.patch 0003-Make-defmacro-preserve-form-metadata.patch 0004-Another-stab-at-implementing-this.patch updated.patch|
|Patch:||Code and Test|
As discussed in http://groups.google.com/group/clojure/browse_thread/thread/2690cb6ca0e8beb8 there is a "surprise factor" when type-hinting an expression that represents a macro, such as with (.length ^String (doto (identity "x") prn)). Here the doto macro discards the metadata on &form, causing a reflective lookup. This has the effect that while expressions representing function calls can be type-hinted, expressions representing macros in general cannot. The doto macro could be rewritten to respect its &form metadata, but doing this for every macro in existence would be tedious and error-prone. Instead, I propose a change to the compiler, to cause macroexpansion to hang onto the metadata automatically.
The first patch attached adds a test for the behavior I propose: this test fails. After applying the second patch, the test passes.
There are a couple points that merit further consideration before accepting my patch:
|Comment by Alan Malloy [ 28/Oct/11 1:12 AM ]|
So I went ahead and did the work of making this change in clojure.core/defmacro instead of clojure.lang.Compiler/macroexpand1. It was even worse than I expected: I didn't realize we don't yet have syntax-quote or apply at this stage in bootstrapping, so writing a non-trivial macroexpansion requires a huge amount of (list `foo (list `bar 'local-name)) and so forth.
I'm sure the version I wrote is not optimal, but it seemed simpler to piggyback on defn, and then use alter-var-root to shim the metadata management in, than it would have been to expand to the correct thing in the first place.
Anyway, attached patch #3 could be applied instead of #2 to resolve the issue in clojure.core instead of clojure.lang. The tests added in patch #1 pass either way.
|Comment by Alan Malloy [ 13/Nov/11 8:29 PM ]|
I realized I can do this with a named private function instead of an anonymous function, reducing the amount of mess defmacro itself has to generate. Patch 4 is, I think, strictly better than Patch 3, if a Clojure implementation is preferred to one in Java.
|Comment by Chouser [ 20/Nov/11 10:43 PM ]|
I prefer patch 0002 in Java over either 0003 or 0004. Patch 0002 keeps the knowledge of how to invoke macro fns (specifically the extra &form and &env args) in one place, macroexpand1 rather than duplicating that knowledge in core.clj as well. Note patch 0001 is just tests.
The proposed default macroexpansion behavior is more useful than what we currently have, but there are two details I'd like to think about a bit more:
1) In exchange for a more useful default, macro writers lose the ability to consume their &form metadata and have control over the resulting form metadata without the &form metadata overridding it. That is, macros are no longer in complete control of their output form.
2) Rule (1) above has hardcoded exceptions for :line and :file, where &form metadata is unable to override the results returned by the macro.
|Comment by Alan Malloy [ 01/Jun/12 2:04 PM ]|
This patch incorporates all previous patches to this issue.
On the clj-dev mailing list, Andy Fingerhut suggested a new metadata key for allowing the macro author to specify "I've looked at their &form metadata, and this form is exactly what I want to expand to, please don't change the metadata any further." I've implemented this, and I think it addresses Chouser's concern about needing a way to "break out" of the improved-default behavior.
One open question is, is :explicit-meta the right key to use? I spent some time tracking down a bug caused by my forgetting the keyword and using :explicit-metadata in my test; perhaps something more difficult to get confused by is available.