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

sly-read-symbol-name + helm => eval error #214

Closed
ramenbytes opened this issue Jan 26, 2019 · 20 comments
Closed

sly-read-symbol-name + helm => eval error #214

ramenbytes opened this issue Jan 26, 2019 · 20 comments

Comments

@ramenbytes
Copy link

$ emacs --version
GNU Emacs 26.1
$ sbcl --version
SBCL 1.4.13
$ emacs -Q -L <helm directory> -L <helm-core directory> -L <async directory> -L <popup directory> -L <sly directory> -l helm-config -l sly-autoloads --eval '(setq inferior-lisp-program "sbcl")' -f sly

This should start emacs with helm and sly loaded and a repl waiting. If I eval this elisp snippet:
(helm-comp-read "foo " (sly--completion-function-wrapper sly-complete-symbol-function))
I get a helm completions buffer with prompt foo , so far so good. If, however, I start typing into that buffer it will take one character and then quit. Running this in IELM yields the following error:
*** Eval error *** Quit during evaluation
Certain characters do not trigger this, they are: !@#$^&*()_=+{}[]|\,.<>?~; and the space and backquote characters.
Unless I missed one, all the other characters I have tried (U.S qwerty keyboard) result in an eval error. Typing one of the exempt characters and then typeing erroring characters is permitted, such as &rest.
I do not think it is something originating in the returned sly completion function based on some debugging, but I am not certain. I do not recall being able to replicate the error with different collection arguments to helm-comp-read.

I'm working through the backtrace and call tree trying to find out what's up, but would appreciate any help and/or input you can offer. Thanks for all your work with Sly, I really appreciate what it adds to my programming experience.

@pouar
Copy link

pouar commented Jan 28, 2019

seems to be caused by a14e64c. I managed to work around the issue by replacing (while (sit-for 30)) with (while (and (not (helm-window)) (sit-for 30)))

@joaotavora
Copy link
Owner

I don't know exactly what is at stake here, but if you must break out of the loop for helm to work, then I'd suggest it's a problem in helm. But I'll take a closer look when I can.

@joaotavora
Copy link
Owner

joaotavora commented Jan 28, 2019

In the meantime, thanks a lot for the analysis @good-as-soma and @pouar

@ramenbytes
Copy link
Author

@joaotavora, I hope to spend more time working on it and find out for sure whether it's sly or helm, and ideally find a satisfactory solution. Something I noticed that might help with your inquiry: Stepping through helm-read-pattern-maybe with edebug and testing the minibuffer input with edebug-eval-expression shows that the minibuffer input seems to start erroring after something in the body. I plan to nail exactly where when I have time. Until then, thanks for the work around and likely cause @pouar.

@pouar
Copy link

pouar commented Jan 28, 2019

I mostly found it with git bisect and came up with the workaround after looking through the Emacs documentation. Only happened inside helm, so I added a check to the function to see if Emacs had a helm window up.

@joaotavora
Copy link
Owner

Hi @good-as-soma, sorry for the delay in answering.

I am still analysing this, and part of my response to it depends on the outcome of this Emacs bug: https://debbugs.gnu.org/34394.

There is also the question the question of why you are calling helm directly on

(sly--completion-function-wrapper sly-complete-symbol-function)

Since those implementation details are supposed to be hidden behind Emacs completion-at-point-functions variable, which admittedly was not tested with Helm (only company and Emacs default completion styles). My question is, how did you first come across this bug?

@ramenbytes
Copy link
Author

Hi @joaotavora, thanks for replying, delayed or not.

I first came across the bug while using Spacemacs with the common-lisp-sly layer from @mfiano. It would occur when using functions (both interactively and programmatically) that relied on sly-read-symbol-name. From there I went through backtraces and the call tree of sly-read-symbol-name and eventually found that calling helm was being called on

(sly--completion-function-wrapper sly-complete-symbol-function)

and that trying the call in isolation reproduced the bug. When debugging, I normally reproduce it by evaluating

(sly-read-symbol-name "")

as it better reflects how I would be interacting with the code. Sorry the initial snippet and report I provided were unclear.

As an update to my debugging, I think the bug may be related to helm-after-update-hook. Setting a breakpoint for edebug on the first element (for me helm--maybe-update-keymap), evaluating (sly-read-symbol-name ""), and then stepping through the function helm--maybe-update-keymap 'till the end prevents the bug from occurring. Though for some reason it takes a few characters in the minibuffer before any completions are shown. setfing the hook to nil and then evaluating (sly-read-symbol-name "") still shows the bug though. Hope this helps with your analysis.

Thank you for your time, and good luck on the emacs bug.

@mfiano
Copy link

mfiano commented Feb 11, 2019

I am not sure if that even still works. I stopped maintaining common-lisp-sly about 6 months ago, due to the bloat and instability of that Emacs distribution.

@ramenbytes
Copy link
Author

Hmm, good to know. Thanks!

@pouar
Copy link

pouar commented Jun 27, 2019

well, M-. seems to be working again in Helm, but completions don't show up and I get No completions for "" in *Messages*, though typing the whole symbol without using completions works. Not sure if it's the same bug though.

@joaotavora
Copy link
Owner

Hi @ramenbytes and @pouar, I finally had a change to test Helm out with SLY to debug these things.

  1. I used ~/.emacs.d/elpa/helm-20190627.758/emacs-helm.sh -P <path-to-emacs-27> to launch a clean Emacs with helm
  2. I wrote and evaluated (add-to-list 'load-path "path/to/sly") (require 'sly-autoloads) in the scratch buffer
  3. I typed M-x sly (my lisp program is setup to point to Allegro CL)

