CLJ-865 - Macroexpansion discards &form metadata

254 views
Skip to first unread message

Alex Miller

unread,
Dec 12, 2013, 10:28:40 PM12/12/13
to cloju...@googlegroups.com
This week Rich and I had a discussion about this ticket and decided to remove it from the 1.6 release (at least for now):

http://dev.clojure.org/jira/browse/CLJ-865

Because this is a highly voted issue I wanted to bring it to this list's attention. I don't think there's any disagreement about the intent of the ticket. 

The primary issue (specifically the reviewed patch 2013-10-11_CLJ-865_Fix-With-Tests.diff) is that this changes the behavior of existing programs. In general, we strongly value release compatibility and did not see this enhancement as important enough to break that compatibility.

Rich proposed an alternative that would instead leave the current default but require macros to be marked (with ^:keep-meta) to get the new behavior. This is admittedly far less satisfying in addressing the original issue (as any macros would need this marking to alter behavior). There is now an alternative patch clj-865.patch on the ticket along these lines.  (A working version of this patch was not available when we had the discussion a couple days ago but is now.)

So, questions to the you fine folks:

1) This is one of the highest voted enhancements in JIRA. Is this alternative solution sufficient to address the need?

2) Is there an alternative solution that does not break existing behavior but is better than this? 

If we can resolve these questions in the near future and there is a strong response and consensus, we would consider moving it back into 1.6.

Alex

Christophe Grand

unread,
Dec 13, 2013, 6:21:35 AM12/13/13
to clojure-dev
Having to opt in for the "correct" behaviour is weird.
We have lived with the broken behaviour for years, we can live another one with it, so what about issuing warnings in 1.6 and switching in 1.7?
The warning could check if mergedMeta differs from ret.meta().

Christophe


--
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to clojure-dev...@googlegroups.com.
To post to this group, send email to cloju...@googlegroups.com.
Visit this group at http://groups.google.com/group/clojure-dev.
For more options, visit https://groups.google.com/groups/opt_out.



--
On Clojure http://clj-me.cgrand.net/
Clojure Programming http://clojurebook.com
Training, Consulting & Contracting http://lambdanext.eu/

Tim Visher

unread,
Dec 13, 2013, 7:39:58 AM12/13/13
to cloju...@googlegroups.com
On Fri, Dec 13, 2013 at 6:21 AM, Christophe Grand <chris...@cgrand.net> wrote:
> Having to opt in for the "correct" behaviour is weird.
> We have lived with the broken behaviour for years, we can live another one
> with it, so what about issuing warnings in 1.6 and switching in 1.7?
> The warning could check if mergedMeta differs from ret.meta().

Nothing more substantive than +1

Kevin Downey

unread,
Dec 13, 2013, 4:13:32 PM12/13/13
to cloju...@googlegroups.com
On 12/13/13, 3:21 AM, Christophe Grand wrote:
> Having to opt in for the "correct" behaviour is weird.

this assumes that the current behavior is incorrect, which is sort of
short sighted. it would certainly be more convenient in some cases if
macro expansion preserved metadata, but intuitively macros substitute
one data structure for another, and from that understanding it does not
follow metadata should be preserved.

I guess generally over time more and more data transformations in
clojure (cons on various datastructures, clojure.walk, etc) to preserve
metadata, but I am pretty sure data transforms preserving metadata is
not a universal guarantee that clojure provides.

given that, there is no "correct" behavior, there is only behavior that
is more or less convenient in different circumstances. Dictating those
kind of choices up front tends to lead to pain later, so some kind of
tagging to allow users to make the choice seems fine.

even if the patch does go in, metadata is not always preserved, if the
macro returns something that metadata cannot be attached to, you just
cannot propagate metadata. so this patch replaces a universal guarantee
"macro expansions clobber metadata" with a provisional guarantee "macro
expansion preserves metadata if it can". as a general rule I think
universal guarantees are more valuable.

obviously lots of people have voted for this issue, so they must think
it is important, but I have never been bitten by it, so leaving it out
indefinitely sounds fine to me :)
And what is good, Phaedrus,
And what is not good—
Need we ask anyone to tell us these things?

signature.asc

Alex Miller

unread,
Dec 13, 2013, 5:04:06 PM12/13/13
to cloju...@googlegroups.com
Yes, I agree that we should not say that either the old or proposed behavior is "correct"; they are just different. 

If there were sufficient evidence that this does *not* break existing programs that would change the discussion. Doing so likely involves testing a sufficient number of representative programs. I don't think those of us at Cognitect have the bandwidth to do this work right now so I would encourage those who are interested in this enhancement to get involved! 

Alex



Chouser

unread,
Dec 13, 2013, 6:20:04 PM12/13/13
to cloju...@googlegroups.com
The current default is unfortunate in that it is more common to type
hint expressions (perhaps forgetting that you're hinting a macro call)
than it is to write a macro that preserves &form metadata. This is why
I'm in favor of changing the default (post 1.6 if this eases anyone's
concerns) to preserve metadata and allowing for a hint on the macro
definition that would indicate the macro is taking care of it's own
metadata.

On the other hand, if changing the default is not acceptable, why not
just provide a keep-metadata macro. That way no special compiler
directives would be needed at all:

(defmacro keep-metadata [& body]
`(with-meta (do ~@body) (meta ~'&form)))

;; Example macro loses metadata:
(defmacro my-vec [& body]
`[~@body])

(meta ^:foo (my-vec 1 2 3))
;=> nil

