Zero byte-compiler warnings in the test suite

Zero byte-compiler warnings in the test suite

[Stefan Kangas]

Dear emacs-devel,

I would like to bring to your attention that we now have a test suite
with no byte-compiler warnings.  This is nice, as there is less noise,
and it will be easier to spot real problems.

In my use, I'm happy that I can now look at a soothing zero in the mode
line of my *Compilation* buffer when saying "make && make check".  ;-)

Thanks to everyone who helped with this.

Re: Zero byte-compiler warnings in the test suite

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> In my use, I'm happy that I can now look at a soothing zero in the mode
> line of my *Compilation* buffer when saying "make && make check".  ;-)

That's great, and it's indeed much more soothing.  😀

But with a native-comp build (at least), I'm getting two
warnings/errors on the current trunk: 

In toplevel form:
lisp/emacs-lisp/eieio-tests/eieio-test-methodinvoke.el:273:1: Warning: ‘defgeneric’ is an obsolete macro (as of 25.1); use ‘cl-defgeneric’ instead.

and

Compiler-macro error for cl-typep: (error "Unknown type notatype")
Compiler-macro error for cl-typep: (error "Unknown type notatype")
Compiler-macro error for cl-typep: (error "Unknown type notatype")
Compiler-macro error for cl-typep: (error "Unknown type notatype")
Compiler-macro error for cl-typep: (error "Unknown type notatype")

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Zero byte-compiler warnings in the test suite

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> But with a native-comp build (at least), I'm getting two
> warnings/errors on the current trunk:
>
> In toplevel form:
> lisp/emacs-lisp/eieio-tests/eieio-test-methodinvoke.el:273:1: Warning: ‘defgeneric’ is an obsolete macro (as of 25.1); use ‘cl-defgeneric’ instead.

Strange, I don't see that here (with native-comp).  Does this help?

