bug#12259: [Mathieu Boespflug] Add delete-trailing-whitespace to list of safe eval forms

bug#12259: [Mathieu Boespflug] Add delete-trailing-whitespace to list of safe eval forms

[Stefan Monnier]

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

Mistakenly sent to emacs-devel.


[-- Attachment #2: Type: message/rfc822, Size: 6729 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 1976 bytes --]

Hi,

I'm trying to add the delete-trailing-whitespace hook to the
.dir-locals.el of a project. Because .dir-locals.el does not support
adding hooks directly, I use an eval clause, as follows:

((nil
  (eval . (add-hook 'write-contents-functions 'delete-trailing-whitespace))))

write-contents-functions is a buffer local hook whereas write-file-hook
and before-save-hook are not, so the above does not tamper with the
user's preferences when editing files in other directories.

The above is problematic however, because Emacs 23 asks the user whether
to run this eval expression *every time the user opens a file in that
directory*. Emacs 24 is better because it allows the user to say "yes"
once and for all and have Emacs never ask again, but it still asks the
first time.

However, I have noticed that by default Emacs already blesses certain
eval forms as being safe in .dir-locals.el and in mode lines. Here is
the content of safe-local-eval-forms in emacs 23.1:

((add-hook (quote write-file-hooks) (quote time-stamp)))

and emacs 24.1:

((add-hook (quote write-file-hooks) (quote time-stamp))
 (add-hook (quote write-file-functions) (quote time-stamp))
 (add-hook (quote before-save-hook) (quote time-stamp)))

It seems as though, if evaluation forms that add 'time-stamp to various
hooks that all run around the time a file is saved are deemed safe by
default, surely evaluation forms that add 'delete-trailing-whitespace
should equally be deemed safe by default.

I have attached a patch at the end of this email that considers eval
forms that add 'delete-trailing-whitespace to various hooks safe by
default. But ideally this patch would be superseded by adding
a mechanism that allows .dir-locals.el to add predefined functions to
hooks (at least buffer local ones) without having to use eval. That way
we wouldn't have to write patches such as this one for every new
sensible stock function that people want to have executed on file saves.

Regards,

-- Mathieu


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2.1.2: 0001-files.el-say-adding-delete-trailing-whitespace-to-ho.patch --]
[-- Type: text/x-patch, Size: 1135 bytes --]

>From 5f71b0dc3bc3b09cfb58d26ca6643b4e4a013a31 Mon Sep 17 00:00:00 2001
From: Mathieu Boespflug <mboes@cs.mcgill.ca>
Date: Mon, 20 Aug 2012 14:25:49 -0400
Subject: [PATCH] files.el: say adding 'delete-trailing-whitespace to hooks is
 safe.

---
 lisp/files.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/files.el b/lisp/files.el
index 5caa468..0b6f60f 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -2837,7 +2837,9 @@ symbol and VAL is a value that is considered safe."
   ;; This should be here at least as long as Emacs supports write-file-hooks.
   '((add-hook 'write-file-hooks 'time-stamp)
     (add-hook 'write-file-functions 'time-stamp)
