Uploaded image for project: 'Clojure'
  1. CLJ-700

contains? broken for transient collections

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Completed
    • Affects versions: Release 1.2
    • Fix versions: Release 1.9
    • Labels:
    • Approval:
      Ok
    • Patch:
      Code and Test

      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
      

      Cause: 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.

      Approach: Expand the types that RT.getFrom(), RT.contains(), and RT.find() can handle to cover the additional transient interfaces.

      Alternative: Other older patches (prob best exemplified by clj-700-8.diff) restructure the collections type hierarchy. That is a much bigger change than the one taken here but is perhaps a better long-term path. That 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.

      Screening Notes re -10 patch

      • the extra conditions in RT add branches to some key functions. get already has a getFrom optimization, but there is no similar containsFrom or findFrom. Is it worth measuring the possible impact of these?
      • I believe the interface refactoring approach (not taken here) is worth separate consideration as an enhancement. If this is done, I think leveraging valAt would be simpler, e.g. allowing HashMap and ArrayMap to share code
      • it is not evident (to me anyway) why some API fns consume ILookup and others do not, among e.g. contains?, get, and find. Possible doc enhancement?
      • there is test code already in place (data_structures.clj) that could easily be expanded to cover transients. It would be nice to do this, or better yet get some test.check tests in place

      Patch: clj-700-14.patch is based on clj-700-10, plus implements Rich's suggestion that entryAt and containsKey should live in ITransientAssociative2 and the existing concrete impls (ATransientMap, TransientVector) now implement it.

        Attachments

        1. 0001-Refactor-of-some-of-the-clojure-.java-code-to-fix-CL.patch
          10 kB
        2. clj-700.diff
          10 kB
        3. clj-700-10.patch
          4 kB
        4. clj-700-11.patch
          12 kB
        5. clj-700-12.patch
          7 kB
        6. clj-700-13.patch
          7 kB
        7. clj-700-14.patch
          7 kB
        8. clj-700-7.diff
          11 kB
        9. clj-700-8.diff
          11 kB
        10. clj-700-9.patch
          4 kB
        11. clj-700-patch4.txt
          11 kB
        12. clj-700-patch6.txt
          11 kB
        13. clj-700-rt.patch
          4 kB

          Activity

            People

            • Assignee:
              richhickey Rich Hickey
              Reporter:
              bendlas Herwig Hochleitner
            • Votes:
              17 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: