<< Back to previous view

[CLJ-1093] Empty PersistentCollections get incorrectly evaluated as their generic clojure counterpart Created: 24/Oct/12  Updated: 06/Jul/14

Status: Reopened
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5
Fix Version/s: Release 1.7

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: collections, compiler

Attachments: Text File 0001-CLJ-1093-fix-compilation-of-empty-PersistentCollecti.patch     Text File clj-1093-fix-empty-record-literal-patch-v2.txt    
Patch: Code and Test
Approval: Incomplete

 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-fix-compilation-of-empty-PersistentCollecti.patch

Screened by:



 Comments   
Comment by Timothy Baldridge [ 27/Nov/12 11:41 AM ]

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.

Comment by Timothy Baldridge [ 27/Nov/12 11:41 AM ]

Could not reproduce in master.

Comment by Nicola Mometto [ 01/Mar/13 1:23 PM ]

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

Comment by Herwig Hochleitner [ 02/Mar/13 8:14 AM ]

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.

Comment by Nicola Mometto [ 10/Mar/13 2:18 PM ]

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

Comment by Andy Fingerhut [ 13/Mar/13 2:04 PM ]

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.

Comment by Nicola Mometto [ 26/Jun/13 8:06 PM ]

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

Comment by Jozef Wagner [ 09/Aug/13 2:08 AM ]

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.

Comment by Alex Miller [ 24/Apr/14 4:44 PM ]

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.

Comment by Alex Miller [ 13/May/14 10:43 PM ]

bump

Comment by Nicola Mometto [ 14/May/14 4:24 AM ]

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

Comment by Nicola Mometto [ 16/May/14 3:57 PM ]

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

Comment by Alex Miller [ 23/May/14 4:21 PM ]

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.

Comment by Nicola Mometto [ 06/Jul/14 10:01 AM ]

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

Comment by Nicola Mometto [ 06/Jul/14 10:15 AM ]

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





[CLJ-787] transient blows up when passed a vector created by subvec Created: 03/May/11  Updated: 23/May/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Alexander Redington Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJ-787-p1.patch    
Patch: Code and Test
Approval: Incomplete

 Description   

Subvectors created with subvec from a PersistentVector cannot be made transient:

user=> (transient (subvec [1 2 3 4] 2))
ClassCastException clojure.lang.APersistentVector$SubVector cannot be cast to clojure.lang.IEditableCollection  clojure.core/transient (core.clj:2864)

Cause: APersistentVector$SubVector does not implement IEditableCollection

Patch: CLJ-787-p1.patch

Approach: Create a TransientSubVector based on an underlying TransientVector.

Two assumptions:

  • It's okay for TransientSubVector to delegate the ensureEditable functionality to the underlying TransientVector (sometimes explicitly, sometimes implicitly) - calling ensureEditable explicitly also requires that the field for the underlying vector be the concrete TransientVector type rather than the ITransientVector interface.
  • When an operation that would throw an exception on a PersistentVector happens from the wrong thread (or after persistent!), we throw that exception rather than the IllegalAccessError that transients throw when accessed inappropriately.


 Comments   
Comment by Stuart Sierra [ 31/May/11 9:28 AM ]

Confirmed. APersistentVector$SubVector does not implement IEditableCollection.

The current implementation of TransientVector depends on implementation details of PersistentVector, so it is not a trivial fix. The simplest fix might be to implement IEditableCollection.asTransient in SubVector by creating a new PersistentVector, but I do not know the performance implications.

Comment by Gary Fredericks [ 25/May/13 8:11 PM ]

We could get the same performance characteristics as SubVector by creating a TransientSubVector based on an underlying TransientVector, right?

Preparing a patch to that effect.

Comment by Gary Fredericks [ 25/May/13 10:58 PM ]

Text from the commit msg:

Made two assumptions:

  • It's okay for TransientSubVector to delegate the ensureEditable
    functionality to the underlying TransientVector (sometimes
    explicitely, sometimes implicitely) – calling ensureEditable
    explicitely also requires that the field for the underlying vector
    be the concrete TransientVector type rather than the
    ITransientVector interface.
  • When an operation that would throw an exception on a
    PersistentVector happens from the wrong thread (or after
    persistent!), we throw that exception rather than the
    IllegalAccessError that transients throw when accessed
    inappropriately.
Comment by Alex Miller [ 11/Oct/13 4:17 PM ]

I think there are some assumptions being made in this patch about the class structure here that do not hold. The structure is, admittedly, quite twisty.

A counter-example that highlights one of a few subtypes of APersistentVector that are not PersistentVector (like MapEntry):

user=> (transient (subvec (first {:a 1}) 0 1))
ClassCastException clojure.lang.MapEntry cannot be cast to clojure.lang.IEditableCollection  clojure.lang.APersistentVector$TransientSubVector.<init> (APersistentVector.java:592)

PersistentVector.SubVector expects to work on anything that implements IPersistentVector. Note that this includes concrete types such as MapEntry and LazilyPersistentVector, but could also be any user-implemented type IPersistentVector type too. TransientSubVector is making the assumption that the IPersistentVector in a SubVector question is also an IEditableCollection (that can be converted to be transient). Note that while PersistentVector implements TransientVector (and IEditableCollection), APersistentVector does not. To really implement this in tandem with SubVector, I think you would need to guarantee that IPersistentVector extended IEditableCollection and I don't think that's something we want to do.

I don't see an easy solution. Any time I see all these modifiers (Transient, Sub, etc) being created in different combinations, it is a clear sign that independent kinds of functionality are being remixed into single inheritance OO trees. You can see the same thing in most collection libraries (even Java's - need a ConcurrentIdentitySortedMap? too bad!).

Needs more thought.

Comment by Andy Fingerhut [ 08/Nov/13 10:17 AM ]

Patch CLJ-787-p1.patch no longer applies cleanly to latest master, but it is only because of some new tests added to the transients.clj file since the patch was created, so it is trivial to update in that sense. Not updating it now due to other more significant issues with the patch described above.

Comment by Alex Miller [ 17/Jan/14 10:19 AM ]

No good solution to consider right now, removing from 1.6.





[CLJ-1161] sources jar has bad versions.properties resource Created: 11/Feb/13  Updated: 25/Apr/14

Status: Reopened
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5
Fix Version/s: Release 1.6, Release 1.7

Type: Defect Priority: Minor
Reporter: Steve Miner Assignee: Stuart Halloway
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-1161-Remove-version.properties-from-sources-JAR.patch    
Patch: Code
Approval: Incomplete

 Description   

The "sources" jar (at least since Clojure 1.4 and including 1.5 RC) has a bad version.properties file in it. The resource clojure/version.properties is literally:

version=${version}

The regular Clojure jar has the correct version string in that resource.

I came across a problem when I was experimenting with the sources jar (as used by IDEs). I naively added the sources jar to my classpath, and Clojure died on start up. The bad clojure/versions.properties file was found first, which led to a parse error as the clojure version was being set.

Solution: patch leaves version.properties file out of sources JAR, where it causes problems for tools.



 Comments   
Comment by Steve Miner [ 11/Feb/13 10:04 AM ]

Notes from the dev mailing list:

The "sources" JAR is generated by another Maven plugin, configured here:
https://github.com/clojure/clojure/blob/clojure-1.5.0-RC15/pom.xml#L169-L181

The simplest solution might be to just exclude the file from the sources jar. It looks like maven-source-plugin has an excludes option which would do the trick:

http://maven.apache.org/plugins/maven-source-plugin/jar-mojo.html#excludes

Comment by Jeff Valk [ 21/Apr/14 8:20 AM ]

This issue is marked closed, but I'm still seeing it: the clojure-1.6.0-sources.jar, clojure-1.5.1-sources.jar, etc on Maven Central still contain the bad version.properties files. More specifically, it looks like the fix has been applied to builds in the SNAPSHOTS repository, but not to RELEASES.

Fix applied: https://oss.sonatype.org/content/repositories/snapshots/org/clojure/clojure/
Not fixed: https://oss.sonatype.org/content/repositories/releases/org/clojure/clojure/

Comment by Alex Miller [ 24/Apr/14 4:15 PM ]

Not sure what's needed here, but marking incomplete.

Comment by Jeff Valk [ 25/Apr/14 11:13 AM ]

Would a fix for this update existing sources jars (1.5.1, 1.6.0, etc) on Central? Or would any fix have to wait on the next Clojure release?

Comment by Alex Miller [ 25/Apr/14 12:37 PM ]

For all the same reasons that mutable state is undesirable, changing an existing release jar in the central Maven repository is also undesirable. While it's not technically impossible, we will not update existing releases and this will need to wait for the next. I've looked at this problem a little and I do not yet know enough to know how to fix it or why it even varies between snapshot and release. Help welcome!

In which tool do you see a resulting problem from this?

Comment by Jeff Valk [ 25/Apr/14 11:56 PM ]

Despite the way I phrased the question, I'd hoped that would be the answer. It's the right policy.

Unfortunately, this issue leaves the released sources jars essentially unusable from a tools standpoint. CIDER now has source code navigation from stacktraces – you can jump into both Clojure and Java function definitions from the error/trace. For the latter, the sources jar (for Clojure or any other Java library) needs to be on the classpath as a dev dependency. There's more host interop support in the works for CIDER too ("embrace the host platform"), but not being able to add a dependency on a stable Clojure sources jar presents a wrinkle.

Are the official Clojure releases built by Hudson? The Hudson build right before the 1.6.0 release (#532) and the one right after (#534) both show the exclusion fix, as does the git clojure-1.6.0 tag, which when I check out and build from source, is fine. The Hudson builds with release tags (e.g. 1.6 = #533, 1.6-RC1 = #512, etc), though, don't show any artifacts other than a pom.xml. This would seem to make it rather hard to audit builds...am I missing something?





Generated at Wed Aug 27 14:04:46 CDT 2014 using JIRA 4.4#649-r158309.