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

Nicola Mometto made changes -
Field Original Value New Value
Attachment 0001-CLJ-1633-fix-PersistentList-creator-handling-of-Arra.patch [ 13728 ]
Nicola Mometto made changes -
Attachment 0001-CLJ-1633-fix-PersistentList-creator-handling-of-Arra.patch [ 13728 ]
Nicola Mometto made changes -
Nicola Mometto made changes -
Summary PersistentList/creator doesn't handle ArrayLists correctly PersistentList/creator doesn't handle ArraySeqs correctly
Alex Miller made changes -
Approval Triaged [ 10120 ]
Nicola Mometto made changes -
Description This should return '(2 3) but returns '(1 2 3) instead:
{code}
user=> ((fn [& args] (apply (fn [a & b] (println a b) (apply list b)) args)) 1 2 3)
1 (2 3)
(1 2 3)
{code}

Note that using vector rather than list returns the correct values:
{code}
user=> ((fn [& args] (apply (fn [a & b] (println a b) (apply vector b)) args)) 1 2 3)
1 (2 3)
[2 3]
{code}

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


This should return '(2 3) but returns '(1 2 3) instead:
{code}
user=> ((fn [& args] (apply (fn [a & b] (println a b) (apply list b)) args)) 1 2 3)
1 (2 3)
(1 2 3)
{code}

Note that using vector rather than list returns the correct values:
{code}
user=> ((fn [& args] (apply (fn [a & b] (println a b) (apply vector b)) args)) 1 2 3)
1 (2 3)
[2 3]
{code}

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:

{code}
user=> (apply list (next (clojure.lang.ArraySeq/create (object-array [1 2 3]))))
(1 2 3)
{code}
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.
Alex Miller made changes -
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
Michael Blume made changes -
Attachment CLJ-1633-v2.patch [ 13731 ]
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
Michael Blume made changes -
Attachment CLJ-1633-v3.patch [ 13732 ]
Hide
Nicola Mometto added a comment -

Thanks for the fix!

Show
Nicola Mometto added a comment - Thanks for the fix!
Nicola Mometto made changes -
Description This should return '(2 3) but returns '(1 2 3) instead:
{code}
user=> ((fn [& args] (apply (fn [a & b] (println a b) (apply list b)) args)) 1 2 3)
1 (2 3)
(1 2 3)
{code}

Note that using vector rather than list returns the correct values:
{code}
user=> ((fn [& args] (apply (fn [a & b] (println a b) (apply vector b)) args)) 1 2 3)
1 (2 3)
[2 3]
{code}

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:

{code}
user=> (apply list (next (clojure.lang.ArraySeq/create (object-array [1 2 3]))))
(1 2 3)
{code}
This should return '(2 3) but returns '(1 2 3) instead:
{code}
user=> ((fn [& args] (apply (fn [a & b] (println a b) (apply list b)) args)) 1 2 3)
1 (2 3)
(1 2 3)
{code}

Note that using vector rather than list returns the correct values:
{code}
user=> ((fn [& args] (apply (fn [a & b] (println a b) (apply vector b)) args)) 1 2 3)
1 (2 3)
[2 3]
{code}

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:

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

*Patch*: CLJ-1633-v3.patch
Nicola Mometto made changes -
Labels collections
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
Michael Blume made changes -
Attachment generative-seq-tests.patch [ 13734 ]
Michael Blume made changes -
Attachment generative-seq-tests-v2.patch [ 13736 ]
Hide
Alex Miller added a comment -

Rich said to move this forward.

Show
Alex Miller added a comment - Rich said to move this forward.
Alex Miller made changes -
Fix Version/s Release 1.7 [ 10250 ]
Approval Triaged [ 10120 ] Ok [ 10007 ]
Stuart Halloway made changes -
Status Open [ 1 ] Closed [ 6 ]
Resolution Completed [ 1 ]
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?
Alex Miller made changes -
Resolution Completed [ 1 ]
Status Closed [ 6 ] Reopened [ 4 ]
Stuart Halloway made changes -
Status Reopened [ 4 ] Closed [ 6 ]
Resolution Completed [ 1 ]

People

Vote (1)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: