Clojure

vec fails on MapEntry

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Completed
  • Affects Version/s: Release 1.7
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
  • Environment:
    1.7.0-alpha5
  • Patch:
    Code
  • Approval:
    Ok

Description

After CLJ-1546:

(vec (first {1 2}))

Cause: (if (vector? coll) (with-meta coll nil) ...) checks that something is IPersistentVector, then sends it to something that takes IObj, so anything that is one but not both throws an error. In Clojure itself, this is the set of classes extending from AMapEntry.

Alternatives:

1. Make AMapEntry implement IObj - this fixes everything in Clojure and keeps the vec code as is but still leaves open this gap for any external implementation of IPersistentVector.
2. Check for this case explicitly in vec. (if (vector? coll) (if (instance? clojure.lang.IObj coll) (with-meta coll nil) (clojure.lang.LazilyPersistentVector/create coll)) ...). Perf testing shows no significant difference in performance with the change.
3. Pull the special check for vector? in vec.
4. Check for this case explicitly in vec and return the same instance if it's not an IObj. See clj-1637-jdevuyst.patch.

Approach: patch takes approach #2

Patch: clj-1637-with-test.patch

Screened by: Stu (also added test)

  1. clj-1637.patch
    12/Jan/15 4:45 PM
    0.8 kB
    Alex Miller
  2. clj-1637-jdevuyst.patch
    14/Jan/15 6:14 AM
    0.8 kB
    Jonas De Vuyst
  3. clj-1637-with-test.patch
    16/Jan/15 12:53 PM
    2 kB
    Stuart Halloway

Activity

Hide
Nicola Mometto added a comment -

The correct fix is to probably make MapEntry an IObj

Show
Nicola Mometto added a comment - The correct fix is to probably make MapEntry an IObj
Hide
Nicola Mometto added a comment - - edited

Actually, making AMapEntry an IObj rather than MapEntry would fix the issue for sorted-map kv-pairs too.

user=> (vec (first (sorted-map 1 1)))
ClassCastException clojure.lang.PersistentTreeMap$BlackVal cannot be cast to clojure.lang.IObj  clojure.core/with-meta--4121 (core.clj:216)
Show
Nicola Mometto added a comment - - edited Actually, making AMapEntry an IObj rather than MapEntry would fix the issue for sorted-map kv-pairs too.
user=> (vec (first (sorted-map 1 1)))
ClassCastException clojure.lang.PersistentTreeMap$BlackVal cannot be cast to clojure.lang.IObj  clojure.core/with-meta--4121 (core.clj:216)
Hide
Alex Miller added a comment -

There are potentially a couple ways to fix this. I'll look at it next week.

Show
Alex Miller added a comment - There are potentially a couple ways to fix this. I'll look at it next week.
Hide
Jonas De Vuyst added a comment -

Slightly modified patch. In the case where coll is a vector but not an IObj, simply return coll.

Show
Jonas De Vuyst added a comment - Slightly modified patch. In the case where coll is a vector but not an IObj, simply return coll.
Hide
Jonas De Vuyst added a comment -

If desired I could also update the patch to fix an analogous—albeit somewhat theoretic—bug in `set`.

It does make me wonder if perhaps a `without-meta` function should be added to `clojure.core`.

I think making AMapEntry an IObj might make sense even after applying the above patches. In `AMapEntry`, perhaps `withMeta(m)` could be implemented as `asVector().withMeta(m)`. This, however, would require changing `asVector()` to return some `IObj ∩ IPersistentVector` type (e.g. `PersistentVector`). This would be straightforward to do, but requires deciding if this change in signature may be propagated to the static methods of `LazilyPersistentVector`.

Show
Jonas De Vuyst added a comment - If desired I could also update the patch to fix an analogous—albeit somewhat theoretic—bug in `set`. It does make me wonder if perhaps a `without-meta` function should be added to `clojure.core`. I think making AMapEntry an IObj might make sense even after applying the above patches. In `AMapEntry`, perhaps `withMeta(m)` could be implemented as `asVector().withMeta(m)`. This, however, would require changing `asVector()` to return some `IObj ∩ IPersistentVector` type (e.g. `PersistentVector`). This would be straightforward to do, but requires deciding if this change in signature may be propagated to the static methods of `LazilyPersistentVector`.
Hide
Alex Miller added a comment -

This is a point of some debate, but the intention with the change in the implementation is to retain current behavior, which always gives you a new vector instance. It's not clear to me that there is any point in attaching meta to map entries (which also does not solve the problem for external IPersistentVector, non-IObj instances outside Clojure).

In any case, I'm going to update the description a bit to add this as an alternative.

Show
Alex Miller added a comment - This is a point of some debate, but the intention with the change in the implementation is to retain current behavior, which always gives you a new vector instance. It's not clear to me that there is any point in attaching meta to map entries (which also does not solve the problem for external IPersistentVector, non-IObj instances outside Clojure). In any case, I'm going to update the description a bit to add this as an alternative.

People

Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: