Clojure

Empty PersistentCollections get incorrectly evaluated as their generic clojure counterpart

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.4, Release 1.5
  • Fix Version/s: Release 1.8
  • Component/s: None
  • Patch:
    Code and Test

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

Nicola, thanks for making this smaller. Two questions:

  • why is the meta check added only to parse, and not analyze
  • do the tests cover both the parse and analyze code paths?
Show
Stuart Halloway added a comment - Nicola, thanks for making this smaller. Two questions:
  • why is the meta check added only to parse, and not analyze
  • do the tests cover both the parse and analyze code paths?
Stuart Halloway made changes -
Approval Vetted [ 10003 ] Incomplete [ 10006 ]
Hide
Nicola Mometto added a comment -

Stuart, the meta check is added only in ConstantExpr.parse and not in analyze because:

  • EmptyExpr.parse which all codepaths delegate to has a meta check that wraps it in a MetaExpr if meta is found
  • We don't want metadata on ConstantExprs to be handled by MetaExpr since MetaExpr handles metadata on non-quoted literals, evaluating the meta.

IOW it encodes the difference between

^{:foo (println "bar")} {}
and
'^{:foo (println "bar")} {}

The tests handle both code paths

Show
Nicola Mometto added a comment - Stuart, the meta check is added only in ConstantExpr.parse and not in analyze because:
  • EmptyExpr.parse which all codepaths delegate to has a meta check that wraps it in a MetaExpr if meta is found
  • We don't want metadata on ConstantExprs to be handled by MetaExpr since MetaExpr handles metadata on non-quoted literals, evaluating the meta.
IOW it encodes the difference between
^{:foo (println "bar")} {}
and
'^{:foo (println "bar")} {}
The tests handle both code paths
Hide
Nicola Mometto added a comment - - edited

Just a note that this patch will probably need to be updated considering CLJ-1517 and CLJ-1610 if the compiler will be changed to emit those unrolled collections (which is not in the current patches)

Show
Nicola Mometto added a comment - - edited Just a note that this patch will probably need to be updated considering CLJ-1517 and CLJ-1610 if the compiler will be changed to emit those unrolled collections (which is not in the current patches)
Hide
Alex Miller added a comment -

Nicola, I think that highlights my biggest qualm with this patch: the hardcoded list of concrete classes in the compiler. To me, that just feels wrong as I do not want that set of classes to be "special". I have not thought about it enough to have an alternative suggestion though.

Show
Alex Miller added a comment - Nicola, I think that highlights my biggest qualm with this patch: the hardcoded list of concrete classes in the compiler. To me, that just feels wrong as I do not want that set of classes to be "special". I have not thought about it enough to have an alternative suggestion though.
Hide
Nicola Mometto added a comment - - edited

Alex, I generally would agree with you that hardcoding concrete classes is a wrong approach but I honestly think that in this particular case it's the sanest thing to do.

This also reflects my opinion that, with the ever growing number of custom collections and the addition of tagged literals and ctor literals in clojure allowing for the embedding custom collections at read-time, the current approach the compiler has of relying on generic interfaces rather than on a closed set of known internal classes to guide code emission, is and will be more and more problematic (see also CLJ-1460,CLJ-1492, CLJ-1575, CLJ-1461)

This is an optimization and as such it makes sense for it to target specific classes: we only want to apply the optimization on such empty literals whose "conversion" to a generic impl won't change behaviour, and this is a decision the compiler can only do using a closed set of classes. Using interfaces or the abstract classes won't be possible (sorted maps implement APersistentMap too).

Other than my proposed solution or removing EmptyExpr altogether I can't think of any other way to fix this issue without

Show
Nicola Mometto added a comment - - edited Alex, I generally would agree with you that hardcoding concrete classes is a wrong approach but I honestly think that in this particular case it's the sanest thing to do. This also reflects my opinion that, with the ever growing number of custom collections and the addition of tagged literals and ctor literals in clojure allowing for the embedding custom collections at read-time, the current approach the compiler has of relying on generic interfaces rather than on a closed set of known internal classes to guide code emission, is and will be more and more problematic (see also CLJ-1460,CLJ-1492, CLJ-1575, CLJ-1461) This is an optimization and as such it makes sense for it to target specific classes: we only want to apply the optimization on such empty literals whose "conversion" to a generic impl won't change behaviour, and this is a decision the compiler can only do using a closed set of classes. Using interfaces or the abstract classes won't be possible (sorted maps implement APersistentMap too). Other than my proposed solution or removing EmptyExpr altogether I can't think of any other way to fix this issue without
Hide
Stuart Halloway added a comment -

More subtle than, but not a bug for the same reasons as, CLJ-1460.

Show
Stuart Halloway added a comment - More subtle than, but not a bug for the same reasons as, CLJ-1460.
Stuart Halloway made changes -
Resolution Declined [ 2 ]
Approval Incomplete [ 10006 ]
Status Reopened [ 4 ] Closed [ 6 ]
Hide
Nicola Mometto added a comment -

Really? The fact that record ctor syntax doesn't roundtrip is not a bug?

Show
Nicola Mometto added a comment - Really? The fact that record ctor syntax doesn't roundtrip is not a bug?
Hide
Andy Fingerhut added a comment -

Is it true that the only record ctors affected by this are those that have 0 fields? At least cursory testing with similar examples as in the description, but with 1 field in the record, show what seems to be the expected behavior.

Show
Andy Fingerhut added a comment - Is it true that the only record ctors affected by this are those that have 0 fields? At least cursory testing with similar examples as in the description, but with 1 field in the record, show what seems to be the expected behavior.
Hide
Nicola Mometto added a comment -

Yes, this ticket is about empty IPCs, and the only way a record can be empty is if it has 0 fields

Show
Nicola Mometto added a comment - Yes, this ticket is about empty IPCs, and the only way a record can be empty is if it has 0 fields
Rich Hickey made changes -
Status Closed [ 6 ] Reopened [ 4 ]
Resolution Declined [ 2 ]
Rich Hickey made changes -
Resolution Completed [ 1 ]
Status Reopened [ 4 ] Resolved [ 5 ]
Hide
Alex Miller added a comment -
Show
Alex Miller added a comment - Rich made a change for this here: https://github.com/clojure/clojure/commit/1d5237f9d7db0bc5f6e929330108d016ac7bf76c
Alex Miller made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

Vote (5)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: