Re: [PATCH] allow function values for `enable-local-eval'

Re: [PATCH] allow function values for `enable-local-eval'

[Richard Stallman]

    This patch allows users to set `enable-local-eval' to a function.  If
    the function returns non-nil, the eval happens, otherwise it does not.

That is a general basis for a solution, but it has no specific
knowledge; it requires that to be provided by the user.  I'm not
necessarily rejecting it, but it would be more helpful to do something
that embody specific knowledge, and does the right thing (for some
cases) without requiring user customization.

    We have a project where virtually every source file has some "eval:"
    forms.

Can you show me some of them?  Maybe we can simply allow these forms
just as the existing code already allows certain `put' expressions.

Re: [PATCH] allow function values for `enable-local-eval'

[Kim F. Storm]

Karl Fogel <kfogel@floss> writes:

> Richard Stallman <rms@gnu.org> writes:
> > That is a general basis for a solution, but it has no specific
> > knowledge; it requires that to be provided by the user.  I'm not
> > necessarily rejecting it, but it would be more helpful to do something
> > that embody specific knowledge, and does the right thing (for some
> > cases) without requiring user customization.
> 
> Yeah, we could provide such functionality if we can think of it, but I
> think it's still necessary to offer the general solution.

I agree with Karl that allowing the user to fully customize this would
be very useful.  I'm working on a project where the files belonging to
one of the modules have a rather complicated -- but harmless -- eval:
which I would really like to be able to allow without confirmation
whenever I open one of those files (which I do fairly often).

And as Karl says, emacs cannot possibly treat this as "standard
knowledge".

For this, user customization is a good thing!

But maybe it would be better to have a more hook-like approach
such as having a special hook like
        enable-local-eval-functions
which are run until success (i.e. any of the functions may return 
true to allow a specific eval to be executed), and if all of them
returns false, the default method is used according to enable-local-eval
(i.e. ignore or ask user for confirmation).

Something like

        (if (or (run-hook-with-args-until-success
                 'enable-local-eval-functions val)
                (eq enable-local-eval t)
                (and enable-local-eval
                     (y-or-n-p ...)))
            (eval ...))

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: [PATCH] allow function values for `enable-local-eval'

[Richard Stallman]

      I'm working on a project where the files belonging to
    one of the modules have a rather complicated -- but harmless -- eval:

Could you show it to us, please?

Re: [PATCH] allow function values for `enable-local-eval'

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>       I'm working on a project where the files belonging to
>     one of the modules have a rather complicated -- but harmless -- eval:
> 
> Could you show it to us, please?
> 

I edited out the details but here it is:

 eval: (c-add-style "StyleX" (quote ((c-basic-offset . 4) (c-offsets-alist (label . *) ...))) t)

Even if we provided a c-add-style: hook similar to c-file-style:
I couldn't just replace the eval: with it...

First, I'm not in control of those files, so I'm not in a position to
change those files, so I have to live with that `eval:'.

Secondly, even if I could change the eval:, that would disturbe other
developers which are still using older versions of emacs (some are
still using 20.7, and are not going to change any time soon).

Consequently, for me, it would be really nice if I could check for
this specific hook and just say it's ok.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: [PATCH] allow function values for `enable-local-eval'

[Richard Stallman]

    I edited out the details but here it is:

     eval: (c-add-style "StyleX" (quote ((c-basic-offset . 4) (c-offsets-alist (label . *) ...))) t)

This suggests we add a feature where a function can be marked
as "ok to call", provided its arguments are all constant.
That way, this would work with no user customization required.
Similar cases with other functions could be enabled globally,
and local customization would be easy too.

We can add something more completely general if desired,
but I suspect this will handle all actual needs.  Its advantage
is that we can make it DTRT for most of the cases, straight out
of the box.

Please try this code.  It seemed to work for me.


(defun hack-one-local-variable-constantp (exp)
  (or (and (not (symbolp exp)) (not (consp exp)))
      (memq exp '(t nil))
      (keywordp exp)
      (hack-one-local-variable-quotep exp)))

(defun hack-one-local-variable-eval-safep (exp)
  "Return t if it is safe to eval EXP when it is found in a file."
  (and (consp exp)
       (or (and (eq (car exp) 'put)
		(hack-one-local-variable-quotep (nth 1 exp))
		(hack-one-local-variable-quotep (nth 2 exp))
		(memq (nth 1 (nth 2 exp))
		      '(lisp-indent-hook))
		;; Only allow safe expues of lisp-indent-hook;
		;; not functions.
		(or (numberp (nth 3 exp))
		    (equal (nth 3 exp) ''defun)))
	   (and (symbolp (car exp))
		(get (car exp) 'safe-local-eval-function)
		(let ((ok t))
		  (dolist (arg (cdr exp))
		    (unless (hack-one-local-variable-constantp arg)
		      (setq ok nil)))
		  ok)))))

(defun hack-one-local-variable (var val)
  "\"Set\" one variable in a local variables spec.
A few patterns are specified so that any name which matches one
is considered risky."
  (cond ((eq var 'mode)
	 (funcall (intern (concat (downcase (symbol-name val))
				  "-mode"))))
	((eq var 'coding)
	 ;; We have already handled coding: tag in set-auto-coding.
	 nil)
	((memq var ignored-local-variables)
	 nil)
	;; "Setting" eval means either eval it or do nothing.
	;; Likewise for setting hook variables.
	((or (get var 'risky-local-variable)
	     (and
	      (string-match "-hooks?$\\|-functions?$\\|-forms?$\\|-program$\\|-command$\\|-predicate$\\|font-lock-keywords$\\|font-lock-keywords-[0-9]+$\\|font-lock-syntactic-keywords$\\|-frame-alist$\\|-mode-alist$\\|-map$\\|-map-alist$"
			    (symbol-name var))
	      (not (get var 'safe-local-variable))))
	 ;; Permit evalling a put of a harmless property.
	 ;; if the args do nothing tricky.
	 (if (or (and (eq var 'eval)
		      (hack-one-local-variable-eval-safep val))
		 ;; Permit eval if not root and user says ok.
		 (and (not (zerop (user-uid)))
		      (or (eq enable-local-eval t)
			  (and enable-local-eval
			       (save-window-excursion
				 (switch-to-buffer (current-buffer))
				 (save-excursion
				   (beginning-of-line)
				   (set-window-start (selected-window) (point)))
				 (setq enable-local-eval
				       (y-or-n-p (format "Process `eval' or hook local variables in %s? "
							 (if buffer-file-name
							     (concat "file " (file-name-nondirectory buffer-file-name))
							   (concat "buffer " (buffer-name)))))))))))
	     (if (eq var 'eval)
		 (save-excursion (eval val))
	       (make-local-variable var)
	       (set var val))
	   (message "Ignoring `eval:' in the local variables list")))
	;; Ordinary variable, really set it.
	(t (make-local-variable var)
	   ;; Make sure the string has no text properties.
	   ;; Some text properties can get evaluated in various ways,
	   ;; so it is risky to put them on with a local variable list.
	   (if (stringp val)
	       (set-text-properties 0 (length val) nil val))
	   (set var val))))

Re: [PATCH] allow function values for `enable-local-eval'

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     I edited out the details but here it is:
> 
>      eval: (c-add-style "StyleX" (quote ((c-basic-offset . 4) (c-offsets-alist (label . *) ...))) t)
> 
> This suggests we add a feature where a function can be marked
> as "ok to call", provided its arguments are all constant.
> That way, this would work with no user customization required.
> Similar cases with other functions could be enabled globally,
> and local customization would be easy too.

This sounds like a good feature.

> 
> We can add something more completely general if desired,
> but I suspect this will handle all actual needs.  Its advantage
> is that we can make it DTRT for most of the cases, straight out
> of the box.

Well, I still need to do
        (put 'c-add-style 'safe-local-eval-function t)
in my .emacs, but that's easier than writing my own
hook function.

> 
> Please try this code.  It seemed to work for me.

Works for me too.  Thanks.

The following comment seems corrupted; "expues" should be "values".
I guess one too many replacement of "val" by "exp" :-)

	;; Only allow safe expues of lisp-indent-hook;
	;; not functions.
	(or (numberp (nth 3 exp))
	    (equal (nth 3 exp) ''defun)))

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: [PATCH] allow function values for `enable-local-eval'

[Richard Stallman]

    Well, I still need to do
	    (put 'c-add-style 'safe-local-eval-function t)
    in my .emacs, but that's easier than writing my own
    hook function.

We will do that by default for c-add-style, and various other
safe functions for which users request it.

Re: [PATCH] allow function values for `enable-local-eval'

[Stefan Monnier]

>     Well, I still need to do
> 	    (put 'c-add-style 'safe-local-eval-function t)
>     in my .emacs, but that's easier than writing my own
>     hook function.
> 
> We will do that by default for c-add-style, and various other
> safe functions for which users request it.

I'm sure crackers will be happy about it.
The argument passed to `c-add-style' allows you to buffer-locally
set any variable to any value whatsoever.  Hardly "safe" in my book.


	Stefan

Re: [PATCH] allow function values for `enable-local-eval'

[Kim F. Storm]

"Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu> writes:

> >     Well, I still need to do
> > 	    (put 'c-add-style 'safe-local-eval-function t)
> >     in my .emacs, but that's easier than writing my own
> >     hook function.
> > 
> > We will do that by default for c-add-style, and various other
> > safe functions for which users request it.
> 
> I'm sure crackers will be happy about it.
> The argument passed to `c-add-style' allows you to buffer-locally
> set any variable to any value whatsoever.  Hardly "safe" in my book.
> 

I think it would be both simpler and safer to just provide a list
variable, e.g. safe-local-eval-list, and let the user add his
well-known eval hooks to it; the hook-code should then simply see if a
questionable eval hook is a "member" of that list.

E.g. I could add that specific 
        '(c-add-style "StyleX" '(....)))
to the list.

And Karl could add his '(load "...") to the list.

A much better approach IMHO.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: [PATCH] allow function values for `enable-local-eval'

[Richard Stallman]

    I'm sure crackers will be happy about it.
    The argument passed to `c-add-style' allows you to buffer-locally
    set any variable to any value whatsoever.  Hardly "safe" in my book.

I didn't realize that.  It will be necessary then to check
the arguments to c-add-style.  Still, it is better to install
this checking by default than to make users install it as
a customization.

Re: [PATCH] allow function values for `enable-local-eval'

[Stefan Monnier]

>     I'm sure crackers will be happy about it.
>     The argument passed to `c-add-style' allows you to buffer-locally
>     set any variable to any value whatsoever.  Hardly "safe" in my book.
> 
> I didn't realize that.  It will be necessary then to check
> the arguments to c-add-style.  Still, it is better to install
> this checking by default than to make users install it as
> a customization.

Huh?  I personally don't see the point of using `c-add-style' in
the local variables section.  My understanding is that one guy did that
and mentioned it here, but I have seen no evidence that anybody
else does.
It seems simpler to just set the corresponding variables one by one
in the local variables section.


	Stefan

Re: [PATCH] allow function values for `enable-local-eval'

[Kim F. Storm]

"Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu> writes:

> >     I'm sure crackers will be happy about it.
> >     The argument passed to `c-add-style' allows you to buffer-locally
> >     set any variable to any value whatsoever.  Hardly "safe" in my book.
> > 
> > I didn't realize that.  It will be necessary then to check
> > the arguments to c-add-style.  Still, it is better to install
> > this checking by default than to make users install it as
> > a customization.
> 
> Huh?  I personally don't see the point of using `c-add-style' in
> the local variables section.  My understanding is that one guy did that
> and mentioned it here, but I have seen no evidence that anybody
> else does.
> It seems simpler to just set the corresponding variables one by one
> in the local variables section.

This requires that you are allowed to change the files!!!

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: [PATCH] allow function values for `enable-local-eval'

[Stefan Monnier]

> "Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu> writes:
> 
> > >     I'm sure crackers will be happy about it.
> > >     The argument passed to `c-add-style' allows you to buffer-locally
> > >     set any variable to any value whatsoever.  Hardly "safe" in my book.
> > > 
> > > I didn't realize that.  It will be necessary then to check
> > > the arguments to c-add-style.  Still, it is better to install
> > > this checking by default than to make users install it as
> > > a customization.
> > 
> > Huh?  I personally don't see the point of using `c-add-style' in
> > the local variables section.  My understanding is that one guy did that
> > and mentioned it here, but I have seen no evidence that anybody
> > else does.
> > It seems simpler to just set the corresponding variables one by one
> > in the local variables section.
> 
> This requires that you are allowed to change the files!!!

My point is just that since it's simpler (and doesn't require confirmation)
to set the variables one by one it's unlikely that this `c-add-tyle'
is often used in local variables sections.

So I see no reason to provide special support for it.
(contrary to the case of `put', for example).


	Stefan

Re: [PATCH] allow function values for `enable-local-eval'

[Kim F. Storm]

"Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu> writes:

> > "Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu> writes:
> > 
> > > >     I'm sure crackers will be happy about it.
> > > >     The argument passed to `c-add-style' allows you to buffer-locally
> > > >     set any variable to any value whatsoever.  Hardly "safe" in my book.
> > > > 
> > > > I didn't realize that.  It will be necessary then to check
> > > > the arguments to c-add-style.  Still, it is better to install
> > > > this checking by default than to make users install it as
> > > > a customization.
> > > 
> > > Huh?  I personally don't see the point of using `c-add-style' in
> > > the local variables section.  My understanding is that one guy did that
> > > and mentioned it here, but I have seen no evidence that anybody
> > > else does.
> > > It seems simpler to just set the corresponding variables one by one
> > > in the local variables section.
> > 
> > This requires that you are allowed to change the files!!!
> 
> My point is just that since it's simpler (and doesn't require confirmation)
> to set the variables one by one it's unlikely that this `c-add-tyle'
> is often used in local variables sections.
> 
> So I see no reason to provide special support for it.
> (contrary to the case of `put', for example).

I fully agree   -- provided there is a more generic hook which can be
used to accept "non-trivial" eval:s

Please note that I was not advocating the solution RMS has added; I
simply said that it would solve my specific problem.  

Now that you have pointed out that c-add-style is unsafe, I think it
really doesn't make sense to try to fix that [there are zillions of
other things that people might do in eval: which we cannot possible
configure before-hand.]


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: [PATCH] allow function values for `enable-local-eval'

[Miles Bader]

"Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu> writes:
> My point is just that since it's simpler (and doesn't require
> confirmation) to set the variables one by one it's unlikely that this
> `c-add-style' is often used in local variables sections.

I don't do either myself, but since c-add-style can change quite a few
individual parameters, I suspect many people might _like_ to use it if
doing so was convenient.  It's also more descriptive, since it names
the style you want to use.

[Of course, I think that the nicest thing to do for this particular case
would be to have some sort of `c-style' variable you could set in the
_header_, e.g.: /* -*- c-style: k&r -*- */ or something... that would be
both convenient and safe, and it looks nice too... :-)]

-miles
-- 
The secret to creativity is knowing how to hide your sources.
  --Albert Einstein

Re: [PATCH] allow function values for `enable-local-eval'

[Kai Großjohann]

Miles Bader <miles@lsi.nec.co.jp> writes:

> [Of course, I think that the nicest thing to do for this particular case
> would be to have some sort of `c-style' variable you could set in the
> _header_, e.g.: /* -*- c-style: k&r -*- */ or something... that would be
> both convenient and safe, and it looks nice too... :-)]

I think that's what c-file-style is for...

kai
-- 
A large number of young women don't trust men with beards.  (BFBS Radio)

Re: [PATCH] allow function values for `enable-local-eval'

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     I'm sure crackers will be happy about it.
>     The argument passed to `c-add-style' allows you to buffer-locally
>     set any variable to any value whatsoever.  Hardly "safe" in my book.
> 
> I didn't realize that.  It will be necessary then to check
> the arguments to c-add-style.  Still, it is better to install
> this checking by default than to make users install it as
> a customization.

Whatever we do to automate this, I don't think we will ever
cover every possible hook that people would like to "accept
by default".

So I still propose that we provide a *simple* customization option
safe-local-eval-forms which is just a list that contains verbatim
copies of the eval hooks which the user considers as safe.

We could then enhance the functionality when the user is
asked to "eval local hook (y/n)" to "eval local hook (y/n/s)"
where "s" saves the hook in safe-local-eval-forms.

But just having the option would be great!

Re: [PATCH] allow function values for `enable-local-eval'

[Richard Stallman]

    So I still propose that we provide a *simple* customization option
    safe-local-eval-forms

Ok, I will do that.

Re: [PATCH] allow function values for `enable-local-eval'

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     Well, I still need to do
> 	    (put 'c-add-style 'safe-local-eval-function t)
>     in my .emacs, but that's easier than writing my own
>     hook function.
> 
> We will do that by default for c-add-style, and various other
> safe functions for which users request it.

lisp/ediff-init.el and ediff-diff.el have these lines which
triggers the eval question:

;;; eval: (put 'ediff-defvar-local 'lisp-indent-hook 'defun)
;;; eval: (put 'ediff-with-current-buffer 'lisp-indent-hook 1)
;;; eval: (put 'ediff-with-current-buffer 'edebug-form-spec '(form body))

I don't think `put' should be a safe-local-eval-function, but maybe
it is ok to allow 'edebug-form-spec in addition to 'lisp-indent-hook ?

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: [PATCH] allow function values for `enable-local-eval'

[Richard Stallman]

    ;;; eval: (put 'ediff-defvar-local 'lisp-indent-hook 'defun)
    ;;; eval: (put 'ediff-with-current-buffer 'lisp-indent-hook 1)
    ;;; eval: (put 'ediff-with-current-buffer 'edebug-form-spec '(form body))

    I don't think `put' should be a safe-local-eval-function, but maybe
    it is ok to allow 'edebug-form-spec in addition to 'lisp-indent-hook ?

It is ok, but meanwhile there is now a way to specify these things in
the macro definition.  Maybe we should change this to use that mechanism
instead.

Kifer, would you like to do that?  Is there a difficulty with doing
that?


** The `defmacro' form may contain declarations specifying how to
indent the macro in Lisp mode and how to debug it with Edebug.  The
syntax of defmacro has been extended to

   (defmacro NAME LAMBDA-LIST [DOC-STRING] [DECLARATION ...] ...)

DECLARATION is a list `(declare DECLARATION-SPECIFIER ...)'.  The
declaration specifiers supported are:

(indent INDENT)
	Set NAME's `lisp-indent-function' property to INDENT.

(edebug DEBUG)
	Set NAME's `edebug-form-spec' property to DEBUG.  (This is
	equivalent to writing a `def-edebug-spec' for the macro.

Re: [PATCH] allow function values for `enable-local-eval'

[Stefan Monnier]

>    (defmacro NAME LAMBDA-LIST [DOC-STRING] [DECLARATION ...] ...)

BTW, the docstring hasn't been updated yet.


	Stefan

Re: [PATCH] allow function values for `enable-local-eval'

[Richard Stallman]

    >    (defmacro NAME LAMBDA-LIST [DOC-STRING] [DECLARATION ...] ...)

    BTW, the docstring hasn't been updated yet.

I will do that--thanks.

Re: [PATCH] allow function values for `enable-local-eval'

[Kevin Ryde]

Richard Stallman <rms@gnu.org> writes:
>
> We can add something more completely general if desired,
> but I suspect this will handle all actual needs.  Its advantage
> is that we can make it DTRT for most of the cases, straight out
> of the box.

Personally, just for what it's worth, I've been enabling eval only for
certain files I know I want, according to their filename.  (By
re-running hack-local-variables in find-file-hooks with
enable-local-eval suitably bound).

For me it's not just a case of what the eval contains, for instance
sometimes I want time-stamp updating (because a file belongs to me)
but usually not (because I'll be submitting a diff somewhere and don't
want to include a spurious change).

Re: [PATCH] allow function values for `enable-local-eval'

[Richard Stallman]

    It's precisely *because* providing such procedures is non-trivial that
    Emacs itself usually won't be able to provide a useful one.

I don't know if it is non-trivial.  Please show us your concrete
application, and then let's discuss what to do about it.

    What if you want to automatically eval only expressions that have been
    cryptographically signed by someone you trust?  Emacs is not in a
    position to implement this in a fully general way right now.

That sounds like TCPA--it is very scary.  We don't want to support
that or anything like that.

    Yes, they all look roughly like this:

       /* 
	* local variables:
	* eval: (load-file "../../tools/dev/svn-dev.el")
	* end:
	*/

What does this file do?  Is there a reason you can't define a major
mode in it, and use that asyou would any other major mode?

Re: [PATCH] allow function values for `enable-local-eval'

[Richard Stallman]

It seems to me that svn-dev.el is not coherent, and these things don't
particularly belong together.  Some of them seem to be meant to
interface Emacs to Subversion; others have to do with writing C and
Python and Perl code in certain styles.  I think the best thing to do
with the interface code is to put it into a separate file and load it
from your .emacs file.  The C, Python and Perl stuff can be handled
with the change I proposed in another message.