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

Re: Rewrite merlin-completion-at-point integration to be faster and better #1827

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

voodoos
Copy link
Collaborator

@voodoos voodoos commented Sep 18, 2024

The first merge tentative of @catern's rewrite of completion-at-point (CAP) lead us to discover a few issues that need fixing before we release it. I reverted the commits and this PR reintroduces changes made by @tmcgilchrist and myself to move the tests in a separate file, fix linting issues and enable testing on version 27 to 29.

There are two issues with the current status of this PR:

  • Even when using the compat module tests do not pass on Emacs 27 and 28.
  • Using the compat module breaks the assumption made by opam user-setup that packages are standalone.

This last issue could be fixed by: 1. shipping the compat package with Merlin or 2. Rewriting our own replacement for the missing features.

catern and others added 5 commits September 18, 2024 12:36
The merlin-completion-at-point integration before was simple and slow.

The new version is much faster and much more featureful:

- Completions are requested for an entire module at once and filtered
locally in Emacs rather than doing the filtering on the Merlin side.

- Completions are cached based on the OCaml atom being completed, so
they are re-used instead of re-requested when a new character is
typed.

- Those completions are cached for a given position inside an OCaml
atom, so that if a user types Li<TAB>ma<TAB> to complete "List.map"
and then decides they actually want the module "Labels", when they
delete the "ist.map" part and hit <TAB>, they'll use the
previously-requested completions.

- We avoid updating Merlin with the new buffer contents as completion
proceeds, so that Merlin doesn't need to re-parse and re-type-check,
substantially improving performance in expensive
files.  (merlin-cap--omit-bounds)

- Completion requests are handled asynchronously and reused, so that
if completion is interrupted and then resumed, we're able to use the
results of the previous completion request.  This makes completion UIs
which use while-no-input (like corfu-mode) much more performant.

- Completion is wrapped in while-no-input when non-essential is set;
this makes completion UIs which don't use while-no-input (like
company-mode) much more responsive.

- Completions are sorted more intelligently: if they're a constructor
or variant or label, they're likely to be more relevant to the user,
so they're sorted first.

- Module names in completions are suffixed with a ., matching Emacs
behavior for file name completion (where directories are suffixed with
a /); this makes completion of module paths much more fluent, since
there's no need to hit . after every module name.

- We use completion boundaries, so the built-in Emacs
partial-completion feature now works: if the user types "Li.ma<TAB>",
it will complete to "List.map".

- Likewise, partial-completion will expand * as a glob, so if the user
types "Deferred.*.map<TAB>" they will be presented with every module
in "Deferred." which contains the method "map"

There are also several tests now, testing the new functionality.
Replace pos-eol from Emacs 29 with older line-end-position function
that exists in Emacs 28 and older.
- Rename position-symbol local value
- Make merlin-cap require the compat package.
- Add compat to merlin mode
Copy link
Contributor

@purcell purcell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of comments re. the packaging aspects

.github/workflows/emacs-lint.yml Outdated Show resolved Hide resolved
emacs/check.sh Outdated

ELS_TO_CHECK=*.el
# To reduce the amount of false positives we only package-lint files
# that are actual installable packages.
PKGS_TO_CHECK="merlin.el merlin-ac.el merlin-company.el merlin-iedit.el"
PKGS_TO_CHECK="merlin.el merlin-ac.el merlin-company.el merlin-iedit.el merlin-cap.el tests/merlin-cap-test.el"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right any more. From a packaging point of view, merlin, merlin-ac, merlin-company and merlin-iedit are separate packages, because their dependencies are different. Each is built into a separate MELPA package, for granular installation.

However, presumably the intention is that merlin-cap will be part of the base merlin package, since it doesn't depend on anything opinionated that ships separately from Emacs. So it wouldn't be appropriate to run package-lint on merlin-cap separately: instead, run package-lint on both merlin-cap.el and merlin.el together, with the package-lint-main-file variable set to merlin.el.

And merlin-cap-test.el doesn't need package-linting, because it's not shipped in the package.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I did that in a72fd55.

There a still spurious merlin-cap.el: error: You should depend on (emacs "28.1") if you need string-search'.` even if we use the compat module.

merlin.el:477:0: error: `merlin/unmake-point' contains a non-standard separator `/', use hyphens instead (see Elisp Coding Conventions).
merlin.el:608:0: error: `merlin/call' contains a non-standard separator `/', use hyphens instead (see Elisp Coding Conventions).
merlin.el:1194:0: error: `merlin/display-in-type-buffer' contains a non-standard separator `/', use hyphens instead (see Elisp Coding Conventions).
merlin-cap.el:10:0: error: Package-Requires outside the main file have no effect.
merlin-cap.el:341:13: error: You should depend on (emacs "28.1") if you need `string-search'.
merlin-cap.el:356:19: error: You should depend on (emacs "28.1") if you need `string-search'.
merlin-cap.el:376:18: error: You should depend on (emacs "28.1") if you need `string-search'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I was overdue to tag a new package-lint release, so this spurious message was due to the build picking up an old version. Once MELPA has rebuilt the new package-lint version this complaint should go away.

emacs/merlin-xref.el Outdated Show resolved Hide resolved
emacs/tests/merlin-cap-test.el Outdated Show resolved Hide resolved
request was for and whose cdr is the request buffer. Return nil
if no pending request exists, or its inputs do not match."
(let ((buf (current-buffer)))
(with-current-buffer (get-buffer-create " *merlin-async-proc-stdout*" t)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get-buffer-create changed the number of arguments between 27 * and 28 ** which is causing linting issues.

(with-current-buffer messages-buffer-name
(save-excursion
(forward-line -1)
(buffer-substring (point) (line-end-position)))))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed pos-eol to line-end-position this might not be necessary with compat package?

Suggested change
(buffer-substring (point) (line-end-position)))))
(buffer-substring (point) (pos-eol)))))

emacs_version:
- '27.2'
- '28.2'
- '29.1'
- '29.3'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- '29.3'
- '29.4'

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

Successfully merging this pull request may close these issues.

4 participants