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
  • Approval:
    Not Approved

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

Ascending order - Click to sort in descending order
Hide
Permalink
Assembla Importer added a comment -

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

Show
Assembla Importer added a comment - Converted from http://www.assembla.com/spaces/clojure/tickets/149
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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.
Hide
Permalink
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.
Hide
Permalink
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.
Hide
Permalink
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.
Hide
Permalink
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})

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.