Clojure

Empty PersistentCollections get incorrectly evaluated as their generic clojure counterpart

Details

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

Description

user> (defrecord x [])
user.x
user> #user.x[]   ;; expect: #user.x{}
{}
user> #user.x{}   ;; expect: #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1)  ;; expect: clojure.lang.PersistentTreeMap
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.
Since EmptyExpr is a compiler optimization that applies only to some concrete clojure collections, making EmptyExpr dispatch on concrete types rather than on generic interfaces makes the compiler behave as expected

Patch: 0001-CLJ-1093-v2.patch

Screened by:

Activity

Nicola Mometto made changes -
Field Original Value New Value
Attachment 001-fix-empty-record-literal.patch [ 11609 ]
Timothy Baldridge made changes -
Assignee Timothy Baldridge [ halgari ]
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.
Timothy Baldridge made changes -
Approval Not Approved [ 10008 ]
Hide
Timothy Baldridge added a comment -

Could not reproduce in master.

Show
Timothy Baldridge added a comment - Could not reproduce in master.
Timothy Baldridge made changes -
Resolution Declined [ 2 ]
Status Open [ 1 ] Resolved [ 5 ]
Stuart Halloway made changes -
Status Resolved [ 5 ] Closed [ 6 ]
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.
Nicola Mometto made changes -
Approval Not Approved [ 10008 ]
Status Closed [ 6 ] Reopened [ 4 ]
Resolution Declined [ 2 ]
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.
Andy Fingerhut made changes -
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
Nicola Mometto made changes -
Attachment 001-fix-empty-record-literal.patch [ 11609 ]
Nicola Mometto made changes -
Attachment 0001-CLJ-1093-fix-empty-records-literal-v2.patch [ 12047 ]
Nicola Mometto made changes -
Description before patch:
user=> (defrecord a [b])
user.a
user=> #user.a{}
{}

