Skip to:

  1. Skip to Jira Navigation
  2. Skip to Side Navigation
  3. Skip to Main Content

Iterate is too eager

Description

1.7's iterate calls its function one extra time than in 1.6

;; clojure 1.6 user=> (take 2 (iterate zero? 0)) (0 true) ;; clojure 1.7-alpha6 user=> (take 2 (iterate zero? 0)) ClassCastException java.lang.Boolean cannot be cast to java.lang.Number clojure.lang.Numbers.isZero (Numbers.java:92)

This is because Iterate.java calls its function in its "next" method rather than in its "first" method.

Approach: Iterate now holds a prevSeed that will allow computation of a lazily computed seed (the first of the iterate sequence). The very first node will be initialized with the initial seed. All other uses of seed need to ensure first is called to force realization before using seed.

Patch: clj-1692-3.patch

  • note there were two copies of test-iterate so one is removed in the test, allowing the more expansive one to be called.

Environment

None

Attachments

3
  • 02 Apr 2015, 06:01 PM
  • 02 Apr 2015, 03:31 PM
  • 02 Apr 2015, 05:45 AM

Activity

Show:

Ambrose Bonnaire-Sergeant April 3, 2015 at 7:48 PM

This issue was not caught by Typed Clojure, rather the peculiarities of its implementation slightly smiling face https://github.com/clojure/core.typed/commit/1027f792accdc3e5da3475745da0106f0ad5e1cc#diff-eec0f851200aca22af17269fcab94719L1759

I was using iterate to dig down a structure, and was being very careful to only `take` exactly enough.

Fogus April 3, 2015 at 7:36 PM

I think patch-3 is sound, and I suspect that only Ambrose would have caught it. Thanks!

Alex Miller April 2, 2015 at 6:01 PM

Ok, fair points. I renamed some fields in the first patch to (in my opinion) better reflect their role. There were also several bugs related to using seed without guaranteeing it's computation, for example both of these threw exceptions:

(first (next (next (iterate inc 0))))
(into [] (take 3) (next (iterate inc 0)))

I added those tests as well. I re-ran the perf test from CLJ-1603 and there is definitely some degradation on (doall (take 1000 (iterate inc 0))) from 75 us to 85 us, but that's still a significant win over the prior version.

Ambrose Bonnaire-Sergeant April 2, 2015 at 4:57 PM

To be clear, removing the redundant deftest actually reveals several tests that were previously hidden.

Nicola Mometto April 2, 2015 at 3:55 PM

Alex, unless I'm reading it wrong, your patch addresses the symptom of this bug rather than its cause (iterate is now eager than it was before), I personally prefer Ambrose's approach.

WRT removing the iterate tests, it looks like test-iterate is duplicated at line 781 and at line 970 of sequences.clj, Ambrose's patch just removes the duplicate test.

Completed

Details

Assignee

Reporter

Labels

Approval

Ok

Patch

Code and Test

Priority

Affects versions

Fix versions

Created April 2, 2015 at 2:14 AM
Updated April 10, 2015 at 4:58 PM
Resolved April 10, 2015 at 4:58 PM