Clojure

pop does not preserve meta-data

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Declined
  • Affects Version/s: None
  • Fix Version/s: Release 1.3
  • Component/s: None
  • 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).

Activity

Hide
Assembla Importer added a comment -
Show
Assembla Importer added a comment - Converted from http://www.assembla.com/spaces/clojure/tickets/149
Hide
Assembla Importer added a comment -

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.

Show
Assembla Importer added a comment - 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.
Hide
Assembla Importer added a comment -

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

Show
Assembla Importer added a comment - chouser@n01se.net said: [file:dBRCkCBBar3QrKeJe5aVNr]: Move metadata from LazilyPersistentVectors to PersistentVectors when the latter are created.
Hide
Assembla Importer added a comment -

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

Show
Assembla Importer added a comment - richhickey said: That diff doesn't seem to be the right one
Hide
Assembla Importer added a comment -

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.

Show
Assembla Importer added a comment - 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.
Hide
Assembla Importer added a comment -

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

Show
Assembla Importer added a comment - richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)
Hide
Alexander Redington added a comment -

Reported behavior is still observed in the latest development sources.

Show
Alexander Redington added a comment - Reported behavior is still observed in the latest development sources.
Alexander Redington made changes -
Field Original Value New Value
Priority Blocker [ 1 ]
Approval None Vetted
Stuart Halloway made changes -
Assignee Stuart Halloway [ stu ]
Stuart Halloway made changes -
Assignee Aaron Bedra [ aaron ]
Reporter Assembla Importer [ importer ]
Priority Blocker [ 1 ] Major [ 3 ]
Hide
Alexander Taggart added a comment -

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

Show
Alexander Taggart added a comment - PersistentList.pop() will now create a new head of the forthcoming list if it doesn't share the same metadata instance.
Alexander Taggart made changes -
Attachment 149.diff [ 10128 ]
Hide
Stuart Halloway added a comment -

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.

Show
Stuart Halloway added a comment - 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.
Stuart Halloway made changes -
Fix Version/s Release.Next [ 10038 ]
Fix Version/s Approved Backlog [ 10034 ]
Approval Vetted Screened
Patch Code
Hide
Rich Hickey added a comment -

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.

Show
Rich Hickey added a comment - 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.
Rich Hickey made changes -
Approval Screened None
Hide
Alexander Taggart added a comment -

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})
Show
Alexander Taggart added a comment - 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})
Alexander Taggart made changes -
Attachment 149.diff [ 10128 ]
Rich Hickey made changes -
Status Open [ 1 ] Resolved [ 5 ]
Resolution Declined [ 2 ]
Stuart Halloway made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: