Clojure

contains? broken for transient collections

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: Release 1.2
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Vetted

Description

Behavior with Clojure 1.6.0:

user=> (contains? (transient {:x "fine"}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentArrayMap$TransientArrayMap  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient (hash-map :x "fine")) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashMap$TransientHashMap  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient [1 2 3]) 0)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentVector$TransientVector  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient #{:x}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashSet$TransientHashSet  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (:x (transient #{:x}))
nil
;; expected: :x

user=> (get (transient #{:x}) :x)
nil
;; expected: :x

Behavior with latest Clojure master as of Jun 27 2014 (same as Clojure 1.6.0) plus patch clj-700-7.diff. In all cases it matches the expected results shown in comments above:

user=> (contains? (transient {:x "fine"}) :x)
true
user=> (contains? (transient (hash-map :x "fine")) :x)
true
user=> (contains? (transient [1 2 3]) 0)
true
user=> (contains? (transient #{:x}) :x)
true
user=> (:x (transient #{:x}))
:x
user=> (get (transient #{:x}) :x) 
:x

Analysis by Alexander Redington: This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Questions on this approach from Stuart Halloway to Rich Hickey:

1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?

2. what are good names for the interfaces introduced here?

Alex Miller: Should also keep an eye on CLJ-787 as it may have some collisions with this one.

Patch: clj-700-7.diff

One 'trailing whitespace' warning is perfectly normal when applying this patch to latest Clojure master as of Jun 27 2014, as shown below. This is simply because of carriage returns at the end of lines in file Associative.java. I know of no way to avoid such a warning without removing CRs from all Clojure source files (e.g. CLJ-1026):

% git am -s --keep-cr --ignore-whitespace < ~/clj/patches/clj-700-7.diff 
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/admin/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq
  1. 0001-Refactor-of-some-of-the-clojure-.java-code-to-fix-CL.patch
    07/Jan/11 2:07 PM
    10 kB
    Alexander Redington
  2. clj-700.diff
    25/Mar/11 4:18 PM
    10 kB
    Alexander Redington
  3. clj-700-7.diff
    08/Nov/13 10:14 AM
    11 kB
    Andy Fingerhut
  4. clj-700-patch4.txt
    08/Jun/12 12:44 PM
    11 kB
    Stuart Halloway
  5. clj-700-patch6.txt
    19/Aug/12 4:47 AM
    11 kB
    Andy Fingerhut

Activity

Hide
Herwig Hochleitner added a comment -

the same is also true for TransientVectors

{{(contains? (transient [1 2 3]) 0)}}

false

Show
Herwig Hochleitner added a comment - the same is also true for TransientVectors {{(contains? (transient [1 2 3]) 0)}}
false
Hide
Herwig Hochleitner added a comment -

As expected, TransientSets have the same issue; plus an additional, probably related one.

(:x (transient #{:x}))

nil

(get (transient #{:x}) :x)

nil

Show
Herwig Hochleitner added a comment - As expected, TransientSets have the same issue; plus an additional, probably related one. (:x (transient #{:x}))
nil
(get (transient #{:x}) :x)
nil
Alexander Redington made changes -
Field Original Value New Value
Approval Vetted
Alexander Redington made changes -
Assignee Alexander Redington [ aredington ]
Hide
Alexander Redington added a comment -

This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Show
Alexander Redington added a comment - This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet. This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()). With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.
Alexander Redington made changes -
Alexander Redington made changes -
Patch Code and Test
Alexander Redington made changes -
Assignee Alexander Redington [ aredington ]
Alexander Redington made changes -
Approval Vetted Test
Hide
Stuart Halloway added a comment -

Rich: Patch doesn't currently apply, but I would like to get your take on approach here. In particular:

  1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?
  2. what are good names for the interfaces introduced here?
Show
Stuart Halloway added a comment - Rich: Patch doesn't currently apply, but I would like to get your take on approach here. In particular:
  1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?
  2. what are good names for the interfaces introduced here?
Stuart Halloway made changes -
Waiting On richhickey
Stuart Halloway made changes -
Approval Test Incomplete
Hide
Alexander Redington added a comment -

Rebased the patch off the latest pull of master as of 3/25/2011, it should apply cleanly now.

Show
Alexander Redington added a comment - Rebased the patch off the latest pull of master as of 3/25/2011, it should apply cleanly now.
Alexander Redington made changes -
Attachment clj-700-v2.diff [ 10162 ]
Alexander Redington made changes -
Attachment clj-700.diff [ 10165 ]
Alexander Redington made changes -
Attachment clj-700-v2.diff [ 10162 ]
Hide
Stuart Sierra added a comment -

Latest patch does not apply as of f5bcf647

Show
Stuart Sierra added a comment - Latest patch does not apply as of f5bcf647
Stuart Sierra made changes -
Waiting On richhickey
Hide
Andy Fingerhut added a comment -

clj-700-patch2.txt does patch cleanly to latest Clojure head as of a few mins ago. No changes to patch except in context around changed lines.

Show
Andy Fingerhut added a comment - clj-700-patch2.txt does patch cleanly to latest Clojure head as of a few mins ago. No changes to patch except in context around changed lines.
Andy Fingerhut made changes -
Attachment clj-700-patch2.txt [ 10922 ]
Hide
Andy Fingerhut added a comment -

Sigh. Git patches applied via 'git am' are fragile beasts indeed. Look at them the wrong way and they fail to apply.

clj-700-patch3.txt applies cleanly to latest master as of Mar 7, 2012, but not if you use this command:

git am -s < clj-700-patch3.txt

I am pretty sure this is because of DOS CR/LF line endings in the file src/jvm/clojure/lang/Associative.java. The patch does apply cleanly if you use this command:

git am --keep-cr -s < clj-700-patch3.txt

Show
Andy Fingerhut added a comment - Sigh. Git patches applied via 'git am' are fragile beasts indeed. Look at them the wrong way and they fail to apply. clj-700-patch3.txt applies cleanly to latest master as of Mar 7, 2012, but not if you use this command: git am -s < clj-700-patch3.txt I am pretty sure this is because of DOS CR/LF line endings in the file src/jvm/clojure/lang/Associative.java. The patch does apply cleanly if you use this command: git am --keep-cr -s < clj-700-patch3.txt
Andy Fingerhut made changes -
Attachment clj-700-patch3.txt [ 10981 ]
Hide
Andy Fingerhut added a comment -

This ticket was changed to Incomplete and waiting on Rich when Stuart Halloway asked for feedback on the approach on 28/Jan/2011. Stuart Sierra changed it to not waiting on Rich on 17/Feb/2012 when he noted the patch didn't apply cleanly. Latest patch clj-700-patch3.txt does apply cleanly, but doesn't change the approach used since the time Stuart Halloway's concern was raised. Should it be marked as waiting on Rich again? Something else?

Show
Andy Fingerhut added a comment - This ticket was changed to Incomplete and waiting on Rich when Stuart Halloway asked for feedback on the approach on 28/Jan/2011. Stuart Sierra changed it to not waiting on Rich on 17/Feb/2012 when he noted the patch didn't apply cleanly. Latest patch clj-700-patch3.txt does apply cleanly, but doesn't change the approach used since the time Stuart Halloway's concern was raised. Should it be marked as waiting on Rich again? Something else?
Andy Fingerhut made changes -
Attachment clj-700-patch2.txt [ 10922 ]
Rich Hickey made changes -
Fix Version/s Release 1.5 [ 10150 ]
Hide
Stuart Halloway added a comment -

Patch 4 incorporates patch 3, and brings it up to date on hashing (i.e. uses hasheq).

Show
Stuart Halloway added a comment - Patch 4 incorporates patch 3, and brings it up to date on hashing (i.e. uses hasheq).
Stuart Halloway made changes -
Approval Incomplete [ 10006 ] Screened [ 10004 ]
Attachment clj-700-patch4.txt [ 11301 ]
Andy Fingerhut made changes -
Attachment clj-700-patch3.txt [ 10981 ]
Hide
Andy Fingerhut added a comment -

Removed clj-700-patch3.txt in favor of Stuart Halloway's improved clj-700-patch4.txt dated June 8, 2012.

Show
Andy Fingerhut added a comment - Removed clj-700-patch3.txt in favor of Stuart Halloway's improved clj-700-patch4.txt dated June 8, 2012.
Rich Hickey made changes -
Fix Version/s Release 1.5 [ 10150 ]
Fix Version/s Approved Backlog [ 10034 ]
Hide
Andy Fingerhut added a comment -

clj-700-patch5.txt dated June 18, 2012 is the same as Stuart Halloway's clj-700-patch4.txt, except for context lines that have changed in Clojure master since Stuart's patch was created. clj-700-patch4.txt no longer applies cleanly.

Show
Andy Fingerhut added a comment - clj-700-patch5.txt dated June 18, 2012 is the same as Stuart Halloway's clj-700-patch4.txt, except for context lines that have changed in Clojure master since Stuart's patch was created. clj-700-patch4.txt no longer applies cleanly.
Andy Fingerhut made changes -
Attachment clj-700-patch5.txt [ 11336 ]
Hide
Andy Fingerhut added a comment -

Adding clj-700-patch6.txt, which is identical to Stuart Halloway's clj-700-patch4.txt, except that it applies cleanly to latest master as of Aug 19, 2012. Note that as described above, you must use the --keep-cr option to 'git am' when applying this patch for it to succeed. Removing clj-700-patch5.txt, since it no longer applies cleanly.

Show
Andy Fingerhut added a comment - Adding clj-700-patch6.txt, which is identical to Stuart Halloway's clj-700-patch4.txt, except that it applies cleanly to latest master as of Aug 19, 2012. Note that as described above, you must use the --keep-cr option to 'git am' when applying this patch for it to succeed. Removing clj-700-patch5.txt, since it no longer applies cleanly.
Andy Fingerhut made changes -
Attachment clj-700-patch6.txt [ 11449 ]
Andy Fingerhut made changes -
Attachment clj-700-patch5.txt [ 11336 ]
Hide
Stuart Sierra added a comment -

Patch fails as of commit 1c8eb16a14ce5daefef1df68d2f6b1f143003140

Show
Stuart Sierra added a comment - Patch fails as of commit 1c8eb16a14ce5daefef1df68d2f6b1f143003140
Stuart Sierra made changes -
Approval Screened [ 10004 ] Incomplete [ 10006 ]
Hide
Andy Fingerhut added a comment -

Which patch did you try, and what command did you use? I tried applying clj-700-patch6.txt to the same commit, using the following command, and it applied, albeit with the warning messages shown:

% git am --keep-cr -s < clj-700-patch6.txt
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/jafinger/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq

Note the --keep-cr option, which is necessary for this patch to succeed. It is recommended in the "Screening Tickets" section of the JIRA workflow wiki page here: http://dev.clojure.org/display/design/JIRA+workflow

Show
Andy Fingerhut added a comment - Which patch did you try, and what command did you use? I tried applying clj-700-patch6.txt to the same commit, using the following command, and it applied, albeit with the warning messages shown: % git am --keep-cr -s < clj-700-patch6.txt Applying: Refactor of some of the clojure .java code to fix CLJ-700. /Users/jafinger/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace. public interface Associative extends IPersistentCollection, IAssociative{ warning: 1 line adds whitespace errors. Applying: more CLJ-700: refresh to use hasheq Note the --keep-cr option, which is necessary for this patch to succeed. It is recommended in the "Screening Tickets" section of the JIRA workflow wiki page here: http://dev.clojure.org/display/design/JIRA+workflow
Hide
Andy Fingerhut added a comment -

Presumptuously changing Approval from Incomplete back to None, since the latest patch does apply cleanly if the --keep-cr option is used. It was in Screened state recently, but I'm not so presumptuous as to change it to Screened

Show
Andy Fingerhut added a comment - Presumptuously changing Approval from Incomplete back to None, since the latest patch does apply cleanly if the --keep-cr option is used. It was in Screened state recently, but I'm not so presumptuous as to change it to Screened
Andy Fingerhut made changes -
Approval Incomplete [ 10006 ]
Alex Miller made changes -
Fix Version/s Approved Backlog [ 10034 ]
Fix Version/s Backlog [ 10035 ]
Alex Miller made changes -
Fix Version/s Backlog [ 10035 ]
Hide
Alex Miller added a comment -

I think through a series of different hands on this ticket it got knocked way back in the list. Re-marking vetted as it's previously been all the way up through screening. Should also keep an eye on CLJ-787 as it may have some collisions with this one.

Show
Alex Miller added a comment - I think through a series of different hands on this ticket it got knocked way back in the list. Re-marking vetted as it's previously been all the way up through screening. Should also keep an eye on CLJ-787 as it may have some collisions with this one.
Alex Miller made changes -
Approval Vetted [ 10003 ]
Hide
Andy Fingerhut added a comment -

clj-700-7.diff is identical to clj-700-patch6.txt, except it applies cleanly to latest master. Only some lines of context in a test file have changed.

When I say "applies cleanly", I mean that there is one warning when using the proper "git am" command from the dev wiki page. This is because one line replaced in Associative.java has a CR/LF at the end of the line, because all lines in that file do.

Show
Andy Fingerhut added a comment - clj-700-7.diff is identical to clj-700-patch6.txt, except it applies cleanly to latest master. Only some lines of context in a test file have changed. When I say "applies cleanly", I mean that there is one warning when using the proper "git am" command from the dev wiki page. This is because one line replaced in Associative.java has a CR/LF at the end of the line, because all lines in that file do.
Andy Fingerhut made changes -
Attachment clj-700-7.diff [ 12458 ]
Alex Miller made changes -
Description {{(contains? (transient {:x "fine"}) :x)}}
{quote}{{false}}{quote}

also

{{(contains? (transient (hash-map :x "fine")) :x)}}
{quote}{{false}}{quote}
{{(contains? (transient {:x "fine"}) :x)}}
{quote}{{false}}{quote}

also

{{(contains? (transient (hash-map :x "fine")) :x)}}
{quote}{{false}}{quote}

*Patch:* clj-700-7.diff
Hide
Herwig Hochleitner added a comment -

Since clojure 1.5, contains? throws an IllegalArgumentException on transients.
In 1.6.0-beta1, transients are no longer marked as alpha.

Does this mean, that we won't be able to distinguish between a nil value and no value on a transient?

Show
Herwig Hochleitner added a comment - Since clojure 1.5, contains? throws an IllegalArgumentException on transients. In 1.6.0-beta1, transients are no longer marked as alpha. Does this mean, that we won't be able to distinguish between a nil value and no value on a transient?
Alex Miller made changes -
Summary contains? broken for TransientMaps contains? broken for transient collections
Rich Hickey made changes -
Fix Version/s Release 1.7 [ 10250 ]
Hide
Stuart Halloway added a comment -

Request for someone to (1) update patch to apply cleanly, and (2) summarize approach so I don't have to read through the comment history.

Show
Stuart Halloway added a comment - Request for someone to (1) update patch to apply cleanly, and (2) summarize approach so I don't have to read through the comment history.
Stuart Halloway made changes -
Approval Vetted [ 10003 ] Incomplete [ 10006 ]
Hide
Andy Fingerhut added a comment - - edited

The latest patch is clj-700-7.diff dated Nov 8, 2013. I believe it is impossible to create a patch that applies any more cleanly using git for source files that have carriage returns in them, which at least one modified source file does. Here is the command I used on latest Clojure master as of today (Jun 27 2014), which is the same as that of March 25 2014:

% git am -s --keep-cr --ignore-whitespace < ~/clj/patches/clj-700-7.diff 
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/admin/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq

If you want a patch that doesn't have the 'trailing whitespace' warning in it, I think someone would have to commit a change that removed the carriage returns from file Associative.java. If you want such a patch, let me know and we can remove all of them from every source file and be done with this annoyance.

Show
Andy Fingerhut added a comment - - edited The latest patch is clj-700-7.diff dated Nov 8, 2013. I believe it is impossible to create a patch that applies any more cleanly using git for source files that have carriage returns in them, which at least one modified source file does. Here is the command I used on latest Clojure master as of today (Jun 27 2014), which is the same as that of March 25 2014:
% git am -s --keep-cr --ignore-whitespace < ~/clj/patches/clj-700-7.diff 
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/admin/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq
If you want a patch that doesn't have the 'trailing whitespace' warning in it, I think someone would have to commit a change that removed the carriage returns from file Associative.java. If you want such a patch, let me know and we can remove all of them from every source file and be done with this annoyance.
Andy Fingerhut made changes -
Description {{(contains? (transient {:x "fine"}) :x)}}
{quote}{{false}}{quote}

also

{{(contains? (transient (hash-map :x "fine")) :x)}}
{quote}{{false}}{quote}

*Patch:* clj-700-7.diff
Behavior with Clojure 1.6.0:

{code}
user=> (contains? (transient {:x "fine"}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentArrayMap$TransientArrayMap clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient (hash-map :x "fine")) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashMap$TransientHashMap clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient [1 2 3]) 0)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentVector$TransientVector clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient #{:x}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashSet$TransientHashSet clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (:x (transient #{:x}))
nil
;; expected: :x

user=> (get (transient #{:x}) :x)
nil
;; expected: :x
{code}


Behavior with latest Clojure master as of Jun 27 2014 (same as Clojure 1.6.0) plus patch clj-700-7.diff. In all cases it matches the expected results shown in comments above:

{code}
user=> (contains? (transient {:x "fine"}) :x)
true
user=> (contains? (transient (hash-map :x "fine")) :x)
true
user=> (contains? (transient [1 2 3]) 0)
true
user=> (contains? (transient #{:x}) :x)
true
user=> (:x (transient #{:x}))
:x
user=> (get (transient #{:x}) :x)
:x
{code}

Analysis by Alexander Redington: This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Questions on this approach from Stuart Halloway to Rich Hickey:

1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?

2. what are good names for the interfaces introduced here?

Alex Miller: Should also keep an eye on CLJ-787 as it may have some collisions with this one.


*Patch:* clj-700-7.diff
Hide
Andy Fingerhut added a comment -

Updated description to contain a copy of only those comments that seemed 'interesting'. Most comments have simply been "attached an updated patch that applies cleanly", or "changed the state of this ticket for reason X".

Show
Andy Fingerhut added a comment - Updated description to contain a copy of only those comments that seemed 'interesting'. Most comments have simply been "attached an updated patch that applies cleanly", or "changed the state of this ticket for reason X".
Andy Fingerhut made changes -
Description Behavior with Clojure 1.6.0:

{code}
user=> (contains? (transient {:x "fine"}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentArrayMap$TransientArrayMap clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient (hash-map :x "fine")) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashMap$TransientHashMap clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient [1 2 3]) 0)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentVector$TransientVector clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient #{:x}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashSet$TransientHashSet clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (:x (transient #{:x}))
nil
;; expected: :x

user=> (get (transient #{:x}) :x)
nil
;; expected: :x
{code}


Behavior with latest Clojure master as of Jun 27 2014 (same as Clojure 1.6.0) plus patch clj-700-7.diff. In all cases it matches the expected results shown in comments above:

{code}
user=> (contains? (transient {:x "fine"}) :x)
true
user=> (contains? (transient (hash-map :x "fine")) :x)
true
user=> (contains? (transient [1 2 3]) 0)
true
user=> (contains? (transient #{:x}) :x)
true
user=> (:x (transient #{:x}))
:x
user=> (get (transient #{:x}) :x)
:x
{code}

Analysis by Alexander Redington: This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Questions on this approach from Stuart Halloway to Rich Hickey:

1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?

2. what are good names for the interfaces introduced here?

Alex Miller: Should also keep an eye on CLJ-787 as it may have some collisions with this one.


*Patch:* clj-700-7.diff
Behavior with Clojure 1.6.0:

{code}
user=> (contains? (transient {:x "fine"}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentArrayMap$TransientArrayMap clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient (hash-map :x "fine")) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashMap$TransientHashMap clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient [1 2 3]) 0)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentVector$TransientVector clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient #{:x}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashSet$TransientHashSet clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (:x (transient #{:x}))
nil
;; expected: :x

user=> (get (transient #{:x}) :x)
nil
;; expected: :x
{code}


Behavior with latest Clojure master as of Jun 27 2014 (same as Clojure 1.6.0) plus patch clj-700-7.diff. In all cases it matches the expected results shown in comments above:

{code}
user=> (contains? (transient {:x "fine"}) :x)
true
user=> (contains? (transient (hash-map :x "fine")) :x)
true
user=> (contains? (transient [1 2 3]) 0)
true
user=> (contains? (transient #{:x}) :x)
true
user=> (:x (transient #{:x}))
:x
user=> (get (transient #{:x}) :x)
:x
{code}

Analysis by Alexander Redington: This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Questions on this approach from Stuart Halloway to Rich Hickey:

1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?

2. what are good names for the interfaces introduced here?

Alex Miller: Should also keep an eye on CLJ-787 as it may have some collisions with this one.


*Patch:* clj-700-7.diff

One 'trailing whitespace' warning is perfectly normal when applying this patch to latest Clojure master as of Jun 27 2014, as shown below. This is simply because of carriage returns at the end of lines in file Associative.java. I know of no way to avoid such a warning:
{code}
% git am -s --keep-cr --ignore-whitespace < ~/clj/patches/clj-700-7.diff
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/admin/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq
{code}
Andy Fingerhut made changes -
Description Behavior with Clojure 1.6.0:

{code}
user=> (contains? (transient {:x "fine"}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentArrayMap$TransientArrayMap clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient (hash-map :x "fine")) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashMap$TransientHashMap clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient [1 2 3]) 0)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentVector$TransientVector clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient #{:x}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashSet$TransientHashSet clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (:x (transient #{:x}))
nil
;; expected: :x

user=> (get (transient #{:x}) :x)
nil
;; expected: :x
{code}


Behavior with latest Clojure master as of Jun 27 2014 (same as Clojure 1.6.0) plus patch clj-700-7.diff. In all cases it matches the expected results shown in comments above:

{code}
user=> (contains? (transient {:x "fine"}) :x)
true
user=> (contains? (transient (hash-map :x "fine")) :x)
true
user=> (contains? (transient [1 2 3]) 0)
true
user=> (contains? (transient #{:x}) :x)
true
user=> (:x (transient #{:x}))
:x
user=> (get (transient #{:x}) :x)
:x
{code}

Analysis by Alexander Redington: This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Questions on this approach from Stuart Halloway to Rich Hickey:

1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?

2. what are good names for the interfaces introduced here?

Alex Miller: Should also keep an eye on CLJ-787 as it may have some collisions with this one.


*Patch:* clj-700-7.diff

One 'trailing whitespace' warning is perfectly normal when applying this patch to latest Clojure master as of Jun 27 2014, as shown below. This is simply because of carriage returns at the end of lines in file Associative.java. I know of no way to avoid such a warning:
{code}
% git am -s --keep-cr --ignore-whitespace < ~/clj/patches/clj-700-7.diff
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/admin/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq
{code}
Behavior with Clojure 1.6.0:

{code}
user=> (contains? (transient {:x "fine"}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentArrayMap$TransientArrayMap clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient (hash-map :x "fine")) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashMap$TransientHashMap clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient [1 2 3]) 0)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentVector$TransientVector clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient #{:x}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashSet$TransientHashSet clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (:x (transient #{:x}))
nil
;; expected: :x

user=> (get (transient #{:x}) :x)
nil
;; expected: :x
{code}


Behavior with latest Clojure master as of Jun 27 2014 (same as Clojure 1.6.0) plus patch clj-700-7.diff. In all cases it matches the expected results shown in comments above:

{code}
user=> (contains? (transient {:x "fine"}) :x)
true
user=> (contains? (transient (hash-map :x "fine")) :x)
true
user=> (contains? (transient [1 2 3]) 0)
true
user=> (contains? (transient #{:x}) :x)
true
user=> (:x (transient #{:x}))
:x
user=> (get (transient #{:x}) :x)
:x
{code}

Analysis by Alexander Redington: This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Questions on this approach from Stuart Halloway to Rich Hickey:

1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?

2. what are good names for the interfaces introduced here?

Alex Miller: Should also keep an eye on CLJ-787 as it may have some collisions with this one.


*Patch:* clj-700-7.diff

One 'trailing whitespace' warning is perfectly normal when applying this patch to latest Clojure master as of Jun 27 2014, as shown below. This is simply because of carriage returns at the end of lines in file Associative.java. I know of no way to avoid such a warning without removing CRs from all Clojure source files (e.g. CLJ-1026):
{code}
% git am -s --keep-cr --ignore-whitespace < ~/clj/patches/clj-700-7.diff
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/admin/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq
{code}
Hide
Alex Miller added a comment -

Looks like Andy did as requested, moving back to Screenable.

Show
Alex Miller added a comment - Looks like Andy did as requested, moving back to Screenable.
Alex Miller made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]

People

Vote (8)
Watch (5)

Dates

  • Created:
    Updated: