<< Back to previous view

[CLJ-1633] PersistentList/creator doesn't handle ArraySeqs correctly Created: 07/Jan/15  Updated: 10/Jan/15  Resolved: 10/Jan/15

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: collections

Attachments: Text File 0001-CLJ-1633-fix-PersistentList-creator-handling-of-Arra.patch     Text File CLJ-1633-v2.patch     Text File CLJ-1633-v3.patch     Text File generative-seq-tests.patch     Text File generative-seq-tests-v2.patch    
Patch: Code and Test
Approval: Ok


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

Comment by Michael Blume [ 07/Jan/15 12:28 PM ]

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

Comment by Alex Miller [ 07/Jan/15 2:27 PM ]

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.

Comment by Michael Blume [ 07/Jan/15 4:59 PM ]

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

Comment by Michael Blume [ 07/Jan/15 5:03 PM ]

Oops, rerolling again to apply cleanly to master

Comment by Nicola Mometto [ 07/Jan/15 5:04 PM ]

Thanks for the fix!

Comment by Michael Blume [ 07/Jan/15 5:40 PM ]

No problem =)

Comment by Michael Blume [ 07/Jan/15 5:42 PM ]

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


[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] expected: (:result res)
     [java]   actual: false
Comment by Alex Miller [ 09/Jan/15 9:12 AM ]

Rich said to move this forward.

Comment by Nicola Mometto [ 09/Jan/15 5:51 PM ]

This ticket has been closed but no patch has been committed

Comment by Alex Miller [ 09/Jan/15 6:22 PM ]

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

Generated at Tue May 21 07:46:09 CDT 2019 using JIRA 4.4#649-r158309.