bug#46834: 28.0.50; byte-compiling the standard counter closure fails

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Pip Cet]

(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.

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Pip Cet]

[-- 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 --]

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Pip Cet]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Lars Ingebrigtsen]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Stefan Monnier]

> 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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Stefan Monnier]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Pip Cet]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Pip Cet]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Stefan Monnier]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Stefan Monnier]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Eli Zaretskii]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Pip Cet]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Stefan Monnier]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Pip Cet]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Lars Ingebrigtsen]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Pip Cet]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Lars Ingebrigtsen]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Pip Cet]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Eli Zaretskii]

> 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.)

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Pip Cet]

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

bug#46834: 28.0.50; byte-compiling the standard counter closure fails

[Eli Zaretskii]

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