Details

Description

This patch changes the behavior of metadata when used in conjunction with macros. The metadata &form is now merged with the metadata of the macro call sexpr. This allows users to either type-hint the inner or the outer form in a macro call and have somewhat better results. In the past, the metadata from the macroexpand was used as-is. This disallowed code like the following, to work without reflection:

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:

I'm not sure I actually got the Java code formatted correctly. My editor is not well-configured to get the clojure/core style right automatically.

My solution is to take the &form metadata, drop :line/:file keys, and then merge with the returned metadata, with &form taking precedence. I'm not sure whether this is the right approach in all cases, even though it works for :tag metadata.

I achieved this with a change to the compiler, which makes it fairly heavy-weight. It should be possible to instead adjust defmacro if changes to the compiler are not desirable. However, I believe this would involve substantially more work and be harder to test (for example, multiple arities complicate things). It seems nicer to treat the macroexpansion as a black box and then make metadata tweaks to the result, rather than modifying their actual defmacro code.

If a macro expands to something that is not an IObj, such as an Integer, then my patch silently discards the caller's metadata. Would it be better to throw an exception?

Activity

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.

Alan Malloy
added a comment - 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.

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.

Alan Malloy
added a comment - 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.

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.

Chouser
added a comment - 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.

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.

Alan Malloy
added a comment - 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.

Sure, I'll put together that patch. I'm worried, though, that if it's not the default, it will just never get used, and we'll be in effectively the same situation we are now, where no macros do this right. I don't foresee anyone going through their libraries to add ^:keep-meta on every macro.

Alan Malloy
added a comment - 03/Dec/13 1:24 AM Sure, I'll put together that patch. I'm worried, though, that if it's not the default, it will just never get used, and we'll be in effectively the same situation we are now, where no macros do this right. I don't foresee anyone going through their libraries to add ^:keep-meta on every macro.

I updated the patch to behave as Rich requested, but it caused a test regression that I can't figure out, in the handling of either refer or private vars. Hopefully someone else can run the tests and figure out what is missing here; my change is supposed to be opt-in, and I can't see where I've gone wrong.

Alan Malloy
added a comment - 03/Dec/13 2:20 AM I updated the patch to behave as Rich requested, but it caused a test regression that I can't figure out, in the handling of either refer or private vars. Hopefully someone else can run the tests and figure out what is missing here; my change is supposed to be opt-in, and I can't see where I've gone wrong.

Alan, your patch clj865.patch dated Dec 3, 2013 has some HTML cruft at the beginning and end, but even after removing that it does not apply cleanly to the latest Clojure master as of today. I understand that you say it needs more work, but it would be easier for others who wish to try it out if it applied cleanly.

Andy Fingerhut
added a comment - 07/Dec/13 10:31 AM Alan, your patch clj865.patch dated Dec 3, 2013 has some HTML cruft at the beginning and end, but even after removing that it does not apply cleanly to the latest Clojure master as of today. I understand that you say it needs more work, but it would be easier for others who wish to try it out if it applied cleanly.

As of Eastwood version 0.2.0, it includes a new warning :unused-meta-on-macro that will warn whenever metadata is applied to a macro invocation, with the known exception of clojure.core/fn, which explicitly uses metadata applied to it. https://github.com/jonase/eastwood#unused-meta-on-macro