after patch:
user=> (defrecord a [b])
user.a
user=> #user.a{}
#user.a{:b nil}
before patch:
user=> (defrecord a [b])
user.a
user=> #user.a{}
{}
user=> (class #clojure.lang.PersistentTreeMap[])
clojure.lang.PersistentArrayMap

after patch:
user=> (defrecord a [b])
user.a
user=> #user.a{}
#user.a{:b nil}
user=> (class #clojure.lang.PersistentTreeMap[])
clojure.lang.PersistentTreeMap

Attachment 0001-CLJ-1093-fix-empty-records-literal-v2.patch [ 12048 ]
Summary Empty record literals gets incorrectly evaluated to array-maps Empty PersistentCollections get incorrectly evaluated as their generic clojure counterpart
Nicola Mometto made changes -
Attachment 0001-CLJ-1093-fix-empty-records-literal-v2.patch [ 12047 ]
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.
Alex Miller made changes -
Approval Triaged [ 10120 ]
Rich Hickey made changes -
Approval Triaged [ 10120 ] Vetted [ 10003 ]
Alex Miller made changes -
Description before patch:
user=> (defrecord a [b])
user.a
user=> #user.a{}
{}
user=> (class #clojure.lang.PersistentTreeMap[])
clojure.lang.PersistentArrayMap

after patch:
user=> (defrecord a [b])
user.a
user=> #user.a{}
#user.a{:b nil}
user=> (class #clojure.lang.PersistentTreeMap[])
clojure.lang.PersistentTreeMap

{code}
user> (defrecord x [])
user.x
user> #user.x[]
{}
user> #user.x{}
{}
{code}
Priority Major [ 3 ] Minor [ 4 ]
Alex Miller made changes -
Description {code}
user> (defrecord x [])
user.x
user> #user.x[]
{}
user> #user.x{}
{}
{code}
{code}
user> (defrecord x [])
user.x
user> #user.x[]
{}
user> #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1)
clojure.lang.PersistentArrayMap
{code}

*Cause:*

*Proposed:*

*Patch:*

*Screened by:*
Alex Miller made changes -
Description {code}
user> (defrecord x [])
user.x
user> #user.x[]
{}
user> #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1)
clojure.lang.PersistentArrayMap
{code}

*Cause:*

*Proposed:*

*Patch:*

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

*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:*
Rich Hickey made changes -
Fix Version/s Release 1.7 [ 10250 ]
Hide
Alex Miller added a comment -

In the change for ObjectExpr.emitValue() where you've added PersistentArrayMap to the PersistentHashMap case, should the IPersistentVector case below that be PersistentVector instead, otherwise it would snare a custom IPersistentVector that's not a PersistentVector, right?

This line: "else if(form instanceof ISeq)" at the end of the Compiler diff has different leading tabs which makes the diff slightly more confusing than it could be.

Would be nice to add a test for the sorted map case in the description.

Marking incomplete to address some of these.

Show
Alex Miller added a comment - In the change for ObjectExpr.emitValue() where you've added PersistentArrayMap to the PersistentHashMap case, should the IPersistentVector case below that be PersistentVector instead, otherwise it would snare a custom IPersistentVector that's not a PersistentVector, right? This line: "else if(form instanceof ISeq)" at the end of the Compiler diff has different leading tabs which makes the diff slightly more confusing than it could be. Would be nice to add a test for the sorted map case in the description. Marking incomplete to address some of these.
Alex Miller made changes -
Approval Vetted [ 10003 ] Incomplete [ 10006 ]
Assignee Timothy Baldridge [ halgari ]
Alex Miller made changes -
Labels collections compiler
Hide
Alex Miller added a comment -

bump

Show
Alex Miller added a comment - bump
Hide
Nicola Mometto added a comment -

Attached patch 0001-CLJ-1093-fix-empty-collection-literal-evaluation.patch which implements your suggestions.

replacing IPersistentVector with PersistentVector in ObjectExpr.emitValue() exposes a print-dup limitation: it expects every IPersistentCollection to have a static "create" method.

This required special casing for MapEntry and APersistentVector$SubVector

Show
Nicola Mometto added a comment - Attached patch 0001-CLJ-1093-fix-empty-collection-literal-evaluation.patch which implements your suggestions. replacing IPersistentVector with PersistentVector in ObjectExpr.emitValue() exposes a print-dup limitation: it expects every IPersistentCollection to have a static "create" method. This required special casing for MapEntry and APersistentVector$SubVector
Nicola Mometto made changes -
Attachment 0001-CLJ-1093-fix-empty-collection-literal-evaluation.patch [ 13015 ]
Alex Miller made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]
Alex Miller made changes -
Description {code}
user> (defrecord x [])
user.x
user> #user.x[]
{}
user> #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1)
clojure.lang.PersistentArrayMap
{code}

*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:*
{code}
user> (defrecord x [])
user.x
user> #user.x[]
{}
user> #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1)
clojure.lang.PersistentArrayMap
{code}

*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-collection-literal-evaluation.patch

*Screened by:*
Hide
Nicola Mometto added a comment -

I updated the patch adding print-dups for APersistentVector$SubVec and other IPersistentVectors rather than special casing them in the compiler

Show
Nicola Mometto added a comment - I updated the patch adding print-dups for APersistentVector$SubVec and other IPersistentVectors rather than special casing them in the compiler
Nicola Mometto made changes -
Attachment 0001-CLJ-1093-fix-empty-collection-literal-evaluation.patch [ 13027 ]
Nicola Mometto made changes -
Attachment 0001-CLJ-1093-fix-empty-collection-literal-evaluation.patch [ 13015 ]
Alex Miller made changes -
Description {code}
user> (defrecord x [])
user.x
user> #user.x[]
{}
user> #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1)
clojure.lang.PersistentArrayMap
{code}

*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-collection-literal-evaluation.patch

*Screened by:*
{code}
user> (defrecord x [])
user.x
user> #user.x[] ;; expect: #user.x{}
{}
user> #user.x{} ;; expect: #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1) ;; expect: clojure.lang.PersistentTreeMap
clojure.lang.PersistentArrayMap
{code}

*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-collection-literal-evaluation.patch

*Screened by:*
Hide
Alex Miller added a comment -

All of the checks on concrete classes in the Compiler parts of this patch don't sit well with me. I understand how you got to this point and I don't have an alternate recommendation (yet) but all of that just feels like the wrong direction.

We want to be built on abstractions such that internal collections are not special; they should conform to the same expectations as an external collection and both should follow the same paths in the compiler - needing to check for those types is a flag for me that something is amiss.

I am marking Incomplete for now based on these thoughts.

Show
Alex Miller added a comment - All of the checks on concrete classes in the Compiler parts of this patch don't sit well with me. I understand how you got to this point and I don't have an alternate recommendation (yet) but all of that just feels like the wrong direction. We want to be built on abstractions such that internal collections are not special; they should conform to the same expectations as an external collection and both should follow the same paths in the compiler - needing to check for those types is a flag for me that something is amiss. I am marking Incomplete for now based on these thoughts.
Alex Miller made changes -
Approval Vetted [ 10003 ] Incomplete [ 10006 ]
Hide
Nicola Mometto added a comment -

I've been thinking for a while about this issue and I've come to the conclusion that in my later patches I've been trying to incorporate fixes for 3 different albeit related issues:

1- Clojure transforms all empty IPersistentCollections in their generic Clojure counterpart

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

2- Clojure transforms all the literals of collections implementing the Clojure interfaces (IPersistentList, IPersistentVector ..) that are NOT defined with deftype or defrecord, to their
generic Clojure counterpart

user=> (class (eval (sorted-map 1 1)))
clojure.lang.PersistentArrayMap ;; expected: clojure.lang.PersistentTreeMap

3- print-dup is broken for some Clojure persistent collections

user=> (print-dup (subvec [1] 0) *out*)
#=(clojure.lang.APersistentVector$SubVector/create [1])
user=> #=(clojure.lang.APersistentVector$SubVector/create [1])
IllegalArgumentException No matching method found: create  clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:53)

I'll keep this ticket regarding issue #1 and open two other tickets for issue #2 and #3

Show
Nicola Mometto added a comment - I've been thinking for a while about this issue and I've come to the conclusion that in my later patches I've been trying to incorporate fixes for 3 different albeit related issues: 1- Clojure transforms all empty IPersistentCollections in their generic Clojure counterpart
user> (defrecord x [])
user.x
user> #user.x[]   ;; expected: #user.x{}
{}
user> #user.x{}   ;; expected: #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1)  ;; expected: clojure.lang.PersistentTreeMap
clojure.lang.PersistentArrayMap
2- Clojure transforms all the literals of collections implementing the Clojure interfaces (IPersistentList, IPersistentVector ..) that are NOT defined with deftype or defrecord, to their generic Clojure counterpart
user=> (class (eval (sorted-map 1 1)))
clojure.lang.PersistentArrayMap ;; expected: clojure.lang.PersistentTreeMap
3- print-dup is broken for some Clojure persistent collections
user=> (print-dup (subvec [1] 0) *out*)
#=(clojure.lang.APersistentVector$SubVector/create [1])
user=> #=(clojure.lang.APersistentVector$SubVector/create [1])
IllegalArgumentException No matching method found: create  clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:53)
I'll keep this ticket regarding issue #1 and open two other tickets for issue #2 and #3
Nicola Mometto made changes -
Attachment 0001-CLJ-1093-fix-empty-records-literal-v2.patch [ 12048 ]
Nicola Mometto made changes -
Attachment 0001-CLJ-1093-fix-empty-collection-literal-evaluation.patch [ 13027 ]
Hide
Nicola Mometto added a comment -

I've attached a new patch fixing only this issue, the approach is explained in the description

Show
Nicola Mometto added a comment - I've attached a new patch fixing only this issue, the approach is explained in the description
Nicola Mometto made changes -
Nicola Mometto made changes -
Description {code}
user> (defrecord x [])
user.x
user> #user.x[] ;; expect: #user.x{}
{}
user> #user.x{} ;; expect: #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1) ;; expect: clojure.lang.PersistentTreeMap
clojure.lang.PersistentArrayMap
{code}

*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-collection-literal-evaluation.patch

*Screened by:*
{code}
user> (defrecord x [])
user.x
user> #user.x[] ;; expect: #user.x{}
{}
user> #user.x{} ;; expect: #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1) ;; expect: clojure.lang.PersistentTreeMap
clojure.lang.PersistentArrayMap
{code}

*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.
Since EmptyExpr is a compiler optimization that applies only to some concrete clojure collections, making EmptyExpr dispatch on concrete types rather than on generic interfaces makes the compiler behave as expected

*Patch:* 0001-CLJ-1093-fix-empty-collection-literal-evaluation.patch

*Screened by:*
Nicola Mometto made changes -
Description {code}
user> (defrecord x [])
user.x
user> #user.x[] ;; expect: #user.x{}
{}
user> #user.x{} ;; expect: #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1) ;; expect: clojure.lang.PersistentTreeMap
clojure.lang.PersistentArrayMap
{code}

*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.
Since EmptyExpr is a compiler optimization that applies only to some concrete clojure collections, making EmptyExpr dispatch on concrete types rather than on generic interfaces makes the compiler behave as expected

*Patch:* 0001-CLJ-1093-fix-empty-collection-literal-evaluation.patch

*Screened by:*
{code}
user> (defrecord x [])
user.x
user> #user.x[] ;; expect: #user.x{}
{}
user> #user.x{} ;; expect: #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1) ;; expect: clojure.lang.PersistentTreeMap
clojure.lang.PersistentArrayMap
{code}

*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.
Since EmptyExpr is a compiler optimization that applies only to some concrete clojure collections, making EmptyExpr dispatch on concrete types rather than on generic interfaces makes the compiler behave as expected

*Patch:* 0001-CLJ-1093-fix-compilation-of-empty-PersistentCollecti.patch

*Screened by:*
Alex Miller made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]
Priority Minor [ 4 ] Major [ 3 ]
Hide
Nicola Mometto added a comment -

0001-CLJ-1093-v2.patch is an updated patch that correctly handles metadata evaluation semantics on empty literals and adds some tests for it

Show
Nicola Mometto added a comment - 0001-CLJ-1093-v2.patch is an updated patch that correctly handles metadata evaluation semantics on empty literals and adds some tests for it
Nicola Mometto made changes -
Attachment 0001-CLJ-1093-v2.patch [ 13337 ]
Nicola Mometto made changes -
Description {code}
user> (defrecord x [])
user.x
user> #user.x[] ;; expect: #user.x{}
{}
user> #user.x{} ;; expect: #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1) ;; expect: clojure.lang.PersistentTreeMap
clojure.lang.PersistentArrayMap
{code}

*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.
Since EmptyExpr is a compiler optimization that applies only to some concrete clojure collections, making EmptyExpr dispatch on concrete types rather than on generic interfaces makes the compiler behave as expected

*Patch:* 0001-CLJ-1093-fix-compilation-of-empty-PersistentCollecti.patch

*Screened by:*
{code}
user> (defrecord x [])
user.x
user> #user.x[] ;; expect: #user.x{}
{}
user> #user.x{} ;; expect: #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1) ;; expect: clojure.lang.PersistentTreeMap
clojure.lang.PersistentArrayMap
{code}

*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.
Since EmptyExpr is a compiler optimization that applies only to some concrete clojure collections, making EmptyExpr dispatch on concrete types rather than on generic interfaces makes the compiler behave as expected

*Patch:* 0001-CLJ-1093-v2.patch

*Screened by:*

People

Vote (4)
Watch (4)

Dates

  • Created:
    Updated: