Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

discrepancy between `helm-read-file-name-handler-1` with 8 arguments versus `helm--completing-read-default` with 10 arguments #2085

Closed
00krishna opened this issue Aug 29, 2018 · 22 comments

Comments

@00krishna
Copy link

@00krishna 00krishna commented Aug 29, 2018

Expected behavior

I use helm with Spacemacs, but I think I found a helm issue. I also appreciate that the lead developer is just doing bug fixes now, so I don't necessarily expect a resolution to this issue. But I figured I would certainly document the problem in case anyone else has a similar issue.

When using dired mode in spacemacs 0.200.13.x on emacs 26.1 I have inconsistent
behavior with the Rename and Copy operations. I can mark a couple of files
and then click R for rename. The first time this will usually work and I get
the prompt for the directory in which to move the files. The second time I try
and do this, I get an error message

apply: Wrong number of arguments: (8 . 8), 10

There does not seem to be anything else wrong, but it makes dired unusable.

NOTE:
I did try running emacs -Q and then executing the same sequence of commands in the Reproduction section. With the vanilla config, the commands work just fine. So something in the Spacemacs config is causing this though it is not clear whether that is a conflict with an existing package or something else.

Actual behavior (from emacs-helm.sh if possible, see note at the bottom)

I just observe the error message apply: Wrong number of arguments: (8 . 8), 10.
The files are not renamed or moved.

Steps to reproduce (recipe)

  • Start Emacs
  • Open dired mode with SPC f j
  • navigate to desired directory
  • mark two files with clicking on m
  • click R to do a rename for the first time, and move the files to a new directory
  • mark two more files by clicking m
  • click R to rename the files again, and experience the error.

Backtraces if any (M-x toggle-debug-on-error)

Debugger entered--Lisp error: (wrong-number-of-arguments (8 . 8) 10)
  helm-read-file-name-handler-1("Rename jarrett-iccv-09.pdf to: " read-file-name-internal file-exists-p nil "~/Downloads/" file-name-history nil nil "dired-do-rename" "*helm-mode-dired-do-rename*")
  apply(helm-read-file-name-handler-1 ("Rename jarrett-iccv-09.pdf to: " read-file-name-internal file-exists-p nil "~/Downloads/" file-name-history nil nil "dired-do-rename" "*helm-mode-dired-do-rename*"))
  helm--completing-read-default("Rename jarrett-iccv-09.pdf to: " read-file-name-internal file-exists-p nil "~/Downloads/" file-name-history nil nil)
  apply(helm--completing-read-default ("Rename jarrett-iccv-09.pdf to: " read-file-name-internal file-exists-p nil "~/Downloads/" file-name-history nil nil))
  #f(advice-wrapper :override completing-read-default helm--completing-read-default)("Rename jarrett-iccv-09.pdf to: " read-file-name-internal file-exists-p nil "~/Downloads/" file-name-history nil nil)
  completing-read("Rename jarrett-iccv-09.pdf to: " read-file-name-internal file-exists-p nil "~/Downloads/" file-name-history nil)
  read-file-name-default("Rename jarrett-iccv-09.pdf to: " "/home/krishnab/Downloads/" nil nil nil nil)
  read-file-name("Rename jarrett-iccv-09.pdf to: " "/home/krishnab/Downloads/" nil nil nil nil)
  ido-read-file-name("Rename jarrett-iccv-09.pdf to: " "/home/krishnab/Downloads/" nil nil nil nil)
  apply(ido-read-file-name ("Rename jarrett-iccv-09.pdf to: " "/home/krishnab/Downloads/" nil nil nil nil))
  #f(advice-wrapper :override #f(advice-wrapper :override read-file-name-default helm--generic-read-file-name) ido-read-file-name)("Rename jarrett-iccv-09.pdf to: " "/home/krishnab/Downloads/" nil nil nil nil)
  read-file-name("Rename jarrett-iccv-09.pdf to: " "/home/krishnab/Downloads/" nil)
  apply(read-file-name ("Rename jarrett-iccv-09.pdf to: " "/home/krishnab/Downloads/" nil))
  dired-mark-pop-up(nil move ("jarrett-iccv-09.pdf") read-file-name "Rename jarrett-iccv-09.pdf to: " "/home/krishnab/Downloads/" nil)
  dired-mark-read-file-name("Rename %s to: " "/home/krishnab/Downloads/" move nil ("jarrett-iccv-09.pdf") nil)
  dired-do-create-files(move dired-rename-file "Move" nil t "Rename")
  dired-do-rename(nil)
  funcall-interactively(dired-do-rename nil)
  call-interactively(dired-do-rename nil nil)
  command-execute(dired-do-rename)

Describe versions of Helm, Emacs, operating system, etc.

Emacs 26.1, Spacemacs 0.200.13.x on Ubuntu Linux 16.04x64 LTS.

Are you using emacs-helm.sh to reproduce this bug (yes/no):

No. Note from the additional information below, we already traced the issue in helm. So did not
need to do additional analysis through emacs-helm.sh.

IMPORTANT NOTE

Helm provides a script named emacs-helm.sh which runs Helm in a neutral
environment: no other packages and only minimal settings.

When possible, use it to reproduce your Helm issue to ensure no other package is
interfering.

To run it, simply switch to the directory where Helm is installed and call ./emacs-helm.sh.
If necessary you can specify emacs executable path on command line with "-P" option.

Thanks.

@00krishna

This comment has been minimized.

Copy link
Author

@00krishna 00krishna commented Aug 29, 2018

I worked with a saavy elisp person through StackExchange to do some root cause analysis. Here is an excerpt from the answer.

https://stackoverflow.com/questions/52052485/emacs-dired-error-when-renaming-file-apply-wrong-number-of-arguments/52067576#52067576

[EXCERPT]
At first glance, this looks to me like a bug in the current version of helm.

Installing helm from MELPA, I see that helm-read-file-name-handler-1 accepts 8 arguments:

(helm-read-file-name-handler-1 PROMPT DIR DEFAULT-FILENAME MUSTMATCH INITIAL PREDICATE NAME BUFFER)

While helm--completing-read-default is guaranteed to call it with 10, as per the stack trace.

helm--completing-read-default looks up dired-do-rename in helm-completing-read-handlers-alist and finds that it is mapped to helm-read-file-name-handler-1. It then recognises the handler as being name-spaced as a helm function and, on that basis, calls it with 2 additional helm-specific arguments.

By default, helm-completing-read-handlers-alist includes:

(dired-do-rename . helm-read-file-name-handler-1)
(dired-do-copy . helm-read-file-name-handler-1)
(dired-do-symlink . helm-read-file-name-handler-1)
(dired-do-relsymlink . helm-read-file-name-handler-1)
(dired-do-hardlink . helm-read-file-name-handler-1))

So this issue affects all of those dired commands.

You could presuambly work around this by removing all of those.

e.g. M-x customize-option RET helm-completing-read-handlers-alist

@phil-s

This comment has been minimized.

Copy link

@phil-s phil-s commented Aug 29, 2018

Noting also that a conflict between helm and ido was the trigger for this error.

Without ido being active, helm uses helm--generic-read-file-name when these dired commands are used (rather than helm--completing-read-default), and the former function is only adding the 2 extra helm-specific arguments to an initial set of 6 giving the correct total of 8, which is obviously how it's intended to work.

To reproduce the error you can use M-x ido-everywhere, after which any of those dired commands goes through the process seen in the quoted stack trace, resulting in ido triggering completing-read which causes helm--completing-read-default to run, and the latter is sending an unexpected larger set of arguments to the handler.

It seems like each handler registered in helm-completing-read-handlers-alist may only be valid when invoked by particular helm functions, yet is still capable of being selected by other incompatible helm functions.

@thierryvolpiatto

This comment has been minimized.

Copy link
Member

@thierryvolpiatto thierryvolpiatto commented Aug 29, 2018

@phil-s

This comment has been minimized.

Copy link

@phil-s phil-s commented Aug 29, 2018

If a change was to be made (and I'm not expecting one), my first impression is that perhaps helm-completing-read-handlers-alist should be split into two or more lists, and each of the functions utilising it be modified to access the particular list which was compatible with itself.

e.g. helm--completing-read-default could only see and invoke handlers which were compatible with the way it calls handlers, and likewise for helm--generic-read-file-name and any others...

Does that sounds like a sane approach? I don't know much about helm, so I'm just speculating.

@thierryvolpiatto

This comment has been minimized.

Copy link
Member

@thierryvolpiatto thierryvolpiatto commented Aug 30, 2018

No, there is no problems with helm-completing-read-handlers-alist as it is, the point is that if ido-everywhere is enabled completing-read will be used (which is wrong for filenames) and without ido, read-file-name will be used which is what Helm expect.
The irony is that it is ido which is calling helm because helm-mode is enabled.
So which one is to blame ido or helm?
So if you use ido-mode don't use helm-mode and if you use helm-mode don't use ido but helm give you the possibility to use ido where needed which is not provided by ido.

@thierryvolpiatto

This comment has been minimized.

Copy link
Member

@thierryvolpiatto thierryvolpiatto commented Aug 30, 2018

Note also that by removing the dired commands from helm-completing-read-handlers-alist you will hit bug #1441.

So to summarize there is no bug if you don't use ido-everywhere and helm-mode at the same time,
the wrong number of arguments is that ido is calling a completing-read whereas the helm handler is a read-file-name handler, so there is nothing wrong with the number of arguments.

@thierryvolpiatto

This comment has been minimized.

Copy link
Member

@thierryvolpiatto thierryvolpiatto commented Aug 30, 2018

Note that a simple assertion at beginning of generic helm functions would be enough:

(cl-assert (null ido-everywhere)
             nil "ido-everywhere is incompatible with helm, please disable it")
@phil-s

This comment has been minimized.

Copy link

@phil-s phil-s commented Aug 30, 2018

Thanks Thierry. I was just about to suggest something similar...

Despite the non-standard name, ido-everywhere turns out to be a global minor mode defined with the standard macro; so we can test the ido-everywhere variable, react in ido-everywhere-hook, and disable it by passing an argument (ido-everywhere -1). Between all that, helm probably has the tools to prevent this conflict from happening.

@phil-s

This comment has been minimized.

Copy link

@phil-s phil-s commented Aug 30, 2018

I wondered whether the name indicated that originally ido-everywhere wasn't a minor mode, and that does turn out to be the case; but the change was 9 years ago in commit e78e280d7dbcd7b77ccba1fd1f914a45430abe0a which was released in emacs-23.2, so I imagine helm could safely rely on that.

@thierryvolpiatto

This comment has been minimized.

Copy link
Member

@thierryvolpiatto thierryvolpiatto commented Aug 30, 2018

@phil-s

This comment has been minimized.

Copy link

@phil-s phil-s commented Aug 30, 2018

Helm itself is enabled or disabled by helm-mode, no? (Apologies if I've misunderstood -- I don't actually use Helm or Ido, so I'm making some assumptions, but IIRC when I played with Helm I invoked helm-mode to get started...)

My thinking was that enabling helm-mode could potentially:

  • Disable ido-everywhere, if it was already enabled.
  • Register an ido-everywhere-hook function which would disable ido-everywhere should anything attempt to enable it.

Or can "each helm generic function" be used even when helm-mode is disabled?

In any case, "returning a good old error saying there is a conflict" sounds entirely reasonable and, as you point out, is more informative, even if it's (initially) a bit more disruptive.

@thierryvolpiatto

This comment has been minimized.

Copy link
Member

@thierryvolpiatto thierryvolpiatto commented Aug 30, 2018

@00krishna

This comment has been minimized.

Copy link
Author

@00krishna 00krishna commented Aug 31, 2018

Okay, so basically I needed to add the instruction:

  '(ido-everywhere nil)

to the custom-set-variables section of my config. Now things seem to be working. That is what was confusing me, since I did not know where to set the change. Meaning that I originally set this change higher up in my config but somewhere along the way it was getting overwritten and re-enabled. So now I set it at the very bottom and it seems to be working.

@phil-s

This comment has been minimized.

Copy link

@phil-s phil-s commented Sep 1, 2018

@00krishna, I suggest that you M-x rgrep your config for ido-everywhere and see if you can identify what's turning it on in the first place, and then you could just disable that code.

@thierryvolpiatto

This comment has been minimized.

Copy link
Member

@thierryvolpiatto thierryvolpiatto commented Sep 1, 2018

@00krishna I suggest also you separate the custom settings in another file to keep your init file clean i.e. only the settings you add yourself manually, for this use:

(setq custom-file "~/.emacs.d/.emacs-custom.el")
(load custom-file)

@phil-s Helm have much better tools to search files and buffers than rgrep ;-)

thierryvolpiatto added a commit that referenced this issue Nov 25, 2018
* helm-mode.el (helm-mode--disable-ido-maybe): New.
(helm-mode--ido-everywhere-hook):              New.
(helm-mode): Disable ido-everywhere when it is enabled and prevent
enabling it when helm-mode is already running.
(helm--generic-read-file-name): With the new behavior
of helm-mode with ido, ido-mode is unable to run when helm-mode is
enabled, fix it.
@thierryvolpiatto

This comment has been minimized.

Copy link
Member

@thierryvolpiatto thierryvolpiatto commented Nov 25, 2018

Finally found the time to work on this.
When helm-mode is enabled and user try to enabled ido, helm prevent this and send an error message.
When ido is enabled and user enable helm-mode, ido is disabled and a message notifying user about this is sent.

@kirk86

This comment has been minimized.

Copy link

@kirk86 kirk86 commented Nov 26, 2018

Why is this recently causing such a problem. Before that we were able to use both helm and ido alongside and I liked that behaviour since I use both. Now it's either one or the other. Also in my case this has affected the kill-buffer ability in a sense where I cannot properly kill all buffers if I want to. The last properly working version of helm is 20181124.1444. Why was it necessary to change the last properly working bahaviour, dunno understand.

@thierryvolpiatto

This comment has been minimized.

Copy link
Member

@thierryvolpiatto thierryvolpiatto commented Nov 26, 2018

@kirk86

This comment has been minimized.

Copy link

@kirk86 kirk86 commented Nov 26, 2018

@thierryvolpiatto I don't understand this. Please forgive my ignorance but how do you verify that there's concurrency between the both in the first place? In my case I have both modes enabled using helm version indicated in my previous post and I do not observe such a concurrency issue that your are referring to. As a matter in fact if I update to the latest helm I observe strange behaviour in my emacs, meaning some keybindings come disabled after upgrade and restart of emacs and furthermore the kill-buffer function is not working properly meaning is not possible to kill all buffers and hangs while killing buffers then it shows the kill ring buffer and when you try to kill that as well you end up in the same place, just like a cycle loop.

No if you used ido-mode with helm-mode together, it worked by chance, because one or the other was loaded before, you can easily understand that if you enable ido-mode for handling C-x b for example and also helm-mode you have concurrency between the both, probably the last enabled will predate the former.

Again forgive my ignorance but what is so catastrophic about this bug, how do we verify that is indeed a bug and why are we observing this supposedly bug only now, and I'm only saying this cause in my 2 years experience of using both never had such an issue before, both modes enabled.

Yes and it is the sane way to go to avoid bugs such as this one.

thierryvolpiatto added a commit that referenced this issue Nov 27, 2018
* helm-mode.el (helm-mode--disable-ido-maybe): Do it.
@thierryvolpiatto

This comment has been minimized.

Copy link
Member

@thierryvolpiatto thierryvolpiatto commented Nov 27, 2018

@kirk86

This comment has been minimized.

Copy link

@kirk86 kirk86 commented Nov 27, 2018

@thierryvolpiatto thanks for the reply!
I will, just for clarity in my config I also verified that the ido-mode and helm-model both come enabled by default but ido-everywhere is disabled by default.

Even when enabling ido-everywhere I don't experience any difficulties. If you want for instance you can use clean emacs installation without any config and on top of that install prelude. You will see that prelude has this configuration where ido-mode and helm-mode come enabled by default but not ido-everywhere, even though enabling ido-everywhere I didn't experience any issues.

Could it be possible that concurrency you mentioned be os specific. Haven't seen any issues on *nix systems, dunno about windows.

I can't reproduce this, please provide a recipe starting from emacs-helm.sh in another bugreport.

@thierryvolpiatto

This comment has been minimized.

Copy link
Member

@thierryvolpiatto thierryvolpiatto commented Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.