Re: master 9ade7ea: Fix Eldoc problem when loading on Emacs 26.3

Re: master 9ade7ea: Fix Eldoc problem when loading on Emacs 26.3

[Glenn Morris]

> branch: master
> commit 9ade7ea7b77ec40c16deb4dff139ce7127a703e2

This change causes 194 test failures.
Ref eg https://hydra.nixos.org/build/123765785

Re: master 9ade7ea: Fix Eldoc problem when loading on Emacs 26.3

[João Távora]

On Fri, Jul 10, 2020 at 5:09 PM Glenn Morris <rgm@gnu.org> wrote:
>
>
> > branch: master
> > commit 9ade7ea7b77ec40c16deb4dff139ce7127a703e2
>
> This change causes 194 test failures.
> Ref eg https://hydra.nixos.org/build/123765785

Reverted it, will have to find some other way. makunbound, as Noam
suggested, doesn't work.

Is there an easy way to "unlocalize" a symbol?

Re: master 9ade7ea: Fix Eldoc problem when loading on Emacs 26.3

[Noam Postavsky]

On Fri, 10 Jul 2020 at 13:10, João Távora <joaotavora@gmail.com> wrote:

> makunbound, as Noam suggested, doesn't work.
>
> Is there an easy way to "unlocalize" a symbol?

Oh, maybe kill-local-variable?

Re: master 9ade7ea: Fix Eldoc problem when loading on Emacs 26.3

[João Távora]

Noam Postavsky <npostavs@gmail.com> writes:

> On Fri, 10 Jul 2020 at 13:10, João Távora <joaotavora@gmail.com> wrote:
>
>> makunbound, as Noam suggested, doesn't work.
>>
>> Is there an easy way to "unlocalize" a symbol?
>
> Oh, maybe kill-local-variable?

No, that doesn't work either.  I was looking at data.c and alloc.c and
apparently the only way is really to unintern and re-intern the symbol.
But of course that seems to break code that you've loaded before and
that expects that symbol in its pre-uninterned state.

I've even tried `garbage-collect` to see if I could trigger the
sym->u.s.redirect = SYMBOL_PLAINVAL from Elisp, but it seems hard.

So I'm a bit lost for options to be honest.  The problem here, seems to
be the fact that eldoc.el is pre-loaded and somehwere in that process
the eldoc-documentation-function variable is localized in some buffer.

Of course, the problem only manifests itself in practice when you're
loading eldoc via package.el in previous Emacs versions, so maybe
there's some way to unintern the symbol at package-initialize time.  But
that probably would break something, too.

Maybe come up with a better idea, or maybe, like Stefan suggests, don't
make it an alias at all, just deprecate it and keep using it instead of
eldoc-documentation-strategy when we find it to be bound.

João

Re: master 9ade7ea: Fix Eldoc problem when loading on Emacs 26.3

[João Távora]

João Távora <joaotavora@gmail.com> writes:

> Noam Postavsky <npostavs@gmail.com> writes:
>>> makunbound, as Noam suggested, doesn't work.
>>> Is there an easy way to "unlocalize" a symbol?
>> Oh, maybe kill-local-variable?
>
> No, that doesn't work either.  I was looking at data.c and alloc.c and

> Maybe come up with a better idea, or maybe, like Stefan suggests, don't
> make it an alias at all, just deprecate it and keep using it instead of
> eldoc-documentation-strategy when we find it to be bound.

That sort of works, but it forces extensions that do want to
"Package-Require" eldoc to remember to unbind
eldoc-documentation-function if they are loaded on older Emacs versions,
which is undesirable.

So I think a better plan is just to reverse the variable alias on older
Emacs versions.  Attaching the patch that does this:

commit b7019ed5e76dda40642db1b20d3dd1484b2536e6
Author: João Távora <joaotavora@gmail.com>
Date:   Fri Jul 10 20:49:54 2020 +0100

    Sort out Eldoc backward compatibility of eldoc-documentation-function
    
    Use eldoc--documentation-strategy-defcustom to decide in which
    direction to make the variable alias, depending on the Emacs version.
    
    * lisp/emacs-lisp/eldoc.el
    (eldoc--documentation-strategy-defcustom): Helper macro.
    (eldoc-documentation-strategy, eldoc-documentation-function): Use it.
    (Version): Bump to 1.5.0

diff --git a/lisp/emacs-lisp/eldoc.el b/lisp/emacs-lisp/eldoc.el
index 7964c4c45a..ccae09a906 100644
--- a/lisp/emacs-lisp/eldoc.el
+++ b/lisp/emacs-lisp/eldoc.el
@@ -5,7 +5,7 @@
 ;; Author: Noah Friedman <friedman@splode.com>
 ;; Keywords: extensions
 ;; Created: 1995-10-06
