Clojure

PersistentList/creator doesn't handle ArraySeqs correctly

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

This should return '(2 3) but returns '(1 2 3) instead:

user=> ((fn [& args] (apply (fn [a & b] (println a b) (apply list b)) args)) 1 2 3)
1 (2 3)
(1 2 3)

Note that using vector rather than list returns the correct values:

user=> ((fn [& args] (apply (fn [a & b] (println a b) (apply vector b)) args)) 1 2 3)
1 (2 3)
[2 3]

The bug was reported in this stackoverflow question: https://stackoverflow.com/questions/27819418/strange-behaviour-of-clojure-trampoline and the bug identified in this comment: https://stackoverflow.com/questions/27819418/strange-behaviour-of-clojure-trampoline#comments-27821793

A simpler example of this bug:

user=> (apply list (next (clojure.lang.ArraySeq/create (object-array [1 2 3]))))
(1 2 3)

Patch: CLJ-1633-v3.patch

  1. 0001-CLJ-1633-fix-PersistentList-creator-handling-of-Arra.patch
    07/Jan/15 11:29 AM
    1 kB
    Nicola Mometto
  2. CLJ-1633-v2.patch
    07/Jan/15 4:59 PM
    2 kB
    Michael Blume
  3. CLJ-1633-v3.patch
    07/Jan/15 5:03 PM
    1 kB
    Michael Blume
  4. generative-seq-tests.patch
    07/Jan/15 5:42 PM
    3 kB
    Michael Blume
  5. generative-seq-tests-v2.patch
    08/Jan/15 1:55 PM
    4 kB
    Michael Blume

Activity

Hide
Michael Blume added a comment -

Very nice catch.

This makes me wonder if there's more we can do with generative testing to catch this class of bugs, maybe along the lines of zach tellman's collection-check

Show
Michael Blume added a comment - Very nice catch. This makes me wonder if there's more we can do with generative testing to catch this class of bugs, maybe along the lines of zach tellman's collection-check
Hide
Alex Miller added a comment -

There's definitely more we can do. collection-check is great and I've started to integrate some of those ideas into Clojure's tests (see for example the new transducer tests that generate random chains of sequence operations and compare seq and transducer executions). If you have ideas about specific test areas, would be happy to see a jira/patch.

Show
Alex Miller added a comment - There's definitely more we can do. collection-check is great and I've started to integrate some of those ideas into Clojure's tests (see for example the new transducer tests that generate random chains of sequence operations and compare seq and transducer executions). If you have ideas about specific test areas, would be happy to see a jira/patch.
Hide
Michael Blume added a comment -

Updated test to get expected list inside the (= ...) form

Show
Michael Blume added a comment - Updated test to get expected list inside the (= ...) form
Hide
Michael Blume added a comment -

Oops, rerolling again to apply cleanly to master

Show
Michael Blume added a comment - Oops, rerolling again to apply cleanly to master
Hide
Nicola Mometto added a comment -

Thanks for the fix!

Show
Nicola Mometto added a comment - Thanks for the fix!
Hide
Michael Blume added a comment -

No problem =)

Show
Michael Blume added a comment - No problem =)
Hide
Michael Blume added a comment -

Ok, this is kind of crude, and mostly a proof of concept, but this does catch the bug.

Output:

[java] FAIL in (seq-gentest) (sequences.clj:105)
     [java] {:acts1 (-> [] next (->> (cons :foo)) (->> (cons :foo)) next),
     [java]  :acts2
     [java]  (->
     [java]   []
     [java]   next
     [java]   (->> (cons :foo))
     [java]   (->> (cons :foo))
     [java]   into-array-seq
     [java]   next
     [java]   (->> (apply list))),
     [java]  :result1 (:foo),
     [java]  :result2 (:foo :foo),
     [java]  :pass false}
     [java]
     [java] expected: (:result res)
     [java]   actual: false
Show
Michael Blume added a comment - Ok, this is kind of crude, and mostly a proof of concept, but this does catch the bug. Output:
[java] FAIL in (seq-gentest) (sequences.clj:105)
     [java] {:acts1 (-> [] next (->> (cons :foo)) (->> (cons :foo)) next),
     [java]  :acts2
     [java]  (->
     [java]   []
     [java]   next
     [java]   (->> (cons :foo))
     [java]   (->> (cons :foo))
     [java]   into-array-seq
     [java]   next
     [java]   (->> (apply list))),
     [java]  :result1 (:foo),
     [java]  :result2 (:foo :foo),
     [java]  :pass false}
     [java]
     [java] expected: (:result res)
     [java]   actual: false
Hide
Alex Miller added a comment -

Rich said to move this forward.

Show
Alex Miller added a comment - Rich said to move this forward.
Hide
Nicola Mometto added a comment -

This ticket has been closed but no patch has been committed

Show
Nicola Mometto added a comment - This ticket has been closed but no patch has been committed
Hide
Alex Miller added a comment -

Stu, doesn't look like this patch was applied but it was closed?

Show
Alex Miller added a comment - Stu, doesn't look like this patch was applied but it was closed?

People

Vote (1)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: