Unsafe local variable in eglot.el

Unsafe local variable in eglot.el

[Eli Zaretskii]

João,

This recent change:

  commit 0a4b1c0102d4062d24e19340f863b9df25e07ab3
  Author:     Joao Távora <joaotavora@gmail.com>
  AuthorDate: Wed Mar 1 13:24:07 2023 +0000
  Commit:     Joao Távora <joaotavora@gmail.com>
  CommitDate: Wed Mar 1 13:27:06 2023 +0000

      ; Eglot: improve bug-reference-url-format/bug-reference-url-regexp

      * lisp/progmodes/eglot.el (eglot--debbugs-or-github-bug-uri): New helper.

causes trouble when visiting eglot.el.

First, this pops up the "variable may not be safe" buffer about the
variable bug-reference-url-format, even if I use Emacs 29, let alone
if I use older Emacs versions.

And second, if I say "y" to the "apply unsafe variable" prompt, then
with the following jit-lock settings:

  (setq jit-lock-stealth-time 16)
  (setq jit-lock-stealth-nice 0.5)
  (setq jit-lock-stealth-verbose t)
  (setq jit-lock-defer-contextually t)
  (setq jit-lock-stealth-load 20)

I get an error from jit-lock-stealth's timer:

  Error running timer ‘jit-lock-stealth-fontify’: (void-function eglot--debbugs-or-github-bug-uri)

The only way of avoiding these two issues is to say "n" to the prompt
asking whether to apply the variable, but that's not really a good
workaround, is it?

Can these problems be solved, please, preferably in a way that older
Emacsen will also be happy (since Eglot is an ELPA package)?

Re: Unsafe local variable in eglot.el

[João Távora]

On Thu, Mar 9, 2023 at 8:00 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> João,
>
> This recent change:
>
>   commit 0a4b1c0102d4062d24e19340f863b9df25e07ab3
>   Author:     Joao Távora <joaotavora@gmail.com>
>   AuthorDate: Wed Mar 1 13:24:07 2023 +0000
>   Commit:     Joao Távora <joaotavora@gmail.com>
>   CommitDate: Wed Mar 1 13:27:06 2023 +0000
>
>       ; Eglot: improve bug-reference-url-format/bug-reference-url-regexp
>
>       * lisp/progmodes/eglot.el (eglot--debbugs-or-github-bug-uri): New helper.
>
> causes trouble when visiting eglot.el.

I've reproduced it with

  cd path/to/Emacs
  src/emacs -Q lisp/progmodes/eglot.el

But curiously, this doesn't reproduce it

  src/emacs -Q
  C-h f eglot RET
  C-x o TAB RET  ;; to visit the "eglot.el" link

Why is that?

Anyway, I just followed the docstring:

  bug-reference-url-format is a variable defined in `bug-reference.el'.

  Its value is `eglot--debbugs-or-github-bug-uri'
  Local in buffer eglot.el; global value is nil

  Format used to turn a bug number into a URL.
  The bug number is supplied as a string, so this should have a single %s.
  This can also be a function designator; it is called without arguments
   and should return a string.
  It can use `match-string' to get parts matched against
  `bug-reference-bug-regexp', specifically:
   1. issue kind (bug, patch, rfe &c)
   2. issue number.

  There is no default setting for this, it must be set per file.
  If you set it to a symbol in the file Local Variables section,
  you need to add a `bug-reference-url-format' property to it:
  (put 'my-bug-reference-url-format 'bug-reference-url-format t)
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  so that it is considered safe, see `enable-local-variables'.

> First, this pops up the "variable may not be safe" buffer about the
> variable bug-reference-url-format, even if I use Emacs 29, let alone
> if I use older Emacs versions.
>
> And second, if I say "y" to the "apply unsafe variable" prompt, then
> with the following jit-lock settings:
>
>   (setq jit-lock-stealth-time 16)
>   (setq jit-lock-stealth-nice 0.5)
>   (setq jit-lock-stealth-verbose t)
>   (setq jit-lock-defer-contextually t)
>   (setq jit-lock-stealth-load 20)
>
> I get an error from jit-lock-stealth's timer:
>
>   Error running timer ‘jit-lock-stealth-fontify’: (void-function eglot--debbugs-or-github-bug-uri)
>
> The only way of avoiding these two issues is to say "n" to the prompt
> asking whether to apply the variable, but that's not really a good
> workaround, is it?
>
> Can these problems be solved, please, preferably in a way that older
> Emacsen will also be happy (since Eglot is an ELPA package)?

Maybe the function can be autoloaded?

But is a significant number of people using older emacsen to edit
Emacs's lisp/progmodes/eglot.el?  As far as I've seen just now,
if you launch an older Emacs to edit Emacs code, you warnings
about unsafe local variables in Emacs's own top-evel .dir-locals.el.

Until a solution is found, feel free to revert this change,
because this is just a minor convenience, and mostly just
for me (though it _is_ quite convenient).

João