-;; Version: 1.4.0
+;; Version: 1.5.0
 ;; Package-Requires: ((emacs "26.3"))
 
 ;; This is a GNU ELPA :core package.  Avoid functionality that is not
@@ -535,10 +535,27 @@ eldoc-documentation-enthusiast
                         (if (stringp str) (funcall callback str))
                         nil))))
 
-(define-obsolete-variable-alias 'eldoc-documentation-function
-  'eldoc-documentation-strategy "eldoc-1.1.0")
-
-(defcustom eldoc-documentation-strategy #'eldoc-documentation-default
+;; JT@2020-07-10: Eldoc is pre-loaded, so in in Emacs < 28 we can't
+;; make the "old" `eldoc-documentation-function' point to the new
+;; `eldoc-documentation-strategy', so we do the reverse.  This allows
+;; for Eldoc to be loaded in those older Emacs versions and work with
+;; whomever (major-modes, extensions, ueser) sets one of the other
+;; variable.
+(defmacro eldoc--documentation-strategy-defcustom
+    (main secondary value docstring &rest more)
+  "Defcustom helper macro for sorting `eldoc-documentation-strategy'."
+  (declare (indent 2))
+  `(if (< emacs-major-version 28)
+       (progn
+         (defcustom ,secondary ,value ,docstring ,@more)
+         (defvaralias ',main ',secondary ,docstring))
+       (progn
+         (defcustom ,main ,value ,docstring  ,@more)
+         (defvaralias ',secondary ',main ,docstring))))
+
+(eldoc--documentation-strategy-defcustom eldoc-documentation-strategy
+    eldoc-documentation-function
+  #'eldoc-documentation-default
   "How to collect and organize results of `eldoc-documentation-functions'.
 
 This variable controls how `eldoc-documentation-functions', which

Re: master 9ade7ea: Fix Eldoc problem when loading on Emacs 26.3

[Stefan Monnier]

>> Maybe come up with a better idea, or maybe, like Stefan suggests, don't
>> make it an alias at all, just deprecate it and keep using it instead of
>> eldoc-documentation-strategy when we find it to be bound.
>
> That sort of works, but it forces extensions that do want to
> "Package-Require" eldoc to remember to unbind
> eldoc-documentation-function if they are loaded on older Emacs versions,
> which is undesirable.

Maybe use a rule like:

    (if (and (buffer-local-p 'eldoc-documentation-function)
             (not (buffer-local-p 'eldoc-documentation-functions)))
        <Use `eldoc-documentation-function`>
      <Use `eldoc-documentation-functions`>


-- Stefan

Re: master 9ade7ea: Fix Eldoc problem when loading on Emacs 26.3

[João Távora]

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

>>> Maybe come up with a better idea, or maybe, like Stefan suggests, don't
>>> make it an alias at all, just deprecate it and keep using it instead of
>>> eldoc-documentation-strategy when we find it to be bound.
>>
>> That sort of works, but it forces extensions that do want to
>> "Package-Require" eldoc to remember to unbind
>> eldoc-documentation-function if they are loaded on older Emacs versions,
>> which is undesirable.
>
> Maybe use a rule like:
>
>     (if (and (buffer-local-p 'eldoc-documentation-function)
>              (not (buffer-local-p 'eldoc-documentation-functions)))
>         <Use `eldoc-documentation-function`>
>       <Use `eldoc-documentation-functions`>

Sounds a bit complicated for me, especially since there's already a
eldoc--supported-p which does similar heuristics.  Wouldn't it break in
some older Emacs version that loads the new Eldoc and has some global
value in eldoc-documentation-functions (say, a spell-checking eldoc
backend)?

For now, I'd like to go with the alias/reverse-alias technique, which is
the simplest, I think, for this simple problem.  I've tested it and it
works correctly.  Have you seen the patch?  Do you see any blockers
there?  We can move on to some more sophisticated version later.

João

Re: master 9ade7ea: Fix Eldoc problem when loading on Emacs 26.3

[Stefan Monnier]

>>>> Maybe come up with a better idea, or maybe, like Stefan suggests, don't
>>>> make it an alias at all, just deprecate it and keep using it instead of
>>>> eldoc-documentation-strategy when we find it to be bound.
>>> That sort of works, but it forces extensions that do want to
>>> "Package-Require" eldoc to remember to unbind
>>> eldoc-documentation-function if they are loaded on older Emacs versions,
>>> which is undesirable.

No.  If they're only written for the old API they set
`eldoc-documentation-function` and live in blissful ignorance of what
they miss.  If they're only written for the new API, they add-hook to
`eldoc-documentation-functions` and live in bliss.

And if they want to support both they do

    (if (boundp 'eldoc-documentation-functions)
        (add-hook 'eldoc-documentation-functions ...)
      (setq eldoc-documentation-function ...))

>> Maybe use a rule like:
>>
>>     (if (and (buffer-local-p 'eldoc-documentation-function)
>>              (not (buffer-local-p 'eldoc-documentation-functions)))
>>         <Use `eldoc-documentation-function`>
>>       <Use `eldoc-documentation-functions`>
>
> Sounds a bit complicated for me, especially since there's already a
> eldoc--supported-p which does similar heuristics.

But that's one of the other benefits.  It lets us distinguish in
`eldoc--supported-p` the case where `eldoc-documentation-strategy` was
set from the case where  `eldoc-documentation-function` was set.

> Wouldn't it break in some older Emacs version that loads the new Eldoc
> and has some global value in eldoc-documentation-functions (say,
> a spell-checking eldoc backend)?

The above `buffer-local-p` tests would be in eldoc.el, not in the client
packages, so I don't see what scenario you're worried about.
And of course the presence of a global value doesn't affect
`buffer-local-p` anyway, so ... I guess I must be missing something?


        Stefan

Re: master 9ade7ea: Fix Eldoc problem when loading on Emacs 26.3

[João Távora]

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

>>>>> Maybe come up with a better idea, or maybe, like Stefan suggests, don't
>>>>> make it an alias at all, just deprecate it and keep using it instead of
>>>>> eldoc-documentation-strategy when we find it to be bound.
>>>> That sort of works, but it forces extensions that do want to
>>>> "Package-Require" eldoc to remember to unbind
>>>> eldoc-documentation-function if they are loaded on older Emacs versions,
>>>> which is undesirable.
>
> No.  If they're only written for the old API they set
> `eldoc-documentation-function` and live in blissful ignorance of what
> they miss.  If they're only written for the new API, they add-hook to
> `eldoc-documentation-functions` and live in bliss.

I was describing my own earlier solution, by the way, where I'm pretty
sure that would happen.

> And if they want to support both they do
>
>     (if (boundp 'eldoc-documentation-functions)
>         (add-hook 'eldoc-documentation-functions ...)
>       (setq eldoc-documentation-function ...))

>>> Maybe use a rule like:
>>>
>>>     (if (and (buffer-local-p 'eldoc-documentation-function)
>>>              (not (buffer-local-p 'eldoc-documentation-functions)))
>>>         <Use `eldoc-documentation-function`>
>>>       <Use `eldoc-documentation-functions`>
>>
>> Sounds a bit complicated for me, especially since there's already a
>> eldoc--supported-p which does similar heuristics.
>
> But that's one of the other benefits.  It lets us distinguish in
> `eldoc--supported-p` the case where `eldoc-documentation-strategy` was
> set from the case where  `eldoc-documentation-function` was set.
>
>> Wouldn't it break in some older Emacs version that loads the new Eldoc
>> and has some global value in eldoc-documentation-functions (say,
>> a spell-checking eldoc backend)?
>
> The above `buffer-local-p` tests would be in eldoc.el, not in the client
> packages, so I don't see what scenario you're worried about.
> And of course the presence of a global value doesn't affect
> `buffer-local-p` anyway, so ... I guess I must be missing something?

Maybe I am, surely.  This is complicated, and I'm probably not seeing
through the complexity as well as you are.  If you want to do these
changes I won't object, but I'd probably mess them up.

At least for now, I really do think the best way is to alias the two
variables.  Whichever way we alias them, it's just as if I never renamed
the singular "e-d-function" to "e-d-strategy" at all, which is, after
all, a much safer bet.  I've even thought about dropping the whole
"rename function to strategy" idea completely, which was just a cosmetic
way to escape the awful mind-bending confusion between "e-d-function"
singular and "e-d-functions" plural.

So I think I'm going to go with the simple alias patch, unless you (or
someone) strongly opposes.

João

Re: master 9ade7ea: Fix Eldoc problem when loading on Emacs 26.3

[Stefan Monnier]

> all, a much safer bet.  I've even thought about dropping the whole
> "rename function to strategy" idea completely, which was just a cosmetic
> way to escape the awful mind-bending confusion between "e-d-function"
> singular and "e-d-functions" plural.

The reason why I was happy to see you use a different name is to make it
possible for eldoc--supported-p to distinguish the case where something
changed the -strategy var (which shouldn't affect
eldoc--supported-p) from the case where something changed
the -function var.


        Stefan

Re: master 9ade7ea: Fix Eldoc problem when loading on Emacs 26.3

[João Távora]

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

>> all, a much safer bet.  I've even thought about dropping the whole
>> "rename function to strategy" idea completely, which was just a cosmetic
>> way to escape the awful mind-bending confusion between "e-d-function"
>> singular and "e-d-functions" plural.
>
> The reason why I was happy to see you use a different name is to make it
> possible for eldoc--supported-p to distinguish the case where something
> changed the -strategy var (which shouldn't affect
> eldoc--supported-p) from the case where something changed
> the -function var.

Yes, I know that now, and I don't oppose it.  I just don't feel
capacitated enough to separated the two things without mixing myself up
:-).  I drafted up this patch yesterday, but it's got this problem I
mentioned.  Maybe you can fix it.  In the meantime, I'm going to go with
the alias.

João

commit c40761d683dcf70fe7f404b675a3a2631edfeaf0
Author: João Távora <joaotavora@gmail.com>
Date:   Fri Jul 10 19:11:01 2020 +0100

    Adjust Eldoc backward compatibility of eldoc-documentation-function
    
    Making a variable alias doesn't work because that variable is
    preloaded and localized.  So just keep the
    eldoc-documentation-function, make it obsolete, and let it be used
    instead of strategy if it is found to be non-nil (either locally or
    globally).
    
    * lisp/emacs-lisp/eldoc.el (eldoc--eval-expression-setup): Set
    eldoc-documentation-function to nil.
    (eldoc-documentation-function): Reinstate variable.
    (eldoc--documentation-strategy): New helper function.
    (eldoc--invoke-strategy): Use eldoc--documentation-strategy.
    (eldoc--supported-p): Reowkr
    (Version): Bump to 1.5.0
    eldoc--supported-p

diff --git a/lisp/emacs-lisp/eldoc.el b/lisp/emacs-lisp/eldoc.el
index 7964c4c45a..29007807aa 100644
--- a/lisp/emacs-lisp/eldoc.el
+++ b/lisp/emacs-lisp/eldoc.el
@@ -5,7 +5,7 @@
 ;; Author: Noah Friedman <friedman@splode.com>
 ;; Keywords: extensions
 ;; Created: 1995-10-06
-;; Version: 1.4.0
+;; Version: 1.5.0
 ;; Package-Requires: ((emacs "26.3"))
 
 ;; This is a GNU ELPA :core package.  Avoid functionality that is not
@@ -233,7 +233,8 @@ eldoc--eval-expression-setup
             #'elisp-eldoc-var-docstring nil t)
   (add-hook 'eldoc-documentation-functions
             #'elisp-eldoc-funcall nil t)
-  (setq eldoc-documentation-strategy 'eldoc-documentation-default)
+  (setq-local eldoc-documentation-function nil)
+  (setq-local eldoc-documentation-strategy 'eldoc-documentation-default)
   (eldoc-mode +1))
 
 ;;;###autoload
@@ -535,8 +536,11 @@ eldoc-documentation-enthusiast
                         (if (stringp str) (funcall callback str))
                         nil))))
                         
-(define-obsolete-variable-alias 'eldoc-documentation-function
-  'eldoc-documentation-strategy "eldoc-1.1.0")
+(make-obsolete-variable
+ 'eldoc-documentation-function
+ "use `eldoc-documentation-strategy' instead." "eldoc-1.5.0")
+
+(defvar eldoc-documentation-function nil)
 
 (defcustom eldoc-documentation-strategy #'eldoc-documentation-default
   "How to collect and organize results of `eldoc-documentation-functions'.
@@ -584,9 +588,14 @@ eldoc-documentation-strategy
                 (function :tag "Other function"))
   :version "28.1")
 
+(defun eldoc--documentation-strategy ()
+  "Return the actual "
+  (or (bound-and-true-p eldoc-documentation-function)
+      eldoc-documentation-strategy))
+
 (defun eldoc--supported-p ()
   "Non-nil if an ElDoc function is set for this buffer."
-  (and (not (memq eldoc-documentation-strategy '(nil ignore)))
+  (and (not (memq (eldoc--documentation-strategy) '(nil ignore)))
        (or eldoc-documentation-functions
            ;; The old API had major modes set `eldoc-documentation-function'
            ;; to provide eldoc support.  It's impossible now to determine
@@ -596,7 +605,8 @@ eldoc--supported-p
            ;; `eldoc-documentation-functions' (as in the new API).
            ;; But at least if it's set buffer-locally it's a good hint that
            ;; there's some eldoc support in the current buffer.
-           (local-variable-p 'eldoc-documentation-strategy))))
+           (local-variable-p 'eldoc-documentation-strategy)
+           (local-variable-p 'eldoc-documentation-function))))
 
 (defvar eldoc--enthusiasm-curbing-timer nil
   "Timer used by the `eldoc-documentation-enthusiast' strategy.
@@ -703,7 +713,7 @@ eldoc--invoke-strategy
                        (display-doc)
                        t))))))
             (let* ((eldoc--make-callback #'make-callback)
-                   (res (funcall eldoc-documentation-strategy)))
+                   (res (funcall (eldoc--documentation-strategy))))
               ;; Observe the old and the new protocol:
               (cond (;; Old protocol: got string, output immediately;
                      (stringp res) (register-doc 0 res nil) (display-doc))