Re: master a0f6029: Fix misuses of `byte-compile-macro-environment`

Re: master a0f6029: Fix misuses of `byte-compile-macro-environment`

[Basil L. Contovounesios]

[-- Attachment #1: Type: text/plain, Size: 1633 bytes --]

monnier@iro.umontreal.ca (Stefan Monnier) writes:

> branch: master
> commit a0f60293d97cda858c033db4ae074e5e5560aab2
> Author: Stefan Monnier <monnier@iro.umontreal.ca>
> Commit: Stefan Monnier <monnier@iro.umontreal.ca>
>
>     Fix misuses of `byte-compile-macro-environment`

This seems to result in the following test failure with 'make check',
but strangely not with 'make test/pcase-tests':

  Test pcase-tests-macro backtrace:
    signal(void-function (pcase-tests-plus--pcase-macroexpander))
    apply(signal (void-function (pcase-tests-plus--pcase-macroexpander))
    #f(compiled-function () #<bytecode -0x130ab4f40ca06d1f>)()
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name pcase-tests-macro :documentation nil 
    ert-run-or-rerun-test(#s(ert--stats :selector (not (or ... ...)) :te
    ert-run-tests((not (or (tag :expensive-test) (tag :unstable))) #f(co
    ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable)))
    ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
    eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
    command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emacs-lisp/pcase-tes
    command-line()
    normal-top-level()
  Test pcase-tests-macro condition:
      (void-function pcase-tests-plus--pcase-macroexpander)
     FAILED  5/9  pcase-tests-macro (0.000163 sec)

BTW, 'make bootstrap' now emits this warning: 

  In cl--sm-macroexpand:
  emacs-lisp/cl-macs.el:2301:61: Warning: Unused lexical argument `dontcare'

(The 'sm' is the artist's signature, right? ;)

Is this the right fix?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Pacify-unused-var-warning-in-cl-sm-macroexpand.patch --]
[-- Type: text/x-diff, Size: 1026 bytes --]

From a29983cbbfb1fbd7cb2773d1e2dc89483df82bf4 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Wed, 3 Mar 2021 12:49:31 +0000
Subject: [PATCH] ; Pacify unused var warning in cl--sm-macroexpand.

---
 lisp/emacs-lisp/cl-macs.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 91146c4d0e..c38dc44ff6 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -2298,7 +2298,7 @@ cl--sm-macroexpand
             ;; The behavior of CL made sense in a dynamically scoped
             ;; language, but nowadays, lexical scoping semantics is more often
             ;; expected.
-            (`(,(or 'let 'let*) . ,(or `(,bindings . ,body) dontcare))
+            (`(,(or 'let 'let*) . ,(or `(,bindings . ,body) pcase--dontcare))
              (let ((nbs ()) (found nil))
                (dolist (binding bindings)
                  (let* ((var (if (symbolp binding) binding (car binding)))
-- 
2.30.1


[-- Attachment #3: Type: text/plain, Size: 20 bytes --]


Thanks,

-- 
Basil

Re: master a0f6029: Fix misuses of `byte-compile-macro-environment`

[Basil L. Contovounesios]

Mattias Engdegård <mattiase@acm.org> writes:

> 3 mars 2021 kl. 15.44 skrev Basil L. Contovounesios <contovob@tcd.ie>:
>
>> This seems to result in the following test failure with 'make check',
>> but strangely not with 'make test/pcase-tests':
>
> Maybe the latter doesn't use the byte-compiled version of the tests.
> Try something like
>
>  make TEST_LOAD_EL=no -C test pcase-tests

Indeed, 'make TEST_LOAD_EL=yes test/pcase-tests' succeeds,
whereas 'make TEST_LOAD_EL=no  test/pcase-tests' fails.

Should 'make test/%' be fixed to behave more like 'make check'?

Thanks,

-- 
Basil

Re: master a0f6029: Fix misuses of `byte-compile-macro-environment`

[Mattias Engdegård]

3 mars 2021 kl. 16.16 skrev Basil L. Contovounesios <contovob@tcd.ie>:

> Indeed, 'make TEST_LOAD_EL=yes test/pcase-tests' succeeds,
> whereas 'make TEST_LOAD_EL=no  test/pcase-tests' fails.
> 
> Should 'make test/%' be fixed to behave more like 'make check'?

Maybe -- test/Makefile.in says:

  # Whether to run tests from .el files in preference to .elc, we do
  # this by default since it gives nicer stacktraces.
  # If you just want a pass/fail, setting this to no is much faster.
  export TEST_LOAD_EL ?= \
      $(if $(findstring $(MAKECMDGOALS), all check check-maybe),no,yes)

For some tests it may be useful to run both .el and .elc; for others, it's just a waste of time.
My guess is that on balance, .elc is more important (and faster) but obviously .el gives better diagnostics when something fails.

Re: master a0f6029: Fix misuses of `byte-compile-macro-environment`

[Lars Ingebrigtsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

>> Maybe the latter doesn't use the byte-compiled version of the tests.
>> Try something like
>>
>>  make TEST_LOAD_EL=no -C test pcase-tests
>
> Indeed, 'make TEST_LOAD_EL=yes test/pcase-tests' succeeds,
> whereas 'make TEST_LOAD_EL=no  test/pcase-tests' fails.

Oh, that explains a lot -- why saying "make foo-tests" often passes while
saying "make check" fails, which has left me scratching my head often.

> Should 'make test/%' be fixed to behave more like 'make check'?

Yes, I think so.

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

Re: master a0f6029: Fix misuses of `byte-compile-macro-environment`

[Stefan Monnier]

>>     Fix misuses of `byte-compile-macro-environment`
>
> This seems to result in the following test failure with 'make check',
> but strangely not with 'make test/pcase-tests':

Hmm... I wonder why the test code worked before that patch.
The problem is that pcase macros defined with `pcase-defmacro` can't be
used in the same file where they're defined (you need to wrap them in
`eval-and-compile`).
Maybe we should fix that, but the problem is not new, so I'm wondering
why the bug didn't trigger earlier.

> BTW, 'make bootstrap' now emits this warning: 
>
>   In cl--sm-macroexpand:
>   emacs-lisp/cl-macs.el:2301:61: Warning: Unused lexical argument `dontcare'

Yes, that's good: the warning wanrs about an actual problem.

> (The 'sm' is the artist's signature, right? ;)

Nah, the artist found a good excuse: "s(ymbol-)m(acrolet)"

> Is this the right fix?

Yes, but it doesn't just pacify the byte-compiler: it fixes the source
code (in Emacs-27, this worked by accident, really).


        Stefan

Re: master a0f6029: Fix misuses of `byte-compile-macro-environment`

[Basil L. Contovounesios]

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

>>>     Fix misuses of `byte-compile-macro-environment`
>>
>> This seems to result in the following test failure with 'make check',
>> but strangely not with 'make test/pcase-tests':
>
> Hmm... I wonder why the test code worked before that patch.
> The problem is that pcase macros defined with `pcase-defmacro` can't be
> used in the same file where they're defined (you need to wrap them in
> `eval-and-compile`).
> Maybe we should fix that, but the problem is not new, so I'm wondering
> why the bug didn't trigger earlier.

Beats me :/.  Even after reverting commits a0f60293d9 and 88ca2280ba,
'make check' still fails.  But both 'git bisect', and checking out its
parent 6ad9b8d677, point to a0f60293d9 as being the culprit.

>> BTW, 'make bootstrap' now emits this warning: 
>>
>>   In cl--sm-macroexpand:
>>   emacs-lisp/cl-macs.el:2301:61: Warning: Unused lexical argument `dontcare'
>
> Yes, that's good: the warning wanrs about an actual problem.
>
>> (The 'sm' is the artist's signature, right? ;)
>
> Nah, the artist found a good excuse: "s(ymbol-)m(acrolet)"

Not the kind of nom de plume you see every day.

>> Is this the right fix?
>
> Yes, but it doesn't just pacify the byte-compiler: it fixes the source
> code (in Emacs-27, this worked by accident, really).

Thanks, pushed with the log message adapted accordingly.

-- 
Basil

Re: master a0f6029: Fix misuses of `byte-compile-macro-environment`

[Stefan Monnier]

> Beats me :/.  Even after reverting commits a0f60293d9 and 88ca2280ba,
> 'make check' still fails.  But both 'git bisect', and checking out its
> parent 6ad9b8d677, point to a0f60293d9 as being the culprit.

It turns out it was because a `no-byte-compile` was hiding the problem.


        Stefan

Re: master a0f6029: Fix misuses of `byte-compile-macro-environment`

[Michael Albinus]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

Hi Basil,

> Should 'make test/%' be fixed to behave more like 'make check'?

It should be rather like 'make check-expensive', that means running also
expensive tests. As it does now.

> Thanks,

Best regards, Michael.

Re: master a0f6029: Fix misuses of `byte-compile-macro-environment`

[Basil L. Contovounesios]

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

>> Beats me :/.  Even after reverting commits a0f60293d9 and 88ca2280ba,
>> 'make check' still fails.  But both 'git bisect', and checking out its
>> parent 6ad9b8d677, point to a0f60293d9 as being the culprit.
>
> It turns out it was because a `no-byte-compile` was hiding the problem.

Sometimes I feel like the compiler doesn't ignore our comments.

-- 
Basil

Re: master a0f6029: Fix misuses of `byte-compile-macro-environment`

[Basil L. Contovounesios]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>>> Maybe the latter doesn't use the byte-compiled version of the tests.
>>> Try something like
>>>
>>>  make TEST_LOAD_EL=no -C test pcase-tests
>>
>> Indeed, 'make TEST_LOAD_EL=yes test/pcase-tests' succeeds,
>> whereas 'make TEST_LOAD_EL=no  test/pcase-tests' fails.
>
> Oh, that explains a lot -- why saying "make foo-tests" often passes while
> saying "make check" fails, which has left me scratching my head often.
>
>> Should 'make test/%' be fixed to behave more like 'make check'?
>
> Yes, I think so.

It seems to be intentional though.  Quoth test/Makefile.in:

  # Whether to run tests from .el files in preference to .elc, we do
  # this by default since it gives nicer stacktraces.
  # If you just want a pass/fail, setting this to no is much faster.
  export TEST_LOAD_EL ?= \
      $(if $(findstring $(MAKECMDGOALS), all check check-maybe),no,yes)

And test/README:

  Note that although the test files are always compiled (unless they set
  no-byte-compile), the source files will be run when expensive or
  unstable tests are involved, to give nicer backtraces.  To run the
  compiled version of a test use

      make TEST_LOAD_EL=no ...

-- 
Basil

Re: master a0f6029: Fix misuses of `byte-compile-macro-environment`

[Stefan Monnier]

>>> Beats me :/.  Even after reverting commits a0f60293d9 and 88ca2280ba,
>>> 'make check' still fails.  But both 'git bisect', and checking out its
>>> parent 6ad9b8d677, point to a0f60293d9 as being the culprit.
>> It turns out it was because a `no-byte-compile` was hiding the problem.
> Sometimes I feel like the compiler doesn't ignore our comments.

I have no doubt we are being watched,


        Stefan