Re: Unsafe local variable in eglot.el

[João Távora]

On Thu, Mar 9, 2023 at 9:31 AM João Távora <joaotavora@gmail.com> wrote:

> But curiously, this doesn't reproduce it
>
>   src/emacs -Q
>   C-h f eglot RET
>   C-x o TAB RET  ;; to visit the "eglot.el" link
>
> Why is that?

It's probably because C-h v loads the file to show the
docstring (did it always do that?)

>
> Maybe the function can be autoloaded?

I played a bit and this patch seems to fix it.  Can you test?

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 2491c86ea5b..94cc86d1e97 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3666,12 +3666,14 @@ eglot-inlay-hints-mode

 ;;; Misc
 ;;;
-(defun eglot--debbugs-or-github-bug-uri ()
-  (format (if (string= (match-string 2) "github")
-              "https://github.com/joaotavora/eglot/issues/%s"
-            "https://debbugs.gnu.org/%s")
-          (match-string 3)))
-(put 'eglot--debbugs-or-github-bug-uri 'bug-reference-url-format t)
+;;;###autoload
+(progn
+  (put 'eglot--debbugs-or-github-bug-uri 'bug-reference-url-format t)
+  (defun eglot--debbugs-or-github-bug-uri ()
+    (format (if (string= (match-string 2) "github")
+                "https://github.com/joaotavora/eglot/issues/%s"
+              "https://debbugs.gnu.org/%s")
+            (match-string 3))))

 ;;; Obsolete
 ;;;

Re: Unsafe local variable in eglot.el

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 9 Mar 2023 09:31:36 +0000
> Cc: emacs-devel@gnu.org
> 
> On Thu, Mar 9, 2023 at 8:00 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > João,
> >
> > This recent change:
> >
> >   commit 0a4b1c0102d4062d24e19340f863b9df25e07ab3
> >   Author:     Joao Távora <joaotavora@gmail.com>
> >   AuthorDate: Wed Mar 1 13:24:07 2023 +0000
> >   Commit:     Joao Távora <joaotavora@gmail.com>
> >   CommitDate: Wed Mar 1 13:27:06 2023 +0000
> >
> >       ; Eglot: improve bug-reference-url-format/bug-reference-url-regexp
> >
> >       * lisp/progmodes/eglot.el (eglot--debbugs-or-github-bug-uri): New helper.
> >
> > causes trouble when visiting eglot.el.
> 
> I've reproduced it with
> 
>   cd path/to/Emacs
>   src/emacs -Q lisp/progmodes/eglot.el
> 
> But curiously, this doesn't reproduce it
> 
>   src/emacs -Q
>   C-h f eglot RET
>   C-x o TAB RET  ;; to visit the "eglot.el" link
> 
> Why is that?

I don't know.  Maybe the latter already has bug-reference.el loaded?

> Anyway, I just followed the docstring:
> 
>   bug-reference-url-format is a variable defined in `bug-reference.el'.
> 
>   Its value is `eglot--debbugs-or-github-bug-uri'
>   Local in buffer eglot.el; global value is nil
> 
>   Format used to turn a bug number into a URL.
>   The bug number is supplied as a string, so this should have a single %s.
>   This can also be a function designator; it is called without arguments
>    and should return a string.
>   It can use `match-string' to get parts matched against
>   `bug-reference-bug-regexp', specifically:
>    1. issue kind (bug, patch, rfe &c)
>    2. issue number.
> 
>   There is no default setting for this, it must be set per file.
>   If you set it to a symbol in the file Local Variables section,
>   you need to add a `bug-reference-url-format' property to it:
>   (put 'my-bug-reference-url-format 'bug-reference-url-format t)
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   so that it is considered safe, see `enable-local-variables'.

Maybe for the above to work it requires bug-reference.el to be loaded?
Tassilo, any ideas?

> But is a significant number of people using older emacsen to edit
> Emacs's lisp/progmodes/eglot.el?

I do that all the time, since my production sessions always run a
released Emacs.

Moreover, since Eglot is an ELPA package, I'm quite sure people who
use Emacs 28 would do that.

Re: Unsafe local variable in eglot.el

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli, João & Sam,

Sam, for the context: when you find eglot.el in the current emacs
master, it'll trigger a query because of the eglot.el local variable
setting

--8<---------------cut here---------------start------------->8---
;; Local Variables:
;; bug-reference-bug-regexp: "\\(\\(github\\|bug\\)#\\([0-9]+\\)\\)"
;; bug-reference-url-format: eglot--debbugs-or-github-bug-uri
;; checkdoc-force-docstrings-flag: nil
;; End:
--8<---------------cut here---------------end--------------->8---

(Now probably this mail will do the same. :-))

>> I've reproduced it with
>> 
>>   cd path/to/Emacs
>>   src/emacs -Q lisp/progmodes/eglot.el
>> 
>> But curiously, this doesn't reproduce it
>> 
>>   src/emacs -Q
>>   C-h f eglot RET
>>   C-x o TAB RET  ;; to visit the "eglot.el" link
>> 
>> Why is that?

