Re: master 11860f8: * test/src/comp-tests.el: Eliminate byte-compiler warnings (Bug#52105).

[Stefan Monnier]

> False positives.  comp-tests.el compiles some test functions, then
> examines the result, referring to the test functions by name.  The
> test functions are in resource files, so the compiler thinks they are
> undefined (and emits warnings) unless the functions are loaded when
> comp-tests.el is compiled.

And I guess there are too many such functions so using
`declare-function` is unwieldy.  I see.

>>   The cleanup could start by moving the (require 'comp-cstr) outside of
>>   the `eval-and-compile` since toplevel `require`s are processed both at
>>   compile and run time anyway.
>
> I didn't know that.  So the "(eval-when-compile (require 'cl-lib))" is
> wrong, too?  Here and in hundreds of other Emacs Lisp files?

The purpose of (eval-when-compile (require 'cl-lib)) is to avoid loading
cl-lib during run-time (i.e. loading it only at compile-time).
So I think it's wrong, indeed, since we use `cl-every` at run-time.

> The test functions here could be loaded with "require" instead of
> "load", and then they could be moved outside of the "eval-and-compile"
> form.  Would that be more elegant?

It's not that big of a difference, but if we move it after the

    (when (native-comp-available-p)
      (message "Compiling tests...")
      (load (native-compile comp-test-src))
      (load (native-compile comp-test-dyn-src)))

then it will save us from loading the files twice.

> We could go even further and eliminate the "defconst" forms, calling
> "ert-resource-file" twice for each file (once here for the "require",
> and once below for the "native-compile").  That rewrite would
> eliminate the ugly "eval-and-compile" entirely.

I think it's better to keep the `eval-and-compile` and avoid
the redundancy.

So, all in all, I pushed the patch below, thank you.


        Stefan


commit 8d67a70e97a7002682f641c05b10e1a9d4586e8b
Author: Stefan Monnier <monnier@iro.umontreal.ca>
Date:   Sat Nov 27 10:10:26 2021 -0500

    * test/src/comp-tests.el: Rework last patch
    
    Move `require`s out of `eval-when-compile` if the functions are called
    at run-time.
    Don't use #' to quote symbols (i.e. at those places where a lambda
    expression couldn't be used).
    Don't pre-load comp-test-45603.el and comp-test-pure.el any more.
    
    (comp-deftest pure): Use `declare-function` after loading 
`comp-test-pure.el`
    to silence the byte-compiler.

diff --git a/test/src/comp-tests.el b/test/src/comp-tests.el
index f66a193205..5b20cf38ec 100644
--- a/test/src/comp-tests.el
+++ b/test/src/comp-tests.el
@@ -27,27 +27,24 @@
 
 (require 'ert)
 (require 'ert-x)
-(eval-when-compile
-  (require 'cl-lib)
-  (require 'comp))
+(require 'cl-lib)
+(require 'comp)
+(require 'comp-cstr)
+
 (eval-and-compile
-  (require 'comp-cstr)          ;in eval-and-compile for its defstruct
   (defconst comp-test-src (ert-resource-file "comp-test-funcs.el"))
-  (defconst comp-test-dyn-src (ert-resource-file "comp-test-funcs-dyn.el"))
-  (defconst comp-test-pure-src (ert-resource-file "comp-test-pure.el"))
-  (defconst comp-test-45603-src (ert-resource-file "comp-test-45603.el"))
-  ;; Load the test code here so the compiler can check the function
-  ;; names used in this file.
-  (load comp-test-src nil t)
-  (load comp-test-dyn-src nil t)
-  (load comp-test-pure-src nil t)
-  (load comp-test-45603-src nil t))
+  (defconst comp-test-dyn-src (ert-resource-file "comp-test-funcs-dyn.el")))
 
 (when (native-comp-available-p)
   (message "Compiling tests...")
   (load (native-compile comp-test-src))
   (load (native-compile comp-test-dyn-src)))
 
+;; Load the test code here so the compiler can check the function
+;; names used in this file.
+(require 'comp-test-funcs comp-test-src)
+(require 'comp-test-dyn-funcs comp-test-dyn-src) ;Non-standard feature name!
+
 (defmacro comp-deftest (name args &rest docstring-and-body)
   "Define a test for the native compiler tagging it as :nativecomp."
   (declare (indent defun)
@@ -75,7 +72,7 @@ comp-deftest
         (copy-file comp-src comp2-src t)
         (let ((load-no-native t))
           (load (concat comp-src "c") nil nil t t))
-        (should-not (subr-native-elisp-p (symbol-function #'native-compile)))
+        (should-not (subr-native-elisp-p (symbol-function 'native-compile)))
         (message "Compiling stage1...")
         (let* ((t0 (current-time))
                (comp1-eln (native-compile comp1-src)))
@@ -372,7 +369,7 @@ comp-deftest
         t)
   (native-compile #'comp-tests-free-fun-f)
 
-  (should (subr-native-elisp-p (symbol-function #'comp-tests-free-fun-f)))
+  (should (subr-native-elisp-p (symbol-function 'comp-tests-free-fun-f)))
   (should (= (comp-tests-free-fun-f) 3))
   (should (string= (documentation #'comp-tests-free-fun-f)
                    "Some doc."))
@@ -386,7 +383,7 @@ comp-deftest
   "Check we are able to compile a single function."
   (eval '(defun comp-tests/free\fun-f ()) t)
   (native-compile #'comp-tests/free\fun-f)
-  (should (subr-native-elisp-p (symbol-function #'comp-tests/free\fun-f))))
+  (should (subr-native-elisp-p (symbol-function 'comp-tests/free\fun-f))))
 
 (comp-deftest bug-40187 ()
   "Check function name shadowing.
@@ -397,7 +394,7 @@ comp-deftest
 (comp-deftest speed--1 ()
   "Check that at speed -1 we do not native compile."
   (should (= (comp-test-speed--1-f) 3))
-  (should-not (subr-native-elisp-p (symbol-function #'comp-test-speed--1-f))))
+  (should-not (subr-native-elisp-p (symbol-function 'comp-test-speed--1-f))))
 
 (comp-deftest bug-42360 ()
   "<https://lists.gnu.org/archive/html/bug-gnu-emacs/2020-07/msg00418.html>."
@@ -446,7 +443,7 @@ comp-test-primitive-redefine-args
 (comp-deftest primitive-redefine ()
   "Test effectiveness of primitive redefinition."
   (cl-letf ((comp-test-primitive-redefine-args nil)
-            ((symbol-function #'-)
+            ((symbol-function '-)
              (lambda (&rest args)
               (setq comp-test-primitive-redefine-args args)
                'xxx)))
@@ -467,11 +464,11 @@ comp-test-primitive-redefine-args
 
 (comp-deftest comp-test-defsubst ()
   ;; Bug#42664, Bug#43280, Bug#44209.
-  (should-not (subr-native-elisp-p (symbol-function #'comp-test-defsubst-f))))
+  (should-not (subr-native-elisp-p (symbol-function 'comp-test-defsubst-f))))
 
 (comp-deftest primitive-redefine-compile-44221 ()
   "Test the compiler still works while primitives are redefined (bug#44221)."
-  (cl-letf (((symbol-function #'delete-region)
+  (cl-letf (((symbol-function 'delete-region)
              (lambda (_ _))))
     (should (subr-native-elisp-p
              (native-compile
@@ -506,13 +503,13 @@ comp-test-primitive-redefine-args
 
 (comp-deftest 45603-1 ()
   "<https://lists.gnu.org/archive/html/bug-gnu-emacs/2020-12/msg01994.html>"
-  (load (native-compile comp-test-45603-src))
-  (should (fboundp #'comp-test-45603--file-local-name)))
+  (load (native-compile (ert-resource-file "comp-test-45603.el")))
+  (should (fboundp 'comp-test-45603--file-local-name)))
 
 (comp-deftest 46670-1 ()
   "<https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-02/msg01413.html>"
   (should (string= (comp-test-46670-2-f "foo") "foo"))
-  (should (equal (subr-type (symbol-function #'comp-test-46670-2-f))
+  (should (equal (subr-type (symbol-function 'comp-test-46670-2-f))
                  '(function (t) t))))
 
 (comp-deftest 46824-1 ()
@@ -742,7 +739,7 @@ comp-test-primitive-redefine-args
 (comp-deftest dynamic-help-arglist ()
   "Test `help-function-arglist' works on lisp/d (bug#42572)."
   (should (equal (help-function-arglist
-                  (symbol-function #'comp-tests-ffuncall-callee-opt-rest-dyn-f)
+                  (symbol-function 'comp-tests-ffuncall-callee-opt-rest-dyn-f)
                   t)
                  '(a b &optional c &rest d))))
 
@@ -815,7 +812,7 @@ comp-tests-tco-checker
                (comp-tests-tco-f (+ a b) a (- count 1))))
           t)
     (native-compile #'comp-tests-tco-f)
-    (should (subr-native-elisp-p (symbol-function #'comp-tests-tco-f)))
+    (should (subr-native-elisp-p (symbol-function 'comp-tests-tco-f)))
     (should (= (comp-tests-tco-f 1 0 10) 55))))
 
 (defun comp-tests-fw-prop-checker-1 (_)
@@ -842,7 +839,7 @@ comp-tests-fw-prop-checker-1
                (length c))) ; <= has to optimize
           t)
     (native-compile #'comp-tests-fw-prop-1-f)
-    (should (subr-native-elisp-p (symbol-function #'comp-tests-fw-prop-1-f)))
+    (should (subr-native-elisp-p (symbol-function 'comp-tests-fw-prop-1-f)))
     (should (= (comp-tests-fw-prop-1-f) 6))))
 
 (defun comp-tests-check-ret-type-spec (func-form ret-type)
@@ -1421,12 +1418,14 @@ comp-tests-pure-checker-2
   (let ((native-comp-speed 3)
         (comp-post-pass-hooks '((comp-final comp-tests-pure-checker-1
                                             comp-tests-pure-checker-2))))
-    (load (native-compile comp-test-pure-src))
+    (load (native-compile (ert-resource-file "comp-test-pure.el")))
+    (declare-function comp-tests-pure-caller-f nil)
+    (declare-function comp-tests-pure-fibn-entry-f nil)
 
-    (should (subr-native-elisp-p (symbol-function #'comp-tests-pure-caller-f)))
+    (should (subr-native-elisp-p (symbol-function 'comp-tests-pure-caller-f)))
     (should (= (comp-tests-pure-caller-f) 4))
 
-    (should (subr-native-elisp-p (symbol-function 
#'comp-tests-pure-fibn-entry-f)))
+    (should (subr-native-elisp-p (symbol-function 
'comp-tests-pure-fibn-entry-f)))
     (should (= (comp-tests-pure-fibn-entry-f) 6765))))
 
 (defvar comp-tests-cond-rw-checked-function nil