Why is lisp saying this parameter is not a list?

Question

I am working through the MP3 database example in Peter Seibel's Practical Common Lisp. Seibel demonstrates how macros can be used to shorten the code for the where function; so now, I am trying to use a macro to shorten the code for the update function. (The original version of the update function is included for reference.) When I run my code, the following error originates from the second-to-last line --

*** - CAR: TERMS is not a list

What am I doing wrong? Here is my code.

(defvar *db* nil)
(defun add-record (cd) 
  (push cd *db*))

(defun dump-db ()
  (dolist (cd *db*)
    (format t "~{~a:~10t~a~%~}~%" cd)))

(defun make-cd (title artist rating ripped)
  (list :title title :artist artist :rating rating :ripped ripped))

(defun prompt-read (prompt)
  (format *query-io* "~a: " prompt)
  (force-output *query-io*)
  (read-line *query-io*))

(defun prompt-for-cd ()
  (make-cd
    (prompt-read "Title")
    (prompt-read "Artist")
    (or (parse-integer (prompt-read "Rating") :junk-allowed t) 0)
    (y-or-n-p "Ripped [y/n]: ")))

(defun add-cds ()
  (loop (add-record (prompt-for-cd) )
        (if (not (y-or-n-p "Another? [y/n]: ")) (return) )))

(defun save-db (filename)
  (with-open-file (out filename
                       :direction :output
                       :if-exists :supersede)
                  (with-standard-io-syntax
                    (print *db* out))))

(defun load-db (filename)
  (with-open-file (in filename)
                  (with-standard-io-syntax
                    (setf *db* (read in) ))))

(defun select (selector-fn)
  (remove-if-not selector-fn *db*))

(defun make-comparison-expr (field value)
  `(equal (getf cd ,field) ,value))

(defun make-comparison-list (func fields)
  (loop while fields
        collecting (funcall func (pop fields) (pop fields))))

(defmacro where (&rest clauses)
  `#'(lambda (cd) (and ,@(make-comparison-list 'make-comparison-expr clauses))))

(defun make-update-expr (field value)
  `(setf (getf row ,field) ,value))

(defmacro make-update-list (fields)
  (make-comparison-list 'make-update-expr fields))

(defun update (selector-fn &rest terms)
  (print (type-of terms))
  (setf *db*
        (mapcar
          #'(lambda (row)
             (when (funcall selector-fn row)
               (make-update-list terms))
             row)
          *db*)))

;(defun update (selector-fn &key title artist rating (ripped nil ripped-p))
;  (setf *db*
;        (mapcar
;          #'(lambda (row)
;             (when (funcall selector-fn row)
;               (if title    (setf (getf row :title) title) )
;               (if artist   (setf (getf row :artist) artist) )
;               (if rating   (setf (getf row :rating) rating) )
;               (if ripped-p (setf (getf row :ripped) ripped) ))
;             row)
;          *db*)))

(defun delete-rows (selector-fn)
  (setf *db* (remove-if selector-fn *db*)))

;(loop (print (eval (read))))
(add-record (make-cd "Be" "Common" 9 nil))
(add-record (make-cd "Like Water for Chocolate" "Common" 9 nil))
(add-record (make-cd "Be" "Beatles" 9 nil))

(dump-db)
(update (where :artist "Common" :title "Be") :rating 8)
(dump-db)

-----Edit-----

I figured it out. The solution was to make update a macro and to make make-update-list a function. This way, make-update-list could evaluate fields at run-time and update can still abstract away some tedious if statements. Here is the updated update and make-update-list below:

(defun make-update-list (fields)
  (make-comparison-list 'make-update-expr fields))

(defmacro update (selector-fn &rest terms)
  `(setf *db*
        (mapcar
          #'(lambda (row)
             (when (funcall ,selector-fn row)
               ,@(make-update-list terms))
             row)
          *db*)))

Answer 1

Macroexpansion of that make-update-list is done in a separate phase (called "macroexpansion phase") - which occurs around the time a piece of code is compiled or loaded; in this case we're talking about compilation / loading of update. The macro gets expanded with fields bound to the symbol terms, which (the symbol itself) is used as a value in make-comparison-list; I suppose that was not what you expected.

Note, if you go and compile the file line-by-line (C-c C-c in Emacs + SLIME), it'll tell you right during compilation of update that the macro expansion fails because "the value TERMS is not of type LIST".

Generally, think of macros as functions that take in their arguments unevaluated - i.e. a form (make-update-list foo) will get expanded with the macro parameter's fields value bound to foo. What you're trying to achieve here - code generation based on run-time values - is a bit more difficult to do.

Answer 2

You are trying to take the car of a symbol!

> (car 'terms)
*** - CAR: TERMS is not a list

Think of macros as a function that, when used, replaces the code with the result of the macro function everywhere it's used. At this time variables are just symbols and have no meaning besides that.

When you do (make-update-list terms) it will call the macro function with the argument fields being the symbol you passed, which is terms. Since it's a symbol it cannot be iterated like you are trying. You may iterate it at runtime when it surely is a list, but as a macro it isn't a list until you are passing it a list like (make-update-list (title artist rating ripped)).

If it is dynamic in runtime then your macro needs to expand to code that does most of its magic at runtime. Thus a macro is just a source rewriting service and should not have anything to do with what variable might be at runtime since then it has already done its thing.