GNU bug report logs -
#24171
25.1; Bytecode returns nil instead of expected closure
Previous Next
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 24171 in the body.
You can then email your comments to 24171 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#24171
; Package
emacs
.
(Sat, 06 Aug 2016 23:23:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Michael Heerdegen <michael_heerdegen <at> web.de>
:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org
.
(Sat, 06 Aug 2016 23:23:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
Hello,
eval this defun:
(defun test ()
(let ((my-cool-fun 'dummy))
(let ((my-cool-fun
(let ((calculate (lambda () 1)))
(lambda () (setq my-cool-fun calculate))))
(return-my-cool-fun (lambda () my-cool-fun)))
(funcall my-cool-fun)
(funcall return-my-cool-fun))))
(test) evals to a closure as expected.
Compile the defun and load it. Then,
(test) -> nil
which is wrong.
Found in
https://lists.gnu.org/archive/html/help-gnu-emacs/2016-08/msg00038.html
Thanks,
Michael.
In GNU Emacs 25.1.1 (x86_64-pc-linux-gnu, GTK+ Version 3.20.6)
of 2016-08-04 built on drachen
Repository revision: 72221f51439d666d54f5d147f00ecdbb3778ab1b
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
System Description: Debian GNU/Linux testing (stretch)
Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND DBUS GSETTINGS NOTIFY
LIBXML2 FREETYPE XFT ZLIB TOOLKIT_SCROLL_BARS GTK3 X11
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#24171
; Package
emacs
.
(Sun, 07 Aug 2016 09:02:01 GMT)
Full text and
rfc822 format available.
Message #8 received at 24171 <at> debbugs.gnu.org (full text, mbox):
Michael Heerdegen <michael_heerdegen <at> web.de> writes:
> eval this defun:
>
> (defun test ()
> (let ((my-cool-fun 'dummy))
> (let ((my-cool-fun
> (let ((calculate (lambda () 1)))
> (lambda () (setq my-cool-fun calculate))))
> (return-my-cool-fun (lambda () my-cool-fun)))
> (funcall my-cool-fun)
> (funcall return-my-cool-fun))))
>
> (test) evals to a closure as expected.
ELISP> (test)
*** Eval error *** Symbol’s value as variable is void: calculate
Andreas.
--
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#24171
; Package
emacs
.
(Sun, 07 Aug 2016 09:31:01 GMT)
Full text and
rfc822 format available.
Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 2016-08-07 05:01, Andreas Schwab wrote:
> Michael Heerdegen <michael_heerdegen <at> web.de> writes:
>
>> eval this defun:
>>
>> (defun test ()
>> (let ((my-cool-fun 'dummy))
>> (let ((my-cool-fun
>> (let ((calculate (lambda () 1)))
>> (lambda () (setq my-cool-fun calculate))))
>> (return-my-cool-fun (lambda () my-cool-fun)))
>> (funcall my-cool-fun)
>> (funcall return-my-cool-fun))))
>>
>> (test) evals to a closure as expected.
>
> ELISP> (test)
> *** Eval error *** Symbol’s value as variable is void: calculate
I can reproduce this. Andreas, are you missing ;; -*- lexical-binding: t; -*- ?
Clément.
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#24171
; Package
emacs
.
(Sun, 07 Aug 2016 11:26:02 GMT)
Full text and
rfc822 format available.
Message #14 received at submit <at> debbugs.gnu.org (full text, mbox):
On So, Aug 07 2016, Clément Pit--Claudel <clement.pit <at> gmail.com> wrote:
> On 2016-08-07 05:01, Andreas Schwab wrote:
>> Michael Heerdegen <michael_heerdegen <at> web.de> writes:
>>
>>> eval this defun:
>>>
>>> (defun test ()
>>> (let ((my-cool-fun 'dummy))
>>> (let ((my-cool-fun
>>> (let ((calculate (lambda () 1)))
>>> (lambda () (setq my-cool-fun calculate))))
>>> (return-my-cool-fun (lambda () my-cool-fun)))
>>> (funcall my-cool-fun)
>>> (funcall return-my-cool-fun))))
>>>
>>> (test) evals to a closure as expected.
>>
>> ELISP> (test)
>> *** Eval error *** Symbol’s value as variable is void: calculate
>
> I can reproduce this. Andreas, are you missing ;; -*- lexical-binding: t; -*- ?
I was following the instructions.
Andreas.
--
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#24171
; Package
emacs
.
(Sun, 07 Aug 2016 14:45:01 GMT)
Full text and
rfc822 format available.
Message #17 received at 24171 <at> debbugs.gnu.org (full text, mbox):
> (defun test ()
> (let ((my-cool-fun 'dummy))
> (let ((my-cool-fun
> (let ((calculate (lambda () 1)))
> (lambda () (setq my-cool-fun calculate))))
> (return-my-cool-fun (lambda () my-cool-fun)))
> (funcall my-cool-fun)
> (funcall return-my-cool-fun))))
Good catch. It's a bug in cconv.el in the case where it decides to use
lambda-lifting. It tries to handle such name-capture (search for "(when
(memq var new-extend)" in cconv.el to see where) but doesn't catch the
above case.
Don't have a fix yet. For the above test case, you can circumvent the
bug by swapping the order of return-my-cool-fun and my-cool-fun in the
let binding.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#24171
; Package
emacs
.
(Sun, 07 Aug 2016 22:58:01 GMT)
Full text and
rfc822 format available.
Message #20 received at 24171 <at> debbugs.gnu.org (full text, mbox):
Andreas Schwab <schwab <at> linux-m68k.org> writes:
> ELISP> (test)
> *** Eval error *** Symbol’s value as variable is void: calculate
Eh, sorry, forgot to mention that the recipe needs lexical-binding.
Thanks,
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#24171
; Package
emacs
.
(Mon, 08 Aug 2016 02:05:02 GMT)
Full text and
rfc822 format available.
Message #23 received at 24171 <at> debbugs.gnu.org (full text, mbox):
You can test the problem with:
M-: (cconv-closure-convert '(let ((x 1)) (let ((x 2) (f (function (lambda (y) (+ y x))))) (funcall f x))))
where you'll see that the lambda-lifting used by cconv.el is too naive
and uses `x' to refer to the outer variable without noticing that that
variable is shadowed by the inner `x'.
The patch below should fix it and is the best I can come up with so far.
Can you confirm that it fixes the original problem?
The bug was filed against 25.1, so I have (very lightly) tested the
patch against the emacs-25 branch, but since this bug dates back to
Emacs-24.1, I think there's no hurry to fix it.
IOW I intend to install it into master. Please holler if you think it
deserves to be on emacs-25.
Stefan
diff --git a/lisp/emacs-lisp/cconv.el b/lisp/emacs-lisp/cconv.el
index 50b1fe3..2d68066 100644
--- a/lisp/emacs-lisp/cconv.el
+++ b/lisp/emacs-lisp/cconv.el
@@ -253,6 +253,32 @@ Returns a form where all lambdas don't have any free variables."
`(internal-make-closure
,args ,envector ,docstring . ,body-new)))))
+(defun cconv--remap-llv (new-env var closedsym)
+ ;; In a case such as:
+ ;; (let* ((fun (lambda (x) (+ x y))) (y 1)) (funcall fun 1))
+ ;; A naive lambda-lifting would return
+ ;; (let* ((fun (lambda (y x) (+ x y))) (y 1)) (funcall fun y 1))
+ ;; Where the external `y' is mistakenly captured by the inner one.
+ ;; So when we detect that case, we rewrite it to:
+ ;; (let* ((closed-y y) (fun (lambda (y x) (+ x y))) (y 1))
+ ;; (funcall fun closed-y 1))
+ ;; We do that even if there's no `funcall' that uses `fun' in the scope
+ ;; where `y' is shadowed by another variable because, to treat
+ ;; this case better, we'd need to traverse the tree one more time to
+ ;; collect this data, and I think that it's not worth it.
+(mapcar (lambda (mapping)
+ (if (not (eq (cadr mapping) 'apply-partially))
+ mapping
+ (cl-assert (eq (car mapping) (nth 2 mapping)))
+ `(,(car mapping)
+ apply-partially
+ ,(car mapping)
+ ,@(mapcar (lambda (arg)
+ (if (eq var arg)
+ closedsym arg))
+ (nthcdr 3 mapping)))))
+ new-env))
+
(defun cconv-convert (form env extend)
;; This function actually rewrites the tree.
"Return FORM with all its lambdas changed so they are closed.
@@ -350,34 +376,13 @@ places where they originally did not directly appear."
(if (assq var new-env) (push `(,var) new-env))
(cconv-convert value env extend)))))
- ;; The piece of code below letbinds free variables of a λ-lifted
- ;; function if they are redefined in this let, example:
- ;; (let* ((fun (lambda (x) (+ x y))) (y 1)) (funcall fun 1))
- ;; Here we can not pass y as parameter because it is redefined.
- ;; So we add a (closed-y y) declaration. We do that even if the
- ;; function is not used inside this let(*). The reason why we
- ;; ignore this case is that we can't "look forward" to see if the
- ;; function is called there or not. To treat this case better we'd
- ;; need to traverse the tree one more time to collect this data, and
- ;; I think that it's not worth it.
- (when (memq var new-extend)
- (let ((closedsym
- (make-symbol (concat "closed-" (symbol-name var)))))
- (setq new-env
- (mapcar (lambda (mapping)
- (if (not (eq (cadr mapping) 'apply-partially))
- mapping
- (cl-assert (eq (car mapping) (nth 2 mapping)))
- `(,(car mapping)
- apply-partially
- ,(car mapping)
- ,@(mapcar (lambda (arg)
- (if (eq var arg)
- closedsym arg))
- (nthcdr 3 mapping)))))
- new-env))
- (setq new-extend (remq var new-extend))
- (push closedsym new-extend)
+ (when (and (eq letsym 'let*) (memq var new-extend))
+ ;; One of the lambda-lifted vars is shadowed, so add
+ ;; a reference to the outside binding and arrange to use
+ ;; that reference.
+ (let ((closedsym (make-symbol (format "closed-%s" var))))
+ (setq new-env (cconv--remap-llv new-env var closedsym))
+ (setq new-extend (cons closedsym (remq var new-extend)))
(push `(,closedsym ,var) binders-new)))
;; We push the element after redefined free variables are
@@ -390,6 +395,21 @@ places where they originally did not directly appear."
(setq extend new-extend))
)) ; end of dolist over binders
+ (when (not (eq letsym 'let*))
+ ;; We can't do the cconv--remap-llv at the same place for let and
+ ;; let* because in the case of `let', the shadowing may occur
+ ;; before we know that the var will be in `new-extend' (bug#24171).
+ (dolist (binder binders-new)
+ (when (memq (car-safe binder) new-extend)
+ ;; One of the lambda-lifted vars is shadowed, so add
+ ;; a reference to the outside binding and arrange to use
+ ;; that reference.
+ (let* ((var (car-safe binder))
+ (closedsym (make-symbol (format "closed-%s" var))))
+ (setq new-env (cconv--remap-llv new-env var closedsym))
+ (setq new-extend (cons closedsym (remq var new-extend)))
+ (push `(,closedsym ,var) binders-new)))))
+
`(,letsym ,(nreverse binders-new)
. ,(mapcar (lambda (form)
(cconv-convert
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#24171
; Package
emacs
.
(Mon, 08 Aug 2016 10:40:01 GMT)
Full text and
rfc822 format available.
Message #26 received at 24171 <at> debbugs.gnu.org (full text, mbox):
Hi,
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> You can test the problem with:
>
> M-: (cconv-closure-convert '(let ((x 1)) (let ((x 2) (f (function (lambda (y) (+ y x))))) (funcall f x))))
>
> where you'll see that the lambda-lifting used by cconv.el is too naive
> and uses `x' to refer to the outer variable without noticing that that
> variable is shadowed by the inner `x'.
>
> The patch below should fix it and is the best I can come up with so far.
>
> Can you confirm that it fixes the original problem?
>
Yes, this fixes the original problem.
(https://lists.gnu.org/archive/html/help-gnu-emacs/2016-08/msg00038.html)
The test is performed on master branch with patch applied.
> The bug was filed against 25.1, so I have (very lightly) tested the
> patch against the emacs-25 branch, but since this bug dates back to
> Emacs-24.1, I think there's no hurry to fix it.
>
> IOW I intend to install it into master. Please holler if you think it
> deserves to be on emacs-25.
>
I have no problem with this.
>
> Stefan
>
>
>
> diff --git a/lisp/emacs-lisp/cconv.el b/lisp/emacs-lisp/cconv.el
> index 50b1fe3..2d68066 100644
> --- a/lisp/emacs-lisp/cconv.el
> +++ b/lisp/emacs-lisp/cconv.el
> @@ -253,6 +253,32 @@ Returns a form where all lambdas don't have any free variables."
> `(internal-make-closure
> ,args ,envector ,docstring . ,body-new)))))
>
> +(defun cconv--remap-llv (new-env var closedsym)
> + ;; In a case such as:
> + ;; (let* ((fun (lambda (x) (+ x y))) (y 1)) (funcall fun 1))
> + ;; A naive lambda-lifting would return
> + ;; (let* ((fun (lambda (y x) (+ x y))) (y 1)) (funcall fun y 1))
> + ;; Where the external `y' is mistakenly captured by the inner one.
> + ;; So when we detect that case, we rewrite it to:
> + ;; (let* ((closed-y y) (fun (lambda (y x) (+ x y))) (y 1))
> + ;; (funcall fun closed-y 1))
> + ;; We do that even if there's no `funcall' that uses `fun' in the scope
> + ;; where `y' is shadowed by another variable because, to treat
> + ;; this case better, we'd need to traverse the tree one more time to
> + ;; collect this data, and I think that it's not worth it.
> +(mapcar (lambda (mapping)
> + (if (not (eq (cadr mapping) 'apply-partially))
> + mapping
> + (cl-assert (eq (car mapping) (nth 2 mapping)))
> + `(,(car mapping)
> + apply-partially
> + ,(car mapping)
> + ,@(mapcar (lambda (arg)
> + (if (eq var arg)
> + closedsym arg))
> + (nthcdr 3 mapping)))))
> + new-env))
> +
> (defun cconv-convert (form env extend)
> ;; This function actually rewrites the tree.
> "Return FORM with all its lambdas changed so they are closed.
> @@ -350,34 +376,13 @@ places where they originally did not directly appear."
> (if (assq var new-env) (push `(,var) new-env))
> (cconv-convert value env extend)))))
>
> - ;; The piece of code below letbinds free variables of a λ-lifted
> - ;; function if they are redefined in this let, example:
> - ;; (let* ((fun (lambda (x) (+ x y))) (y 1)) (funcall fun 1))
> - ;; Here we can not pass y as parameter because it is redefined.
> - ;; So we add a (closed-y y) declaration. We do that even if the
> - ;; function is not used inside this let(*). The reason why we
> - ;; ignore this case is that we can't "look forward" to see if the
> - ;; function is called there or not. To treat this case better we'd
> - ;; need to traverse the tree one more time to collect this data, and
> - ;; I think that it's not worth it.
> - (when (memq var new-extend)
> - (let ((closedsym
> - (make-symbol (concat "closed-" (symbol-name var)))))
> - (setq new-env
> - (mapcar (lambda (mapping)
> - (if (not (eq (cadr mapping) 'apply-partially))
> - mapping
> - (cl-assert (eq (car mapping) (nth 2 mapping)))
> - `(,(car mapping)
> - apply-partially
> - ,(car mapping)
> - ,@(mapcar (lambda (arg)
> - (if (eq var arg)
> - closedsym arg))
> - (nthcdr 3 mapping)))))
> - new-env))
> - (setq new-extend (remq var new-extend))
> - (push closedsym new-extend)
> + (when (and (eq letsym 'let*) (memq var new-extend))
> + ;; One of the lambda-lifted vars is shadowed, so add
> + ;; a reference to the outside binding and arrange to use
> + ;; that reference.
> + (let ((closedsym (make-symbol (format "closed-%s" var))))
> + (setq new-env (cconv--remap-llv new-env var closedsym))
> + (setq new-extend (cons closedsym (remq var new-extend)))
> (push `(,closedsym ,var) binders-new)))
>
> ;; We push the element after redefined free variables are
> @@ -390,6 +395,21 @@ places where they originally did not directly appear."
> (setq extend new-extend))
> )) ; end of dolist over binders
>
> + (when (not (eq letsym 'let*))
> + ;; We can't do the cconv--remap-llv at the same place for let and
> + ;; let* because in the case of `let', the shadowing may occur
> + ;; before we know that the var will be in `new-extend' (bug#24171).
> + (dolist (binder binders-new)
> + (when (memq (car-safe binder) new-extend)
> + ;; One of the lambda-lifted vars is shadowed, so add
> + ;; a reference to the outside binding and arrange to use
> + ;; that reference.
> + (let* ((var (car-safe binder))
> + (closedsym (make-symbol (format "closed-%s" var))))
> + (setq new-env (cconv--remap-llv new-env var closedsym))
> + (setq new-extend (cons closedsym (remq var new-extend)))
> + (push `(,closedsym ,var) binders-new)))))
> +
> `(,letsym ,(nreverse binders-new)
> . ,(mapcar (lambda (form)
> (cconv-convert
Thanks,
Alex
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#24171
; Package
emacs
.
(Tue, 09 Aug 2016 03:27:01 GMT)
Full text and
rfc822 format available.
Message #29 received at 24171 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> IOW I intend to install it into master.
master sounds good to me.
But wait, what's this:
(defun test ()
(let ((x 1))
(let ((x 2)
(f (function (lambda (y) (/ y x)))))
(funcall f x))))
(test) => 2
compile...
(test) ==> 1 ?
Thanks,
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#24171
; Package
emacs
.
(Tue, 09 Aug 2016 03:49:01 GMT)
Full text and
rfc822 format available.
Message #32 received at 24171 <at> debbugs.gnu.org (full text, mbox):
Michael Heerdegen <michael_heerdegen <at> web.de> writes:
> (test) => 2
>
> compile...
>
> (test) ==> 1 ?
That was false alarm, everything is fine, I forgot to enable
lexical-binding or whatever.
Thanks for the quick fix,
Michael.
Reply sent
to
Stefan Monnier <monnier <at> iro.umontreal.ca>
:
You have taken responsibility.
(Tue, 09 Aug 2016 17:12:01 GMT)
Full text and
rfc822 format available.
Notification sent
to
Michael Heerdegen <michael_heerdegen <at> web.de>
:
bug acknowledged by developer.
(Tue, 09 Aug 2016 17:12:01 GMT)
Full text and
rfc822 format available.
Message #37 received at 24171-done <at> debbugs.gnu.org (full text, mbox):
> Yes, this fixes the original problem.
> (https://lists.gnu.org/archive/html/help-gnu-emacs/2016-08/msg00038.html)
> The test is performed on master branch with patch applied.
Thanks, installed.
Stefan
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org
.
(Wed, 07 Sep 2016 11:24:05 GMT)
Full text and
rfc822 format available.
This bug report was last modified 8 years and 338 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.