Re: Can the byte-compiler check whether functions passed by name are defined?

[Klaus-Dieter Bauer <bauer.klaus.dieter@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3562 bytes --]

2013/8/1 Stefan Monnier <monnier@iro.umontreal.ca>

> >>> 1. Has to be done, though maybe through a macro, for every higher-order
> >>> function.
> >> Note that macroexp.el already has special handling for the main
> >> higher-order functions (to warn about '(lambda ...)).  So it could be
> >> implemented there.
> > I presume the list of “main higher-order” functions is hard-coded
> > then, isn't it?
>
> Currently, yes.  It could be moved from macroexp.el's main loop to
> a bunch of compiler-macros (the code predates the introduction of
> compiler macros in core Elisp), if needed.
>
> > Is there a symbol property or "declare" form to mark specific
> > arguments as function arguments, to have the same warnings for higher
> > order functions from 3rd party libraries, too?  Similar to how
> > "docstring" arguments for macros are handled?
>
> No, and I think it would be overkill.  But 3rd party can also use
> a compiler-macro to turn the ' into a #'.
>
> The approach I use for '(lambda ...) is to emit warnings in the common
> cases, so that coders will slowly learn, which then benefits all cases.
>
> For '(lambda ...) that works well, because removing the ' (or using #')
> is often useful (making it possible to byte-compile the code, or using
> lexically bound vars), but for single quoted symbols, the benefit is
> a lot less clear ("turn 'foo into #'foo to get rid of the silly new
> warning" only to then get another warning because the compiler doesn't
> understand that `foo' will very much exist by the time we try to call it).
>
>
>         Stefan
>

 I have written an implementation of the compile-time check, see the
attached patch to "lisp/bytecomp.el". Since I also introduced as new
declare form for `defun', I have also attached a patch to
"doc/functions.texi".

The code handles both the cases (function SYMBOL) and (quote SYMBOL). In
the latter case it requires a symbol property "higher-order-arguments" to
be set, which should preferably be done by a declare form, but for now is
implemented by checking for a set of common functions (apply, funcall, all
functions I found starting with "map" or "cl-map") for the property and
setting it if it is not yet set. The value of the property should be a list
of integers, which give the positions of the arguments that are expected to
be functions, counting from 0.

If the patch becomes part of the emacs sources, I can also provide a patch
that adds the needed declare forms to the various definitions, though I
don't know how to do it for functions defined in the C-code such as
`mapcar'.

Currently such a function (in this case requiring lexical binding)
definition would read

(defun my-combine (func1 func2)
(declare (higher-order-arguments 0 1)
(lambda (arg)
(funcall func1 (funcall func2 arg))))

or

(defun my-map (func list)
(declare (higher-order 0))
(mapcar func list))

I was thinking of allowing to declare the number of arguments the function
will be passed, but didn't have the time for that detail (yet?).

One thing I am not sure about: I define the handler for the declare form in
"bytecomp.el", hence the handler is not known before loading that file.
When loading a file from source the declare form will therefore cause a
warning. When compiling it doesn't seem to cause issues though.

I couldn't define the handler in "byte-run.el" however, as when I added it
to the declaration of `defun-declaration-alist', it would suddenly be
missing again during compilation.

- Klaus

[-- Attachment #1.2: Type: text/html, Size: 4541 bytes --]

[-- Attachment #2: functions.texi.patch --]
[-- Type: application/octet-stream, Size: 702 bytes --]

--- functions.texi.orig	2013-07-01 10:21:08.404397000 +0200
+++ functions.texi	2013-08-04 14:05:41.516553400 +0200
@@ -1307,6 +1307,13 @@
 which case the warning message gives no extra details).  @var{when}
 should be a string indicating when the function or macro was first
 made obsolete.
+
+@item (higher-order-arguments [@var{pos} ..])
+Tell the byte compiler that the argument at position @var{pos} is
+expected to be a function. @var{pos} starts from 0. When the actual
+parameter is a quoted symbol, the byte compiler will warn about an
+unknown function, if the symbol is not known to be bound to a function
+at compile time. Zero or more @var{pos} can be specified.
 @end table
 @end defmac
 

[-- Attachment #3: bytecomp.el.patch --]
[-- Type: application/octet-stream, Size: 9504 bytes --]

--- bytecomp.el.orig	2013-07-02 22:28:34.936731600 +0200
+++ bytecomp.el	2013-08-04 20:38:38.534651900 +0200
@@ -1496,6 +1496,7 @@
          (byte-compile-const-variables nil)
          (byte-compile-free-references nil)
          (byte-compile-free-assignments nil)
+	 (byte-compile--higher-order--unresolved-functions nil)
          ;;
          ;; Close over these variables so that `byte-compiler-options'
          ;; can change them on a per-file basis.
@@ -1939,6 +1940,7 @@
 	;; Make warnings about unresolved functions
 	;; give the end of the file as their position.
 	(setq byte-compile-last-position (point-max))
+	(byte-compile--higher-order--warn-about-unresolved-function-names)
 	(byte-compile-warn-about-unresolved-functions))
       ;; Fix up the header at the front of the output
       ;; if the buffer contains multibyte characters.
@@ -2960,6 +2962,7 @@
 (defun byte-compile-normal-call (form)
   (when (and (byte-compile-warning-enabled-p 'callargs)
              (symbolp (car form)))
+    (byte-compile--higher-order--check-arguments form)
     (if (memq (car form)
               '(custom-declare-group custom-declare-variable
                                      custom-declare-face))
@@ -4659,6 +4662,170 @@
 	      (indent-to 40)
 	      (insert (int-to-string n) "\n")))
 	(setq i (1+ i))))))
+
+;;;; (DECLARE (HIGHER-ORDER-ARGUMENTS)) AND CHECK [BEGINNING] ;;;;;;;;
+
+(defvar byte-compile--higher-order--unresolved-functions nil
+  "A list of functions that were passed by name to higher order functions but where not known to be defined.
+
+Used during compilation to report functions referred to by (quote
+SYMBOL) or (function SYMBOL) that are not known to be defined.
+
+ See also
+`byte-compile--higher-order--warn-about-unresolved-function-names',
+`byte-compile--higher-order--check-arguments'.")
+
+(defun byte-compile--higher-order--warn-about-unresolved-function-names ()
+  "Emit compiler warnings when there are unresolved function a symbols passed to higher order functions.
+See also `byte-compile--higher-order--unresolved-functions'."
+  ;; The logic is similiar to `byte-compile-warn-about-unresolved-functions'.
+  (when (byte-compile-warning-enabled-p 'unresolved)
+    (let ((byte-compile-current-form :end)
+	  not-defined-at-runtime-maybe-symbol-list
+	  resolved-to-function-list
+	  resolved-to-macro-list
+	  unresolved-symbol-list)
+    (dolist (function-name byte-compile--higher-order--unresolved-functions)
+      (cond ((assq function-name byte-compile-function-environment)
+	     (push function-name resolved-to-function-list))
+	    ((assq function-name byte-compile-macro-environment)
+	     (push function-name resolved-to-macro-list))
+	    ((fboundp function-name)
+	     (push function-name not-defined-at-runtime-maybe-symbol-list))
+	    (t (push function-name unresolved-symbol-list))))
+    (dolist (resolved-name resolved-to-function-list)
+      (setq byte-compile--higher-order--unresolved-functions
+	    (delq resolved-name
+		  byte-compile--higher-order--unresolved-functions)))
+    (byte-compile-print-syms 
+     "The symbol `%S' was passed to a function as if the name of a function, but is bound to a macro instead."
+     "The following symbols were passed to a function as if the names functions but are bound to macros instead:"
+     resolved-to-macro-list)
+    (byte-compile-print-syms
+     "The symbol `%S' was used as function name but the function is not known to be defined."
+     "The following symbols were used as function names but the functions are not known to be defined:"
+     unresolved-symbol-list)
+    (byte-compile-print-syms
+     "The function `%S' was used as function name but the function might not be defined at runtime."
+     "The following symbols were used as function names but the functions might not be defined at runtime:"
+     not-defined-at-runtime-maybe-symbol-list))))
+
+(defun byte-compile--higher-order--check-arguments (form)
+  "On FORM, which is a function call, check whether functions passed by name are known to be defined.
+
+If a parameter has the form (function SYMBOL) or it is known to
+be expected to be a function through the `higher-order-arguments'
+property of the symbol (car FORM) and has the form (quote SYMBOL)
+we either warn right away, if SYMBOL is known to be bound to
+something that cannot be passed to higher order functions such as
+special forms and macros. If SYMBOL's function cell is empty, we
+queue the symbol into
+`byte-compile--higher-order--unresolved-functions' for checking
+at the end of the file, see
+`byte-compile--higher-order--warn-about-unresolved-function-names'."
+  (let ((higher-order-arguments
+	 (and (symbolp (car form))
+	      (get (car form) 'higher-order-arguments)))
+	(pos -1))
+    (dolist (subform (cdr form))
+      (setq pos (1+ pos))
+      ;; Check when subform has form (function SYMBOL) or (quote SYMBOL)
+      ;; and position matches one mentioned in higher-order-arguments.
+      (when (or (and (listp subform) ;; case: (function SYMBOL)
+		     (eq (car subform) 'function)
+		     (symbolp (car (cdr subform)))
+		     (null (cdr (cdr subform))))
+		(and (listp subform) ;; case: (quote SYMBOL)
+		     (eq (car subform) 'quote)
+		     (symbolp (car (cdr subform)))
+		     (null (cdr (cdr subform)))
+		     (member pos higher-order-arguments)))
+	(let ((function-name (car (cdr subform))))
+	  ;; Is the function already defined or already known to be
+	  ;; defined in this file? Note that macros are not allowed!
+	  (cond ((assq function-name byte-compile-function-environment)) ;; Do nothing.
+		((assq function-name byte-compile-macro-environment)
+		 (byte-compile-warn "Invalid argument `%S' to function `%S': Not a function."
+				    function-name (car form)))
+		((functionp function-name)) ;; No nothing, is ok.
+		((fboundp function-name)
+		 (byte-compile-warn "Invalid argument `%S' to function `%S': Not a function."
+				    function-name (car form)))
+		(t ;; Unknown symbol
+		 (add-to-list 'byte-compile--higher-order--unresolved-functions
+			      function-name))))))))
+
+;; Placing the entry inside the definition of
+;; `defun-declaration-alist' directly for some reason doesn't work.
+(add-to-list 'defun-declarations-alist
+	     '(higher-order-arguments 
+	       byte-compile--higher-order--declare-form-handler))
+
+(defun byte-compile--higher-order--declare-form-handler (f _args &rest val)
+  "Handler for the (declare (higher-order-arguments [NUM ...])) form, see `defun-declarations-alist'.
+
+F is the name of the function, _ARGS the formal parameter list,
+VAL should be (NUM ..).
+
+LIMITATION: When the function has been used before it was
+defined, a compiler warning is emitted pointing out that prior
+use might not have been checked."
+  ;; Check that VAL is list of integers.
+  (if (or (not (listp val))
+	  (delete nil
+	    (mapcar (lambda (e) (not (integerp e)))
+	      val)))
+      ;; Warn about invalid `declare' form
+      (let ((warn-text 
+	     (concat "(declare ..) form (higher-order-arguments [NUM ..]) with invalid arguments "
+		     (mapconcat (lambda (e) (format "%S" e)) val " "))))
+	(if byte-compile-current-file 
+	    (byte-compile-warn "%s" warn-text)
+	  (warn "%s" warn-text))
+	nil)
+    ;; Return a `put' form, but also perform it at compile time.
+    ;; Limitation: Affacts only higher order functions defined before
+    ;; their first use.
+    (when (assq f byte-compile-unresolved-functions)
+      (byte-compile-warn 
+       "%s"
+       (concat "Higher order function " (symbol-name f) " was used before it was defined. "
+	       "Actual parameters were not checked for validity.")))
+    (list 'eval-and-compile 
+	  (list 'put (list 'quote f) ''higher-order-arguments
+		(list 'quote val)))))
+
+;; Setting the property for certain known functions.
+
+(defmacro byte-compile--higher-order--register-known-functions (pos &rest symbol-list)
+  "For elements of SYMBOL-LIST set the higher-order-arguments property to (POS) when it is not yet set.
+
+Only for backward compatibility use in bytecomp.el. If you want
+to set the property for your own functions use
+the (declare (higher-order [NUM ..])) form."
+  (declare (indent 1))
+  (let (forms)
+    (dolist (function-name symbol-list)
+      (push (list 'if (list 'not (list 'get (list 'quote function-name) ''higher-order-arguments))
+		  (list 'put (list 'quote function-name) ''higher-order-arguments 
+			(list 'quote (list pos))))
+	    forms))
+    (cons 'progn (reverse forms))))
+
+(byte-compile--higher-order--register-known-functions 0
+  ;; Functions defined in C-source.
+  apply funcall map-char-table map-charset-chars map-keymap
+  map-keymap-internal mapatoms mapc mapcar mapconcat maphash
+  ;; Functions defined in standard elisp files
+  map-keymap-sorted map-query-replace-regexp map-y-or-n-p
+  ;; From the deprecated `cl'
+  map mapcan mapcar* mapcon mapl maplist
+  ;; Non-obsolete non-internal functions from `cl-lib'
+  cl-map cl-mapc cl-mapcan cl-mapcar cl-mapcon cl-maphash cl-mapl
+  cl-maplist)
+
+;;;; (DECLARE (HIGHER-ORDER-ARGUMENTS)) AND CHECK [END] ;;;;;;;;;;;;;;
+
 \f
 ;; To avoid "lisp nesting exceeds max-lisp-eval-depth" when bytecomp compiles
 ;; itself, compile some of its most used recursive functions (at load time).