Clojure

contains? broken for TransientMaps

Details

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

Description

(contains? (transient {:x "fine"}) :x)

false

also

(contains? (transient (hash-map :x "fine")) :x)

false

Attachments

  1. Java Source File
    0001-Refactor-of-some-of-the-clojure-.java-code-to-fix-CL.patch
    07/Jan/11 2:07 PM
    10 kB
    Alexander Redington
  2. File
    clj-700.diff
    25/Mar/11 4:18 PM
    10 kB
    Alexander Redington
  3. Text File
    clj-700-patch4.txt
    08/Jun/12 12:44 PM
    11 kB
    Stuart Halloway
  4. Text File
    clj-700-patch6.txt
    19/Aug/12 4:47 AM
    11 kB
    Andy Fingerhut

Activity

Descending order - Click to sort in ascending order
Hide
Permalink
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
Hide
Permalink
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
Permalink
Stuart Sierra added a comment -

Patch fails as of commit 1c8eb16a14ce5daefef1df68d2f6b1f143003140

Show
Stuart Sierra added a comment - Patch fails as of commit 1c8eb16a14ce5daefef1df68d2f6b1f143003140
Hide
Permalink
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.
Hide
Permalink
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.
Hide
Permalink
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.
Hide
Permalink
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).
Hide
Permalink
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?
Hide
Permalink
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
Hide
Permalink
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.
Hide
Permalink
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
Hide
Permalink
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.
Hide
Permalink
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?
Hide
Permalink
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.
Hide
Permalink
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
Hide
Permalink
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

People

Vote (3)
Watch (4)

Dates

  • Created:
    Updated:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.