From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Christophe Junke Newsgroups: gmane.emacs.bugs Subject: bug#31783: [PATCH v2] ido.el: define a special ido-fallback variable Date: Mon, 11 Jun 2018 20:52:07 +0200 Message-ID: References: <83bmcktpef.fsf@gnu.org> <20180611082340.28727-1-junke.christophe@gmail.com> <87efhdzfbc.fsf@gmail.com> <83h8m9qr5e.fsf@gnu.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="000000000000da44bf056e6240cc" X-Trace: blaine.gmane.org 1528743069 19235 195.159.176.226 (11 Jun 2018 18:51:09 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 11 Jun 2018 18:51:09 +0000 (UTC) To: 31783@debbugs.gnu.org Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Mon Jun 11 20:51:05 2018 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 1fSRuC-0004q5-8U for geb-bug-gnu-emacs@m.gmane.org; Mon, 11 Jun 2018 20:51:04 +0200 Original-Received: from localhost ([::1]:50837 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fSRwH-0005dH-TE for geb-bug-gnu-emacs@m.gmane.org; Mon, 11 Jun 2018 14:53:13 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:47199) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fSRw9-0005ap-FI for bug-gnu-emacs@gnu.org; Mon, 11 Jun 2018 14:53:07 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fSRw6-0007HW-2Q for bug-gnu-emacs@gnu.org; Mon, 11 Jun 2018 14:53:05 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:35806) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fSRw5-0007HS-TE for bug-gnu-emacs@gnu.org; Mon, 11 Jun 2018 14:53:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1fSRw5-00023u-JE for bug-gnu-emacs@gnu.org; Mon, 11 Jun 2018 14:53:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Christophe Junke Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 11 Jun 2018 18:53:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 31783 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 31783-submit@debbugs.gnu.org id=B31783.15287431657903 (code B ref 31783); Mon, 11 Jun 2018 18:53:01 +0000 Original-Received: (at 31783) by debbugs.gnu.org; 11 Jun 2018 18:52:45 +0000 Original-Received: from localhost ([127.0.0.1]:43703 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1fSRvo-00023P-Ra for submit@debbugs.gnu.org; Mon, 11 Jun 2018 14:52:45 -0400 Original-Received: from mail-ot0-f196.google.com ([74.125.82.196]:43674) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1fSRvn-00023D-CQ for 31783@debbugs.gnu.org; Mon, 11 Jun 2018 14:52:43 -0400 Original-Received: by mail-ot0-f196.google.com with SMTP id i19-v6so25039817otk.10 for <31783@debbugs.gnu.org>; Mon, 11 Jun 2018 11:52:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=Chjw5cozRh0pMGbf52Bft/HF+hl66gPnDShf6CHOwsU=; b=nTPwOg3DPZmnrHrw9sl5Iecq1Ngq3v0Mc/I5sMnep/1t42s8n1rzbjXvYN32QvgdQo 3rbL/qStjzMjOnstvkrrEMxroEt4604LtqNMs+KPGaTcrmt2yKX6SeNxtun+hbBAxZcl h3rOCTph7WEr9yWZySh4c7bEfiRprk1bGDPvcUOmahlLDT3ngb+jLyrt68AUBNwfXSVY apmoWpAHQIyECoRsundbNncF1qUG8KqQf+sfJtWLSG7LH2c8YIJ7iX6qzhv9OORj68Hi EBhzmGk9lKRwo22NbrtajEXVoT2BeOJ6lNj9qllJUYNuW/BMnDHDq7/rudGzfgWYPSp1 6qPw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=Chjw5cozRh0pMGbf52Bft/HF+hl66gPnDShf6CHOwsU=; b=Dn41O5aRTdGDCKPv10tdUo9JHE605clBNcsfq9/Vunefdx8X/0i3xOoA6GB5SFV3Bj BE7qOTcruqqTapYb9jp6RvKBPEG8N+EmWmASr6Qpp4TfCYqCY4WAcqHB9eK5IlHY0x1e JXYWgLGZfT8pDyGDUrP0JYwxDv7Vsz/1neFyGZxTYgT35/rc0B24WvFyZuPBK8vrixe1 R0zl7LIY4SjVpNJPT3mLwdaxesis8RWcebtxEl0IErf/Q/BF/v88iboT6hM9Ioy8XK6P edpaBkH44aAdZBj5hso/yPfLRuV97V5lukJJAOydFMCZLBdpELzykxH495mSRKDOEAU+ IbhA== X-Gm-Message-State: APt69E0WFCl1/+MTKGOdwBZM78ym3R4UxCBk2Yr0NYZpaapZyjRZ37Or g9ifWZb0copY4t6ieG9uH/GiafAzP8YXuu33raY1Tg== X-Google-Smtp-Source: ADUXVKJlDCrDBab+sOZCiLPP2uNabi0aH+aLegNy4RuQWMJHkoVJ8PkugVmDn+mhter+wGhdHOUe3a+VgnlRgsZ/ROg= X-Received: by 2002:a9d:caf:: with SMTP id b44-v6mr246858otb.270.1528743157448; Mon, 11 Jun 2018 11:52:37 -0700 (PDT) In-Reply-To: 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:147301 Archived-At: --000000000000da44bf056e6240cc Content-Type: text/plain; charset="UTF-8" (I forgot to cc the list, here is the message I sent previously) > I thought that was all that was needed, and I definitely didn't suggest to rename anything. > What did I miss? With respect to naming: my possibly wrong understanding of Emacs Lisp coding conventions is that special variables should be prefixed by the package's name, and that's why I (re)named the variable ido-fallback in both patches. Also, here is a summary of the original problem, as I see it. Consider an IDO function which accepts a parameter named FALLBACK, and which calls PROMPT. Inside PROMPT, we set FALLBACK to another value. For example: ;; -*- lexical-binding: nil -*- (defun prompt () (setf fallback 10)) (defun ido (fallback) (prompt) fallback) When the above is evaluated with lexical binding being nil, the fallback variable is set from within "prompt" and the result from calling ido is always 10, no matter its input argument. This is not the case if the code is evaluated with lexical-binding set to T, in which case "ido" returns the value bound to the lexical fallback variable: (ido 5) returns 5. The intent of the patches was to allow fallback to be changed again. As I learnt with the second patch, globally declaring "fallback" as a special variable with defvar, with lexical-binding set to T, does not give the same behaviour as with dynamic scoping rules: the "fallback" parameter is still bound lexically, shadowing the dynamic binding. The first patch introduces a globally scoped ido-fallback variable, different from the "fallback" argument. Yet another possibility would be to get rid of the optional "fallback" argument and keep only a global "ido-fallback", like "ido-exit", but that requires to change all call sites. I hope this is clear. Suggestions are welcome, please tell me if I misunderstood something or if there are better ways to reach the original goal. On Mon, Jun 11, 2018 at 5:28 PM, Eli Zaretskii wrote: > > From: Noam Postavsky > > Date: Mon, 11 Jun 2018 08:19:03 -0400 > > Cc: 31783@debbugs.gnu.org > > > > Christophe Junke writes: > > > > > I agree that it is simpler to rename the existing variable, and just > > > add a defvar declaration. Here is a different version of the patch > > > which does only this. > > > > > +;; Indicates which fallback command to call when ido-exit is > 'fallback. > > > +(defvar ido-fallback nil) > > > > > -(defun ido-buffer-internal (method &optional fallback prompt default > initial switch-cmd) > > > +(defun ido-buffer-internal (method &optional ido-fallback prompt > default initial switch-cmd) > > > > I believe this doesn't work, function parameters are always lexically > > bound. Compare > > > > ; -*- lexical-binding: t -*- > > (setq lexical-binding t) ; for use in *scratch* > > > > (defvar x nil) > > > > (disassemble (lambda (x y) > > (+ x y))) > > > > (let ((x 1)) > > (disassemble (lambda (y) > > (+ x y)))) > > > > So I think your first patch was fine. > > There's some misunderstanding here, most probably mine. Sorry; please > help me understand what am I missing. > > The original report said that the problem was caused by using > lexical-binding in ido.el, so I proposed to defvar the offending > variable to make it dynamically bound, which is the boilerplate > solution for all such problems. I thought that was all that was > needed, and I definitely didn't suggest to rename anything. > > What did I miss? > --000000000000da44bf056e6240cc Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
(I forgot to cc the list, here = is the message I sent previously)

> I thought that was all that was
needed, and I definitely didn't suggest to rename anything.
<= div>> What did I miss?

With respect to nami= ng: my possibly wrong understanding of
Emacs Lisp coding con= ventions is that special variables should be
prefixed by the= package's name, and that's why I (re)named
the vari= able ido-fallback in both patches.

Also, here= is a summary of the original problem, as I see it.

Consider an ID= O function which accepts a parameter named FALLBACK, and
which calls PRO= MPT. Inside PROMPT, we set FALLBACK to another
value. For example:
=C2=A0=C2=A0=C2=A0 ;; -*- lexical-binding: nil -*-

=C2=A0=C2=A0=C2= =A0 (defun prompt ()
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (setf fallback 10))<= br>=C2=A0=C2=A0=C2=A0
=C2=A0=C2=A0=C2=A0 (defun ido (fallback)
=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 (prompt)
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fall= back)

When the above is evaluated with lexical binding being nil, th= e
fallback variable is set from within "prompt" and the result= from
calling ido is always 10, no matter its input argument.

Thi= s is not the case if the code is evaluated with lexical-binding set
to T= , in which case "ido" returns the value bound to the lexical
f= allback variable: (ido 5) returns 5.

The intent of t= he patches was to allow fallback to be changed again.

As I learnt with the second patch, globally declaring "fallback&quo= t; as a
special variable with defvar, with lexical-binding set to T, doe= s not
give the same behaviour as with dynamic scoping rules: the "f= allback"
parameter is still bound lexically, shadowing the dynamic = binding.

The first patch introduces a globally scoped ido-fallback v= ariable,
different from the "fallback" argument.

Yet another possibility would be to get rid of the optional= "fallback" argument
and keep only a global "= ido-fallback", like "ido-exit", but that requires to change =
all call sites.

I hope this is clear.
<= br>
Suggestions are welcome, please tell me if I misunderstood somethin= g or
if there are better ways to reach the original goal.




<= /div>




On Mon, Jun 11, 2018 at 5:28 PM, Eli Zaretskii <eliz@gnu.org= > wrote:
> From: Noam Po= stavsky <npostav= s@gmail.com>
> Date: Mon, 11 Jun 2018 08:19:03 -0400
> Cc: 31783@d= ebbugs.gnu.org
>
> Christophe Junke <junke.christophe@gmail.com> writes:
>
> > I agree that it is simpler to rename the existing variable, and j= ust
> > add a defvar declaration. Here is a different version of the patc= h
> > which does only this.
>
> > +;; Indicates which fallback command to call when ido-exit is = 9;fallback.
> > +(defvar ido-fallback nil)
>
> > -(defun ido-buffer-internal (method &optional fallback prompt= default initial switch-cmd)
> > +(defun ido-buffer-internal (method &optional ido-fallback pr= ompt default initial switch-cmd)
>
> I believe this doesn't work, function parameters are always lexica= lly
> bound.=C2=A0 Compare
>
>=C2=A0 =C2=A0 =C2=A0; -*- lexical-binding: t -*-
>=C2=A0 =C2=A0 =C2=A0(setq lexical-binding t) ; for use in *scratch*
>
>=C2=A0 =C2=A0 =C2=A0(defvar x nil)
>
>=C2=A0 =C2=A0 =C2=A0(disassemble (lambda (x y)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (= + x y)))
>
>=C2=A0 =C2=A0 =C2=A0(let ((x 1))
>=C2=A0 =C2=A0 =C2=A0 =C2=A0(disassemble (lambda (y)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 (+ x y))))
>
> So I think your first patch was fine.

There's some misunderstanding here, most probably mine.=C2=A0 So= rry; please
help me understand what am I missing.

The original report said that the problem was caused by using
lexical-binding in ido.el, so I proposed to defvar the offending
variable to make it dynamically bound, which is the boilerplate
solution for all such problems.=C2=A0 I thought that was all that was
needed, and I definitely didn't suggest to rename anything.

What did I miss?

--000000000000da44bf056e6240cc--