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

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)

Answer 2

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 3

In your first example, the return value from the function is the return value from the outer loop. It doesn't collect any values (the inner loop does) and thus most probably just returns a nil.

In your second example, your function explicitly returns the value of res.