I got a working SLY repl. Completions are accessible through C-c TAB. This uses SLY's own interface, not Helm's. Is that what this bug is about?

helm-sly

Anyway, I'm removing the "serious" label, but keeping the issue open while I try to understand and fix the original problem.

joaotavora added a commit that referenced this issue Jun 27, 2019
* lib/sly-completion.el (sly-read-symbol-name): Ensure
completing-read-function is the default.
@joaotavora
Copy link
Owner

I've pushed a workaround. You can now use SLY with Helm loaded, but SLY's interface for completing symbols will be used instead of Helm's (you'll still probably use Helm's for everything else).

I'll open an issue in the Helm repository for some support.

@pouar
Copy link

pouar commented Jun 27, 2019

I've been using it for sly-edit-definition, but I worked around it with (setf (alist-get 'sly-edit-definition helm-completing-read-handlers-alist) 'completing-read)

@joaotavora
Copy link
Owner

I've been using it for sly-edit-definition, but I worked around it with (setf (alist-get 'sly-edit-definition helm-completing-read-handlers-alist) 'completing-read)

Oh, ok. Can you check if my recent commit ff6771d has the same effect as your workaround?

I'm still keeping the issue open so I can eventually get Helm working properly with SLY (i.e. using Helm's UI).

@pouar
Copy link

pouar commented Jun 27, 2019

doesn't seem to

@pouar
Copy link

pouar commented Jun 27, 2019

although using

(defun pouar-completing-read-sly (prompt collection &optional predicate require-match
                                     initial-input hist def inherit-input-method)
    (sly--with-sly-minibuffer
        (let ((icomplete-mode nil)
                 (completing-read-function #'completing-read-default))
            (completing-read prompt collection predicate require-match
                initial-input hist def inherit-input-method))))
(setf (alist-get 'sly-edit-definition helm-completing-read-handlers-alist) 'pouar-completing-read-sly)

does

@joaotavora
Copy link
Owner

Can you check if my recent commit ff6771d has the same effect as your workaround?
doesn't seem to

@pouar I can't reproduce this behaviour. I just tried with the latest git heads of both SLY and Helm and any symbol query works as expected (i.e. it uses SLY's UI).

Perhaps you have a stale .elc file when you upgraded to the latest SLY head?

@pouar
Copy link

pouar commented Jun 28, 2019

ok, it works, I forgot that I installed from MELPA instead of git

joaotavora added a commit that referenced this issue Jul 1, 2019
This introduces a new "backenbd" completion style (per
completion-styles-alist) that allows SLY's function backend to be
written more concisely and comply more closely with other UI's (such
as icomplete or others), which can be used if the user turns off
sly-symbol-completion-mode.

Also motivated by emacs-helm/helm#2165.

* lib/sly-completion.el (completion-styles-alist): Add backend
completion-style.
(completion--backend-call, completion-backend-try-completion)
(completion-backend-all-completions): New helpers.
(sly--completion-function-wrapper): Simplify, and rewrite.
(sly--completion-request-completions): Remove "no completion message"
(sly-simple-completions): Add a bit of doc.
(sly--completion-function-wrapper): Rewrite.
(sly--completion-setup-target-buffer): Remove.
(sly--setup-completion): New helper.
(sly-symbol-completion-mode): New minor mode
(sly-mode-hook): Use sly--setup-completion.
(sly--completion-in-region-function): Cleanup slightly.
(sly--with-sly-minibuffer): Simplify.
(sly-minibuffer-setup-hook): Add a bit of doc.
@joaotavora
Copy link
Owner

Here's an update on the situation:

  • I pushed to master a commit that makes SLY be more compliant with Emacs's completion API.

  • It means that if you have Helm installed and activated, and when using functions that need SLY's symbol completion from the minibuffer, the Helm interface is bypassed, by default, and you are presented with SLY's own UI.

  • If one disables sly-symbol-completion-mode, SLY will use whatever Emacs has configured for completion-in-region-function. That means Helm, if configured and activated, will likely kick in. However, the conclusion of the Helm issue in which this is discussed (Allow using completion-styles in Helm and implementation of own helm style  emacs-helm/helm#2165) is clear: Helm won't let you navigate to all the symbols, since it doesn't fully respect Emacs completion API. The Helm developer doesn't have any short-term plans to address this incompatibility.

  • Other pluggables for completion-in-region-function, such as Emacs's own icomplete-mode, will work correctly.

I'm closing this, may reopen if someone thinks there is still something to be done on the SLY side.

joaotavora added a commit that referenced this issue Jul 7, 2019
This introduces a new "backend" completion style (per
completion-styles-alist) that allows SLY's function backend to be
written more concisely and comply more closely with other UI's (such
as icomplete or others), which can be used if the user turns off
sly-symbol-completion-mode.

Also motivated by emacs-helm/helm#2165.

* lib/sly-completion.el (completion-styles-alist): Add backend
completion-style.
(completion--backend-call, completion-backend-try-completion)
(completion-backend-all-completions): New helpers.
(sly--completion-function-wrapper): Simplify, and rewrite.
(sly--completion-request-completions): Remove "no completion message"
(sly-simple-completions): Add a bit of doc.
(sly--completion-function-wrapper): Rewrite.
(sly--completion-setup-target-buffer): Remove.
(sly--setup-completion): New helper.
(sly-symbol-completion-mode): New minor mode
(sly-mode-hook): Use sly--setup-completion.
(sly--completion-in-region-function): Cleanup slightly.
(sly--with-sly-minibuffer): Simplify.
(sly-minibuffer-setup-hook): Add a bit of doc.
@ramenbytes
Copy link
Author

Great! Thanks @joaotavora and @pouar for the work put into fixing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants