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

[Stefan Monnier <monnier@iro.umontreal.ca>]

>>> 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