* bug#46834: 28.0.50; byte-compiling the standard counter closure fails @ 2021-02-28 15:04 Pip Cet 2021-02-28 19:57 ` Pip Cet 0 siblings, 1 reply; 21+ messages in thread From: Pip Cet @ 2021-02-28 15:04 UTC (permalink / raw) To: 46834 (My apologies if this is well-known, documented, or plain stupid on my part, but I think it's an interesting gotcha. Feel free to close immediately in those cases.) Steps to reproduce from emacs -Q: Evaluate the following in a lexically-bound Emacs Lisp buffer: (byte-compile (let ((l 0)) (lambda () (cl-incf l)))) Expected result: A byte code object which will increment its return value by one every time it is called. Actual result: A byte code object which always returns 1. ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-02-28 15:04 bug#46834: 28.0.50; byte-compiling the standard counter closure fails Pip Cet @ 2021-02-28 19:57 ` Pip Cet 2021-02-28 20:34 ` Pip Cet 2021-03-01 13:16 ` Lars Ingebrigtsen 0 siblings, 2 replies; 21+ messages in thread From: Pip Cet @ 2021-02-28 19:57 UTC (permalink / raw) To: 46834 [-- Attachment #1: Type: text/plain, Size: 1339 bytes --] On Sun, Feb 28, 2021 at 6:08 PM Pip Cet <pipcet@gmail.com> wrote: > (My apologies if this is well-known, documented, or plain stupid on my > part, but I think it's an interesting gotcha. Feel free to close > immediately in those cases.) > > Steps to reproduce from emacs -Q: > Evaluate the following in a lexically-bound Emacs Lisp buffer: > > (byte-compile (let ((l 0)) (lambda () (cl-incf l)))) > > Expected result: > > A byte code object which will increment its return value by one every > time it is called. > > Actual result: > > A byte code object which always returns 1. So, investigating a little, byte-compile--reify-function does not do, and as far as I can tell has never done, what it claims to do in its docstring. (byte-compile--reify-function (let ((x 0)) (lambda () (cl-incf x)))) => (lambda nil (let ((x '0)) (setq x (1+ x)))) Obviously, the closure generated on the LHS is not equivalent to that generated by evaluating the RHS. Also, while we're there: (byte-compile--reify-function (let ((x 0)) (let ((x 1)) (lambda () x)))) => (lambda nil (let ((x '1) (x '0)) x)) The closure on the LHS returns 1 (correctly); the transformed closure generated by evaluating the RHS returns 0. Patch attached. I've looked through the generated bytecode for all of lisp/ and there appear to be no significant differences. Pip [-- Attachment #2: 0001-Compile-closures-that-modify-their-bound-vars-correc.patch --] [-- Type: application/x-patch, Size: 1448 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-02-28 19:57 ` Pip Cet @ 2021-02-28 20:34 ` Pip Cet 2021-03-01 14:23 ` Stefan Monnier 2021-03-01 13:16 ` Lars Ingebrigtsen 1 sibling, 1 reply; 21+ messages in thread From: Pip Cet @ 2021-02-28 20:34 UTC (permalink / raw) To: 46834 [-- Attachment #1: Type: text/plain, Size: 242 bytes --] On Sun, Feb 28, 2021 at 7:57 PM Pip Cet <pipcet@gmail.com> wrote: > Patch attached. I've looked through the generated bytecode for all of > lisp/ and there appear to be no significant differences. And I suppose we have to test it, too. Pip [-- Attachment #2: 0001-Compile-closures-that-modify-their-bound-vars-correc.patch --] [-- Type: text/x-patch, Size: 2886 bytes --] From 7cdb59b37b278d5f3e95b2b5b1b8758defe70acf Mon Sep 17 00:00:00 2001 From: Pip Cet <pipcet@gmail.com> Date: Sun, 28 Feb 2021 19:43:09 +0000 Subject: [PATCH] Compile closures that modify their bound vars correctly (Bug#46834) * lisp/emacs-lisp/bytecomp.el (byte-compile--reify-function): Don't move let bindings into the lambda. Don't reverse list of bindings. * test/lisp/emacs-lisp/bytecomp-tests.el (bytecomp-reify-function): Add tests. --- lisp/emacs-lisp/bytecomp.el | 8 ++------ test/lisp/emacs-lisp/bytecomp-tests.el | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el index a2fe37a1ee586..7d00b453caf1c 100644 --- a/lisp/emacs-lisp/bytecomp.el +++ b/lisp/emacs-lisp/bytecomp.el @@ -2785,16 +2785,12 @@ byte-compile--reify-function (dolist (binding env) (cond ((consp binding) - ;; We check shadowing by the args, so that the `let' can be moved - ;; within the lambda, which can then be unfolded. FIXME: Some of those - ;; bindings might be unused in `body'. - (unless (memq (car binding) args) ;Shadowed. - (push `(,(car binding) ',(cdr binding)) renv))) + (push `(,(car binding) ',(cdr binding)) renv)) ((eq binding t)) (t (push `(defvar ,binding) body)))) (if (null renv) `(lambda ,args ,@preamble ,@body) - `(lambda ,args ,@preamble (let ,(nreverse renv) ,@body))))) + `(let ,renv (lambda ,args ,@preamble ,@body))))) \f ;;;###autoload (defun byte-compile (form) diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el index fb84596ad3f40..03c267ccd0fef 100644 --- a/test/lisp/emacs-lisp/bytecomp-tests.el +++ b/test/lisp/emacs-lisp/bytecomp-tests.el @@ -1199,6 +1199,29 @@ bytecomp-local-defvar (should (equal (funcall (eval fun t)) '(c d))) (should (equal (funcall (byte-compile fun)) '(c d)))))) +(ert-deftest bytecomp-reify-function () + "Check that closures that modify their bound variables are +compiled correctly." + (cl-letf ((lexical-binding t) + ((symbol-function 'counter) nil)) + (let ((x 0)) + (defun counter () (cl-incf x)) + (should (equal (counter) 1)) + (should (equal (counter) 2)) + ;; byte compiling should not cause counter to always return the + ;; same value (bug#46834) + (byte-compile 'counter) + (should (equal (counter) 3)) + (should (equal (counter) 4))) + (let ((x 0)) + (let ((x 1)) + (defun counter () x) + (should (equal (counter) 1)) + ;; byte compiling should not cause the outer binding to shadow + ;; the inner one (bug#46834) + (byte-compile 'counter) + (should (equal (counter) 1)))))) + ;; Local Variables: ;; no-byte-compile: t ;; End: -- 2.30.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-02-28 20:34 ` Pip Cet @ 2021-03-01 14:23 ` Stefan Monnier 2021-03-01 15:01 ` Pip Cet 0 siblings, 1 reply; 21+ messages in thread From: Stefan Monnier @ 2021-03-01 14:23 UTC (permalink / raw) To: Pip Cet; +Cc: 46834 > diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el > index a2fe37a1ee586..7d00b453caf1c 100644 > --- a/lisp/emacs-lisp/bytecomp.el > +++ b/lisp/emacs-lisp/bytecomp.el > @@ -2785,16 +2785,12 @@ byte-compile--reify-function > (dolist (binding env) > (cond > ((consp binding) > - ;; We check shadowing by the args, so that the `let' can be moved > - ;; within the lambda, which can then be unfolded. FIXME: Some of those > - ;; bindings might be unused in `body'. > - (unless (memq (car binding) args) ;Shadowed. > - (push `(,(car binding) ',(cdr binding)) renv))) > + (push `(,(car binding) ',(cdr binding)) renv)) > ((eq binding t)) > (t (push `(defvar ,binding) body)))) > (if (null renv) > `(lambda ,args ,@preamble ,@body) > - `(lambda ,args ,@preamble (let ,(nreverse renv) ,@body))))) > + `(let ,renv (lambda ,args ,@preamble ,@body))))) This looks good, thanks, but it changes the nature of the output of `byte-compile` from a function value to an expression whose evaluation returns a function value. So I think we should tweak `byte-compile` so it calls `eval` on the result in this particular case. Stefan ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-01 14:23 ` Stefan Monnier @ 2021-03-01 15:01 ` Pip Cet 2021-03-01 16:02 ` Stefan Monnier 0 siblings, 1 reply; 21+ messages in thread From: Pip Cet @ 2021-03-01 15:01 UTC (permalink / raw) To: Stefan Monnier; +Cc: 46834 [-- Attachment #1: Type: text/plain, Size: 1428 bytes --] On Mon, Mar 1, 2021 at 2:23 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote: > > diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el > > index a2fe37a1ee586..7d00b453caf1c 100644 > > --- a/lisp/emacs-lisp/bytecomp.el > > +++ b/lisp/emacs-lisp/bytecomp.el > > @@ -2785,16 +2785,12 @@ byte-compile--reify-function > > (dolist (binding env) > > (cond > > ((consp binding) > > - ;; We check shadowing by the args, so that the `let' can be moved > > - ;; within the lambda, which can then be unfolded. FIXME: Some of those > > - ;; bindings might be unused in `body'. > > - (unless (memq (car binding) args) ;Shadowed. > > - (push `(,(car binding) ',(cdr binding)) renv))) > > + (push `(,(car binding) ',(cdr binding)) renv)) > > ((eq binding t)) > > (t (push `(defvar ,binding) body)))) > > (if (null renv) > > `(lambda ,args ,@preamble ,@body) > > - `(lambda ,args ,@preamble (let ,(nreverse renv) ,@body))))) > > + `(let ,renv (lambda ,args ,@preamble ,@body))))) > > This looks good, thanks, but it changes the nature of the output of > `byte-compile` from a function value to an expression whose evaluation > returns a function value. So I think we should tweak `byte-compile` so > it calls `eval` on the result in this particular case. Thanks! That's a good catch :-) Is this what you meant? Pip [-- Attachment #2: 0001-Compile-closures-that-modify-their-bound-vars-correc.patch --] [-- Type: text/x-patch, Size: 5074 bytes --] From 0b11af35fb3414fa1abb72bd33f4c6f769aa8847 Mon Sep 17 00:00:00 2001 From: Pip Cet <pipcet@gmail.com> Date: Sun, 28 Feb 2021 19:43:09 +0000 Subject: [PATCH] Compile closures that modify their bound vars correctly (Bug#46834) * lisp/emacs-lisp/bytecomp.el (byte-compile--reify-function): Don't move let bindings into the lambda. Don't reverse list of bindings. (byte-compile): Evaluate the return value if it was previously reified. * test/lisp/emacs-lisp/bytecomp-tests.el (bytecomp-reify-function): Add tests. --- lisp/emacs-lisp/bytecomp.el | 46 +++++++++++++------------- test/lisp/emacs-lisp/bytecomp-tests.el | 23 +++++++++++++ 2 files changed, 46 insertions(+), 23 deletions(-) diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el index a2fe37a1ee586..4e00fe6121e82 100644 --- a/lisp/emacs-lisp/bytecomp.el +++ b/lisp/emacs-lisp/bytecomp.el @@ -2785,16 +2785,12 @@ byte-compile--reify-function (dolist (binding env) (cond ((consp binding) - ;; We check shadowing by the args, so that the `let' can be moved - ;; within the lambda, which can then be unfolded. FIXME: Some of those - ;; bindings might be unused in `body'. - (unless (memq (car binding) args) ;Shadowed. - (push `(,(car binding) ',(cdr binding)) renv))) + (push `(,(car binding) ',(cdr binding)) renv)) ((eq binding t)) (t (push `(defvar ,binding) body)))) (if (null renv) `(lambda ,args ,@preamble ,@body) - `(lambda ,args ,@preamble (let ,(nreverse renv) ,@body))))) + `(let ,renv (lambda ,args ,@preamble ,@body))))) \f ;;;###autoload (defun byte-compile (form) @@ -2819,23 +2815,27 @@ byte-compile (if (symbolp form) form "provided")) fun) (t - (when (or (symbolp form) (eq (car-safe fun) 'closure)) - ;; `fun' is a function *value*, so try to recover its corresponding - ;; source code. - (setq lexical-binding (eq (car fun) 'closure)) - (setq fun (byte-compile--reify-function fun))) - ;; Expand macros. - (setq fun (byte-compile-preprocess fun)) - (setq fun (byte-compile-top-level fun nil 'eval)) - (if (symbolp form) - ;; byte-compile-top-level returns an *expression* equivalent to the - ;; `fun' expression, so we need to evaluate it, tho normally - ;; this is not needed because the expression is just a constant - ;; byte-code object, which is self-evaluating. - (setq fun (eval fun t))) - (if macro (push 'macro fun)) - (if (symbolp form) (fset form fun)) - fun)))))) + (let (final-eval) + (when (or (symbolp form) (eq (car-safe fun) 'closure)) + ;; `fun' is a function *value*, so try to recover its corresponding + ;; source code. + (setq lexical-binding (eq (car fun) 'closure)) + (setq fun (byte-compile--reify-function fun)) + (setq final-eval t)) + ;; Expand macros. + (setq fun (byte-compile-preprocess fun)) + (setq fun (byte-compile-top-level fun nil 'eval)) + (if (symbolp form) + ;; byte-compile-top-level returns an *expression* equivalent to the + ;; `fun' expression, so we need to evaluate it, tho normally + ;; this is not needed because the expression is just a constant + ;; byte-code object, which is self-evaluating. + (setq fun (eval fun t))) + (if final-eval + (setq fun (eval fun t))) + (if macro (push 'macro fun)) + (if (symbolp form) (fset form fun)) + fun))))))) (defun byte-compile-sexp (sexp) "Compile and return SEXP." diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el index fb84596ad3f40..03c267ccd0fef 100644 --- a/test/lisp/emacs-lisp/bytecomp-tests.el +++ b/test/lisp/emacs-lisp/bytecomp-tests.el @@ -1199,6 +1199,29 @@ bytecomp-local-defvar (should (equal (funcall (eval fun t)) '(c d))) (should (equal (funcall (byte-compile fun)) '(c d)))))) +(ert-deftest bytecomp-reify-function () + "Check that closures that modify their bound variables are +compiled correctly." + (cl-letf ((lexical-binding t) + ((symbol-function 'counter) nil)) + (let ((x 0)) + (defun counter () (cl-incf x)) + (should (equal (counter) 1)) + (should (equal (counter) 2)) + ;; byte compiling should not cause counter to always return the + ;; same value (bug#46834) + (byte-compile 'counter) + (should (equal (counter) 3)) + (should (equal (counter) 4))) + (let ((x 0)) + (let ((x 1)) + (defun counter () x) + (should (equal (counter) 1)) + ;; byte compiling should not cause the outer binding to shadow + ;; the inner one (bug#46834) + (byte-compile 'counter) + (should (equal (counter) 1)))))) + ;; Local Variables: ;; no-byte-compile: t ;; End: -- 2.30.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-01 15:01 ` Pip Cet @ 2021-03-01 16:02 ` Stefan Monnier 2021-03-02 7:00 ` Lars Ingebrigtsen 0 siblings, 1 reply; 21+ messages in thread From: Stefan Monnier @ 2021-03-01 16:02 UTC (permalink / raw) To: Pip Cet; +Cc: 46834 >> This looks good, thanks, but it changes the nature of the output of >> `byte-compile` from a function value to an expression whose evaluation >> returns a function value. So I think we should tweak `byte-compile` so >> it calls `eval` on the result in this particular case. > > Thanks! That's a good catch :-) > > Is this what you meant? Yes, LGTM, thanks, Stefan ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-01 16:02 ` Stefan Monnier @ 2021-03-02 7:00 ` Lars Ingebrigtsen 2021-03-02 7:31 ` Pip Cet 0 siblings, 1 reply; 21+ messages in thread From: Lars Ingebrigtsen @ 2021-03-02 7:00 UTC (permalink / raw) To: Stefan Monnier; +Cc: 46834, Pip Cet Stefan Monnier <monnier@iro.umontreal.ca> writes: >>> This looks good, thanks, but it changes the nature of the output of >>> `byte-compile` from a function value to an expression whose evaluation >>> returns a function value. So I think we should tweak `byte-compile` so >>> it calls `eval` on the result in this particular case. >> >> Thanks! That's a good catch :-) >> >> Is this what you meant? > > Yes, LGTM, thanks, Pip, are you pushing this fix yourself, or do you want me to do it? -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-02 7:00 ` Lars Ingebrigtsen @ 2021-03-02 7:31 ` Pip Cet 2021-03-02 7:34 ` Lars Ingebrigtsen 0 siblings, 1 reply; 21+ messages in thread From: Pip Cet @ 2021-03-02 7:31 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: 46834, Stefan Monnier On Tue, Mar 2, 2021 at 7:00 AM Lars Ingebrigtsen <larsi@gnus.org> wrote: > Pip, are you pushing this fix yourself, or do you want me to do it? It's been a while since I pushed, did I get that right? Pip ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-02 7:31 ` Pip Cet @ 2021-03-02 7:34 ` Lars Ingebrigtsen 2021-03-02 7:36 ` Pip Cet 2021-03-02 13:16 ` Eli Zaretskii 0 siblings, 2 replies; 21+ messages in thread From: Lars Ingebrigtsen @ 2021-03-02 7:34 UTC (permalink / raw) To: Pip Cet; +Cc: 46834, Stefan Monnier Pip Cet <pipcet@gmail.com> writes: > On Tue, Mar 2, 2021 at 7:00 AM Lars Ingebrigtsen <larsi@gnus.org> wrote: >> Pip, are you pushing this fix yourself, or do you want me to do it? > > It's been a while since I pushed, did I get that right? Yup; looks perfect. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-02 7:34 ` Lars Ingebrigtsen @ 2021-03-02 7:36 ` Pip Cet 2021-03-02 13:16 ` Eli Zaretskii 1 sibling, 0 replies; 21+ messages in thread From: Pip Cet @ 2021-03-02 7:36 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: 46834-done, Stefan Monnier On Tue, Mar 2, 2021 at 7:34 AM Lars Ingebrigtsen <larsi@gnus.org> wrote: > > It's been a while since I pushed, did I get that right? > > Yup; looks perfect. Thanks for checking, and I'm closing the bug. Pip ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-02 7:34 ` Lars Ingebrigtsen 2021-03-02 7:36 ` Pip Cet @ 2021-03-02 13:16 ` Eli Zaretskii 2021-03-02 13:19 ` Pip Cet 1 sibling, 1 reply; 21+ messages in thread From: Eli Zaretskii @ 2021-03-02 13:16 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: 46834, monnier, pipcet > From: Lars Ingebrigtsen <larsi@gnus.org> > Date: Tue, 02 Mar 2021 08:34:30 +0100 > Cc: 46834@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca> > > Pip Cet <pipcet@gmail.com> writes: > > > On Tue, Mar 2, 2021 at 7:00 AM Lars Ingebrigtsen <larsi@gnus.org> wrote: > >> Pip, are you pushing this fix yourself, or do you want me to do it? > > > > It's been a while since I pushed, did I get that right? > > Yup; looks perfect. Almost perfect: * lisp/emacs-lisp/bytecomp.el (byte-compile--reify-function): Don't move let bindings into the lambda. Don't reverse list of bindings. (byte-compile): Evaluate the return value if it was previously reified. The "(byte-compile):" part should have begun on a new line. (It is always better to use "C-x 4 a" or similar commands to write log entries, as then Emacs will format the messages for you.) ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-02 13:16 ` Eli Zaretskii @ 2021-03-02 13:19 ` Pip Cet 2021-03-02 13:48 ` Eli Zaretskii 0 siblings, 1 reply; 21+ messages in thread From: Pip Cet @ 2021-03-02 13:19 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 46834, Lars Ingebrigtsen, Stefan Monnier On Tue, Mar 2, 2021 at 1:16 PM Eli Zaretskii <eliz@gnu.org> wrote: > * lisp/emacs-lisp/bytecomp.el (byte-compile--reify-function): Don't > move let bindings into the lambda. Don't reverse list of > bindings. (byte-compile): Evaluate the return value if it was > previously reified. > > The "(byte-compile):" part should have begun on a new line. Thanks! > (It is always better to use "C-x 4 a" or similar commands to write log > entries, as then Emacs will format the messages for you.) I did use C-x 4 a, but I think I must have hit M-q. I won't do that again, sorry. Pip ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-02 13:19 ` Pip Cet @ 2021-03-02 13:48 ` Eli Zaretskii 0 siblings, 0 replies; 21+ messages in thread From: Eli Zaretskii @ 2021-03-02 13:48 UTC (permalink / raw) To: Pip Cet; +Cc: 46834, larsi, monnier > From: Pip Cet <pipcet@gmail.com> > Date: Tue, 2 Mar 2021 13:19:49 +0000 > Cc: Lars Ingebrigtsen <larsi@gnus.org>, 46834@debbugs.gnu.org, > Stefan Monnier <monnier@iro.umontreal.ca> > > I did use C-x 4 a, but I think I must have hit M-q. I won't do that > again, sorry. No need to feel sorry, it isn't a catastrophe. People make worse mistakes in log messages every day, sigh... ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-02-28 19:57 ` Pip Cet 2021-02-28 20:34 ` Pip Cet @ 2021-03-01 13:16 ` Lars Ingebrigtsen 2021-03-01 14:34 ` Stefan Monnier 1 sibling, 1 reply; 21+ messages in thread From: Lars Ingebrigtsen @ 2021-03-01 13:16 UTC (permalink / raw) To: Pip Cet; +Cc: 46834, Stefan Monnier Pip Cet <pipcet@gmail.com> writes: >> Steps to reproduce from emacs -Q: >> Evaluate the following in a lexically-bound Emacs Lisp buffer: >> >> (byte-compile (let ((l 0)) (lambda () (cl-incf l)))) >> >> Expected result: >> >> A byte code object which will increment its return value by one every >> time it is called. >> >> Actual result: >> >> A byte code object which always returns 1. Huh, that's such a standard example of using closures that it's surprising that we haven't tripped on this before... but I guess we don't really write code like that much in Emacs. (I can confirm that the test case doesn't work in Emacs 28.) > Patch attached. I've looked through the generated bytecode for all of > lisp/ and there appear to be no significant differences. I've added Stefan M to the CCs; perhaps he has some comments. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-01 13:16 ` Lars Ingebrigtsen @ 2021-03-01 14:34 ` Stefan Monnier 2021-03-01 15:16 ` Pip Cet 0 siblings, 1 reply; 21+ messages in thread From: Stefan Monnier @ 2021-03-01 14:34 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: 46834, Pip Cet >>> (byte-compile (let ((l 0)) (lambda () (cl-incf l)))) > Huh, that's such a standard example of using closures that it's > surprising that we haven't tripped on this before... but I guess we > don't really write code like that much in Emacs. (I can confirm that > the test case doesn't work in Emacs 28.) The problem is not in the way the actual byte compiler handles such code (e.g. when you compile a whole file, which works just fine), it's just for the special case where we pass to `byte-compile` a function *value* rather than an *expression* (in which case, `byte-compile` first needs to turn this value back into a corresponding expression). This is a very special situation, whose main use case is when you do `(byte-compile 'SYMBOL)` and which we don't handle 100% correctly, anyway. E.g. (using lexical-binding, of course): M-: (let ((x 1)) (defun counter1 () (cl-incf x)) (defun counter2 () (cl-incf x))) RET then do M-x byte-compile RET counter1 RET and then try M-: (list (counter1) (counter2) (counter1) (counter2)) and notice that the counter is not shared between the two functions :-( > I've added Stefan M to the CCs; perhaps he has some comments. Thanks, Stefan ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-01 14:34 ` Stefan Monnier @ 2021-03-01 15:16 ` Pip Cet 2021-03-01 16:15 ` Stefan Monnier 2021-03-01 16:31 ` Eli Zaretskii 0 siblings, 2 replies; 21+ messages in thread From: Pip Cet @ 2021-03-01 15:16 UTC (permalink / raw) To: Stefan Monnier; +Cc: 46834, Lars Ingebrigtsen On Mon, Mar 1, 2021 at 2:34 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote: > and notice that the counter is not shared between the two functions :-( I'd noticed that, but figured Stefan wouldn't accept "partial byte compilation" as a reasonable bug scenario :-) That said, the comment in byte-compile--reify-function is incorrect: since closures use alists and "let" uses proper lists, we can't share structure between them, so the return value will be equivalent to a snapshot of FUN, not "equal" to FUN itself. OTOH, even changing that wouldn't help as byte-compiled closures use a third format to store the bindings, IIUC. I've been meaning to ask, is there anything like an XFAIL test in our current framework? This would be an excellent use for one of those. Pip ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-01 15:16 ` Pip Cet @ 2021-03-01 16:15 ` Stefan Monnier 2021-03-01 16:41 ` Pip Cet 2021-03-01 16:31 ` Eli Zaretskii 1 sibling, 1 reply; 21+ messages in thread From: Stefan Monnier @ 2021-03-01 16:15 UTC (permalink / raw) To: Pip Cet; +Cc: 46834, Lars Ingebrigtsen >> and notice that the counter is not shared between the two functions :-( > I'd noticed that, but figured Stefan wouldn't accept "partial byte > compilation" as a reasonable bug scenario :-) I'd have to ask him, but he'd probably roll his eyes and complain about this use case yet again. > That said, the comment in byte-compile--reify-function is incorrect: [ I don't see which comment you're referring to. ] > since closures use alists and "let" uses proper lists, we can't share > structure between them, so the return value will be equivalent to a > snapshot of FUN, not "equal" to FUN itself. OTOH, even changing that > wouldn't help as byte-compiled closures use a third format to store > the bindings, IIUC. I think we could make the shared state work if we turned (closure ((VAR . VAL) ...) ...) into something like (cl-symbol-macrolet ((VAR (cdr ',CONSCELL)) ...) ...) I find the idea pretty repulsive, tho. > I've been meaning to ask, is there anything like an XFAIL test in our > current framework? This would be an excellent use for one of those. I don't know what is XFAIL, and I'm not very knowledgeable about our test infrastructure, so you might want to ask elsewhere. Stefan ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-01 16:15 ` Stefan Monnier @ 2021-03-01 16:41 ` Pip Cet 2021-03-01 17:01 ` Stefan Monnier 0 siblings, 1 reply; 21+ messages in thread From: Pip Cet @ 2021-03-01 16:41 UTC (permalink / raw) To: Stefan Monnier; +Cc: 46834, Lars Ingebrigtsen On Mon, Mar 1, 2021 at 4:15 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote: > > That said, the comment in byte-compile--reify-function is incorrect: > > [ I don't see which comment you're referring to. ] The docstring, sorry. It says the return value evaluates to FUN, which is incorrect (but, IMHO at least, this behavior is desirable and consistent, at least, with the way byte-compile changes string identities). > > since closures use alists and "let" uses proper lists, we can't share > > structure between them, so the return value will be equivalent to a > > snapshot of FUN, not "equal" to FUN itself. OTOH, even changing that > > wouldn't help as byte-compiled closures use a third format to store > > the bindings, IIUC. > > I think we could make the shared state work if we turned > > (closure ((VAR . VAL) ...) ...) > > into something like > > (cl-symbol-macrolet ((VAR (cdr ',CONSCELL)) ...) ...) > > I find the idea pretty repulsive, tho. I agree, it is repulsive. I wasn't going to mention it for that reason :-) (Also for the reason that I wasn't sure whether anything would break out of the cl-symbol-macrolet jail (no luck so far...)) Pip ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-01 16:41 ` Pip Cet @ 2021-03-01 17:01 ` Stefan Monnier 2021-03-01 17:13 ` Pip Cet 0 siblings, 1 reply; 21+ messages in thread From: Stefan Monnier @ 2021-03-01 17:01 UTC (permalink / raw) To: Pip Cet; +Cc: 46834, Lars Ingebrigtsen >> > That said, the comment in byte-compile--reify-function is incorrect: >> [ I don't see which comment you're referring to. ] > The docstring, sorry. It says the return value evaluates to FUN, which > is incorrect (but, IMHO at least, this behavior is desirable and > consistent, at least, with the way byte-compile changes string > identities). Ah, that. Yes, we could clarify that it's not 100% equivalent, but it's an internal function anyway. The doc there is only intended to explain what the function is supposed to do. > I agree, it is repulsive. I wasn't going to mention it for that reason > :-) (Also for the reason that I wasn't sure whether anything would > break out of the cl-symbol-macrolet jail (no luck so far...)) I'm not quite sure it'll always get it right, indeed, tho I think it should. Stefa ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-01 17:01 ` Stefan Monnier @ 2021-03-01 17:13 ` Pip Cet 0 siblings, 0 replies; 21+ messages in thread From: Pip Cet @ 2021-03-01 17:13 UTC (permalink / raw) To: Stefan Monnier; +Cc: 46834, Lars Ingebrigtsen On Mon, Mar 1, 2021 at 5:01 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote: > >> > That said, the comment in byte-compile--reify-function is incorrect: > >> [ I don't see which comment you're referring to. ] > > The docstring, sorry. It says the return value evaluates to FUN, which > > is incorrect (but, IMHO at least, this behavior is desirable and > > consistent, at least, with the way byte-compile changes string > > identities). > > Ah, that. Yes, we could clarify that it's not 100% equivalent, but it's > an internal function anyway. The doc there is only intended to explain > what the function is supposed to do. I think we should, generally, document that bytecode compilation (and native compilation soon) take a snapshot of the function as it stands at compilation time. You can't modify the lambda list you turned into a native function and expect the native function to change. > > I agree, it is repulsive. I wasn't going to mention it for that reason > > :-) (Also for the reason that I wasn't sure whether anything would > > break out of the cl-symbol-macrolet jail (no luck so far...)) > > I'm not quite sure it'll always get it right, indeed, tho I think > it should. Haven't found a way to break out yet, but this detects that we're in c-s-m, at least: (let ((cell (cons nil nil))) (cl-symbol-macrolet ((x (car cell))) (condition-case error (funcall #'setq x 5) (error (message "I know I'm in a c-s-m jail!"))))) And, yes, that relies on another bug... Pip ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#46834: 28.0.50; byte-compiling the standard counter closure fails 2021-03-01 15:16 ` Pip Cet 2021-03-01 16:15 ` Stefan Monnier @ 2021-03-01 16:31 ` Eli Zaretskii 1 sibling, 0 replies; 21+ messages in thread From: Eli Zaretskii @ 2021-03-01 16:31 UTC (permalink / raw) To: Pip Cet; +Cc: 46834, larsi, monnier > From: Pip Cet <pipcet@gmail.com> > Date: Mon, 1 Mar 2021 15:16:22 +0000 > Cc: 46834@debbugs.gnu.org, Lars Ingebrigtsen <larsi@gnus.org> > > I've been meaning to ask, is there anything like an XFAIL test in our > current framework? Yes, see the node "Expected Failures" in the ert manual. ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-03-02 13:48 UTC | newest] Thread overview: 21+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-02-28 15:04 bug#46834: 28.0.50; byte-compiling the standard counter closure fails Pip Cet 2021-02-28 19:57 ` Pip Cet 2021-02-28 20:34 ` Pip Cet 2021-03-01 14:23 ` Stefan Monnier 2021-03-01 15:01 ` Pip Cet 2021-03-01 16:02 ` Stefan Monnier 2021-03-02 7:00 ` Lars Ingebrigtsen 2021-03-02 7:31 ` Pip Cet 2021-03-02 7:34 ` Lars Ingebrigtsen 2021-03-02 7:36 ` Pip Cet 2021-03-02 13:16 ` Eli Zaretskii 2021-03-02 13:19 ` Pip Cet 2021-03-02 13:48 ` Eli Zaretskii 2021-03-01 13:16 ` Lars Ingebrigtsen 2021-03-01 14:34 ` Stefan Monnier 2021-03-01 15:16 ` Pip Cet 2021-03-01 16:15 ` Stefan Monnier 2021-03-01 16:41 ` Pip Cet 2021-03-01 17:01 ` Stefan Monnier 2021-03-01 17:13 ` Pip Cet 2021-03-01 16:31 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).