Welcome! Please see the About page for a little more info on how this works.

0 votes
in Clojure by

If you pass a non-positive value of 'n' or 'step' to partition, you get an infinite loop. Here are a few examples:

(partition 0 (link: 1 2 3))
(partition 1 -1 (link: 1 2 3))

Cause: partition and partition-all do not check that n and step are positive.

Approach: Add checks to partition and partition-all that n and step are positive.

Patch: clj-1647_3.patch

Prescreened by: Alex Miller

29 Answers

0 votes
by

Comment made by: alexmiller

Also see CLJ-764

0 votes
by

Comment made by: alexmiller

Needs a perf check when done.

0 votes
by

Comment made by: kworam

patch file to fix clj-1647

0 votes
by

Comment made by: kworam

Since 'n' and 'step' remain unchanged from the initial function call through all of the recursive self-calls, I only need to verify that they are positive once, on the initial call.

I therefore created functions 'internal-partition' and 'internal-partition-all' whose implementations are identical to the current versions of 'partition' and 'partition-all'.

I then added preconditions that 'step' and 'n' must be positive to the 'partition' and 'partition-all' functions, and made them call 'internal-partition' and 'internal-partition-all' respectively to do the work.

0 votes
by

Comment made by: alexmiller

There are a lot of unrelated whitespace changes in this patch - can you supply a smaller patch with only the change at issue? Also needs tests.

0 votes
by

Comment made by: kworam

I will supply a patch file without the whitespace changes.

I know there are some existing functionality tests for 'partition' and 'partition-all' in test_clojure\sequences.clj and test_clojure\transducers.clj. I don't think I need to add more functionality tests, but I think I should add:

  1. Tests that verify that non-positive 'step' and 'n' parameters are rejected.
  2. Tests that show that 'partition' and 'partition-all' performance has not degraded significantly.

Could you give me some guidance on how to develop and add these tests?

0 votes
by

Comment made by: alexmiller

You should add #1 to the patch. For #2, you can just do the timings before/after (criterium is a good tool for this) and put the results in the description.

0 votes
by
_Comment made by: kworam_

I have coded up the tests for #1 and taken some 'before' timings for #2 using criterium.  

I have been stumped by a problem for hours now and I need to get some help. I made my changes to 'partition' and 'partition-all' in core.clj and then did 'mvn package' to build the jar.  I executed 'target>java -cp clojure-1.7.0-master-SNAPSHOT.jar clojure.main' to test out my patched version of clojure interactively.  The (source ...) function shows that my source changes for both 'partition' and 'partition-all' are in place.  My change to 'partition-all' seems to be working but my change to 'partition' is not. As far as I can tell, they should both throw an AssertionError with the input parameters I am providing.  

Any help would be greatly appreciated.


user=> (source partition)
(defn partition
  "Returns a lazy sequence of lists of n items each, at offsets step
  apart. If step is not supplied, defaults to n, i.e. the partitions
  do not overlap. If a pad collection is supplied, use its elements as
  necessary to complete last partition upto n items. In case there are
  not enough padding elements, return a partition with less than n items."
  {:added "1.0"
   :static true}
  ([n coll]
     {:pre [(pos? n)]}
     (partition n n coll))
  ([n step coll]
     {:pre [(pos? n) (pos? step)]}
     (internal-partition n step coll))
  ([n step pad coll]
     {:pre [(pos? n) (pos? step)]}
     (internal-partition n step pad coll)))
nil
user=> (partition -1 [1 2 3])
()
user=> (source partition-all)
(defn partition-all
  "Returns a lazy sequence of lists like partition, but may include
  partitions with fewer than n items at the end.  Returns a stateful
  transducer when no collection is provided."
  {:added "1.2"
   :static true}
  ([^long n]
     (internal-partition-all n))
  ([n coll]
     (partition-all n n coll))
  ([n step coll]
     {:pre [(pos? n) (pos? step)]}
     (internal-partition-all n step coll)))
nil
user=> (partition-all -1 [1 2 3])
AssertionError Assert failed: (pos? n)  clojure.core/partition-all (core.clj:6993)


0 votes
by

Comment made by: alexmiller

Did you mvn clean? Or rm target?

0 votes
by

Comment made by: kworam

Yes, I did mvn clean and verified that clojure-1.7.0-master-SNAPSHOT.jar had the expected date-time stamp before doing the interactive test. I even went so far as to retrace my steps on my Macbook on the theory that maybe there was a Windows-specific build problem.

My change to partition-all works as expected but my change to partition does not. However, if I copy the result of the call to (source partition) and execute it (replacing clojure.core/partition with user/partition), user/partition works as expected. I don't understand why my change to clojure.core/partition isn't taking effect.

0 votes
by

Comment made by: jafingerhut

Kevin, I do not know the history of your Clojure source tree, but if you ever ran 'ant' in it, that creates jar files in the root directory, whereas 'mvn package' creates them in the target directory. It wasn't clear from your longer comment above whether the 'java -cp ...' command you ran pointed at the one in the target directory. That may not be the cause of the issue you are seeing, but I don't yet have any guesses what else it could be.

0 votes
by

Comment made by: alexmiller

What's the status of this?

0 votes
by

Comment made by: kworam

Alex, I moved to Seattle and took a permanent position with Microsoft recently. This has kept me very busy and I haven't been able to spend time on Clojure at all. I probably won't be able to devote time to Clojure for another month or two.

0 votes
by

Comment made by: alexmiller

Thanks for the heads up.

0 votes
by

Comment made by: alex+import

Kevin, Alex, I could pick this up if you like?

...