diff --git a/test/lisp/emacs-lisp/eieio-tests/eieio-test-methodinvoke.el
b/test/lisp/emacs-lisp/eieio-tests/eieio-test-methodinvoke.el
index e881e46a2d..bc89f23b58 100644
--- a/test/lisp/emacs-lisp/eieio-tests/eieio-test-methodinvoke.el
+++ b/test/lisp/emacs-lisp/eieio-tests/eieio-test-methodinvoke.el
@@ -271,6 +271,7 @@ E-base2
 (defclass E (E-base1 E-base2) () :method-invocation-order :breadth-first)

 (with-suppressed-warnings ((obsolete defmethod)
+                           (obsolete defgeneric)
                            (obsolete next-method-p)
                            (obsolete call-next-method))
   (defmethod eitest-F ((_p E))

> Compiler-macro error for cl-typep: (error "Unknown type notatype")
> Compiler-macro error for cl-typep: (error "Unknown type notatype")
> Compiler-macro error for cl-typep: (error "Unknown type notatype")
> Compiler-macro error for cl-typep: (error "Unknown type notatype")
> Compiler-macro error for cl-typep: (error "Unknown type notatype")

I've seen these as well, but they weren't counted by compilation-mode so
I just sort of ignored them.  It would be good to fix them as well of
course.

AFAICT, they come from here:

  ELC+ELN  lisp/emacs-lisp/pcase-tests.elc
Compiler-macro error for cl-typep: (error "Unknown type notatype")
Compiler-macro error for cl-typep: (error "Unknown type notatype")
Compiler-macro error for cl-typep: (error "Unknown type notatype")
Compiler-macro error for cl-typep: (error "Unknown type notatype")
Compiler-macro error for cl-typep: (error "Unknown type notatype")
  GEN      lisp/emacs-lisp/pcase-tests.log

Grepping for notatype leads me to:

./lisp/emacs-lisp/pcase-tests.el:111:                  ((cl-type
notatype) 'integer))))

Does anyone know what is going on here?

Re: Zero byte-compiler warnings in the test suite

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Strange, I don't see that here (with native-comp).  Does this help?

[...]

>  (with-suppressed-warnings ((obsolete defmethod)
> +                           (obsolete defgeneric)

Yup, that fixes it.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Zero byte-compiler warnings in the test suite

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Tue, 21 Dec 2021 04:36:51 -0800
> 
> Dear emacs-devel,
> 
> I would like to bring to your attention that we now have a test suite
> with no byte-compiler warnings.

Thanks to all those who work relentlessly on making the tests
warning-free.

Re: Zero byte-compiler warnings in the test suite

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Yup, that fixes it.

Thanks, pushed to master as commit 1a923e5ac6.

Re: Zero byte-compiler warnings in the test suite

[Stefan Monnier]

> Compiler-macro error for cl-typep: (error "Unknown type notatype")
>   GEN      lisp/emacs-lisp/pcase-tests.log
>
> Grepping for notatype leads me to:
>
> ./lisp/emacs-lisp/pcase-tests.el:111:                  ((cl-type
> notatype) 'integer))))
>
> Does anyone know what is going on here?

I thought it was pretty obvious: `c-type` takes a type as argument, but
`notatype` is ... not declared as a type, so we get an error.

The test expects the error to be signal'd at run time whereas the
compiler macro prefers to signal the error right away.
As a bias observer, I'd suggest to fix the test rather than the compiler
macro ;-)


        Stefan

Re: Zero byte-compiler warnings in the test suite

[Stefan Kangas]

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

> The test expects the error to be signal'd at run time whereas the
> compiler macro prefers to signal the error right away.
> As a bias observer, I'd suggest to fix the test rather than the compiler
> macro ;-)

How do we usually test compiler macros?  I guess we should create a
separate file and try to byte-compile it?

Or should we just disable the test in non-interactive sessions?

Re: Zero byte-compiler warnings in the test suite

[Stefan Monnier]

>> The test expects the error to be signal'd at run time whereas the
>> compiler macro prefers to signal the error right away.
>> As a bias observer, I'd suggest to fix the test rather than the compiler
>> macro ;-)
>
> How do we usually test compiler macros?  I guess we should create a
> separate file and try to byte-compile it?

I'd do something like

    (should-error
     (macroexpand-all '(pcase ...)))


-- Stefan

Re: Zero byte-compiler warnings in the test suite

[Stefan Kangas]

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

>> How do we usually test compiler macros?  I guess we should create a
>> separate file and try to byte-compile it?
>
> I'd do something like
>
>     (should-error
>      (macroexpand-all '(pcase ...)))

macroexpand-all didn't work, but I installed this, which seems to have
fixed it:

diff --git a/test/lisp/emacs-lisp/pcase-tests.el
b/test/lisp/emacs-lisp/pcase-tests.el
index 7ad01e7aef..40ae9809b5 100644
--- a/test/lisp/emacs-lisp/pcase-tests.el
+++ b/test/lisp/emacs-lisp/pcase-tests.el
@@ -107,8 +107,11 @@ pcase-tests-cl-type
   (should (equal (pcase 1
                    ((cl-type (integer 0 2)) 'integer-0<=n<=2))
                  'integer-0<=n<=2))
-  (should-error (pcase 1
-                  ((cl-type notatype) 'integer))))
+  (should-error
+   ;; Avoid error at compile time due to compiler macro.
+   (eval '(pcase 1
+            ((cl-type notatype) 'integer))
+         t)))

 (ert-deftest pcase-tests-setq ()
   (should (equal (let (a b)

Weird non-caught error in ERT (was: Zero byte-compiler warnings in the test suite)

[Stefan Monnier]

>> I'd do something like
>>
>>     (should-error
>>      (macroexpand-all '(pcase ...)))
>
> macroexpand-all didn't work, but I installed this, which seems to have
> fixed it:

Indeed, how odd.  I tried:

    (should
     (condition-case err
         (progn
           (macroexpand-all
            '(pcase 1
               ((cl-type notatype) 'integer)))
           nil)
       (error (string-match "Unknown type notabug" (cadr err))))))

and that gives me:

    Test pcase-tests-cl-type backtrace:
      error("Unknown type %S" notatype)
      cl-typep--inliner((cl-typep 1 'notatype) 1 'notatype)
      apply(cl-typep--inliner (cl-typep 1 'notatype) (1 'notatype))
      macroexp--compiler-macro(cl-typep--inliner (cl-typep 1 'notatype))
      macroexp--expand-all((pcase--flip cl-typep 'notatype 1))
      macroexp--all-forms((if (pcase--flip cl-typep 'notatype 1) (let nil 
      macroexp--expand-all((pcase 1 ((cl-type notatype) 'integer)))
      macroexpand-all((pcase 1 ((cl-type notatype) 'integer)))
      (progn (macroexpand-all '(pcase 1 ((cl-type notatype) 'integer))) ni
      (condition-case err (progn (macroexpand-all '(pcase 1 ((cl-type nota
    [...]

Notice the place where I truncated the stack: we're inside the
`condition-case`!  So why do we get a backtrace instead of letting
`condition-case` catch the error?


        Stefan

Re: Weird non-caught error in ERT (was: Zero byte-compiler warnings in the test suite)

[Philipp Stephani]

Am Do., 23. Dez. 2021 um 01:07 Uhr schrieb Stefan Monnier
<monnier@iro.umontreal.ca>:
>
> >> I'd do something like
> >>
> >>     (should-error
> >>      (macroexpand-all '(pcase ...)))
> >
> > macroexpand-all didn't work, but I installed this, which seems to have
> > fixed it:
>
> Indeed, how odd.  I tried:
>
>     (should
>      (condition-case err
>          (progn
>            (macroexpand-all
>             '(pcase 1
>                ((cl-type notatype) 'integer)))
>            nil)
>        (error (string-match "Unknown type notabug" (cadr err))))))
>
> and that gives me:
>
>     Test pcase-tests-cl-type backtrace:
>       error("Unknown type %S" notatype)
>       cl-typep--inliner((cl-typep 1 'notatype) 1 'notatype)
>       apply(cl-typep--inliner (cl-typep 1 'notatype) (1 'notatype))
>       macroexp--compiler-macro(cl-typep--inliner (cl-typep 1 'notatype))
>       macroexp--expand-all((pcase--flip cl-typep 'notatype 1))
>       macroexp--all-forms((if (pcase--flip cl-typep 'notatype 1) (let nil
>       macroexp--expand-all((pcase 1 ((cl-type notatype) 'integer)))
>       macroexpand-all((pcase 1 ((cl-type notatype) 'integer)))
>       (progn (macroexpand-all '(pcase 1 ((cl-type notatype) 'integer))) ni
>       (condition-case err (progn (macroexpand-all '(pcase 1 ((cl-type nota
>     [...]
>
> Notice the place where I truncated the stack: we're inside the
> `condition-case`!  So why do we get a backtrace instead of letting
> `condition-case` catch the error?
>

Maybe because `should' installs a signal hook function?