From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Alex Vong Newsgroups: gmane.emacs.bugs Subject: bug#24171: 25.1; Bytecode returns nil instead of expected closure Date: Mon, 08 Aug 2016 18:38:53 +0800 Message-ID: <87twevmtoi.fsf@gmail.com> References: <877fbtqyhp.fsf@web.de> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Trace: blaine.gmane.org 1470653034 1472 195.159.176.226 (8 Aug 2016 10:43:54 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 8 Aug 2016 10:43:54 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) Cc: Michael Heerdegen , 24171@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Mon Aug 08 12:43:48 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bWi27-0007hO-7t for geb-bug-gnu-emacs@m.gmane.org; Mon, 08 Aug 2016 12:43:47 +0200 Original-Received: from localhost ([::1]:56181 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bWi23-0007l7-Qm for geb-bug-gnu-emacs@m.gmane.org; Mon, 08 Aug 2016 06:43:43 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:56825) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bWhyY-0004b3-CG for bug-gnu-emacs@gnu.org; Mon, 08 Aug 2016 06:40:07 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bWhyU-0001YA-35 for bug-gnu-emacs@gnu.org; Mon, 08 Aug 2016 06:40:05 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:34048) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bWhyT-0001Y6-Vs for bug-gnu-emacs@gnu.org; Mon, 08 Aug 2016 06:40:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1bWhyT-00078y-Ri for bug-gnu-emacs@gnu.org; Mon, 08 Aug 2016 06:40:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Alex Vong Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 08 Aug 2016 10:40:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 24171 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 24171-submit@debbugs.gnu.org id=B24171.147065275027388 (code B ref 24171); Mon, 08 Aug 2016 10:40:01 +0000 Original-Received: (at 24171) by debbugs.gnu.org; 8 Aug 2016 10:39:10 +0000 Original-Received: from localhost ([127.0.0.1]:59574 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bWhxd-00077g-RG for submit@debbugs.gnu.org; Mon, 08 Aug 2016 06:39:10 -0400 Original-Received: from mail-pa0-f66.google.com ([209.85.220.66]:36551) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bWhxc-00077T-MB for 24171@debbugs.gnu.org; Mon, 08 Aug 2016 06:39:09 -0400 Original-Received: by mail-pa0-f66.google.com with SMTP id ez1so23792180pab.3 for <24171@debbugs.gnu.org>; Mon, 08 Aug 2016 03:39:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version:content-transfer-encoding; bh=r1Z775zYgjbgL7e0I8M01RDjfhJ64kjQHyRr4k/Pmgo=; b=iThguY16JyoXrdUmmoFDkm+eYf3JH0iolJ2wXYvVGBQNZqCspk+4CnGqISpg56uIn7 M1PM2WubN+ff9I9AHIcE4HeL4d7pHeyUa3edjXSkQsAdcldNeMhiHRPJ209ruhAl1OL3 6//pni70ofzSH4NuU2hWaO4xtAw/arW+IfuLuGnmLXGqeIYnfKEyyoar3SzM3Pn3puc2 R/VkZPLEkYU2gIYztIvEfMSsmF0lO7m0kkQALpwL+n1iO5WYkVkoCdT1uMiqef8VCOn4 P1LzbNLenziPS7t3vugAtYNu/sTSVL6+aLdD42Ft65rGXLLL3EFT1IQT4LjNwWp0hfIP CSOg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=r1Z775zYgjbgL7e0I8M01RDjfhJ64kjQHyRr4k/Pmgo=; b=FcU6EA8oSrGYE79Vfifpz0KlDfiuCVv9C+jOGFgKe95d3DE9O8oHREWTGan3KTQgpL Pj7LAjpKVAEGIrCs6Kv9fXNKTRvtAcazPBayjQXrPpl5lBvtEFbpLR7CRdsAam9O0hfW wN7nCzpMnxVIcHTsuGojGJDRihtoS4IEnK50tbluDvytkML8vA6bzCzH3kXQGj9BaUEH 9k8KOeAZqxfEHqJDkyb5ZoFAYEGA+ySn+PeO/UuxUftQV7Ho1iAAqBKh7yLisjLv1B4H eORxVyz5oHKNP793J56jCkmRNhMJgHyGzF3xE0PNNPBJpY3UKi00r8TgemKKmC4DLuD1 oQCw== X-Gm-Message-State: AEkoouuZI5iRYm+4ugCx7+LtnYzxZpkLttZDXYpy5d8RAZyhtT9BdBej50DFafofLJ2LJw== X-Received: by 10.66.127.10 with SMTP id nc10mr88337030pab.109.1470652742710; Mon, 08 Aug 2016 03:39:02 -0700 (PDT) Original-Received: from debian (1-64-207-052.static.netvigator.com. [1.64.207.52]) by smtp.gmail.com with ESMTPSA id 191sm46936965pfx.68.2016.08.08.03.39.00 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 08 Aug 2016 03:39:01 -0700 (PDT) In-Reply-To: (Stefan Monnier's message of "Sun, 07 Aug 2016 22:04:15 -0400") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:121960 Archived-At: Hi, Stefan Monnier writes: > You can test the problem with: > > M-: (cconv-closure-convert '(let ((x 1)) (let ((x 2) (f (function (lam= bda (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))))) >=20=20 > +(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 appea= r." > (if (assq var new-env) (push `(,var) new-env)) > (cconv-convert value env extend))))) >=20=20 > - ;; The piece of code below letbinds free variables of a =CE= =BB-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 redefine= d. > - ;; 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 bette= r we'd > - ;; need to traverse the tree one more time to collect this da= ta, 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-partia= lly)) > - mapping > - (cl-assert (eq (car mapping) (nth 2 map= ping))) > - `(,(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))) >=20=20 > ;; 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 >=20=20 > + (when (not (eq letsym 'let*)) > + ;; We can't do the cconv--remap-llv at the same place for let a= nd > + ;; let* because in the case of `let', the shadowing may occur > + ;; before we know that the var will be in `new-extend' (bug#241= 71). > + (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