bug#67696: 30.0.50; Help deal with multiple versions in load-path

bug#67696: 30.0.50; Help deal with multiple versions in load-path

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

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

Package: Emacs
Version: 30.0.50


With packages being available both as bundled with Emacs and as ELPA
packages, it has become a lot more common place to have two versions of
a package in the `load-path` and to have to deal with situations
where the "incorrect" version has been loaded before `load-path`
was changed.

These kinds of problems manifest in various ways and we try to
circumvent them in `package.el` in some cases but that can't cover
all cases.

I suggest we introduce a new function to help packages susceptible to
those problems.  The patch below introduces a new function which
I tentatively called `require-with-check` and shows how it could be used
in the case of `eglot.el` (which relies on several core packages also
distributed via GNU ELPA and currently uses a hack which slows it down
unnecessarily in the normal case).

As mentioned in a FIXME in there, maybe we should also consider
adding to `seq` (and `eldoc`) something like

    ;;;###autoload
    (if (featurep 'seq) (require-with-check 'seq 'reload))


-- Stefan

[-- Attachment #2: require-with-check.patch --]
[-- Type: text/x-diff, Size: 4107 bytes --]

diff --git a/lisp/files.el b/lisp/files.el
index 1cdcec23b11..e1e885462ce 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1246,6 +1246,29 @@ load-library
   (interactive (list (read-library-name)))
   (load library))
 
+(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).
+If it's the case, we either signal an error (the default), or forcibly reload
+the new file (if NOERROR is equal to `reload'), or otherwise emit a warning."
+  (let ((lh load-history)
+        (res (require feature filename (if (eq noerror 'reload) nil noerror))))
+    ;; If the `feature' was not yet provided, `require' just loaded the right
+    ;; file, so we're done.
+    (when (eq lh load-history)
+      ;; 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))))
+        (cond
+         ((assoc fn load-history) nil)  ;We loaded the right file.
+         ((eq noerror 'reload) (load fn nil 'nomessage))
+         (t (funcall (if noerror #'warn #'error)
+                     "Feature provided by other file: %S" feature)))))
+    res))
+
 (defun file-remote-p (file &optional identification connected)
   "Test whether FILE specifies a location on a remote system.
 A file is considered remote if accessing it is likely to
diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index d410367f902..8124a3f52e0 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -116,13 +116,16 @@
 ;; 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"))
+  ;; For those packages that are preloaded, reload them if needed,
+  ;; since that's the best we can do anyway.
+  ;; FIXME: Maybe the ELPA packages for those preloaded packages should
+  ;; force-reload themselves eagerly when the package is activated!
+  (require-with-check 'eldoc nil 'reload)
+  (require-with-check 'seq nil 'reload)
+  ;; For those packages which are not preloaded OTOH, signal an error if
+  ;; the loaded file is not the one that should have been loaded.
+  (mapc #'require-with-check
+        '(project flymake xref jsonrpc external-completion)))
 
 ;; forward-declare, but don't require (Emacs 28 doesn't seem to care)
 (defvar markdown-fontify-code-blocks-natively)
@@ -138,11 +141,12 @@ tramp-use-ssh-controlmaster-options
                         'eglot-managed-mode-hook "1.6")
 (define-obsolete-variable-alias 'eglot-confirm-server-initiated-edits
   'eglot-confirm-server-edits "1.16")
-(define-obsolete-function-alias 'eglot--uri-to-path 'eglot-uri-to-path "1.16")
-(define-obsolete-function-alias 'eglot--path-to-uri 'eglot-path-to-uri "1.16")
-(define-obsolete-function-alias 'eglot--range-region 'eglot-range-region "1.16")
-(define-obsolete-function-alias 'eglot--server-capable 'eglot-server-capable "1.16")
-(define-obsolete-function-alias 'eglot--server-capable-or-lose 'eglot-server-capable-or-lose "1.16")
+(define-obsolete-function-alias 'eglot--uri-to-path #'eglot-uri-to-path "1.16")
+(define-obsolete-function-alias 'eglot--path-to-uri #'eglot-path-to-uri "1.16")
+(define-obsolete-function-alias 'eglot--range-region #'eglot-range-region "1.16")
+(define-obsolete-function-alias 'eglot--server-capable #'eglot-server-capable "1.16")
+(define-obsolete-function-alias 'eglot--server-capable-or-lose
+  #'eglot-server-capable-or-lose "1.16")
 (define-obsolete-function-alias
   'eglot-lsp-abiding-column 'eglot-utf-16-linepos "1.12")
 (define-obsolete-function-alias

bug#67696: 30.0.50; Help deal with multiple versions in load-path

[Stefan Kangas]

Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> With packages being available both as bundled with Emacs and as ELPA
> packages, it has become a lot more common place to have two versions of
> a package in the `load-path` and to have to deal with situations
> where the "incorrect" version has been loaded before `load-path`
> was changed.
>
> These kinds of problems manifest in various ways and we try to
> circumvent them in `package.el` in some cases but that can't cover
> all cases.
>
> I suggest we introduce a new function to help packages susceptible to
> those problems.  The patch below introduces a new function which
> I tentatively called `require-with-check` and shows how it could be used
> in the case of `eglot.el` (which relies on several core packages also
> distributed via GNU ELPA and currently uses a hack which slows it down
> unnecessarily in the normal case).

Will this be useful only for :core packages?  If so, it would be nice to
not have to introduce more functions and extra complexity just to deal
with this situation.  It seems like a problem we should be able to fix
without it leaking into our API.

I didn't think deeply about this so here's a probably naive question:
could we make `require' reload the file, if a newer one is detected
earlier in the load-path?

Are there any other alternative approaches that you considered?

bug#67696: 30.0.50; Help deal with multiple versions in load-path

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

>> With packages being available both as bundled with Emacs and as ELPA
>> packages, it has become a lot more common place to have two versions of
>> a package in the `load-path` and to have to deal with situations
>> where the "incorrect" version has been loaded before `load-path`
>> was changed.
>>
>> These kinds of problems manifest in various ways and we try to
>> circumvent them in `package.el` in some cases but that can't cover
>> all cases.
>>
>> I suggest we introduce a new function to help packages susceptible to
>> those problems.  The patch below introduces a new function which
>> I tentatively called `require-with-check` and shows how it could be used
>> in the case of `eglot.el` (which relies on several core packages also
>> distributed via GNU ELPA and currently uses a hack which slows it down
>> unnecessarily in the normal case).
> Will this be useful only for :core packages?

AFAIK it's useful only to detect and/or work around problems linked to
interleavings of loading files and changing `load-path`.

I can't think of any reason why this would be limited to :core packages,
but it's a problem that shows up more commonly for :core packages, indeed.

> If so, it would be nice to not have to introduce more functions and
> extra complexity just to deal with this situation.  It seems like
> a problem we should be able to fix without it leaking into our API.

Another option is to tweak `require` directly: the new function works
basically "identically" to `require` expect for the added checks.

> I didn't think deeply about this so here's a probably naive question:
> could we make `require' reload the file, if a newer one is detected
> earlier in the load-path?

By default I made the function signal an error rather than reload,
because reloading is not a really reliable solution (`defvar` won't be
updated to the new value, renamed functions will linger, ...).

But yes, we could change `require` to behave more like
`require-with-check`.  The downside is that it's more risky, and it may
come with a performance cost (it means that calling `require` repeatedly
isn't as cheap as `(memq F features)`).

> Are there any other alternative approaches that you considered?

Many other approaches have been mentioned/considered in the long
discussion around Org mode's attempt to deal with similar problems.

I found this one approach to be the clea[rn]est because it seemed to get
to the actual core of the problem and deals with a known problem (file
shadowing) that's reasonably easy&cheap to check reliably at runtime.

As you can see in my patch, Eglot took the other approach to blindly
reload the files (which kind of works but is less efficient and suffers
from further subtle differences with `require` such as the presence of
a load message, or the absence of an entry in `load-history`).

Org took yet another approach (signal an error if the loaded version is
different from the file's own notion of version), but it suffers from
annoying false positives and requires more ad-hoc support (it relies on
having a version, on hard-coding that version info in the .elc files,
and on someone manually updating that version info when needed), so it's
harder to turn it into a generic solution.


        Stefan

bug#67696: 30.0.50; Help deal with multiple versions in load-path

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Another option is to tweak `require` directly: the new function works
> basically "identically" to `require` expect for the added checks.
[...]
> But yes, we could change `require` to behave more like
> `require-with-check`.  The downside is that it's more risky, and it may
> come with a performance cost (it means that calling `require` repeatedly
> isn't as cheap as `(memq F features)`).

Right.  On the other hand, perhaps hitting the file system is to some
extent expected once you start asking for libraries to be loaded.  You
only get the best case `(memq F features)' if that evaluates to `t'.

Personally, I don't have a good view of how common these problems are.
Perhaps they are relatively uncommon, and it's too much to ask all users
of `require' to pay a cost for added correctness in unusual cases.

bug#67696: 30.0.50; Help deal with multiple versions in load-path

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

> Right.  On the other hand, perhaps hitting the file system is to some
> extent expected once you start asking for libraries to be loaded.  You
> only get the best case `(memq F features)' if that evaluates to `t'.

For top-level uses of `require`, the performance impact is negligible.
But for `require`s used within functions (basically performing manual
autoloads), I'm worried that it could be problematic.

> Personally, I don't have a good view of how common these problems are.
> Perhaps they are relatively uncommon, and it's too much to ask all users
> of `require' to pay a cost for added correctness in unusual cases.

I don't either.  That's why I preferred to define a new function, which
lets us gain some experience with it.  We may later opt to merge (some
of) its functionality into `require`, of course.


        Stefan

bug#67696: 30.0.50; Help deal with multiple versions in load-path

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Right.  On the other hand, perhaps hitting the file system is to some
>> extent expected once you start asking for libraries to be loaded.  You
>> only get the best case `(memq F features)' if that evaluates to `t'.
>
> For top-level uses of `require`, the performance impact is negligible.
> But for `require`s used within functions (basically performing manual
> autoloads), I'm worried that it could be problematic.
>
>> Personally, I don't have a good view of how common these problems are.
>> Perhaps they are relatively uncommon, and it's too much to ask all users
>> of `require' to pay a cost for added correctness in unusual cases.
>
> I don't either.  That's why I preferred to define a new function, which
> lets us gain some experience with it.  We may later opt to merge (some
> of) its functionality into `require`, of course.

Makes sense to me, thanks.

bug#67696: 30.0.50; Help deal with multiple versions in load-path

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

Stefan Kangas [2023-12-29 06:22:36] wrote:
> Makes sense to me, thanks.

Thanks, pushed, closing,


        Stefan