<< Back to previous view

[CLJ-149] pop does not preserve meta-data Created: 10/Jul/09  Updated: 01/Mar/13  Resolved: 19/Mar/11

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.3

Type: Defect Priority: Major
Reporter: Assembla Importer Assignee: Aaron Bedra
Resolution: Declined Votes: 0
Labels: None

Patch: Code

 Description   

As discussed with chouser on irc.

Example:

=> (meta (conj (with-meta '(1 2 3) {:my :meta}) 4))
{:my :meta}
=> (meta (pop (with-meta '(1 2 3) {:my :meta}))) 
nil

Both calls should yield the same result.

This occurs in 1.0.0 as well as trunk.

This might be part of a larger issue with when meta-data should be preserved (e.g., butlast).



 Comments   
Comment by Assembla Importer [ 03/Oct/10 4:08 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/149

Comment by Assembla Importer [ 03/Oct/10 4:08 PM ]

chouser@n01se.net said: Alex, sorry if my various statements mislead you. What you described may be a bug, but I don't know that for sure. Rich would have to rule on what the desired behavior is there.

However, I'm pretty sure that the metadata is getting lost here incorrectly:

<pre>
(let [v (with-meta [1 2 3] {:my :meta})]
(map meta [(conj v 4) (pop v)]))

returns: (nil nil)
</pre>

Because if you start with a PersistentVector instead of a LazilyPersistentVector, the metadata is preserved:

<pre>
(let [v (with-meta (pop [1 2 3]) {:my :meta})]
(map meta [(conj v 4) (pop v)]))

returns: ({:my :meta} {:my :meta})
</pre>

The problem seems to be that metadata is lost when the LazilyPersistentVector is converted to a PersistentVector. I'll attach a patch to address this.

Comment by Assembla Importer [ 03/Oct/10 4:08 PM ]

chouser@n01se.net said: [file:dBRCkCBBar3QrKeJe5aVNr]: Move metadata from LazilyPersistentVectors to PersistentVectors when the latter are created.

Comment by Assembla Importer [ 03/Oct/10 4:08 PM ]

richhickey said: That diff doesn't seem to be the right one

Comment by Assembla Importer [ 03/Oct/10 4:08 PM ]

chouser@n01se.net said: Bother. I think I deleted my local copy of the correct patch after attaching the incorrect one here.

It just added .withMeta(meta()) in LazilyPersistentVector's v() method, plus some related unit tests. I'll recreate it when I get a chance.

Comment by Assembla Importer [ 03/Oct/10 4:08 PM ]

richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)

Comment by Alexander Redington [ 12/Nov/10 10:15 AM ]

Reported behavior is still observed in the latest development sources.

Comment by Alexander Taggart [ 01/Mar/11 1:45 AM ]

PersistentList.pop() will now create a new head of the forthcoming list if it doesn't share the same metadata instance.

Comment by Stuart Halloway [ 04/Mar/11 3:27 PM ]

This works as advertised, but I am unclear on what should happen. Given

(a b)

where both the conses have metadata, what rule specifies which metadata should be left after pop? If there is a rule, we should document it, and I bet there are other breakages.

Comment by Rich Hickey [ 10/Mar/11 11:32 AM ]

This is a non-problem (but the vector one Chouser found is a real problem). Lists are not going to create new heads to convey metadata on pop. Lists really are chains of things, not encapsulations of chains of things. The metadata of the second link is what it was, and shouldn't be lost by pop, i.e. pop should not construct a new list as it goes.

Comment by Alexander Taggart [ 10/Mar/11 12:50 PM ]

Very well. Note that the failure example chouser gave no longer occurs:

user=> (let [v (with-meta [1 2 3] {:my :meta})]
         (map meta [(conj v 4) (pop v)]))
({:my :meta} {:my :meta})
Generated at Thu Dec 18 22:49:39 CST 2014 using JIRA 4.4#649-r158309.