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

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

[Stefan Monnier]

> +(eval-when-compile
> +  (require 'cl-lib)
> +  (require 'comp))
> +(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))

This looks pretty ugly, in my book.

Were the warnings false positives or were they diagnosing real problems?

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.


        Stefan

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

[Stephen Gildea]

Stefan Monnier <monnier@iro.umontreal.ca> wrote:

>   > +(eval-when-compile
>   > +  (require 'cl-lib)
>   > +  (require 'comp))
>   > +(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))
>   
>   This looks pretty ugly, in my book.
>   
>   Were the warnings false positives or were they diagnosing real problems?

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.

>   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 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?

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.

 < Stephen

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