Error when using `completion-preview-mode` and `completion-preview-complete`

[verdammelt]

Minimal replication with emacs -Q (assuming sly installed at "~/.emacs/elpa/sly-20240809.2119")

Evaluate

(global-completion-preview-mode 1)
(toggle-debug-on-error)
(push "~/.emacs.d/elpa/sly-20240809.2119" load-path)
(load-library "sly")
(sly "sbcl")

Then type (form into the REPL - completion-preview-mode will show first possible match, use completion-preview-complete (bound to M-i by default) to list all completions and get the following backtrace:

Debugger entered--Lisp error: (invalid-function ("format" "formatter" "force-output"))
  ("format" "formatter" "force-output")(nil nil sly--identify)
  sly--completion-in-region-function(#<marker at 463 in *sly-mrepl for sbcl*> 466 ("format" "formatter" "force-output") nil)
  apply(sly--completion-in-region-function (#<marker at 463 in *sly-mrepl for sbcl*> 466 ("format" "formatter" "force-output") nil))
  #f(compiled-function (oldfun &rest args) #<bytecode -0x257aee4dacd0d45>)(#[128 "\301\302\300!\2\"\207" [completion-in-region-function apply default-value] 4 advice--forward] #<marker at 463 in *sly-mrepl for sbcl*> 466 ("format" "formatter" "force-output") nil)
  apply(#f(compiled-function (oldfun &rest args) #<bytecode -0x257aee4dacd0d45>) #[128 "\301\302\300!\2\"\207" [completion-in-region-function apply default-value] 4 advice--forward] (#<marker at 463 in *sly-mrepl for sbcl*> 466 ("format" "formatter" "force-output") nil))
  #f(advice #f(compiled-function (oldfun &rest args) #<bytecode -0x257aee4dacd0d45>) :around #[128 "\301\302\300!\2\"\207" [completion-in-region-function apply default-value] 4 advice--forward] ((name . sly--setup-completion)))(#<marker at 463 in *sly-mrepl for sbcl*> 466 ("format" "formatter" "force-output") nil)
  completion-in-region(#<marker at 463 in *sly-mrepl for sbcl*> 466 ("format" "formatter" "force-output") nil)
  completion-at-point()
  completion-preview-complete()
  funcall-interactively(completion-preview-complete)
  call-interactively(completion-preview-complete nil nil)
  command-execute(completion-preview-complete)

It appears that sly--completion-in-region-function expects a function as its third argument but is given a list of matches here.

What is the responsibility of this function argument? I am thinking I might at least hack together a fix with an advice...

[verdammelt]

I see in the documentation to completion-at-point-functions that such functions should expect the args (START END COLLECTION . PROPS) so perhaps if sly--completion-in-region-function is meant to be such a function it needs to be changed?

[monnier]

It appears that `sly--completion-in-region-function` expects a function as its third argument but is given a list of matches here.

That arg is often a function but can be any completion table, so I think a patch like the one below is needed [too bad GitHub is too stupid to accept Markdown over email. It could learn something from Gitlab in this respect]. Stefan ``` diff --git a/lib/sly-completion.el b/lib/sly-completion.el index d3bfcf029a..9cbec4ba79 100644 --- a/lib/sly-completion.el +++ b/lib/sly-completion.el @@ -362,9 +362,10 @@ Intended to go into `completion-at-point-functions'" sly-complete-symbol)) (add-function :around (local 'completion-in-region-function) (lambda (oldfun &rest args) - (if sly-symbol-completion-mode - (apply #'sly--completion-in-region-function args) - (apply oldfun args))) + (apply (if sly-symbol-completion-mode + #'sly--completion-in-region-function + oldfun) + args)) '((name . sly--setup-completion)))) (define-minor-mode sly-symbol-completion-mode "Fancy SLY UI for Lisp symbols" t @@ -384,7 +385,8 @@ Intended to go into `completion-at-point-functions'" ;;; (defun sly--completion-in-region-function (beg end function pred) (cond - ((funcall function nil nil 'sly--identify) + ((and (functionp function) + (funcall function nil nil 'sly--identify)) (let* ((pattern (buffer-substring-no-properties beg end)) (all (all-completions pattern function pred)) ```

[joaotavora]

@monnier or @aadcg feel free to apply the patch with a suitable commit message, if this is successfully tested. It seems function should be renamed collection in sly--completion-in-region-function

[aadcg]

@monnier since it's your patch, I'd suggest you moving forward. Let me know.

[verdammelt]

I applied this change locally and it will keep things from erroring.

However won't this circumvent all the nice features of sly-symbol-completion-mode?

[joaotavora]

However won't this circumvent all the nice features of sly-symbol-completion-mode?

It might. You're the only one testing it, afaik. Can you tell us which features you suspect or observe being circumvented?

[verdammelt]

I gave it a quick test:

1. modified function does not signal error
2. modified function results in standard completion behavior (for me that includes popping up *Completions* buffer.
3. modified function does not produced *sly-completions* buffer which has the additional information about the the type of the symbol etc.

It seems to me that the modified function is good, as it allows for completion to work well with completion-preview-mode.

[joaotavora]

modified function does not produced sly-completions buffer which has the additional information about the the type of the symbol etc.

This is a no-go, unless this behaviour is recovered when completion-preview-mode is turned off again.

[joaotavora]

IOW, people using SLY as they have been for many years shall not be impacted by this patch.

[verdammelt]

modified function does not produced sly-completions buffer which has the additional information about the the type of the symbol etc.

This is a no-go, unless this behaviour is recovered when completion-preview-mode is turned off again.

Sorry to cause confusion. The behavior appears to be the normal *sly-completions* behavior when not using completion-preview-mode.

[monnier]

@monnier or @aadcg feel free to apply the patch with a suitable commit message, if this is successfully tested. It seems `function` should be renamed `collection` in `sly--completion-in-region-function`

I pushed a slightly cleaner (IMO) version of that patch, thanks.

[aadcg]

@monnier why did you push to sly/master instead of master?

[monnier]

@monnier why did you push to `sly/master` instead of `master`?

Oops. Should be fixed now, thanks.