bug#31783: [PATCH v2] ido.el: define a special ido-fallback variable

[Christophe Junke <junke.christophe@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3578 bytes --]

(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 <eliz@gnu.org> wrote:

> > From: Noam Postavsky <npostavs@gmail.com>
> > Date: Mon, 11 Jun 2018 08:19:03 -0400
> > Cc: 31783@debbugs.gnu.org
> >
> > Christophe Junke <junke.christophe@gmail.com> 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?
>

[-- Attachment #2: Type: text/html, Size: 5063 bytes --]