Clojure

Empty PersistentCollections get incorrectly evaluated as their generic clojure counterpart

Details

  • Type: Defect Defect
  • Status: Reopened Reopened
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: Release 1.4, Release 1.5
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Vetted

Description

user> (defrecord x [])
user.x
user> #user.x[]
{}
user> #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1)
clojure.lang.PersistentArrayMap

Cause: Compiler's ConstantExpr parser returns an EmptyExpr for all empty persistent collections, even if they are of types other than the core collections (for example: records, sorted collections, custom collections). EmptyExpr reports its java class as one the classes - IPersistentList/IPersistentVector/IPersistentMap/IPersistentSet rather than the original type.

Proposed: If one of the Persistent* classes, then create EmptyExpr as before, otherwise retain the ConstantExpression of the original collection.

Patch: 0001-CLJ-1093-fix-empty-records-literal-v2.patch

Screened by:

Activity

Hide
Timothy Baldridge added a comment -

Unable to reproduce this bug on latest version of master. Most likely fixed by some of the recent changes to data literal readers.

Marking Not-Approved.

Show
Timothy Baldridge added a comment - Unable to reproduce this bug on latest version of master. Most likely fixed by some of the recent changes to data literal readers. Marking Not-Approved.
Hide
Timothy Baldridge added a comment -

Could not reproduce in master.

Show
Timothy Baldridge added a comment - Could not reproduce in master.
Hide
Nicola Mometto added a comment -

I just checked, and the problem still exists for records with no arguments:

Clojure 1.6.0-master-SNAPSHOT
user=> (defrecord a [])
user.a
user=> #user.a[]
{}

Admittedly it's an edge case and I see little usage for no-arguments records, but I think it should be addressed aswell since the current behaviour is not what one would expect

Show
Nicola Mometto added a comment - I just checked, and the problem still exists for records with no arguments: Clojure 1.6.0-master-SNAPSHOT user=> (defrecord a []) user.a user=> #user.a[] {} Admittedly it's an edge case and I see little usage for no-arguments records, but I think it should be addressed aswell since the current behaviour is not what one would expect
Hide
Herwig Hochleitner added a comment -

Got the following REPL interaction:

% java -jar ~/.m2/repository/org/clojure/clojure/1.5.0/clojure-1.5.0.jar
user=> (defrecord a [])
user.a
user=> (a.)
#user.a{}
user=> #user.a{}
{}
#user.a[]
{}

This should be reopened or declined for another reason than reproducability.

Show
Herwig Hochleitner added a comment - Got the following REPL interaction:
% java -jar ~/.m2/repository/org/clojure/clojure/1.5.0/clojure-1.5.0.jar
user=> (defrecord a [])
user.a
user=> (a.)
#user.a{}
user=> #user.a{}
{}
#user.a[]
{}
This should be reopened or declined for another reason than reproducability.
Hide
Nicola Mometto added a comment -

I'm reopening this since the bug is still there.

Show
Nicola Mometto added a comment - I'm reopening this since the bug is still there.
Hide
Andy Fingerhut added a comment -

Patch clj-1093-fix-empty-record-literal-patch-v2.txt dated Mar 13, 2013 is identical to Bronsa's patch 001-fix-empty-record-literal.patch dated Oct 24, 2012, except that it applies cleanly to latest master. I'm not sure why the older patch doesn't but git doesn't like something about it.

Show
Andy Fingerhut added a comment - Patch clj-1093-fix-empty-record-literal-patch-v2.txt dated Mar 13, 2013 is identical to Bronsa's patch 001-fix-empty-record-literal.patch dated Oct 24, 2012, except that it applies cleanly to latest master. I'm not sure why the older patch doesn't but git doesn't like something about it.
Hide
Nicola Mometto added a comment - - edited

Patch 0001-CLJ-1093-fix-empty-records-literal-v2.patch solves more issues than the previous patch that was not evident to me at the time.

Only collections that are either PersistentList or PersistentVector or PersistentHash[Map|Set] or PersistentArrayMap can now be EmptyExpr.
This is because we don't want every IPersistentCollection to be emitted as a generic one eg.

user=> (class #clojure.lang.PersistentTreeMap[])
clojure.lang.PersistentArrayMap

Incidentally, this patch also solves CLJ-1187
This patch should be preferred over the one on CLJ-1187 since it's more general

Show
Nicola Mometto added a comment - - edited Patch 0001-CLJ-1093-fix-empty-records-literal-v2.patch solves more issues than the previous patch that was not evident to me at the time. Only collections that are either PersistentList or PersistentVector or PersistentHash[Map|Set] or PersistentArrayMap can now be EmptyExpr. This is because we don't want every IPersistentCollection to be emitted as a generic one eg. user=> (class #clojure.lang.PersistentTreeMap[]) clojure.lang.PersistentArrayMap Incidentally, this patch also solves CLJ-1187 This patch should be preferred over the one on CLJ-1187 since it's more general
Hide
Jozef Wagner added a comment -

Maybe this is related:

user=> (def x `(quote ~(list 1 (clojure.lang.PersistentTreeMap/create (seq [1 2 3 4])))))
#'user/x
user=> x
(quote (1 {1 2, 3 4}))
user=> (class (second (second x)))
clojure.lang.PersistentTreeMap
user=> (eval x)
(1 {1 2, 3 4})
user=> (class (second (eval x)))
clojure.lang.PersistentArrayMap

Even if the collection is not evaluated, it is still converted to the generic clojure counterpart.

Show
Jozef Wagner added a comment - Maybe this is related: user=> (def x `(quote ~(list 1 (clojure.lang.PersistentTreeMap/create (seq [1 2 3 4]))))) #'user/x user=> x (quote (1 {1 2, 3 4})) user=> (class (second (second x))) clojure.lang.PersistentTreeMap user=> (eval x) (1 {1 2, 3 4}) user=> (class (second (eval x))) clojure.lang.PersistentArrayMap Even if the collection is not evaluated, it is still converted to the generic clojure counterpart.

People

Vote (2)
Watch (1)

Dates

  • Created:
    Updated: