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

Previous Next

Package: emacs;

Reported by: Christophe Junke <junke.christophe <at> gmail.com>

Date: Mon, 11 Jun 2018 08:27:02 UTC

Severity: normal

Tags: fixed, patch

Merged with 31707

Fixed in version 26.2

Done: Noam Postavsky <npostavs <at> gmail.com>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 31783 in the body.
You can then email your comments to 31783 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#31783; Package emacs. (Mon, 11 Jun 2018 08:27:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Christophe Junke <junke.christophe <at> gmail.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Mon, 11 Jun 2018 08:27:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Christophe Junke <junke.christophe <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: Christophe Junke <junke.christophe <at> gmail.com>
Subject: [PATCH v2] ido.el: define a special ido-fallback variable
Date: Mon, 11 Jun 2018 10:23:40 +0200
- Introduce a global, special ido-fallback variable
- Rename existing occurrences of fallback to ido-fallback

This allows to retain the behaviour that was available prior to
introducing per-file lexical-scope, namely that other modules could
change what fallback command to call when ido-exit is set to
'fallback.
---

Hi,

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.

Thank you for your review and your time.

 lisp/ido.el | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/lisp/ido.el b/lisp/ido.el
index 705e7dd630..358e856d4a 100644
--- a/lisp/ido.el
+++ b/lisp/ido.el
@@ -1242,6 +1242,9 @@ Only used if `ido-use-virtual-buffers' is non-nil.")
 ;; Dynamically bound in ido-read-internal.
 (defvar ido-completing-read)
 
+;; Indicates which fallback command to call when ido-exit is 'fallback.
+(defvar ido-fallback nil)
+
 ;;; FUNCTIONS
 
 (defun ido-active (&optional merge)
@@ -2213,13 +2216,13 @@ If cursor is not at the end of the user input, move to end of input."
     (exit-minibuffer)))
 
 ;;; MAIN FUNCTIONS
-(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)
   ;; Internal function for ido-switch-buffer and friends
   (if (not ido-mode)
       (progn
 	(run-hook-with-args 'ido-before-fallback-functions
-			    (or fallback 'switch-to-buffer))
-	(call-interactively (or fallback 'switch-to-buffer)))
+			    (or ido-fallback 'switch-to-buffer))
+	(call-interactively (or ido-fallback 'switch-to-buffer)))
     (let* ((ido-context-switch-command switch-cmd)
 	   (ido-current-directory nil)
 	   (ido-directory-nonreadable nil)
@@ -2245,7 +2248,7 @@ If cursor is not at the end of the user input, move to end of input."
 
        ((eq ido-exit 'fallback)
 	(let ((read-buffer-function nil))
-	  (setq this-command (or fallback 'switch-to-buffer))
+	  (setq this-command (or ido-fallback 'switch-to-buffer))
 	  (run-hook-with-args 'ido-before-fallback-functions this-command)
 	  (call-interactively this-command)))
 
@@ -2337,7 +2340,7 @@ If cursor is not at the end of the user input, move to end of input."
   ;; Add final slash to result in case it was missing from DEFAULT-DIRECTORY.
   (ido-final-slash (expand-file-name (or dir default-directory)) t))
 
-(defun ido-file-internal (method &optional fallback default prompt item initial switch-cmd)
+(defun ido-file-internal (method &optional ido-fallback default prompt item initial switch-cmd)
   ;; Internal function for ido-find-file and friends
   (unless item
     (setq item 'file))
@@ -2412,7 +2415,7 @@ If cursor is not at the end of the user input, move to end of input."
 	;; we don't want to change directory of current buffer.
 	(let ((default-directory ido-current-directory)
 	      (read-file-name-function nil))
-	  (setq this-command (or fallback 'find-file))
+	  (setq this-command (or ido-fallback 'find-file))
 	  (run-hook-with-args 'ido-before-fallback-functions this-command)
 	  (call-interactively this-command)))
 
@@ -2496,10 +2499,10 @@ If cursor is not at the end of the user input, move to end of input."
        ((eq method 'read-only)
 	(ido-record-work-file filename)
 	(setq filename (concat ido-current-directory filename))
-	(ido-record-command fallback filename)
+	(ido-record-command ido-fallback filename)
 	(ido-record-work-directory)
-	(run-hook-with-args 'ido-before-fallback-functions fallback)
-	(funcall fallback filename))
+	(run-hook-with-args 'ido-before-fallback-functions ido-fallback)
+	(funcall ido-fallback filename))
 
        ((eq method 'insert)
 	(ido-record-work-file filename)
-- 
2.14.1





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#31783; Package emacs. (Mon, 11 Jun 2018 12:20:02 GMT) Full text and rfc822 format available.

Message #8 received at 31783 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Christophe Junke <junke.christophe <at> gmail.com>
Cc: 31783 <at> debbugs.gnu.org
Subject: Re: bug#31783: [PATCH v2] ido.el: define a special ido-fallback
 variable
Date: Mon, 11 Jun 2018 08:19:03 -0400
merge 31783 31707
quit

Christophe Junke <junke.christophe <at> 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.




Merged 31707 31783. Request was from Noam Postavsky <npostavs <at> gmail.com> to control <at> debbugs.gnu.org. (Mon, 11 Jun 2018 12:20:02 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#31783; Package emacs. (Mon, 11 Jun 2018 12:56:02 GMT) Full text and rfc822 format available.

Message #13 received at 31783 <at> debbugs.gnu.org (full text, mbox):

From: Christophe Junke <junke.christophe <at> gmail.com>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 31783 <at> debbugs.gnu.org
Subject: Re: bug#31783: [PATCH v2] ido.el: define a special ido-fallback
 variable
Date: Mon, 11 Jun 2018 14:54:52 +0200
[Message part 1 (text/plain, inline)]
 > I believe this doesn't work, function parameters are always lexically
bound.

Indeed, there is even a warning from disassemble about the lexical argument
shadowing the dynamic one.

Thank you for noticing that.

> So I think your first patch was fine.

Fine.

To recap, the first patch also redefines "ido-fallback-command" so that it
accepts an optional parameter (the fallback command).
Is that ok for you? The idea was to let Ido be exited with a custom
fallback command through a function, and without requiring other packages
to set a variable directly.


Thank you.




On Mon, Jun 11, 2018 at 2:19 PM, Noam Postavsky <npostavs <at> gmail.com> wrote:

> merge 31783 31707
> quit
>
> Christophe Junke <junke.christophe <at> 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.
>
[Message part 2 (text/html, inline)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#31783; Package emacs. (Mon, 11 Jun 2018 15:29:01 GMT) Full text and rfc822 format available.

Message #16 received at 31783 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: junke.christophe <at> gmail.com, 31783 <at> debbugs.gnu.org
Subject: Re: bug#31783: [PATCH v2] ido.el: define a special ido-fallback
 variable
Date: Mon, 11 Jun 2018 18:28:13 +0300
> From: Noam Postavsky <npostavs <at> gmail.com>
> Date: Mon, 11 Jun 2018 08:19:03 -0400
> Cc: 31783 <at> debbugs.gnu.org
> 
> Christophe Junke <junke.christophe <at> 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?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#31783; Package emacs. (Mon, 11 Jun 2018 16:56:02 GMT) Full text and rfc822 format available.

Message #19 received at 31783 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Christophe Junke <junke.christophe <at> gmail.com>
Cc: 31783 <at> debbugs.gnu.org, Noam Postavsky <npostavs <at> gmail.com>
Subject: Re: bug#31783: [PATCH v2] ido.el: define a special ido-fallback
 variable
Date: Mon, 11 Jun 2018 19:55:42 +0300
> From: Christophe Junke <junke.christophe <at> gmail.com>
> Date: Mon, 11 Jun 2018 18:16:07 +0200
> 
> 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.

Ah, okay.  Thanks for explaining this, I agree that your original
change was TRT.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#31783; Package emacs. (Mon, 11 Jun 2018 18:53:01 GMT) Full text and rfc822 format available.

Message #22 received at 31783 <at> debbugs.gnu.org (full text, mbox):

From: Christophe Junke <junke.christophe <at> gmail.com>
To: 31783 <at> debbugs.gnu.org
Subject: [PATCH v2] ido.el: define a special ido-fallback variable
Date: Mon, 11 Jun 2018 20:52:07 +0200
[Message part 1 (text/plain, inline)]
(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 <at> gnu.org> wrote:

> > From: Noam Postavsky <npostavs <at> gmail.com>
> > Date: Mon, 11 Jun 2018 08:19:03 -0400
> > Cc: 31783 <at> debbugs.gnu.org
> >
> > Christophe Junke <junke.christophe <at> 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?
>
[Message part 2 (text/html, inline)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#31783; Package emacs. (Fri, 22 Jun 2018 00:35:02 GMT) Full text and rfc822 format available.

Message #25 received at 31783 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Christophe Junke <junke.christophe <at> gmail.com>, 31783 <at> debbugs.gnu.org
Subject: Re: bug#31783: [PATCH v2] ido.el: define a special ido-fallback
 variable
Date: Thu, 21 Jun 2018 20:34:10 -0400
Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Christophe Junke <junke.christophe <at> gmail.com>
>> Date: Mon, 11 Jun 2018 18:16:07 +0200
>> 
>> 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.
>
> Ah, okay.  Thanks for explaining this, I agree that your original
> change was TRT.

So should this go to emacs-26?  It's introducing a new feature, but that
makes up for a semi-accidental feature that was lost.

And what's the copyright situation?





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#31783; Package emacs. (Fri, 22 Jun 2018 06:35:01 GMT) Full text and rfc822 format available.

Message #28 received at 31783 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: junke.christophe <at> gmail.com, 31783 <at> debbugs.gnu.org
Subject: Re: bug#31783: [PATCH v2] ido.el: define a special ido-fallback
 variable
Date: Fri, 22 Jun 2018 09:34:58 +0300
> From: Noam Postavsky <npostavs <at> gmail.com>
> Cc: Christophe Junke <junke.christophe <at> gmail.com>,  31783 <at> debbugs.gnu.org
> Date: Thu, 21 Jun 2018 20:34:10 -0400
> 
> > Ah, okay.  Thanks for explaining this, I agree that your original
> > change was TRT.
> 
> So should this go to emacs-26?

Yes, please.

> It's introducing a new feature, but that makes up for a
> semi-accidental feature that was lost.

What new feature is being introduced there?  I couldn't see anything
new, just fixing a breakage.

> And what's the copyright situation?

There's no assignment.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#31783; Package emacs. (Fri, 22 Jun 2018 08:26:01 GMT) Full text and rfc822 format available.

Message #31 received at 31783 <at> debbugs.gnu.org (full text, mbox):

From: Christophe Junke <junke.christophe <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 31783 <at> debbugs.gnu.org, Noam Postavsky <npostavs <at> gmail.com>
Subject: Re: bug#31783: [PATCH v2] ido.el: define a special ido-fallback
 variable
Date: Fri, 22 Jun 2018 10:24:44 +0200
[Message part 1 (text/plain, inline)]
>
> > And what's the copyright situation?
>
> There's no assignment.
>

I am willing to assign copyright for this change (and future changes).
Can you send me a form?
Thanks.
[Message part 2 (text/html, inline)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#31783; Package emacs. (Fri, 22 Jun 2018 09:03:02 GMT) Full text and rfc822 format available.

Message #34 received at 31783 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Christophe Junke <junke.christophe <at> gmail.com>
Cc: 31783 <at> debbugs.gnu.org, npostavs <at> gmail.com
Subject: Re: bug#31783: [PATCH v2] ido.el: define a special ido-fallback
 variable
Date: Fri, 22 Jun 2018 12:02:03 +0300
> From: Christophe Junke <junke.christophe <at> gmail.com>
> Date: Fri, 22 Jun 2018 10:24:44 +0200
> Cc: Noam Postavsky <npostavs <at> gmail.com>, 31783 <at> debbugs.gnu.org
> 
>  > And what's the copyright situation?
> 
>  There's no assignment.
> 
> I am willing to assign copyright for this change (and future changes).
> Can you send me a form?

For sent off-list.  However, this patch is small enough to be accepted
without waiting for the legal paperwork to be completed.

Thanks.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#31783; Package emacs. (Fri, 22 Jun 2018 11:33:01 GMT) Full text and rfc822 format available.

Message #37 received at 31783 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: junke.christophe <at> gmail.com, 31783 <at> debbugs.gnu.org
Subject: Re: bug#31783: [PATCH v2] ido.el: define a special ido-fallback
 variable
Date: Fri, 22 Jun 2018 07:32:14 -0400
Eli Zaretskii <eliz <at> gnu.org> writes:
>
> What new feature is being introduced there?  I couldn't see anything
> new, just fixing a breakage.

The optional parameter to ido-fallback-command is new.

The ido-fallback is sort of a new variable, although it's arguably just
a proper prefixing of the old `fallback' one.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#31783; Package emacs. (Fri, 22 Jun 2018 12:46:02 GMT) Full text and rfc822 format available.

Message #40 received at 31783 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: junke.christophe <at> gmail.com, 31783 <at> debbugs.gnu.org
Subject: Re: bug#31783: [PATCH v2] ido.el: define a special ido-fallback
 variable
Date: Fri, 22 Jun 2018 15:45:20 +0300
> From: Noam Postavsky <npostavs <at> gmail.com>
> Cc: junke.christophe <at> gmail.com,  31783 <at> debbugs.gnu.org
> Date: Fri, 22 Jun 2018 07:32:14 -0400
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> >
> > What new feature is being introduced there?  I couldn't see anything
> > new, just fixing a breakage.
> 
> The optional parameter to ido-fallback-command is new.
> 
> The ido-fallback is sort of a new variable, although it's arguably just
> a proper prefixing of the old `fallback' one.

Maybe I'm confused about which patch will eventually go in.  Can you
show it?




Added tag(s) fixed. Request was from Noam Postavsky <npostavs <at> gmail.com> to control <at> debbugs.gnu.org. (Tue, 26 Jun 2018 00:41:02 GMT) Full text and rfc822 format available.

bug marked as fixed in version 26.2, send any further explanations to 31707 <at> debbugs.gnu.org and Christophe Junke <junke.christophe <at> gmail.com> Request was from Noam Postavsky <npostavs <at> gmail.com> to control <at> debbugs.gnu.org. (Tue, 26 Jun 2018 00:41:02 GMT) Full text and rfc822 format available.

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Tue, 24 Jul 2018 11:24:04 GMT) Full text and rfc822 format available.

This bug report was last modified 5 years and 249 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.