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

corfu-next deletes text lines when corfu-auto-prefix is 0 or when completion-at-point on a line with a single character #350

Closed
t-c-acc opened this issue Jul 27, 2023 · 10 comments

Comments

@t-c-acc
Copy link

t-c-acc commented Jul 27, 2023

Using Debian 12 with the packaged Emacs 28.2 with GNOME

When corfu-auto is true, and corfu-auto-prefix is 0, pressing down-arrow/M-n/C-n (all bound only to corfu-next) during a completion, deletes some of the lines of text after the line being completed.

This does not happen when corfu-auto-prefix is 1.

Minimal init:

(require 'package)
(setq package-archives '(("GNU ELPA" . "https://elpa.gnu.org/packages/")
                         ("NonGNU ELPA" . "https://elpa.nongnu.org/nongnu/")
                         ("MELPA Stable" . "https://stable.melpa.org/packages/")
                         ("MELPA" . "https://melpa.org/packages/"))
        package-archive-priorities '(("GNU ELPA" . 10)
                                     ("NonGNU ELPA" . 9)
                                     ("MELPA Stable" . 5)
                                     ("MELPA" . 1)))
(package-initialize)
(unless package-archive-contents (package-refresh-contents))
(unless (package-installed-p 'use-package) (package-install 'use-package))
(require 'use-package)
(require 'use-package-ensure)
(setq use-package-always-ensure t)

(use-package corfu
    :init (global-corfu-mode)
    :custom
    (corfu-auto t)
    (corfu-auto-prefix 1))

Edit:

It also happens when trying to compete (with C-M-i) a line that has only one character, not having corfu-auto enabled

Minimal init:

Same as above but without the ":custom [...]"

@minad
Copy link
Owner

minad commented Jul 27, 2023

Hi! Can you please tell me how to reproduce this exactly? What keys do I have to press exactly step by step in the scratch buffer to trigger the problem? But anyway, a setting of corfu-auto-prefix=0 is invalid. The smallest reasonable value is 1.

@minad minad closed this as completed Jul 27, 2023
@t-c-acc t-c-acc changed the title corfu-next deletes text lines when corfu-auto-prefix is 0 corfu-next deletes text lines when corfu-auto-prefix is 0 or when completion-at-point on a line with a single character Jul 27, 2023
@t-c-acc
Copy link
Author

t-c-acc commented Jul 27, 2023

Just pressing the down arrow. It also happens without autocomplete and pressing C-M-i on a line with a single char.

(I edited the issue)

@minad
Copy link
Owner

minad commented Jul 27, 2023

I cannot reproduce the problem. Can you give me the exact buffer content, point position and key sequence needed to demonstrate the problem? Corfu should not delete any text - there is no code in Corfu which does this. Maybe you want corfu-preview-current=nil to disable the preview overlay?

@t-c-acc
Copy link
Author

t-c-acc commented Jul 27, 2023

With corfu-preview-current set to 'insert, there are lines deleted. When the next line is not empty, it deletes just one line, when there are empty lines after the completion, it deletes more.

The lines are deleted when previewing and if the completion is canceled they come back. If the completion is confirmed, they stay deleted.

Edit: I was mistaken, it does not fix it. Confirming the selection deletes lines.

@t-c-acc
Copy link
Author

t-c-acc commented Jul 27, 2023

With emacs -q --load ~/testinit.el, testinit.el being what I posted above, without corfu-auto.

  • I open a .el file (my init.el) (with M-x M-f .emacs/init.el RET)
  • I click on an empty line on the middle of the file (it is followed by other empty lines) or I press down-arrow some times.
  • I press '(' to enter a bracket.
  • I press M-C-i to open completion frame.
  • I press down-arrow (or M-n or C-n)

This deletes lines as described above.

It seems to delete lines with some meaning...

For example at the following code

(use-package modus-themes
   :config
   (
   ;; Add all your customizations prior to loading the themes
   (setq modus-themes-italic-constructs t
         modus-themes-bold-constructs t
         modus-themes-mixed-fonts t)

   ;; Maybe define some palette overrides, such as by using our presets
   (setq modus-themes-common-palette-overrides
         nil)
[...]

when I press C-M-i at the '(' after the :config it deletes up to the line before ;; Maybe [...]

In the following where everything is on the same level of indentation it only deletes one line (the (setq-default cursor-type '(bar . 2))):

(setq-default visible-bell t)
(
(setq-default cursor-type '(bar . 2))
(set-fringe-mode 4)
(scroll-bar-mode -1)

@t-c-acc
Copy link
Author

t-c-acc commented Jul 27, 2023

It is not fixed by setting corfu-preview-current to nil, after all

@t-c-acc
Copy link
Author

t-c-acc commented Jul 27, 2023

OK. I tested and the bug is with Emacs completion-at-point. Sorry to bother you.

@minad
Copy link
Owner

minad commented Jul 27, 2023

Oh wow, I can reproduce this now. It could be a bug in elisp-completion-at-point or the emacs22 completion style. I tried the following snippet in emacs -Q:

(package-initialize)
(global-corfu-mode)

(setq completion-styles '(emacs22))

  ( ;; <-- place the cursor directly after the ( and press M-TAB
;; comment
something

The problem is that the completion bounds are somehow wrong, but only if the emacs22 completion style is used. The completion bounds cross the newlines over the ;; comment. It also happens with default completion-at-point without Corfu, such that multiple lines are deleted. It does not happen with the basic, emacs21 or substring completion styles.

This must be quite an old bug. Congratulations for finding it. If you consider the problem serious enough, you may report it upstream. ;)

@t-c-acc
Copy link
Author

t-c-acc commented Jul 27, 2023

Thanks :P

I sent a bug report to bug-gnu-emacs@gnu.org. I included a link to this bug report for your observations.

@minad
Copy link
Owner

minad commented Jul 27, 2023

Thanks. Link to the bug: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=64903

catern added a commit to janestreet/emacs that referenced this issue Jan 24, 2024
elisp-completion-at-point calculates the completion region by running
backward-sexp from (point) and storing that position as the beginning,
then running forward-sexp from the beginning and storing that position
as the end.

elisp-completion-at-point does symbol completion, so the completion
region should only cover a symbol.  Therefore,
elisp-completion-at-point checks whether the beginning of the
completion region (the result from backward-sexp) is a quotation mark
or open paren, which would indicate that the region covers a string or
list, and returns nil in that case.

If point is already at the start of a sexp (e.g. immediately after an
open paren) then backward-sexp will not move point and the beginning
of the completion region would (correctly) be the original point.

forward-sexp, in this case, will move over the next sexp after point
and include it in the completion region, even if it's a string or
list.

That has several bad effects:

- An unrelated next sexp can be deleted by doing completion at the
start of some earlier sexp (bug#64903, bug#68514)

- The completion region can be very large, breaking some completion
styles; if the next sexp is large enough, completion will fail with::
(invalid-regexp "Regular expression too big")

- External completion packages can be broken by this, including corfu:
minad/corfu#350

We fix this by mirroring the check on the character at start of the
region.  We now also check if the character at the end of the
completion region is a quotation mark or close paren, and set the end
of the region equal to the beginning in that case.  This way, we avoid
including a string or list in the completion region, but still allow
completion to proceed.

* lisp/progmodes/elisp-mode.el (elisp-completion-at-point): Avoid
returning a completion region which includes a string or
list. (bug#64903)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants