bug#67480: 30.0.50; Cannot start eglot

bug#67480: 30.0.50; Cannot start eglot

[Mou Tong]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

 1. `emacs -Q`
 2. Open a src file and `M-x eglot`
 3. Get the following message:

 ```
 Loading project (native compiled elisp)...done
 Loading eldoc (native compiled elisp)...done
 Loading seq (native compiled elisp)...done
 Loading flymake (native compiled elisp)...done
 Loading xref (native compiled elisp)...done
 Loading jsonrpc (native compiled elisp)...done
 Loading external-completion (native compiled elisp)...done
 Unbound slot: eglot-lsp-server, "#<eglot-lsp-server
 eglot-lsp-server-1feea1d8dd60>", -events-buffer, oref
 error in process filter: Unbound slot: eglot-lsp-server,
 "#<eglot-lsp-server eglot-lsp-server-1feea1d8dd60>", -events-buffer, oref
 [2 times]
 ```

 I tested `.c` using `c-mode` or `c-ts-mode`, `.rs` with `rust-ts-mode`,
 all of them show the above error, so I guess it's not major mode's problem.

---

In GNU Emacs 30.0.50 (build 1, x86_64-apple-darwin21.6.0, NS
 appkit-2113.60 Version 12.7.1 (Build 21G920)) of 2023-11-27 built on
 dalum.local
Repository revision: 2407f810136739da376ff0929b247a49dc196299
Repository branch: master
Windowing system distributor 'Apple', version 10.3.2113
System Description:  macOS 12.7.1

Configured using:
 'configure --with-native-compilation=aot'

Configured features:
ACL DBUS GIF GLIB GMP GNUTLS JPEG JSON LCMS2 LIBXML2 MODULES NATIVE_COMP
NOTIFY KQUEUE NS PDUMPER PNG RSVG SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XIM ZLIB

Important settings:
  value of $LC_COLLATE: C
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: C/*



[-- Attachment #2: Type: text/html, Size: 5311 bytes --]

bug#67480: 30.0.50; Cannot start eglot

[Eli Zaretskii]

> From: Mou Tong <mou.tong@outlook.com>
> Date: Mon, 27 Nov 2023 08:12:35 +0000
> 
>  1. `emacs -Q`
>  2. Open a src file and `M-x eglot`
>  3. Get the following message:
> 
>  ```
>  Loading project (native compiled elisp)...done
>  Loading eldoc (native compiled elisp)...done
>  Loading seq (native compiled elisp)...done
>  Loading flymake (native compiled elisp)...done
>  Loading xref (native compiled elisp)...done
>  Loading jsonrpc (native compiled elisp)...done
>  Loading external-completion (native compiled elisp)...done

Some of these files are preloaded, so I don't understand why you see
these messages.  I suspect some problem with your build.

bug#67480: 30.0.50; Cannot start eglot

[Mou Tong]

[-- Attachment #1: Type: text/plain, Size: 585 bytes --]

2023/11/27 21:10, Eli Zaretskii <eliz@gnu.org<mailto:eliz@gnu.org>> wrote:

> Some of these files are preloaded, so I don't understand why you see
> these messages.

Eglot need these libraries to work better, like `project` to manage
src as a project, `eldoc` to provide doc...

> I suspect some problem with your build.

After I tried to build different version, I can confirm the problem
occurs after this commit:

https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=6c47931a1ad4de4af3f147b9604169c2441100fe

After I reverted this commit, everything works fine.


[-- Attachment #2: Type: text/html, Size: 2162 bytes --]

bug#67480: 30.0.50; Cannot start eglot

[Eli Zaretskii]

> From: Mou Tong <mou.tong@outlook.com>
> CC: "67480@debbugs.gnu.org" <67480@debbugs.gnu.org>
> Date: Tue, 28 Nov 2023 06:06:25 +0000
> 
> 2023/11/27 21:10, Eli Zaretskii <eliz@gnu.org> wrote: 
> 
> > Some of these files are preloaded, so I don't understand why you see
> > these messages.
> 
> Eglot need these libraries to work better, like `project` to manage
> src as a project, `eldoc` to provide doc...

But eldoc is preloaded, so why would Emacs need to load it again when
using Eglot?

> > I suspect some problem with your build.
> 
> After I tried to build different version, I can confirm the problem
> occurs after this commit:
> 
> https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=6c47931a1ad4de4af3f147b9604169c2441100fe
> 
> 
> After I reverted this commit, everything works fine.

Let's see what the guilty parties (CC'ed) have to say about that...

bug#67480: 30.0.50; Cannot start eglot

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Some of these files are preloaded, so I don't understand why you see
> these messages.  I suspect some problem with your build.

[ Note: This is unrelated to bug#67480.  ]

No, it's a problem in `eglot.el`:

    ;; These dependencies are also GNU ELPA core packages.  Because of
    ;; bug#62576, since there is a risk that M-x package-install, despite
    ;; having installed them, didn't correctly re-load them over the
    ;; built-in versions.
    (eval-and-compile
      (load "project")
      (load "eldoc")
      (load "seq")
      (load "flymake")
      (load "xref")
      (load "jsonrpc")
      (load "external-completion"))

I suspect this should be fixed the same way I proposed to fix these
kinds of problems in Org, i.e. with something like:

    (defun require-with-check (feature &optional filename noerror)
      "If FEATURE is not already loaded, load it from FILENAME.
    This is like `require' except if FEATURE is already a member of the list
    `features’, then we check if this was provided by a different file than the
    one that we would load now (presumably because `load-path' has been
    changed since the file was loaded)."
      (let ((lh load-history)
            (res (require feature filename noerror)))
        ;; If the `feature' was not yet provided, `require' just loaded the right
        ;; file, so we're done.
        (if (not (eq lh load-history)) res
          ;; If `require' did nothing, we need to make sure that was warranted.
          (let ((fn (locate-file (or filename (symbol-name feature))
                                 load-path (get-load-suffixes))))
            ;; If the right file was indeed loaded already, we're done.
            (if (assoc fn load-history) res
              (funcall (if noerror #'warn #'error)
                       "Feature provided by other file: %S" feature)
              res)))))

This sample code doesn't try to handle preloaded packages, so it
would/will need some tweak for that.


        Stefan

bug#67480: 30.0.50; Cannot start eglot

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>  1. `emacs -Q`
>  2. Open a src file and `M-x eglot`
>  3. Get the following message:
[...]
>  Loading external-completion (native compiled elisp)...done
>  Unbound slot: eglot-lsp-server, "#<eglot-lsp-server
>  eglot-lsp-server-1feea1d8dd60>", -events-buffer, oref
>  error in process filter: Unbound slot: eglot-lsp-server,
>  "#<eglot-lsp-server eglot-lsp-server-1feea1d8dd60>", -events-buffer, oref
>  [2 times]
>  ```
[...]
> After I tried to build different version, I can confirm the problem
> occurs after this commit:
> https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=6c47931a1ad4de4af3f147b9604169c2441100fe

Thanks, that was very helpful.  This commit makes it so `:accessor`
functions behave like `:reader` functions, i.e. behave like
`slot-value`, whereas the old code returned nil if the slot was
"unbound".

Maybe the better fix is something like the patch below?
João?


        Stefan


diff --git a/lisp/jsonrpc.el b/lisp/jsonrpc.el
index 52ffb220d8b..4298d75c5bf 100644
--- a/lisp/jsonrpc.el
+++ b/lisp/jsonrpc.el
@@ -71,6 +71,7 @@ jsonrpc-connection
     :accessor jsonrpc--request-continuations
     :documentation "A hash table of request ID to continuation lambdas.")
    (-events-buffer
+    :initform nil
     :accessor jsonrpc--events-buffer
     :documentation "A buffer pretty-printing the JSONRPC events")
    (-events-buffer-scrollback-size

bug#67480: 30.0.50; Cannot start eglot

[João Távora]

On Tue, Nov 28, 2023 at 2:52 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> diff --git a/lisp/jsonrpc.el b/lisp/jsonrpc.el
> index 52ffb220d8b..4298d75c5bf 100644
> --- a/lisp/jsonrpc.el
> +++ b/lisp/jsonrpc.el
> @@ -71,6 +71,7 @@ jsonrpc-connection
>      :accessor jsonrpc--request-continuations
>      :documentation "A hash table of request ID to continuation lambdas.")
>     (-events-buffer
> +    :initform nil
>      :accessor jsonrpc--events-buffer
>      :documentation "A buffer pretty-printing the JSONRPC events")
>     (-events-buffer-scrollback-size

Seem sensible, and feel free to push, please.

But it'd also be nice to have a backtrace to that error to
see why jsonrpc.el is trying to access the jsonrpc--events-buffer
"too early".

And kudos for going through these EIEIO bugs, even if in backward
incompatible ways ;-).  Accessor functions definitely must go thru
the slot-value protocol.

João

bug#67480: 30.0.50; Cannot start eglot

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> diff --git a/lisp/jsonrpc.el b/lisp/jsonrpc.el
>> index 52ffb220d8b..4298d75c5bf 100644
>> --- a/lisp/jsonrpc.el
>> +++ b/lisp/jsonrpc.el
>> @@ -71,6 +71,7 @@ jsonrpc-connection
>>      :accessor jsonrpc--request-continuations
>>      :documentation "A hash table of request ID to continuation lambdas.")
>>     (-events-buffer
>> +    :initform nil
>>      :accessor jsonrpc--events-buffer
>>      :documentation "A buffer pretty-printing the JSONRPC events")
>>     (-events-buffer-scrollback-size
>
> Seem sensible, and feel free to push, please.
>
> But it'd also be nice to have a backtrace to that error to

Have you tried the recipe sent by Mou Tong?

> see why jsonrpc.el is trying to access the jsonrpc--events-buffer
> "too early".

Not sure what you mean by "too early".  Where is this slot filled?
The only place I see is in `jsonrpc-events-buffer` where we always read
it before setting it.


        Stefan

bug#67480: 30.0.50; Cannot start eglot

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

On Tue, Nov 28, 2023, 15:48 Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> >> diff --git a/lisp/jsonrpc.el b/lisp/jsonrpc.el
> >> index 52ffb220d8b..4298d75c5bf 100644
> >> --- a/lisp/jsonrpc.el
> >> +++ b/lisp/jsonrpc.el
> >> @@ -71,6 +71,7 @@ jsonrpc-connection
> >>      :accessor jsonrpc--request-continuations
> >>      :documentation "A hash table of request ID to continuation
> lambdas.")
> >>     (-events-buffer
> >> +    :initform nil
> >>      :accessor jsonrpc--events-buffer
> >>      :documentation "A buffer pretty-printing the JSONRPC events")
> >>     (-events-buffer-scrollback-size
> >
> > Seem sensible, and feel free to push, please.
> >
> > But it'd also be nice to have a backtrace to that error to
>
> Have you tried the recipe sent by Mou Tong?
>

No. Didn't have the chance.

> see why jsonrpc.el is trying to access the jsonrpc--events-buffer
> > "too early".
>
> Not sure what you mean by "too early".  Where is this slot filled?
>

No, I meant where it is read.

The only place I see is in `jsonrpc-events-buffer` where we always read
> it before setting it.


I don't have the code in front of me, but ok, so a lazy slot.

So maybe another fix would be to put a :before in the accessor generic.
Then the accessor could lose the '--', and simplify things. AFAIR this is a
standard CLOS technique for lazy slots. But the nil initial value works
fine too.

João

[-- Attachment #2: Type: text/html, Size: 2523 bytes --]

bug#67480: 30.0.50; Cannot start eglot

[João Távora]

On Tue, Nov 28, 2023 at 5:27 PM João Távora <joaotavora@gmail.com> wrote:

> I don't have the code in front of me, but ok, so a lazy slot.

I've now fixed this and a bunch of other similar problems in
master.  But let's wait a few more days if some more related
flak comes in before closing this.

Maybe it also makes sense to merge this with bug#67480?

João

PS:  I wrote earlier that the :before on the accessor generic is
standard CLOS technique for lazy slots.  It could be, but
slot-unbound of the slot-value protocol is more correct.  Just
nitpicking myself here.  EIEIO seems to support slot-unbound.

bug#67480: 30.0.50; Cannot start eglot

[João Távora]

On Wed, Nov 29, 2023 at 12:40 AM João Távora <joaotavora@gmail.com> wrote:
>
> On Tue, Nov 28, 2023 at 5:27 PM João Távora <joaotavora@gmail.com> wrote:
>
> > I don't have the code in front of me, but ok, so a lazy slot.
>
> I've now fixed this and a bunch of other similar problems in
> master.  But let's wait a few more days if some more related
> flak comes in before closing this.

I think the problem has been fixed, so closing.  I hope
the other bugs merged with this one will be closed as well
as a consequence.

João

bug#67480: 30.0.50; Cannot start eglot

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I suspect this should be fixed the same way I proposed to fix these
> kinds of problems in Org, i.e. with something like:
>
>     (defun require-with-check (feature &optional filename noerror)
>       "If FEATURE is not already loaded, load it from FILENAME.
>     This is like `require' except if FEATURE is already a member of the list
>     `features’, then we check if this was provided by a different file than the
>     one that we would load now (presumably because `load-path' has been
>     changed since the file was loaded)."
>       (let ((lh load-history)
>             (res (require feature filename noerror)))
>         ;; If the `feature' was not yet provided, `require' just loaded the right
>         ;; file, so we're done.
>         (if (not (eq lh load-history)) res
>           ;; If `require' did nothing, we need to make sure that was warranted.
>           (let ((fn (locate-file (or filename (symbol-name feature))
>                                  load-path (get-load-suffixes))))
>             ;; If the right file was indeed loaded already, we're done.
>             (if (assoc fn load-history) res
>               (funcall (if noerror #'warn #'error)
>                        "Feature provided by other file: %S" feature)
>               res)))))
>
> This sample code doesn't try to handle preloaded packages, so it
> would/will need some tweak for that.

Actually, it seems it does work with preloaded files as well because the
`load-history` is adjusted at startup to make it look right for
preloaded packages.

So maybe we should add the above function to `subr.el` and then install
the patch below, WDYT?


        Stefan


diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index d410367f902..468606086ec 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -116,13 +116,8 @@
 ;; having installed them, didn't correctly re-load them over the
 ;; built-in versions.
 (eval-and-compile
-  (load "project")
-  (load "eldoc")
-  (load "seq")
-  (load "flymake")
-  (load "xref")
-  (load "jsonrpc")
-  (load "external-completion"))
+  (mapc #'require-with-check
+        '(project eldoc seq flymake xref jsonrpc external-completion)))
 
 ;; forward-declare, but don't require (Emacs 28 doesn't seem to care)
 (defvar markdown-fontify-code-blocks-natively)