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

Allow :initializationOptions with all forms of contact in eglot-server-programs #1038

Closed
wants to merge 1 commit into from

Conversation

astoff
Copy link
Contributor

@astoff astoff commented Sep 17, 2022

Currently, passing :initialiationOptions to a server in eglot-server-programs is not supported when the connection is over TCP or the contact is a class.

This PR lifts this restriction this by making the :initializationOptions feature orthogonal to the details of how to launch a server program.

(The initial submission of this PR phrased it differently, so note that the ensuing discussion doesn't address the description above directly.)

See als Emacs bug#57725.

Closes #940

* eglot.el (eglot-server-programs): Update docstring.
(eglot--connect): Extract a :initializationOptions entry in all cases,
not only in the `(stringp (car contact))' case; revert the latter to
its previous value.  Include the :initializationOption entry in the
saved-initiargs slot, so that `eglot-initialization-options' picks it
up.
@joaotavora
Copy link
Owner

I'd be more comfortable with a more surgical change such as:

diff --git a/eglot.el b/eglot.el
index ff94d5ca5f..3b1b1a9536 100644
--- a/eglot.el
+++ b/eglot.el
@@ -796,6 +796,8 @@ treated as in `eglot-dbind'."
   :documentation
   "Represents a server. Wraps a process for LSP communication.")
 
+(cl-defmethod initialize-instance :before ((_server eglot-lsp-server) &optional args)
+  (cl-remf args :initializationOptions))
 
 ;;; Process management
 (defvar eglot--servers-by-project (make-hash-table :test #'equal)

This seems to do the right thing in my simple tests.

@astoff
Copy link
Contributor Author

astoff commented Sep 17, 2022

With this patch and setting

(setf (alist-get 'python-mode eglot-server-programs)
        '("jedi-language-server"
          "-v"
          :initializationOptions
          (:workspace (:symbols (:maxSymbols 200)))))

I get the following error

Error in post-command-hook (#[0 "\303\304\300\242\305#\210\306\301!\205�\0r\301q\210
?\205�\0\307\310\311 \")\207" [(#0) #<buffer x.py> eglot--managed-mode remove-hook post-command-hook nil buffer-live-p apply eglot--connect eglot--guess-contact] 4]): (wrong-type-argument stringp :initializationOptions)

This happens because, as you know

ELISP> (let ((data '(even :odd 1))) (cl-remf data :odd) data)
(even :odd 1)

So these horrible manipulations in the PR

            (when-let ((probe (cl-position :initializationOptions contact)))
                     (prog1 (nth (1+ probe) contact)
                       (setq contact (append (cl-subseq contact 0 probe)
                                             (cl-subseq contact (+ 2 probe)))))))

seem necessary.

@astoff
Copy link
Contributor Author

astoff commented Sep 17, 2022

Oops, the above reason for the error is wrong. It's rather something in eglot--guess-contact.

@astoff
Copy link
Contributor Author

astoff commented Sep 17, 2022

By the way, my PR is longer than your suggestion because it makes :initializationOptions work in combination with :autoport or any of the other forms an entry of eglot-server-programs can have (testing pending).

joaotavora added a commit that referenced this pull request Sep 17, 2022
Also see #1038.

This feature was poorly tested, and simply wouldn't work when trying
to initialize the server object.

The simple solution is to ignore :initializationOptions initarg in
this context.  It is still stored separately as and accessed as the
'eglot--saved-initargs' slot.

Another complication arises in eglot--guess-contact, which tried too
hard to be able to compose an interactive prompt (when the server
program can't be found).  The solution is just to give up when
:autoport or :initializationOptions is found.  It's not easy or
practical to have the user provide non-string arguments via a string
interface like the minibuffer.

* eglot.el (initialize-instance :before eglot-lsp-server): Don't pass
:initializationOptions initarg onward.
(eglot--guess-contact): Simplify.  Don't try heroics with
:autoport and :initializationOptions.
@joaotavora
Copy link
Owner

Oops, the above reason for the error is wrong. It's rather something in eglot--guess-contact.

Yes, I see the error now, thanks for reporting it.

See my new commit, which simplifies eglot--guess-contact to not worry about providing an interactive prompt when keywords are found in the contact. It's just not worth it. So, barring any more errors, I think we can fix this by actually removing complexity.

@astoff
Copy link
Contributor Author

astoff commented Sep 17, 2022

So, barring any more errors, I think we can fix this by actually removing complexity.

I hope this means my version, since it prunes down the possible forms of CONTACT in eglot-server-programs from 6 to 5, while at the same time adding flexibility :-).

@joaotavora
Copy link
Owner

No, I'm sorry. It's too much code for and complicates the structure of the eglot-server-programs docstring. I'd rather something more surgical. Actually I think your very first suggestion over at the Emacs bug tracker also worked fine (except for the eglot--guess-contact thing).

prunes down the possible forms of CONTACT in eglot-server-programs from 6 to 5,

It just moves a section down to the end. I actually think that complicates the docstring.

Also I don't think :autoport is a widely used feature. And the docstring doesn't promise that :autoport works with :initializationOptions. Making it so doesn't seem very complicated (one just needs to prune out keywords in eglot--inferior-bootstrap) But I don't think it's worth it anyway.

joaotavora added a commit that referenced this pull request Sep 17, 2022
Also see #1038.

This feature was poorly tested, and simply wouldn't work when trying
to initialize the server object.

The simple solution is to ignore :initializationOptions initarg in
this context.  It is still stored separately as and accessed as the
'eglot--saved-initargs' slot.

Another complication arises in eglot--guess-contact, which tried too
hard to be able to compose an interactive prompt (when the server
program can't be found).  The solution is just to give up when
:autoport or :initializationOptions is found.  It's not easy or
practical to have the user provide non-string arguments via a string
interface like the minibuffer.

* eglot.el (initialize-instance :before eglot-lsp-server): Don't pass
:initializationOptions initarg onward.
(eglot--guess-contact): Simplify.  Don't try heroics with
:autoport and :initializationOptions.
joaotavora added a commit that referenced this pull request Sep 17, 2022
Also see #1038.

This feature was poorly tested, and simply wouldn't work when trying
to initialize the server object.

The simple solution is to ignore :initializationOptions initarg in
this context.  It is still stored separately as and accessed as the
'eglot--saved-initargs' slot.

Another complication arises in eglot--guess-contact, which tried too
hard to be able to compose an interactive prompt (when the server
program can't be found).  The solution is just to give up when
:autoport or :initializationOptions is found.  It's not easy or
practical to have the user provide non-string arguments via a string
interface like the minibuffer.

* eglot.el (initialize-instance :before eglot-lsp-server): Don't pass
:initializationOptions initarg onward.
(eglot--guess-contact): Simplify.  Don't try heroics with
:autoport and :initializationOptions.
joaotavora added a commit that referenced this pull request Sep 17, 2022
Also see #1038.

This feature was poorly tested, and simply wouldn't work when trying
to initialize the server object.

The simple solution is to ignore :initializationOptions initarg in
this context.  It is still stored separately as and accessed as the
'eglot--saved-initargs' slot.

Another complication arises in eglot--guess-contact, which tried too
hard to be able to compose an interactive prompt (when the server
program can't be found).  The solution is just to give up when
:autoport or :initializationOptions is found.  It's not easy or
practical to have the user provide non-string arguments via a string
interface like the minibuffer.

* eglot.el (initialize-instance :before eglot-lsp-server): Don't pass
:initializationOptions initarg onward.
(eglot--guess-contact): Simplify.  Don't try heroics with
:autoport and :initializationOptions.

* eglot-tests.el (eglot-server-programs-simple-missing-executable):
  Update test.
joaotavora added a commit that referenced this pull request Sep 17, 2022
Also see #1038.

This feature was poorly tested, and simply wouldn't work when trying
to initialize the server object.

The simple solution is to ignore :initializationOptions initarg in
this context.  It is still stored separately as and accessed as the
'eglot--saved-initargs' slot.

Another complication arises in eglot--guess-contact, which tried too
hard to be able to compose an interactive prompt (when the server
program can't be found).  The solution is just to give up when
:autoport or :initializationOptions is found.  It's not easy or
practical to have the user provide non-string arguments via a string
interface like the minibuffer.

* eglot.el (initialize-instance :before eglot-lsp-server): Don't pass
:initializationOptions initarg onward.
(eglot--guess-contact): Simplify.  Don't try heroics with
:autoport and :initializationOptions.

* eglot-tests.el (eglot-server-programs-simple-missing-executable):
  Update test.
@astoff
Copy link
Contributor Author

astoff commented Sep 18, 2022

Well, it's true that this is not the minimal hack necessary to make :initializationOptions to work at all.

This change is slightly longer because it makes :initializationOptions orthogonal to the rest of the other forms a contact can have in eglot-server-programs. So instead of a table of 5 × 2 possibilities where 4 of them are forbidden, there are 5 × 2 combinations and all work. To me this is simpler, although of course the code is a couple lines longer.

Anyway, I'll rename the PR to reflect what it does, even though I don't expect it will be merged.

@astoff astoff changed the title Handle :initializationOptions correctly in eglot--connect Allow :initializationOptions with all forms of contact in eglot-server-programs Sep 18, 2022
Comment on lines -1142 to +1156
(let* ((probe (cl-position-if #'keywordp contact))
(more-initargs (and probe (cl-subseq contact probe)))
(contact (cl-subseq contact 0 probe)))
`(:process
,(lambda ()
(let ((default-directory default-directory))
(make-process
:name readable-name
:command (setq server-info (eglot--cmd contact))
:connection-type 'pipe
:coding 'utf-8-emacs-unix
:noquery t
:stderr (get-buffer-create
(format "*%s stderr*" readable-name))
:file-handler t)))
,@more-initargs)))))
`(:process
,(lambda ()
(let ((default-directory default-directory))
(make-process
:name readable-name
:command (setq server-info (eglot--cmd contact))
:connection-type 'pipe
:coding 'utf-8-emacs-unix
:noquery t
:stderr (get-buffer-create
(format "*%s stderr*" readable-name))
:file-handler t)))))))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this hunk, which is the biggest code change here, is just reverting things to their state before :initializationOptions was initially introduced.

@joaotavora
Copy link
Owner

So instead of a table of 5 × 2 possibilities where 4 of them are forbidden, there are 5 × 2 combinations and all work. To me this is simpler, although of course the code is a couple lines longer.

In my opinion, it is not simpler. I went to some lengths to make that list a flat list of 6 forms and I hope never to have to extend it again.

And it's not a questions of making the code "a couple of lines longer": it's about unmotivated changes. If I don't witness an actual need for making something,I won't. Here my need is to make :initializationOptions work at all: it was totally botched and now it's not (hopefully).

Eglot is about 4 years old. Well-meaning ambitious contributors have come and gone. I myself drift away often: I need the codebase to be as minimal and as stable as possible. See for example all the TRAMP related bugs: I didn't implement TRAMP support and I don't use it regularly. There were some proposals for adding this to Eglot and I took some time to choose the simplest one. Even that seems to cause intermittent problems I just don't have time to debug.

@astoff
Copy link
Contributor Author

astoff commented Sep 18, 2022

See for example all the TRAMP related bugs: I didn't implement TRAMP support and I don't use it regularly.

The T in Tramp is for "transparent", so Eglot shouldn't do anything at all — except for translating some magic file names to the local name in the remote machine, of course. I completely agree with your approach there.

I guess I should close this after all, so thanks for your patience :-).

@astoff astoff closed this Sep 18, 2022
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 18, 2022
…r-programs

Also see joaotavora/eglot#1038.

This feature was poorly tested, and simply wouldn't work when trying
to initialize the server object.

The simple solution is to ignore :initializationOptions initarg in
this context.  It is still stored separately as and accessed as the
'eglot--saved-initargs' slot.

Another complication arises in eglot--guess-contact, which tried too
hard to be able to compose an interactive prompt (when the server
program can't be found).  The solution is just to give up when
:autoport or :initializationOptions is found.  It's not easy or
practical to have the user provide non-string arguments via a string
interface like the minibuffer.

* eglot.el (initialize-instance :before eglot-lsp-server): Don't pass
:initializationOptions initarg onward.
(eglot--guess-contact): Simplify.  Don't try heroics with
:autoport and :initializationOptions.

* eglot-tests.el (eglot-server-programs-simple-missing-executable):
  Update test.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
…r-programs

Also see joaotavora/eglot#1038.

This feature was poorly tested, and simply wouldn't work when trying
to initialize the server object.

The simple solution is to ignore :initializationOptions initarg in
this context.  It is still stored separately as and accessed as the
'eglot--saved-initargs' slot.

Another complication arises in eglot--guess-contact, which tried too
hard to be able to compose an interactive prompt (when the server
program can't be found).  The solution is just to give up when
:autoport or :initializationOptions is found.  It's not easy or
practical to have the user provide non-string arguments via a string
interface like the minibuffer.

* eglot.el (initialize-instance :before eglot-lsp-server): Don't pass
:initializationOptions initarg onward.
(eglot--guess-contact): Simplify.  Don't try heroics with
:autoport and :initializationOptions.

* eglot-tests.el (eglot-server-programs-simple-missing-executable):
  Update test.
bhankas pushed a commit to bhankas/emacs that referenced this pull request Sep 19, 2022
Also see #1038.

This feature was poorly tested, and simply wouldn't work when trying
to initialize the server object.

The simple solution is to ignore :initializationOptions initarg in
this context.  It is still stored separately as and accessed as the
'eglot--saved-initargs' slot.

Another complication arises in eglot--guess-contact, which tried too
hard to be able to compose an interactive prompt (when the server
program can't be found).  The solution is just to give up when
:autoport or :initializationOptions is found.  It's not easy or
practical to have the user provide non-string arguments via a string
interface like the minibuffer.

* eglot.el (initialize-instance :before eglot-lsp-server): Don't pass
:initializationOptions initarg onward.
(eglot--guess-contact): Simplify.  Don't try heroics with
:autoport and :initializationOptions.

* eglot-tests.el (eglot-server-programs-simple-missing-executable):
  Update test.

#940: joaotavora/eglot#940
#1038: joaotavora/eglot#1038
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 12, 2022
Also see joaotavora/eglot#1038.

This feature was poorly tested, and simply wouldn't work when trying
to initialize the server object.

The simple solution is to ignore :initializationOptions initarg in
this context.  It is still stored separately as and accessed as the
'eglot--saved-initargs' slot.

Another complication arises in eglot--guess-contact, which tried too
hard to be able to compose an interactive prompt (when the server
program can't be found).  The solution is just to give up when
:autoport or :initializationOptions is found.  It's not easy or
practical to have the user provide non-string arguments via a string
interface like the minibuffer.

* eglot.el (initialize-instance :before eglot-lsp-server): Don't pass
:initializationOptions initarg onward.
(eglot--guess-contact): Simplify.  Don't try heroics with
:autoport and :initializationOptions.

* eglot-tests.el (eglot-server-programs-simple-missing-executable):
  Update test.

GitHub-reference: fix joaotavora/eglot#940
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Oct 20, 2022
Also see joaotavora/eglot#1038.

This feature was poorly tested, and simply wouldn't work when trying
to initialize the server object.

The simple solution is to ignore :initializationOptions initarg in
this context.  It is still stored separately as and accessed as the
'eglot--saved-initargs' slot.

Another complication arises in eglot--guess-contact, which tried too
hard to be able to compose an interactive prompt (when the server
program can't be found).  The solution is just to give up when
:autoport or :initializationOptions is found.  It's not easy or
practical to have the user provide non-string arguments via a string
interface like the minibuffer.

* eglot.el (initialize-instance :before eglot-lsp-server): Don't pass
:initializationOptions initarg onward.
(eglot--guess-contact): Simplify.  Don't try heroics with
:autoport and :initializationOptions.

* eglot-tests.el (eglot-server-programs-simple-missing-executable):
  Update test.

GitHub-reference: fix joaotavora/eglot#940
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.

:initializationOptions key in eglot-server-programs does not work
2 participants