I can reproduce it, too.

> I don't know.  Maybe the latter already has bug-reference.el loaded?

I think the reason is that bug-reference-url-format is a safe local
variable if it is

  (a) a string, or
  (b) a symbol which itself has the bug-reference-url-format property
      set to a non-nil value

When you find eglot.el the function eglot--debbugs-or-github-bug-uri
defined in eglot.el doesn't have the property because eglot.el itself is
not loaded yet.

When you visit eglot.el following a help link, it seems eglot gets
loaded, and therefore the bug-reference-url-format function
eglot--debbugs-or-github-bug-uri is defined and has the right property.

Not sure what to do here.  Maybe adding an autoload-cookie to
eglot--debbugs-or-github-bug-uri does the trick?  Of course, then you
cannot find eglot.el without loading it at the same time...

I've added Sam to the Cc because he added the (b) clause.  Before the
bug url format has been considered safe if it was string or function but
I think even that would trigger the issue.

Bye,
Tassilo

Re: Unsafe local variable in eglot.el

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 9 Mar 2023 11:24:37 +0000
> Cc: emacs-devel@gnu.org
> 
> I played a bit and this patch seems to fix it.  Can you test?

I suggest to install this, and I will test then.

Thanks.

Re: Unsafe local variable in eglot.el

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> Why is that?
>
> I don't know.  Maybe the latter already has bug-reference.el loaded?

See my other email.  I think it's because C-h f autoloads the file early
to show the docstring.

> Moreover, since Eglot is an ELPA package, I'm quite sure people who
> use Emacs 28 would do that.

But probably only to peruse the file, not to edit it, as this would mean
editing a released version.

Anyway, I think the autoload-patch I provided in the other mail should
at least solve the jit-lock-related problem you found in for those
versions, as package-initialize would load the autoloads file.

João

Re: Unsafe local variable in eglot.el

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Date: Thu, 9 Mar 2023 11:24:37 +0000
>> Cc: emacs-devel@gnu.org
>> 
>> I played a bit and this patch seems to fix it.  Can you test?
>
> I suggest to install this, and I will test then.

Done.

Re: Unsafe local variable in eglot.el

[João Távora]

Tassilo Horn <tsdh@gnu.org> writes:

> Not sure what to do here.  Maybe adding an autoload-cookie to
> eglot--debbugs-or-github-bug-uri does the trick?  Of course, then you
> cannot find eglot.el without loading it at the same time...

See my 01b65d442a commit to Emacs 29.  It's autoloading a "progn" block
in eglot.el.  So visiting the file with bug-reference-mode does trigger
a call to the helper function, which now exists.  But that call by
itself won't cause eglot.el to to be loaded.  It's not very elegant, but
it does solve the program effectively, I think (and the links work, of
course).

João

Re: Unsafe local variable in eglot.el

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Thu, 09 Mar 2023 13:07:52 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: João Távora <joaotavora@gmail.com>
> >> Date: Thu, 9 Mar 2023 11:24:37 +0000
> >> Cc: emacs-devel@gnu.org
> >> 
> >> I played a bit and this patch seems to fix it.  Can you test?
> >
> > I suggest to install this, and I will test then.
> 
> Done.

Thanks, the problem is gone, AFAICT (with Emacs 29 only, of course).

Re: Unsafe local variable in eglot.el

[Tassilo Horn]

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

>> Not sure what to do here.  Maybe adding an autoload-cookie to
>> eglot--debbugs-or-github-bug-uri does the trick?  Of course, then you
>> cannot find eglot.el without loading it at the same time...
>
> See my 01b65d442a commit to Emacs 29.  It's autoloading a "progn"
> block in eglot.el.  So visiting the file with bug-reference-mode does
> trigger a call to the helper function, which now exists.  But that
> call by itself won't cause eglot.el to to be loaded.  It's not very
> elegant, but it does solve the program effectively, I think (and the
> links work, of course).

Ah, and indeed that's the documented way according to (info "(elisp)
File Local Variables"):

--8<---------------cut here---------------start------------->8---
   When defining a user option using ‘defcustom’, you can set its
‘safe-local-variable’ property by adding the arguments ‘:safe FUNCTION’
to ‘defcustom’ (*note Variable Definitions::).  However, a safety
predicate defined using ‘:safe’ will only be known once the package that
contains the ‘defcustom’ is loaded, which is often too late.  As an
alternative, you can use the autoload cookie (*note Autoload::) to
assign the option its safety predicate, like this:

     ;;;###autoload (put 'VAR 'safe-local-variable 'PRED)

The safe value definitions specified with ‘autoload’ are copied into the
package’s autoloads file (‘loaddefs.el’ for most packages bundled with
Emacs), and are known to Emacs since the beginning of a session.
--8<---------------cut here---------------end--------------->8---

Great that it works now as intended.

Bye,
Tassilo