bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

Basil L. Contovounesios
Alex Branham <[hidden email]> writes:

> Emacs 27.1 ships with eldoc that defines eldoc-documentation-function as
> #'ignore. If users install eglot from ELPA, this pulls in an updated
> eldoc that attempts to redefine eldoc-documentation-function. However,
> the value does not change (since eldoc-documentation-function is already
> bound, defcustom does not update it when called in
> eldoc--documentation-strategy-defcustom) so the value is still ignore,
> meaning users get:
>
>     There is no ElDoc support in this buffer
>
> since eldoc-documentation-function is #'ignore, regardless of the value
> of eldoc-documentation-functions.
>
> Here's the bug report we received with the original issue:
> https://github.com/emacs-ess/ESS/issues/1130

Thanks, CCing João.

--
Basil



Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

Alex Branham
On Fri 26 Mar 2021 at 18:50, João Távora <[hidden email]> wrote:

> Alex, exactly under what circunstances do you see that "There is no
> ElDoc support in this buffer"?  In what buffer, doing what?  I
> appreciate that you have investigated the implementation, but I need to
> be able to understand -- and potentially reproduce -- the problem
> exactly as it happened to you, so please provide as clear a recipe as
> possible, perhaps starting with "downloaded Emacs 27.1, then cleared my
> ELPA directory, ..., installed Eglot from xyz, ..., ".

Sure thing (though note I'm not the person who originally ran into the
bug, just was able to reproduce). From a clean install of 27.1 (no init
file, no packages), you should be able to reproduce by:

1. Add MELPA to package-archives
2. Install ESS
3. Update eldoc (potentially by installing eglot)
4. Restart Emacs
5. (require 'eglot)
6. Open an R file (eg ~/test.R)
7. At this point, eldoc-documentation-function will be #'ignore so eg
   M-x eldoc-mode will show "There is no ElDoc support in this buffer".

That's because ESS sets up eldoc in R buffers like so:

  (if (boundp 'eldoc-documentation-functions)
      (add-hook 'eldoc-documentation-functions #'ess-r-eldoc-function nil t)
    (add-function :before-until (local 'eldoc-documentation-function)
                  #'ess-r-eldoc-function))

so we don't touch eldoc-documentation-function in newer eldoc versions
where it is supposed to be a user-facing customization.

> Meanwhile (but not excluding the earlier valuable exercise), can you
> show the output of:
>
>   C-h v eldoc-documentation-function RET
>   C-h v eldoc-documentation-strategy RET

Unfortunately I can't at the moment but from memory
eldoc-documentation-strategy was an obsolete alias for
eldoc-documentation-function and the value was the function ignore.

Hope that helps.



Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

João Távora
Alex Branham <[hidden email]> writes:

> On Fri 26 Mar 2021 at 18:50, João Távora <[hidden email]> wrote:
>
>> Alex, exactly under what circunstances do you see that "There is no
>> ElDoc support in this buffer"?  In what buffer, doing what?  I
>> appreciate that you have investigated the implementation, but I need to
>> be able to understand -- and potentially reproduce -- the problem

> 7. At this point, eldoc-documentation-function will be #'ignore so eg
                                                       ^^^^^^^^
So this is where I'm completely baffled.  You say this ||||||||

But earlier you said, accurately, in fact it was your first sentence, that:

>>>> Emacs 27.1 ships with eldoc that defines
>>>> eldoc-documentation-function as #'ignore.

So if it was #'ignore and still is #'ignore what exactly is the
difference that (require 'eldoc) or (require 'eglot) is introducing??

João



Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

Alex Branham
Because then we modify el-doc-functions because of the newer version of eldoc loaded. 

But the ignore value of el-doc-function prevents eldoc from working

Hopefully that helps, sorry for the brevity am on phone

On Fri, Mar 26, 2021, 3:57 PM João Távora <[hidden email]> wrote:
Alex Branham <[hidden email]> writes:

> On Fri 26 Mar 2021 at 18:50, João Távora <[hidden email]> wrote:
>
>> Alex, exactly under what circunstances do you see that "There is no
>> ElDoc support in this buffer"?  In what buffer, doing what?  I
>> appreciate that you have investigated the implementation, but I need to
>> be able to understand -- and potentially reproduce -- the problem

> 7. At this point, eldoc-documentation-function will be #'ignore so eg
                                                       ^^^^^^^^
So this is where I'm completely baffled.  You say this ||||||||

But earlier you said, accurately, in fact it was your first sentence, that:

>>>> Emacs 27.1 ships with eldoc that defines
>>>> eldoc-documentation-function as #'ignore.

So if it was #'ignore and still is #'ignore what exactly is the
difference that (require 'eldoc) or (require 'eglot) is introducing??

João
Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

João Távora
On Fri, Mar 26, 2021 at 8:24 PM Alex Branham <[hidden email]> wrote:
Because then we modify el-doc-functions because of the newer version of eldoc loaded. 

But the ignore value of el-doc-function prevents eldoc from working

Hopefully that helps, sorry for the brevity am on phone

Ah, I get it, so ess-mode does something different.  Then
maybe it should also set eldoc-documentation-strategy
to something sensible.  Eldoc-documentation-strategy is
just the new name of eldoc-documentation-function.
If you find an `ignore` there, you're safe to change it
, buffer-locally, as you did before.  Maybe you should
even do it even if you _don't_ find an ignore there.
The fact that it's now defcustom doesn't change much
in my opinion, I don't see defcustom's as off-limits to major/minor
modes, as long as the user is still given override power
in the mode hooks.

Alternatively, ess-mode should just use the old interface.
It's still available and functional. 

But I think you should select a strategy that suits `ess-mode`'s
functions. 

Hope this helps,
João

Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

Alex Branham
On Fri 26 Mar 2021 at 20:34, João Távora <[hidden email]> wrote:

> Ah, I get it, so ess-mode does something different.

I'm not sure I understand - different from what?

> Then maybe it should also set eldoc-documentation-strategy to
> something sensible. Eldoc-documentation-strategy is just the new name
> of eldoc-documentation-function. If you find an `ignore` there, you're
> safe to change it , buffer-locally, as you did before. Maybe you
> should even do it even if you _don't_ find an ignore there. The fact
> that it's now defcustom doesn't change much in my opinion, I don't see
> defcustom's as off-limits to major/minor modes, as long as the user is
> still given override power in the mode hooks.
>
> Alternatively, ess-mode should just use the old interface.
> It's still available and functional.
>
> But I think you should select a strategy that suits `ess-mode`'s
> functions.

So the recommended strategy to support 27.1+old eldoc, 27.1+new eldoc,
and 28.0+new eldoc is something like the below?

(if (function-equal #'ignore eldoc-documentation-function)
    (add-hook 'eldoc-documentation-functions #'ess-r-eldoc-function nil t)
  (add-function :before-until (local 'eldoc-documentation-function)
                #'ess-r-eldoc-function))



Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

Alex Branham
On Sat 27 Mar 2021 at 12:53, João Távora <[hidden email]> wrote:

> I'm not sure you're improving much here.  My idea is that,
> if you find eldoc-documentation-functions (plural) to be bound you
> may need to check that eldoc-documentation-strategy (or
> eldoc-documentation-function (singular)) ISN'T #ignore and
> buffer-locally adjust it accordingly to something of your preference
> or your users' preference.

Thanks! It sounds like the below is what you're suggesting modes do in
order to support all the possible eldoc+Emacs versions, let me know if
I've misunderstood.

(if (not (boundp 'eldoc-documentation-functions))
    ;; old eldoc
    (add-function :before-until (local 'eldoc-documentation-function)
                  #'ess-r-eldoc-function)
  ;; new eldoc
  (add-hook 'eldoc-documentation-functions #'ess-r-eldoc-function nil t)
  ;; new eldoc + Emacs 27.1
  (when (and (fboundp 'eldoc-documentation-default)
             (function-equal #'ignore eldoc-documentation-function))
    (setq-local eldoc-documentation-function #'eldoc-documentation-default)))

If that's right, it seems like a step backwards, ease-of-setup-wise at
least, from the simple

(add-function :before-until (local 'eldoc-documentation-function) #'ess-r-eldoc-function)

that we did before new eldoc. But maybe it's necessary, I haven't really
played around with the new eldoc features at all.

Thanks again!



Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

João Távora
Alex Branham <[hidden email]> writes:

> On Sat 27 Mar 2021 at 12:53, João Távora <[hidden email]> wrote:
>
>> I'm not sure you're improving much here.  My idea is that,
>> if you find eldoc-documentation-functions (plural) to be bound you
>> may need to check that eldoc-documentation-strategy (or
>> eldoc-documentation-function (singular)) ISN'T #ignore and
>> buffer-locally adjust it accordingly to something of your preference
>> or your users' preference.
>
> Thanks! It sounds like the below is what you're suggesting modes do in
> order to support all the possible eldoc+Emacs versions, let me know if
> I've misunderstood.
>
> (if (not (boundp 'eldoc-documentation-functions))
>     ;; old eldoc
>     (add-function :before-until (local 'eldoc-documentation-function)
>                   #'ess-r-eldoc-function)
>   ;; new eldoc
>   (add-hook 'eldoc-documentation-functions #'ess-r-eldoc-function nil t)
>   ;; new eldoc + Emacs 27.1
>   (when (and (fboundp 'eldoc-documentation-default)
>              (function-equal #'ignore eldoc-documentation-function))
>     (setq-local eldoc-documentation-function
> #'eldoc-documentation-default)))

You don't need to be as complex as this:

(if (not (boundp 'eldoc-documentation-strategy))
    (add-function :before-until (local 'eldoc-documentation-function)
                   #'ess-r-eldoc-function)
  (add-hook 'eldoc-documentation-functions #'ess-r-eldoc-function nil t)
  (when (function-equal #'ignore eldoc-documentation-function)
        (setq-local eldoc-documentation-function #'eldoc-documentation-default|compose|whatever))))

> If that's right, it seems like a step backwards, ease-of-setup-wise at
> least, from the simple
>
> (add-function :before-until (local 'eldoc-documentation-function)
> #'ess-r-eldoc-function)

You can still just that and call it a day.  Just that, because the new
eldoc in the yet-unreleased Emacs 28 is backward compatible.  I've tried
to explain this three times already in this exchange.

João




Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

Alex Branham
On Sun 28 Mar 2021 at 15:50, João Távora <[hidden email]> wrote:

> Alex Branham <[hidden email]> writes:
>
>> On Sat 27 Mar 2021 at 12:53, João Távora <[hidden email]> wrote:
>>
>> Thanks! It sounds like the below is what you're suggesting modes do in
>> order to support all the possible eldoc+Emacs versions, let me know if
>> I've misunderstood.
>>
>> (if (not (boundp 'eldoc-documentation-functions))
>>     ;; old eldoc
>>     (add-function :before-until (local 'eldoc-documentation-function)
>>                   #'ess-r-eldoc-function)
>>   ;; new eldoc
>>   (add-hook 'eldoc-documentation-functions #'ess-r-eldoc-function nil t)
>>   ;; new eldoc + Emacs 27.1
>>   (when (and (fboundp 'eldoc-documentation-default)
>>              (function-equal #'ignore eldoc-documentation-function))
>>     (setq-local eldoc-documentation-function
>> #'eldoc-documentation-default)))
>
> You don't need to be as complex as this:
>
> (if (not (boundp 'eldoc-documentation-strategy))
>     (add-function :before-until (local 'eldoc-documentation-function)
>                    #'ess-r-eldoc-function)
>   (add-hook 'eldoc-documentation-functions #'ess-r-eldoc-function nil t)
>   (when (function-equal #'ignore eldoc-documentation-function)
>         (setq-local eldoc-documentation-function #'eldoc-documentation-default|compose|whatever))))

The fboundp check will avoid a byte compiler warning on Emacs<28.

>> If that's right, it seems like a step backwards, ease-of-setup-wise at
>> least, from the simple
>>
>> (add-function :before-until (local 'eldoc-documentation-function)
>> #'ess-r-eldoc-function)
>
> You can still just that and call it a day.  Just that, because the new
> eldoc in the yet-unreleased Emacs 28 is backward compatible.  I've tried
> to explain this three times already in this exchange.

Thanks. I'd like to support the new way (so users can customize
eldoc-documentation-function) and the old way, which I thought I had
said previously. It seems like the above is the officially blessed way
to do both so I suppose that's what we'll go with.

Thanks again, closing this bug report.



Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

Stefan Monnier
In reply to this post by João Távora
>   (when (function-equal #'ignore eldoc-documentation-function)
>         (setq-local eldoc-documentation-function #'eldoc-documentation-default|compose|whatever))))

BTW, using code like the one above is tolerable, usually for historical
reasons, but new APIs should aim not to need such things: comparing
functions is fundamentally always a hack.


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

João Távora
In reply to this post by Alex Branham
Alex Branham <[hidden email]> writes:

>>> If that's right, it seems like a step backwards, ease-of-setup-wise at
>>> least, from the simple
>>>
>>> (add-function :before-until (local 'eldoc-documentation-function)
>>> #'ess-r-eldoc-function)
>>
>> You can still just that and call it a day.  Just that, because the new
>> eldoc in the yet-unreleased Emacs 28 is backward compatible.  I've tried
>> to explain this three times already in this exchange.
>
> Thanks. I'd like to support the new way (so users can customize
> eldoc-documentation-function) and the old way, which I thought I had
> said previously. It seems like the above is the officially blessed way
> to do both so I suppose that's what we'll go with.

No idea what the "above" is, let me just summarize the cases:

(1) You have Emacs <= 27.1 and no new eldoc.el package
(2) You have Emacs <= 27.1 and a new eldoc.el package
(3) You have Emacs >= 28

The typical (fboundp 'eldoc-documentation-functions) check works great
for cases 1 and 3: you support the new and the old way.  Now, sometime
in 2020 I introduced the possibility of delivering new Eldoc features to
released versions, via the Eldoc ELPA "core" package.  That gave rise to
case 2: it's offering a new possiblity to support new Eldoc features
early, it doesn't remove any possibilities.

So:

a) If you want to support the new Eldoc features in cases 2 and 3, you
   have to use the more complex snippet (with or without the
   byte-compiler warning, you decide if it bothers you)

b) If, on the other hand, you just want to support 1 and 3, just use a
   check on emacs-major-version, instead of the fboundp check.

c) If you're confident that your users won't be missing the new eldoc.el
   features in rss-mode specifically (there's a good chance they won't
   since it's presumably what they're used to in rss-mode), just use the
   simple :before-until snippet.  And if these users do install something
   like Eglot, it will take care to do the right thing.

d) If you want to only support 2 and 3, and join the wave of the future,
   you can: Just make the eldoc.el ELPA package a dependency of the
   rss-mode, much like Eglot.

So I think you have lots of options here.

João





Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

Stefan Monnier
In reply to this post by Stefan Monnier
> Note that that all this mess has to do with the silliness that is having
> eldoc.el be preloaded just becuase elisp-mode.el.  How would we go about
> fixing that?

We could stop preloading `elisp-mode`.


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

João Távora

On Sun, Mar 28, 2021 at 8:09 PM Stefan Monnier <[hidden email]> wrote:
> Note that that all this mess has to do with the silliness that is having
> eldoc.el be preloaded just becuase elisp-mode.el.  How would we go about
> fixing that?

We could stop preloading `elisp-mode`.

Even better, but can you say why it's even preloaded at all?
Certainly not somebody's whim, right?

João
Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

Stefan Monnier
>> > Note that that all this mess has to do with the silliness that is having
>> > eldoc.el be preloaded just becuase elisp-mode.el.  How would we go about
>> > fixing that?
>> We could stop preloading `elisp-mode`.
> Even better, but can you say why it's even preloaded at all?
> Certainly not somebody's whim, right?

The code for `emacs-lisp-mode` used to be in `lisp-mode.el` which also
contains various functions useful in all modes, so it's been preloaded
since before I started using Emacs.
When I moved (some of) the code to `progmodes/elisp-mode.el` I just
preserved that pre-existing situation.

I don't know for a fact, but based on my vague memory of what's in
`elisp-mode.el` I don't think there's a strong technical reason to
preload it.

IOW it's only preloaded because we expect it would be loaded sooner or
later anyway in many/most Emacs sessions.  I don't know if it's indeed
the case (tho it is clearly the case for *my* sessions), so you could
try to float the idea on `emacs-devel`.


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

Stefan Monnier
In reply to this post by Stefan Monnier
> Note that that all this mess has to do with the silliness that is having
> eldoc.el be preloaded just becuase elisp-mode.el.  How would we go about
> fixing that?

BTW, note also that the reason `eldoc.el` is preloaded is because
`global-eldoc-mode` is enabled by default, rather than because
`elisp-mode` is itself preloaded.


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#47388: 27.1; eldoc-documentation-function is ignore in updated eldoc

Stefan Monnier
In reply to this post by Stefan Monnier
>>>   (when (function-equal #'ignore eldoc-documentation-function)
>>>         (setq-local eldoc-documentation-function
>>> #'eldoc-documentation-default|compose|whatever))))
>>
>> BTW, using code like the one above is tolerable, usually for historical
>> reasons, but new APIs should aim not to need such things: comparing
>> functions is fundamentally always a hack.
>
> So is run-time version checking, the risk is mostly the same.

I just wanted to make sure that it's only needed for backward
compatibility reasons.


        Stefan