Spacemacs breaks if eshell is used before company is loaded

marienz commented 8 years ago

Description :octocat:

Spacemacs breaks if eshell is used before company is loaded.

Reproduction guide :beetle:

Start Emacs with the shell and auto-completion layers loaded (may also need ivy, I did not test with helm).
Open eshell (SPC ' in my configuration, SPC SPC eshell RET probably works too).
Select some non-eshell buffer (SPC b h to open the *spacemacs* buffer should do).
Try to switch buffers (SPC b b), use describe-variable, or one of many other things.

Observed behaviour: :eyes: :broken_heart: Most commands fail with "void-variable company-frontends".

Using featurep and describe-variable from eshell immediately after opening eshell shows company did load, but the global value for company-frontends did not get set:

~ λ describe-variable (quote company-frontends)
company-frontends is a variable defined in ‘company.el’.
Its value is
(company-pseudo-tooltip-unless-just-one-frontend company-echo-metadata-frontend company-preview-if-just-one-frontend)

Original value was
(company-pseudo-tooltip-unless-just-one-frontend company-preview-if-just-one-frontend company-echo-metadata-frontend)

Local in buffer *eshell-1*; globally void

I think what happened is that spacemacs//eshell-switch-company-frontend set company-frontends buffer-locally, and then company got autoloaded while that buffer-local binding was in effect. defcustom and defvar warn against this (they recommend loading files containing defvar or defcustom outside of any local bindings, which seems not very practical for autoloads). The value of eshell-mode-hook fits:

~ λ describe-variable (quote eshell-mode-hook)
eshell-mode-hook is a variable defined in ‘esh-mode.el’.
Its value is (#[0 "\302\301!\21\211\207"
     [eshell-special-chars-outside-quoting comint-file-name-quote-list make-local-variable]
     2]
 t)

Original value was nil
Local in buffer *eshell-1*; global value is
(spacemacs/init-eshell-xterm-color spacemacs//eshell-switch-company-frontend spacemacs/disable-hl-line-mode spacemacs//init-eshell eldoc-mode spacemacs//init-company-eshell-mode company-mode)

spacemacs//init-company-eshell-mode presumably triggers autoloading of company and comes after spacemacs//eshell-switch-company-frontend.

I've tried adding a with-eval-after-load 'company around the setq-local in spacemacs//eshell-switch-company-frontend, and that fixed it. company-frontend in the eshell buffer is now:

company-frontends is a variable defined in ‘company.el’.
Its value is (company-preview-frontend)
Original value was 
(company-pseudo-tooltip-unless-just-one-frontend company-preview-if-just-one-frontend company-echo-metadata-frontend)

Local in buffer *eshell-1*; global value is 
(company-pseudo-tooltip-unless-just-one-frontend company-echo-metadata-frontend company-preview-if-just-one-frontend)

However, I'm not sure if that fix is entirely correct. If company is not immediately autoloaded (while eshell is still the current buffer), the setq-local would presumably run on the wrong buffer.

Expected behaviour: :heart: :smile: No errors.

System Info :computer:

OS: gnu/linux
Emacs: 25.1.1
Spacemacs: 0.200.5
Spacemacs branch: develop (rev. 58c8fd5)
Graphic display: t
Distribution: spacemacs
Editing style: hybrid
Completion: ivy

Layers:

(ivy auto-completion better-defaults emacs-lisp
 (go :variables go-tab-width 4)
 python c-c++ markdown sql systemd shell-scripts git
 (shell :variables shell-default-shell 'eshell shell-default-height 30 shell-default-position 'bottom)
 syntax-checking version-control)

System configuration features: XPM JPEG TIFF GIF PNG RSVG SOUND DBUS NOTIFY ACL GNUTLS FREETYPE XFT ZLIB TOOLKIT_SCROLL_BARS GTK3 X11

Backtrace :paw_prints:


Debugger entered--Lisp error: (void-variable company-frontends)
  company-call-frontends(hide)
  company-cancel(abort)
  company-abort()
  ivy-overlay-cleanup()
  #[0 "\305\306\307\"\210\310 \210\300\n\311H>\204

david-sawatzke commented 7 years ago

I have the same problem:

If I use counsel-find-file directly after starting emacs, it works. But if I call eshell-command before, it fails to switch directories, with the error that company-frontend is void. When I use company-mode before calling eshell-command, everything works fine.

marienz commented 7 years ago

I was trying to make spacemacs//eshell-switch-company-frontend work without (auto)loading company, but that is a little silly. It's only used by shell/pre-init-company, which adds it to eshell-mode-hook. But it calls spacemacs|add-company-backends with :modes eshell-mode immediately before, which adds company-mode to eshell-mode-hook.

And sure enough, changing the add-hook call to (add-hook 'eshell-mode-hook 'spacemacs//eshell-switch-company-frontend t) fixes it, by putting it after company-mode on eshell-mode-hook. That's a nicer fix because we don't need to worry about setq-local running while the wrong buffer is current, but it could potentially break the same way it currently does if the order in which eshell-mode-hook is populated changes.

So adding an explicit (require 'company) to spacemacs//eshell-switch-company-frontend to make sure the global version of company-frontends is set (either instead of or in addition to adding the function to the end of eshell-mode-hook) seems wise. Worst case for that is some unnecessary load time, and currently we're incurring that load anyway (immediately afterwards, without the reordering).

joelreymont commented 7 years ago

Is there a patch or PR for this issue?

marienz commented 7 years ago

I haven't sent a PR / attached a patch mostly because I can't make up my mind on which fix is better. But if you make one or both of the following changes, that should fix it:

in layers/+tools/shell/packages.el, change (add-hook 'eshell-mode-hook 'spacemacs//eshell-switch-company-frontend) to (add-hook 'eshell-mode-hook 'spacemacs//eshell-switch-company-frontend t)
in layers/+tools/shell/funcs.el, add (require 'company) above (setq-local company-frontends '(company-preview-frontend))

mrkgnao commented 7 years ago

I have this too. I'll try out the fix above.

benreyn commented 7 years ago

I unexpectedly ran into this bug this morning, and went ahead and made a patch. See #9360

bmag commented 7 years ago

@marienz good research work, thank you. I prefer requiring company directly, because it's a 100% solution and doesn't rely on the functions' order in the hook (that may change in the future because of other reasons, who knows). @benjamreynolds I've commented on the PR accordingly.

bmag commented 7 years ago

Now fixed in develop. The fix will arrive in master branch in the next release.

Xe commented 6 years ago

Still running into this.

benreyn commented 6 years ago

@xe Are you running the current version of the develop branch? This should be fixed there (assuming new changes didnt cause a regression to my patch) but still occurring on master.

Xe commented 6 years ago

I am running git master. How do you switch to develop?

benreyn commented 6 years ago

From your .emacs.d directory run git checkout develop, but I would be careful with this and probably run spacemacs/ediff-dotfile-and-template. The develop branch is pretty far ahead of master at this point, but I do believe we are getting close to the 0.300 release. (Friendly ping @syl20bnr, is there a tracking issue or milestone for the nest release?)

io12 commented 5 years ago

I can still reproduce this.

santiweight commented 5 years ago

This still happens

github-actions[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please let us know if this issue is still valid!

syl20bnr / spacemacs