-    (add-hook 'before-save-hook 'time-stamp))
+    (add-hook 'before-save-hook 'time-stamp)
+    (add-hook 'write-file-functions 'delete-trailing-whitespace)
+    (add-hook 'write-content-functions 'delete-trailing-whitespace))
   "Expressions that are considered safe in an `eval:' local variable.
 Add expressions to this list if you want Emacs to evaluate them, when
 they appear in an `eval' local variable specification, without first
-- 
1.7.11.4

bug#12259: Add delete-trailing-whitespace to list of safe eval forms

[Stefan Monnier]

> It seems as though, if evaluation forms that add 'time-stamp to various
> hooks that all run around the time a file is saved are deemed safe by
> default, surely evaluation forms that add 'delete-trailing-whitespace
> should equally be deemed safe by default.

Agreed, thanks.

> I have attached a patch at the end of this email that considers eval
> forms that add 'delete-trailing-whitespace to various hooks safe by
> default.

Actually, I wonder whether we want to accept/encourage those uses
instead of (add-hook 'before-save-hook 'delete-trailing-whitespace).

IOW I think we should only add the before-save-hook version but not
the others (and I guess the same holds for time-stamp, tho we'll
probably keep the other ones for time-stamp for backward-compatibility
reasons).

> But ideally this patch would be superseded by adding a mechanism that
> allows .dir-locals.el to add predefined functions to hooks (at least
> buffer local ones) without having to use eval.

Why?

> That way we wouldn't have to write patches such as this one for every
> new sensible stock function that people want to have executed on
> file saves.

You don't have to write patches like this one.  You can just customize
safe-local-eval-forms.  There is a problem, indeed, tho: if you
customize this var and we later add things to it, you'll keep using your
customized version and won't benefit from the expanded list.
So we should keep the default value of safe-local-eval-forms as nil, and
allow things like those add-hook some other way (e.g. a new var).


        Stefan

bug#12259: Add delete-trailing-whitespace to list of safe eval forms

[Glenn Morris]

Stefan Monnier wrote:

> Actually, I wonder whether we want to accept/encourage those uses
> instead of (add-hook 'before-save-hook 'delete-trailing-whitespace).

OT: I wouldn't encourage that either. :)
Blind application of such a hook has removed trailing whitespace that
was supposed to be there in the Emacs sources a few times.

> You don't have to write patches like this one.  You can just customize
> safe-local-eval-forms.  There is a problem, indeed, tho: if you
> customize this var and we later add things to it, you'll keep using your
> customized version and won't benefit from the expanded list.
> So we should keep the default value of safe-local-eval-forms as nil, and
> allow things like those add-hook some other way (e.g. a new var).

(add-to-list 'safe-local-eval-forms ...)

There's also the long-standing Todo item to create a "diff-list" custom
type http://debbugs.gnu.org/7812, and use it for such things.

bug#12259: Add delete-trailing-whitespace to list of safe eval forms

[Stefan Monnier]

>> Actually, I wonder whether we want to accept/encourage those uses
>> instead of (add-hook 'before-save-hook 'delete-trailing-whitespace).
> OT: I wouldn't encourage that either. :)
> Blind application of such a hook has removed trailing whitespace that
> was supposed to be there in the Emacs sources a few times.

That's a different issue.  The question is not whether it's a good idea
for a user to use such a setting but:
- whether having such a setting in the file-(or directory-)local
  variables can be used as an attack vector.
- which hook to use.  And I believe before-save-hook is always the better
  choice here.

> (add-to-list 'safe-local-eval-forms ...)
> There's also the long-standing Todo item to create a "diff-list" custom
> type http://debbugs.gnu.org/7812, and use it for such things.

Indeed for safe-local-eval-forms a simple diff-list would be sufficient
since safe-local-eval-forms is really a set (implemented as a list) so
we don't need to worry about ordering/repetitions/...
Could someone provide a patch for that?


        Stefan

bug#12259: Add delete-trailing-whitespace to list of safe eval forms

[Mathieu Boespflug]

>> I have attached a patch at the end of this email that considers eval
>> forms that add 'delete-trailing-whitespace to various hooks safe by
>> default.
>
> Actually, I wonder whether we want to accept/encourage those uses
> instead of (add-hook 'before-save-hook 'delete-trailing-whitespace).

The problem with the method above is that before-save-hook isn't made
a buffer-local variable by hack-local-variables. Therefore, using
(add-hook 'before-save-hook 'delete-trailing-whitespace) causes
delete-trailing-whitespace to be run even for buffers that are not in
the directory hierarchy of .dir-locals.el.

This is undesirable because .dir-locals.el is often used by free
software projects to enforce a common set of guidelines and style for
editing code. Any changes to hooks should therefore ideally be directory
local, so as to apply only to those files that are part of some
particular free software repository.

> IOW I think we should only add the before-save-hook version but not
> the others (and I guess the same holds for time-stamp, tho we'll
> probably keep the other ones for time-stamp for backward-compatibility
> reasons).

(See above.)

>> But ideally this patch would be superseded by adding a mechanism that
>> allows .dir-locals.el to add predefined functions to hooks (at least
>> buffer local ones) without having to use eval.
>
> Why?

Because using eval for the purposes of adding new functions to hooks
feels overkill, and causes several problems. The
affecting-hooks-that-are-not-buffer-local problem is one of them.
Another problem is that there are many equivalent ways of modifying
a hook (using add-hook, using setq, etc), so adding new entries to
safe-local-eval-forms would never catch them all.

>> That way we wouldn't have to write patches such as this one for every
>> new sensible stock function that people want to have executed on
>> file saves.
>
> You don't have to write patches like this one.  You can just customize
> safe-local-eval-forms.  There is a problem, indeed, tho: if you
> customize this var and we later add things to it, you'll keep using your
> customized version and won't benefit from the expanded list.
> So we should keep the default value of safe-local-eval-forms as nil, and
> allow things like those add-hook some other way (e.g. a new var).

... and that's the third problem caused by using eval to set hooks.

Besides, customizing safe-local-eval-forms isn't a great solution in the
scenario discussed above: the whole point for a free software project to
have a .dir-locals.el at the root of the repo is so that none of the
(potentially hundreds of) developers of that project need to fiddle with
customize manually.

-- Mathieu

bug#12259: Add delete-trailing-whitespace to list of safe eval forms

[Stefan Monnier]

>>> I have attached a patch at the end of this email that considers eval
>>> forms that add 'delete-trailing-whitespace to various hooks safe by
>>> default.
>> Actually, I wonder whether we want to accept/encourage those uses
>> instead of (add-hook 'before-save-hook 'delete-trailing-whitespace).

> The problem with the method above is that before-save-hook isn't made
> a buffer-local variable by hack-local-variables. Therefore, using
> (add-hook 'before-save-hook 'delete-trailing-whitespace) causes
> delete-trailing-whitespace to be run even for buffers that are not in
> the directory hierarchy of .dir-locals.el.

Indeed, you have to use
  (add-hook 'before-save-hook 'delete-trailing-whitespace nil t)

With very few (historical) exceptions, all hooks are neither global-only
nor buffer-local-only, so the "nil t" args should always be used
for buffer-local settings.

So we have a bug in the current setting of safe-local-eval-forms.
  
>>> But ideally this patch would be superseded by adding a mechanism that
>>> allows .dir-locals.el to add predefined functions to hooks (at least
>>> buffer local ones) without having to use eval.
>> Why?
> Because using eval for the purposes of adding new functions to hooks
> feels overkill, and causes several problems. The
> affecting-hooks-that-are-not-buffer-local problem is one of them.
> Another problem is that there are many equivalent ways of modifying
> a hook (using add-hook, using setq, etc), so adding new entries to
> safe-local-eval-forms would never catch them all.

setq is a wrong way to modify a hook, and safe-local-eval-forms does not
need to catch them all, only to allow the ones that are known safe and
that are useful.

That fact that using eval is overkill doesn't matter, since
safe-local-eval-forms restricts this "overkill power" to something very
much less powerful.  

The shape of the setting has to be "<something>: <somethingelse>", so
for adding a function to a hook, it could be
"add-hook: (write-file-functions time-stamp)", but that's not terribly
more convenient than "eval: (add-hook 'write-file-functions 'time-stamp)"
while having the disadvantage that eval re-uses an existing syntax.

Now, admittedly, because of the `local' argument, the choice is really between

    add-hook: (write-file-functions time-stamp)
