why is inner loop collect not returning results?

Question

I was trying to use standard loop facilities to collect into a result but it just returns nil. Why is this? it feels like this should work:

 (defun coll-intersects (bounds mv)
   (let ((res (list))
     (loop for x from (first bounds) to (+ (first bounds) (third bounds)) do
       (loop for y from (second bounds) to (+ (second bounds) (fourth bounds))
             if (not (member (cl-byte (aref mapa x y)) mv))
             collect (aref mapa x y) into res
             ))))

but no, i have to do this:

  (defun coll-intersects (bounds mv)
    (let ((res (list)))
     (loop for x from (first bounds) to (+ (first bounds) (third bounds)) do
       (loop for y from (second bounds) to (+ (second bounds) (fourth bounds))
            do
               (if (not (member (cl-byte (aref mapa x y)) mv))
                   (push (aref mapa x y) res))
            ))
      res))

why? i was really confused why the first one was not working

Answer 1

As Ehvince's answer says, the problem is that

(loop ...
      collect ... into x
      ...)

binds x. The purpose of this construct is really so you can collect multiple lists:

(defun partition (l)
  (loop for e in l
        if (evenp e)
        collect e into evens
        else
        collect e into odds
        finally (return (values evens odds))))

for instance.

In the case where you want to collect a single list from nested loops and you care about order you can do this trick:

(defun sublist-evens (l)
  (loop for s in l
        nconcing
        (loop for e in s
              when (evenp e)
              collect e)))

Here the outer loop essentially nconcs together the results from the inner loop. This can nest of course:

(loop ...
      nconcing
      (loop ...
            nconcing
            (loop ...
                  collect ...)))

will work. It is also possible that loop is smart enough to keep a tail pointer to the list it's building with nconc / nconcing, although you'd have to check that.

However in cases where you want to build some list in-order from some deeply-nested loop (or any other search process) I find it's almost always more pleasant to use a collecting macro to do that (disclaimer: I wrote this one). With such a macro the above sublist-evens function looks like this:

(defun sublist-evens (l)
  (collecting
    (dolist (s l)
      (dolist (e s)
        (when (evenp e) (collect e))))))

and

> (sublist-evens '((1 2 3) (4 5 6)))
(2 4 6)

And you can do better:

(defun tree-partition (tree)
  (with-collectors (evens odds)
    (labels ((search (it)
               (typecase it
                 (list
                  (dolist (e it)
                    (search e)))
                 (integer
                  (if (evenp it)
                      (evens it)
                    (odds it)))
                 (t
                  (warn "unexpected ~A" (type-of it))))))
      (search tree))))

and now

> (tree-partition '(((1 2 3) (4)) 5))
(2 4)
(1 3 5)

(And for hack value you can use another macro to express the above more concisely:

(defun tree-partition (tree)
  (with-collectors (evens odds)
    (iterate search ((it tree))
      (typecase it
        (list
         (dolist (e it)
           (search e)))
        (integer
         (if (evenp it)
             (evens it)
           (odds it)))
        (t
         (warn "unexpected ~A" (type-of it)))))))

Disclaimer: I wrote that macro too.)

Answer 2

Here's the first snippet, with the let parenthesis corrected, simplified to be made reproducible:

(defun coll-intersects (bounds mv)
   (let ((res (list))) ;; <-- third closing paren 
     (loop for x from (first bounds) to (+ (first bounds) (third bounds)) do
       (loop for y from (second bounds) to (+ (second bounds) (fourth bounds))
             if (evenp y)
             collect y into res
             ))))

Now when I enter it into the REPL, SBCL warns me about an unused res:

; caught STYLE-WARNING:
;   The variable RES is defined but never used.

That's a big hint.

The issues I see:

• you use do for the outer loop, not collect, and you don't return res, so the functions always returns nil.
• collect … into presumably uses an internal variables, not your res :S In addition the loop doesn't say what to do with it. I added finally (return res) and I get results. You can also use push like in the second example. But it doesn't seem necessary to use into, just use collect y.
• it is usually not necessary to declare intermediate variables with an outer let.

Here's a simpler function that returns (dumb) results:

(defun coll-intersects (bounds)
     (loop for x from (first bounds) to (+ (first bounds) (third bounds)) collect
       (loop for y from (second bounds) to (+ (second bounds) (fourth bounds))
             if (evenp y)
             collect y)))

(coll-intersects '(1 2 3 4))
((2 4 6) (2 4 6) (2 4 6) (2 4 6))

If you use nconcing instead of the first collect, you'll get a flat list (as pointed out by @tfb).

or:

(defun coll-intersects (bounds)
   (let ((res (list)))
     (loop for x from (first bounds) to (+ (first bounds) (third bounds)) do
       (loop for y from (second bounds) to (+ (second bounds) (fourth bounds))
             if (evenp y)
             do (push y res)
             ))
     res))

(coll-intersects '(1 2 3 4))
(6 4 2 6 4 2 6 4 2 6 4 2)