bug#14422: 24.3; Eager Macro Expansion

bug#14422: 24.3; Eager Macro Expansion

[Achim Gratz]

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


In GNU Emacs 24.3.1 (i686-suse-linux-gnu, GTK+ Version 3.8.1) of 2013-04-27
Windowing system distributor `The X.Org Foundation', version 11.0.11302000
System Description:	openSUSE 12.3/Tumbleweed (i586)

The following test case demonstrates a problem that has been distilled
from Org's test suite.  Org has since switched to use defun instead of
defmacro to work around this issue, but it seems that this might be a
corner case that eager macro expansion produces or not yet warn about
(whatever the intended behaviour might be).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: eme.el --]
[-- Type: text/x-emacs-lisp, Size: 336 bytes --]

(require 'ert)

(defvar ll nil)

(defmacro one (p)
  `(progn (push ',p ll)))

(defmacro two (p)
  (let (pp)
    (setq pp (append ll p))
  `(progn (push ',pp ll))))
 
(ert-deftest surprise ()
    (should
     (equal '((one . two) one)
	    (progn
	      (one one)
	      (two two)
	      ll))))

(ert-run-tests-batch-and-exit 'surprise)

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


This passes on all Emacs versions until 24.2, but fails on trunk:

eme> emacs-24.2 -batch -Q -l eme.el
Running 1 tests (2013-05-19 16:16:06+0200)
   passed  1/1  surprise

Ran 1 tests, 1 results as expected (2013-05-19 16:16:06+0200)

eme> emacs-24.3.50 -batch -Q -l eme.el
Running 1 tests (2013-05-19 16:16:16+0200)
Test surprise backtrace:
  (if (unwind-protect (setq value-2 (apply fn-0 args-1)) (setq form-de
  (let (form-description-4) (if (unwind-protect (setq value-2 (apply f
  (let ((value-2 (quote ert-form-evaluation-aborted-3))) (let (form-de
  (let ((fn-0 (function equal)) (args-1 (list (quote ((one . two) one)
  (lambda nil (let ((fn-0 (function equal)) (args-1 (list (quote ((one
  #[0 "\306\307!r\211q\210\310\311\312\313\314\315!\316\"\317\320%DC
  funcall(#[0 "\306\307!r\211q\210\310\311\312\313\314\315!\316\"\31
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  #[0 "r\304\305!q\210\306 )\307\310\311\312\313\314!\315\"\316\317%
  funcall(#[0 "r\304\305!q\210\306 )\307\310\311\312\313\314!\315\"\
  ert-run-test([cl-struct-ert-test surprise nil (lambda nil (let ((fn-
  ert-run-or-rerun-test([cl-struct-ert--stats surprise [[cl-struct-ert
  ert-run-tests(surprise #[385 "\306\307\"\203D\211\211G\310U\203\
  ert-run-tests-batch(surprise)
  ert-run-tests-batch-and-exit(surprise)
  eval-buffer(#<buffer  *load*> nil "/eme/eme.el"
  load-with-code-conversion("/eme/eme.el" "/eme/
  load("/eme/eme.el" nil t)
  command-line-1(("-l" "eme.el"))
  command-line()
  normal-top-level()
Test surprise condition:
    (ert-test-failed
     ((should
       (equal '...
        (progn ... ... ll)))
      :form
      (equal
       ((one . two)
        one)
       (two one))
      :value nil :explanation
      (list-elt 0
                (different-types
                 (one . two)
                 two))))
   FAILED  1/1  surprise

Ran 1 tests, 0 results as expected, 1 unexpected (2013-05-19 16:16:16+0200)

1 unexpected results:
   FAILED  surprise


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldUserWavetables

bug#14422: 24.3; Eager Macro Expansion

[Stefan Monnier]

> The following test case demonstrates a problem that has been distilled
> from Org's test suite.

   % emacs24 -Q --batch -f batch-byte-compile eme.el
   
   In toplevel form:
   eme.el:3:1:Warning: global/dynamic var `ll' lacks a prefix
   eme.el:13:1:Error: Symbol's value as variable is void: ll
   %

So the code has a problem, since byte-compiling it doesn't work
(emacs24 is 24.1, here).  No wonder eager macro-expansion also leads
to problems.


        Stefan

bug#14422: 24.3; Eager Macro Expansion

[Achim Gratz]

Stefan Monnier writes:
>> The following test case demonstrates a problem that has been distilled
>> from Org's test suite.
>
>    % emacs24 -Q --batch -f batch-byte-compile eme.el
>    
>    In toplevel form:
>    eme.el:3:1:Warning: global/dynamic var `ll' lacks a prefix
>    eme.el:13:1:Error: Symbol's value as variable is void: ll
>    %
>
> So the code has a problem, since byte-compiling it doesn't work

The code isn't meant to be byte-compiled, but if you want to I'd have to
split it into two seperate files.  The ERT portion of the code is never
byte-compiled in Org and I don't know if it would make sense to do this.

> (emacs24 is 24.1, here).  No wonder eager macro-expansion also leads
> to problems.

Well, the code does declare the variable symbol special and initializes
it nil, so finding the symbol undefined during compilation and/or macro
expansion would constitute a bug in either ERT or Emacs, no?


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldUserWavetables

bug#14422: 24.3; Eager Macro Expansion

[Stefan Monnier]

> Well, the code does declare the variable symbol special and initializes
> it nil, so finding the symbol undefined during compilation and/or macro
> expansion would constitute a bug in either ERT or Emacs, no?

The defvar is only executed at run time (although it does have an
effect at compile time, which is to tell the compiler that the variable
will exist at run time).

So using `ll' during the macro expansion is wrong.

If you want `ll' to defined earlier, you can wrap it in
`eval-and-compile' (tho it's better not to abuse it).  I can't tell what
solution I'd recommend in your case, since your distilled test case is
"too distilled" to understand what it's trying to do.


        Stefan

bug#14422: 24.3; Eager Macro Expansion

[Achim Gratz]

Stefan Monnier writes:
> The defvar is only executed at run time (although it does have an
> effect at compile time, which is to tell the compiler that the variable
> will exist at run time).
>
> So using `ll' during the macro expansion is wrong.

That may well be a bug in the original code, although of course the
defvar is in a different file that has been loaded before the test
definition would expand the macro, so the expectation is that the symbol
should exist and have nil value when trying to run the tests.

> If you want `ll' to defined earlier, you can wrap it in
> `eval-and-compile' (tho it's better not to abuse it).  I can't tell what
> solution I'd recommend in your case, since your distilled test case is
> "too distilled" to understand what it's trying to do.

I'll have to check again how things were supposed to have been
initialized in Org, but the assumption that the (no longer existing)
macro definitions made was clearly that the stand-in for the ll symbol
was pre-existing at macro expansion time.  I'll try to re-create the
test case to match that behaviour and come back to you.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

bug#14422: 24.3; Eager Macro Expansion

[Achim Gratz]

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

Stefan Monnier writes:
> So the code has a problem, since byte-compiling it doesn't work
> (emacs24 is 24.1, here).  No wonder eager macro-expansion also leads
> to problems.

Here's the revised test case that compiles cleanly and still has the
same problem:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: eme.el --]
[-- Type: text/x-emacs-lisp, Size: 180 bytes --]

(defvar eme-ll nil)

(defmacro one (p)
  `(progn (push ',p eme-ll)))

(defmacro two (p)
  (let (pp)
    (setq pp (append eme-ll p))
  `(progn (push ',pp eme-ll))))

(provide 'eme)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: eme-test.el --]
[-- Type: text/x-emacs-lisp, Size: 207 bytes --]

(require 'eme)
(require 'ert)

 
(ert-deftest surprise ()
    (should
     (equal '((one . two) one)
	    (progn
	      (one one)
	      (two two)
	      eme-ll))))

(ert-run-tests-batch-and-exit 'surprise)

[-- Attachment #4: Type: text/plain, Size: 2034 bytes --]


--8<---------------cut here---------------start------------->8---
eme> emacs-24.3.50 -batch -Q -L . --eval '(byte-compile-file "eme.el")'
Wrote /home/eme/eme.elc
eme> emacs-24.3.50 -batch -Q -L . --eval '(byte-compile-file "eme-test.el")'
Wrote /home/eme/eme-test.elc
eme> emacs-24.3.50 -batch -Q -L . -l eme-test
Running 1 tests (2013-05-30 19:37:09+0200)
Test surprise backtrace:
  #[nil "\305\306\30B\31B\211D▒\311312\313\216\314\n    \"\211
  #[0 "\306\307!r\211q\210\310\311\312\313\314\315!\316\"\317\320%DC
  funcall(#[0 "\306\307!r\211q\210\310\311\312\313\314\315!\316\"\31
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  #[0 "r\304\305!q\210\306 )\307\310\311\312\313\314!\315\"\316\317%
  funcall(#[0 "r\304\305!q\210\306 )\307\310\311\312\313\314!\315\"\
  ert-run-test([cl-struct-ert-test surprise nil #[nil "\305\306\30B
  ert-run-or-rerun-test([cl-struct-ert--stats surprise [[cl-struct-ert
  ert-run-tests(surprise #[385 "\306\307\"\203D\211\211G\310U\203\
  ert-run-tests-batch(surprise)
  ert-run-tests-batch-and-exit(surprise)
  byte-code("\301\302!\210\301\303!\210\304\305\306\307\305\310\311\31
  load("eme-test" nil t)
  command-line-1(("-L" "." "-l" "eme-test"))
  command-line()
  normal-top-level()
Test surprise condition:
    (ert-test-failed
     ((should
       (equal '...
        (progn ... ... eme-ll)))
      :form
      (equal
       ((one . two)
        one)
       (two one))
      :value nil :explanation
      (list-elt 0
                (different-types
                 (one . two)
                 two))))
   FAILED  1/1  surprise

Ran 1 tests, 0 results as expected, 1 unexpected (2013-05-30 19:37:09+0200)

1 unexpected results:
   FAILED  surprise
--8<---------------cut here---------------end--------------->8---


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldUserWavetables

bug#14422: 24.3; Eager Macro Expansion

[Stefan Monnier]

> Here's the revised test case that compiles cleanly and still has the
> same problem:


> (defvar eme-ll nil)

> (defmacro one (p)
>   `(progn (push ',p eme-ll)))

> (defmacro two (p)
>   (let (pp)
>     (setq pp (append eme-ll p))
>   `(progn (push ',pp eme-ll))))

> (provide 'eme)


> (require 'eme)
> (require 'ert)

 
> (ert-deftest surprise ()
>     (should
>      (equal '((one . two) one)
> 	    (progn
> 	      (one one)
> 	      (two two)
> 	      eme-ll))))

I see the test fails, but that's just because the test is wrong.
Try to create a new file foo.el:

   (require 'eme)
   
   (message "Result = %s"
            (progn
   	      (one one)
   	      (two two)
   	      eme-ll))

Then byte-compile it.  Then do

   emacs23 --batch -Q -l ~/tmp/foo.el
and
   emacs23 --batch -Q -l ~/tmp/foo.elc

You'll see that your code behaves differently when byte-compiled.


        Stefan


Analysis:

  (one one)

will add `one' to eme-ll at run-time.

  (two two)

reads the macroexpansion-time (e.g. compilation-time, load-time, or
run-time) value of eme-ll and adds it to eme-ll at run-time.

  eme-ll

returns the run-time value of eme-ll.

bug#14422: 24.3; Eager Macro Expansion

[Achim Gratz]

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

Stefan Monnier writes:
> I see the test fails, but that's just because the test is wrong.
[…]
> You'll see that your code behaves differently when byte-compiled.

Yes, we've already established that the original code itself had a bug.
The correct code would look like this


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: eme.el --]
[-- Type: text/x-emacs-lisp, Size: 171 bytes --]

(defvar eme-ll nil)

(defmacro one (p)
  `(progn (push ',p eme-ll)))

(defmacro two (p)
  `(let ((pp (append eme-ll ',p)))
     (progn (push pp eme-ll))))

(provide 'eme)

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


The remaining point is that the ERT test still fails in exactly the same
way when it is _not_ byte-compiled and batch-tested (like the original
case in Org), but it produces the correct result when testing in
interactive mode or in the debugger.  I guess I'm asking for a warning
for recursive macro expansions that manipulate the same variable both at
expansion and at runtime in separate macros.  Alternatively if the buggy
code would always fail in the same way that would at least ensure it can
be found more easily.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldUserWavetables

bug#14422: 24.3; Eager Macro Expansion

[Stefan Monnier]

retitle 14422 Apply eager-macroexpansion everywhere (eval-region, ...)
thanks

> The remaining point is that the ERT test still fails in exactly the same
> way when it is _not_ byte-compiled and batch-tested (like the original
> case in Org), but it produces the correct result when testing in
> interactive mode or in the debugger.  I guess I'm asking for a warning
> for recursive macro expansions that manipulate the same variable both at
> expansion and at runtime in separate macros.  Alternatively if the buggy
> code would always fail in the same way that would at least ensure it can
> be found more easily.

Indeed, that's part of the reason why I introduced this "eager
macroexpansion", which makes it that my previous foo.el test now behaves
identically whether it's byte-compiled or not.

The behavior is still different in a few other cases (where eager macro
expansion is not performed, typically M-C-x and things like that), but
I hope to reduce/eliminate them at some point.


        Stefan

bug#14422: 24.3; Eager Macro Expansion

[Stefan Monnier]

> The behavior is still different in a few other cases (where eager macro
> expansion is not performed, typically M-C-x and things like that), but
> I hope to reduce/eliminate them at some point.

It just occurred to me that the patch below which I just installed
covers several of those cases.


        Stefan


--- lisp/emacs-lisp/lisp-mode.el	2013-05-29 14:14:16 +0000
+++ lisp/emacs-lisp/lisp-mode.el	2013-06-03 14:23:31 +0000
@@ -809,6 +809,7 @@
 (defun eval-sexp-add-defvars (exp &optional pos)
   "Prepend EXP with all the `defvar's that precede it in the buffer.
 POS specifies the starting position where EXP was found and defaults to point."
+  (setq exp (macroexpand-all exp))      ;Eager macro-expansion.
   (if (not lexical-binding)
       exp
     (save-excursion

bug#14422: 24.3; Eager Macro Expansion

[Lars Ingebrigtsen]

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

>> The behavior is still different in a few other cases (where eager macro
>> expansion is not performed, typically M-C-x and things like that), but
>> I hope to reduce/eliminate them at some point.
>
> It just occurred to me that the patch below which I just installed
> covers several of those cases.
>
>         Stefan
>
> --- lisp/emacs-lisp/lisp-mode.el	2013-05-29 14:14:16 +0000
> +++ lisp/emacs-lisp/lisp-mode.el	2013-06-03 14:23:31 +0000
> @@ -809,6 +809,7 @@
>  (defun eval-sexp-add-defvars (exp &optional pos)
>    "Prepend EXP with all the `defvar's that precede it in the buffer.
>  POS specifies the starting position where EXP was found and defaults to point."
> +  (setq exp (macroexpand-all exp))      ;Eager macro-expansion.
>    (if (not lexical-binding)
>        exp
>      (save-excursion

It's unclear whether this fixes the reported bug?  I tried the final
test case, and I didn't get any errors (but I'm not sure I'm testing
that the right way).

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

bug#14422: 24.3; Eager Macro Expansion

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> It's unclear whether this fixes the reported bug?  I tried the final
> test case, and I didn't get any errors (but I'm not sure I'm testing
> that the right way).

More information was requested, but none was given, so I'm closing this
bug report.  If there's still problems in this area, please respond to
the debbugs address, and we'll reopen the report.

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