and
    eval: (add-hook 'write-file-functions 'time-stamp nil t)
or
    eval: (add-local-hook 'write-file-functions 'time-stamp)

I much prefer one of the last two since it is familiar to Elisp coders,
and for those for whom it's not familiar, it's a useful syntax to learn
since they can also use it in their .emacs.

>> You don't have to write patches like this one.  You can just customize
>> safe-local-eval-forms.  There is a problem, indeed, tho: if you
>> customize this var and we later add things to it, you'll keep using your
>> customized version and won't benefit from the expanded list.
>> So we should keep the default value of safe-local-eval-forms as nil, and
>> allow things like those add-hook some other way (e.g. a new var).
> ... and that's the third problem caused by using eval to set hooks.

No, the same problem would appear with a special "add-hook" setting,
since we'd need a new safe-local-add-hooks which would suffer from the
same complications.

> Besides, customizing safe-local-eval-forms isn't a great solution in the
> scenario discussed above: the whole point for a free software project to
> have a .dir-locals.el at the root of the repo is so that none of the
> (potentially hundreds of) developers of that project need to fiddle with
> customize manually.

There's no clearly safe subset of Elisp, so we're limited to listing
a few known safe cases which we know are used.  Note that adding an
element to safe-local-eval-forms is a lot easier than changing your
.emacs so that the files of project X (and only those files) are opened
with the right settings, so the use of .dir-local.el is still very
useful even if it has to use an eval form that's not in the default
value of safe-local-eval-forms.

I've just installed the patch below in the emacs-24 branch.


        Stefan


=== modified file 'lisp/files.el'
--- lisp/files.el	2012-08-15 16:29:11 +0000
+++ lisp/files.el	2012-08-23 12:15:31 +0000
@@ -2837,7 +2837,8 @@
   ;; This should be here at least as long as Emacs supports write-file-hooks.
   '((add-hook 'write-file-hooks 'time-stamp)
     (add-hook 'write-file-functions 'time-stamp)
-    (add-hook 'before-save-hook 'time-stamp))
+    (add-hook 'before-save-hook 'time-stamp nil t)
+    (add-hook 'before-save-hook 'delete-trailing-whitespace nil t))
   "Expressions that are considered safe in an `eval:' local variable.
 Add expressions to this list if you want Emacs to evaluate them, when
 they appear in an `eval' local variable specification, without first

bug#12259: Add delete-trailing-whitespace to list of safe eval forms

[Mathieu Boespflug]

> I've just installed the patch below in the emacs-24 branch.

Thanks Stefan. That should do the trick.

-- Mathieu

> === modified file 'lisp/files.el'
> --- lisp/files.el	2012-08-15 16:29:11 +0000
> +++ lisp/files.el	2012-08-23 12:15:31 +0000
> @@ -2837,7 +2837,8 @@
>    ;; This should be here at least as long as Emacs supports write-file-hooks.
>    '((add-hook 'write-file-hooks 'time-stamp)
>      (add-hook 'write-file-functions 'time-stamp)
> -    (add-hook 'before-save-hook 'time-stamp))
> +    (add-hook 'before-save-hook 'time-stamp nil t)
> +    (add-hook 'before-save-hook 'delete-trailing-whitespace nil t))
>    "Expressions that are considered safe in an `eval:' local variable.
>  Add expressions to this list if you want Emacs to evaluate them, when
>  they appear in an `eval' local variable specification, without first