;; Use keep-metadata to preserve:
(defmacro my-vec [& body]
(keep-metadata
`[~@body]))

(meta ^:foo (my-vec 1 2 3))
;=> {:foo true, :column 7, :line 1}

--Chouser

Stuart Halloway

unread,
Dec 14, 2013, 7:53:16 AM12/14/13
to cloju...@googlegroups.com
I agree with Kevin's points, and like Chouser's non-breaking `keep-metadata` proposal.

Stu


Nicola Mometto

unread,
Dec 14, 2013, 8:31:57 AM12/14/13
to cloju...@googlegroups.com

I want to point out that metadata is lost in all forms that get
"changed" in macroexpansion, not only macros:

user=> (meta (macroexpand-1 '^:foo (String. "")))
nil

This means that a change that only affects user-defined macros will not
be sufficient IMHO.

Thanks,
Nicola Mometto

Mikera

unread,
Dec 16, 2013, 2:51:31 PM12/16/13
to cloju...@googlegroups.com
Hmm this actually explains why some of my type hinting hasn't worked :-)

Ultimately I think it should be expected behaviour to honour type hints added by the user, and that losing the metadata should be treated as a defect.

To answer your specific questions:

1) I really don't like the :keep-meta solution. The type hinting is a decision by the macro user, but this would put the burden on macro writers to add this metadata everywhere and then require macro users to figure out whether any particular macro supports it or not. Not only that, it would be a moving target over time as libraries are updated. Eeek.

So I think it is much better to just get it right now. Of course we don't like changing the behaviour of existing programs, but against that you have to weigh:
a) Confusion/complexity caused by maintaining inconsistent/incorrect behaviour
b) The cost of workarounds (like adding :keep-meta everywhere, or people writing extra wrappers so that the type hints work)
c) If you are going to break anything, better to do it sooner rather than later. Arguably any program relying on the current behaviour is subtly broken anyway (it would have to be relying on a type hint being silently removed - that's got to be a WTF surely??)

2) An alternative might be to merge the metadata as per the earlier patch, but give priority to the metadata returned by the macro expansion. This is less likely to cause breakage, since it maintains existing behaviour in the case that the macro returns different metadata from that specified on the form.

Alex Miller

unread,
Dec 16, 2013, 3:12:19 PM12/16/13
to cloju...@googlegroups.com
I don't think either of these will be seriously considered unless someone does the work to determine the scope of potential breakage. 

I think Chouser's macro is something that could be considered as a workaround now?


--

Mikera

unread,
Dec 17, 2013, 9:46:15 AM12/17/13
to cloju...@googlegroups.com
Isn't that what alphas are for? i.e. a chance to test out fixes that look like good improvements but give a chance to roll back if needed?

My humble recommendation is to put "valuable but needs testing" patches like this out there now in the alphas, with the idea that people can test it out and we can revisit the decision if any real issues emerge. 

Also: Is the current behaviour (i.e. silently ignoring type hint metadata) actually specified / documented anywhere? If not, I don't think we should feel any special obligation to maintain it.

Note: I admire and strongly support Clojure's general commitment to backwards compatibility. But this should not be confused with a misguided attempt to maintain identical behaviour in cases where behaviour was either undocumented or a defect. Otherwise you risk ending up in a nightmare scenario like the old Windows code base where you have to maintain thousands of old API quirks / defects just because far too many people started to rely on broken behaviour. 

Chouser's macro is undoubtedly a feasible workaround, but I think it is far better for the language ecosystem in the long term to fix things properly in core rather than encourage a proliferation of workarounds in library/user code.

Stuart Halloway

unread,
Dec 26, 2013, 2:33:18 PM12/26/13
to cloju...@googlegroups.com
Hi Mike,

I don't think we have decided that this is a good improvement, so I would argue more consideration is in order, not testing on the community.

Stu

Mikera

unread,
Dec 26, 2013, 6:08:02 PM12/26/13
to cloju...@googlegroups.com
Hi Stu,

Well - it's just a suggestion: I still think that alphas are a good way to efficiently test proposed improvements. I'm personally very willing to test alpha releases against my code bases and provide useful feedback / fixes - I suspect this is true for many in the community. 

Regarding the specific issue at hand: the thought occurs that this issue is essentially a conflict between two expectations:
- The logical expectation that macro expansion creates a complete new form, and hence clobbers metadata
- The reasonable user expectation that they can type hint an arbitrary form, and have that type hint honoured

In light of this: Maybe the real issue is that the compiler is reading the type hint *after* macro expansion, whereas in reality it should be read *before* macro expansion - then it doesn't matter what tag the macro expansion returns, the user-provided hint will still take precedence. I think this simple solution would restore expected behaviour for users, without requiring us to put special case handling in the way that macros treat metadata.

kovas boguta

unread,
Dec 27, 2013, 5:04:51 PM12/27/13
to cloju...@googlegroups.com
On Thu, Dec 26, 2013 at 6:08 PM, Mikera <mike_r_...@yahoo.co.uk> wrote:

> In light of this: Maybe the real issue is that the compiler is reading the
> type hint *after* macro expansion, whereas in reality it should be read
> *before* macro expansion - then it doesn't matter what tag the macro
> expansion returns, the user-provided hint will still take precedence. I
> think this simple solution would restore expected behaviour for users,
> without requiring us to put special case handling in the way that macros
> treat metadata.

This is a pretty good point. If the problem is that type hinting
doesn't work, then fix the type hinting.

Until there is a clojure-in-clojure compiler, minimal changes to these
mechanisms seem like a sensible approach.
Reply all
Reply to author
Forward
0 new messages