<< Back to previous view

[CLJ-865] Macroexpansion discards &form metadata Created: 26/Oct/11  Updated: 10/Apr/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: Compiler

Attachments: Text File 0001-Add-test-for-macroexpansion-metadata-preservation.patch     Text File 0002-Preserve-form-metadata-on-macroexpanded-forms.patch     Text File 0003-Make-defmacro-preserve-form-metadata.patch     Text File 0004-Another-stab-at-implementing-this.patch     File 2013-10-11_CLJ-865_Fix-With-Tests.diff     Text File clj-865.patch     Text File clj865.patch     Text File clj-865-updated-v2-patch.txt     Text File updated.patch    
Patch: Code and Test
Approval: Vetted


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:

(.trim ^String (when true "hello "))

Patch: 2013-10-11_CLJ-865_Fix-With-Tests.diff
Screened by: Timothy Baldridge

--------- Implementation Details ----------

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?

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.

Comment by Andy Fingerhut [ 14/Aug/13 8:05 PM ]

clj-865-updated-v2-patch.txt dated Aug 14 2013 is identical to Alan Malloy's updated.patch dated Jun 1 2012. I simply updated the patch to apply cleanly to latest master after some context lines in the test file macros.clj had gone bad due to recent commits.

Comment by Timothy Baldridge [ 11/Oct/13 9:23 AM ]

Added updated patch that works against master, and also removes COLUMN_KEY from the macro's metadata

Comment by Timothy Baldridge [ 11/Oct/13 12:50 PM ]

Added patch that contains all fixes plus a few more tests.

Comment by Rich Hickey [ 22/Nov/13 11:19 AM ]

Since this could break things, we could just take metadata on the macro name to ask for this:

(defmacro ^:keep-meta simple-macro [f arg]
  `(~f ~arg))

or something

Comment by Alan Malloy [ 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.

Comment by Alan Malloy [ 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.

Comment by Andy Fingerhut [ 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.

Comment by Alan Malloy [ 10/Dec/13 1:06 PM ]

Sorry Andy, and thanks for noticing. I haven't been on a very developer-friendly computer recently, but I'll try to fix the patch tonight.

Comment by Alan Malloy [ 11/Dec/13 10:26 AM ]

Here's a fix to the patch. I verified that this applies cleanly to current master.

Comment by Alan Malloy [ 11/Dec/13 10:27 AM ]

To clarify, it's the file named clj-865.patch. I didn't realize JIRA wouldn't make it clear which file I uploaded along with the comment.

Generated at Sat Apr 19 22:48:29 CDT 2014 using JIRA 4.4#649-r158309.