bug#8613: "mode:" for minor-mode breaks set-visited-file-name

bug#8613: "mode:" for minor-mode breaks set-visited-file-name

[Glenn Morris]

Package: emacs
Version: 23.3

The fact that "mode:" is allowed to indicate both major and minor modes
in file local variables is problematic. Things would be much simpler if
a different convention were used for minor modes.

There is bug#2355 (specifying a minor-mode in the first line breaks
major-mode detection); there is bug#5239 (major "mode:" specifications
only work right if they are at the start of the local variables list. It
might be nice to automatically move "mode:" to the front of the local
variables list, so that people don't have to worry about this; but since
"mode:" is also used for minor modes, and these probably need to come
AFTER setting of their relevant variables, this cannot be done.); there
is also bug#8586 (directory-local variables and files with "mode:"
cookies; I cannot see how to solve this given that "mode:" might mean
either a major or minor mode).

Here's another issue, affecting set-visited-file-name, caused by the
fact that the hack-local-variables MODE-ONLY feature cannot work right,
because all it does is check for a "mode:" cookie, and it has no way to
know if this is specifying a major-mode or a minor-mode:

>| foo.el
emacs -Q foo.el
M-x set-visited-file-name RET foo.f90 RET
  -> buffer switches to f90 mode

cat >| foo2.el <<EOF
;; Local Variables:
;; mode: outline-minor
;; End:
EOF

emacs -Q foo2.el
M-x set-visited-file-name RET foo2.f90 RET
  -> buffer stays in emacs-lisp-mode

bug#8613: "mode:" for minor-mode breaks set-visited-file-name

[Stefan Monnier]

> The fact that "mode:" is allowed to indicate both major and minor modes
> in file local variables is problematic. Things would be much simpler if
> a different convention were used for minor modes.

Agreed,


        Stefan

bug#8613: "mode:" for minor-mode breaks set-visited-file-name

[Glenn Morris]

Stefan Monnier wrote:

>> The fact that "mode:" is allowed to indicate both major and minor modes
>> in file local variables is problematic. Things would be much simpler if
>> a different convention were used for minor modes.
>
> Agreed,

So; shall we try to come up with a new convention for enabling
minor-modes via file-local variables, and deprecate using mode: for that?

All "mode:" does is funcall FOO-mode [1], which is nothing that can't be
done with :eval, except the latter will prompt for confirmation. Rather
than adding eg "minor-mode:", maybe safe-local-eval-forms could be
extended with a regexp element matching something like "([a-z]+-mode)".

[1] Well, it also ignores unknown modes.

bug#8613: "mode:" for minor-mode breaks set-visited-file-name

[Stefan Monnier]

>>> The fact that "mode:" is allowed to indicate both major and minor modes
>>> in file local variables is problematic. Things would be much simpler if
>>> a different convention were used for minor modes.
> So; shall we try to come up with a new convention for enabling
> minor-modes via file-local variables, and deprecate using mode: for that?

Yes, please.

> All "mode:" does is funcall FOO-mode [1], which is nothing that can't be
> done with :eval, except the latter will prompt for confirmation.  Rather
> than adding eg "minor-mode:", maybe safe-local-eval-forms could be
> extended with a regexp element matching something like "([a-z]+-mode)".

Not a bad idea, tho I'd much rather add
a (run-hook-with-args-until-success 'safe-local-eval-functions) instead
rather than rely on regexps.

Another option is to let define-minor-mode place
a `safe-local-eval-function' property on the minor-mode's symbol.


        Stefan

bug#8613: "mode:" for minor-mode breaks set-visited-file-name

[Glenn Morris]

Stefan Monnier wrote:

> Not a bad idea, tho I'd much rather add
> a (run-hook-with-args-until-success 'safe-local-eval-functions) instead
> rather than rely on regexps.

Regexps were the only way I could think of that did not require changes
to the definition of each minor mode, however...

> Another option is to let define-minor-mode place
> a `safe-local-eval-function' property on the minor-mode's symbol.

... that's nice and simple (I didn't know `safe-local-eval-function'
existed); but I had to tweak make-autoload to get it to work right with
autoloaded minor-modes (eg outline-minor).


*** lisp/emacs-lisp/autoload.el      2011-05-04 15:38:41 +0000
--- lisp/emacs-lisp/autoload.el      2011-05-05 02:14:30 +0000
***************
*** 190,195 ****
--- 190,197 ----
             (if (member ',file loads) nil
               (put ',groupname 'custom-loads (cons ',file loads))))))
  
+      ((eq car 'put) form)
+ 
       ;; nil here indicates that this is not a special autoload form.
       (t nil))))
  

*** lisp/emacs-lisp/easy-mmode.el	2011-01-25 04:08:28 +0000
--- lisp/emacs-lisp/easy-mmode.el	2011-05-05 00:44:36 +0000
***************
*** 115,120 ****
--- 115,122 ----
  :lighter SPEC	Same as the LIGHTER argument.
  :keymap MAP	Same as the KEYMAP argument.
  :require SYM	Same as in `defcustom'.
+ :safe PROP	Set the MODE function's `safe-local-eval-function' property
+ 		to PROP (default t).
  :variable PLACE	The location (as can be used with `setf') to use instead
  		of the variable MODE to store the state of the mode.  PLACE
  		can also be of the form (GET . SET) where GET is an expression
***************
*** 156,161 ****
--- 158,164 ----
           (setter nil)            ;The function (if any) to set the mode var.
           (modefun mode)          ;The minor mode function name we're defining.
  	 (require t)
+ 	 (safe t)
  	 (hook (intern (concat mode-name "-hook")))
  	 (hook-on (intern (concat mode-name "-on-hook")))
  	 (hook-off (intern (concat mode-name "-off-hook")))
***************
*** 174,179 ****
--- 177,183 ----
  	(:group (setq group (nconc group (list :group (pop body)))))
  	(:type (setq type (list :type (pop body))))
  	(:require (setq require (pop body)))
+ 	(:safe (setq safe (pop body)))
  	(:keymap (setq keymap (pop body)))
          (:variable (setq variable (pop body))
           (if (not (functionp (cdr-safe variable)))
***************
*** 264,269 ****
--- 268,275 ----
  	 ;; Return the new setting.
  	 ,mode)
  
+        (put ',modefun 'safe-local-eval-function ,safe)
+ 
         ;; Autoloading a define-minor-mode autoloads everything
         ;; up-to-here.
         :autoload-end

bug#8613: "mode:" for minor-mode breaks set-visited-file-name

[Stefan Monnier]

>> Not a bad idea, tho I'd much rather add
>> a (run-hook-with-args-until-success 'safe-local-eval-functions)
>> instead rather than rely on regexps.
> Regexps were the only way I could think of that did not require changes
> to the definition of each minor mode, however...

With a safe-local-eval-functions, you can do

(add-hook 'safe-local-eval-functions
          (lambda (form)
            (and (null (cdr form))
                 (symbolp (car form))
                 (string-match "-mode\\'" (symbol-name (car form))))))

>> Another option is to let define-minor-mode place
>> a `safe-local-eval-function' property on the minor-mode's symbol.

> ... that's nice and simple (I didn't know `safe-local-eval-function'
> existed); but I had to tweak make-autoload to get it to work right with
> autoloaded minor-modes (eg outline-minor).


> *** lisp/emacs-lisp/autoload.el      2011-05-04 15:38:41 +0000
> --- lisp/emacs-lisp/autoload.el      2011-05-05 02:14:30 +0000
> ***************
> *** 190,195 ****
> --- 190,197 ----
>              (if (member ',file loads) nil
>                (put ',groupname 'custom-loads (cons ',file loads))))))
>   
> +      ((eq car 'put) form)
> + 
>        ;; nil here indicates that this is not a special autoload form.
>        (t nil))))

Hmm... that will affect the (push (nth 1 form) autoloads-done).
Not sure if it matters.

> *** lisp/emacs-lisp/easy-mmode.el	2011-01-25 04:08:28 +0000
> --- lisp/emacs-lisp/easy-mmode.el	2011-05-05 00:44:36 +0000
> ***************
> *** 115,120 ****
> --- 115,122 ----
>   :lighter SPEC	Same as the LIGHTER argument.
>   :keymap MAP	Same as the KEYMAP argument.
>   :require SYM	Same as in `defcustom'.
> + :safe PROP	Set the MODE function's `safe-local-eval-function' property
> + 		to PROP (default t).

Not sure if we need it (currently all the -mode functions are presumed
safe when called without argument, as in the "mode:" cookie).
But if we do need it, it's better to reverse the meaning since we
should usually aim to make ":prop nil" behave the same as when
it's absent.


        Stefan

bug#8613: "mode:" for minor-mode breaks set-visited-file-name

[Glenn Morris]

Stefan Monnier wrote:

>>> Not a bad idea, tho I'd much rather add
>>> a (run-hook-with-args-until-success 'safe-local-eval-functions)
>>> instead rather than rely on regexps.
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[...]
> With a safe-local-eval-functions, you can do
>
> (add-hook 'safe-local-eval-functions
>           (lambda (form)
>             (and (null (cdr form))
>                  (symbolp (car form))
>                  (string-match "-mode\\'" (symbol-name (car form))))))
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Does not compute.

Anyway, that would work, tho' perhaps it could just be hard-coded in
hack-one-local-variable-eval-safep, since the safety of mode: is not
customizable at present, and having hooks with non-empty defaults is
sub-optimal.

> Hmm... that will affect the (push (nth 1 form) autoloads-done).
> Not sure if it matters.

FWIW, I checked loaddefs.el without and without this change, and saw
only the expected differences.

>> + :safe PROP	Set the MODE function's `safe-local-eval-function' property
>> + 		to PROP (default t).
>
> Not sure if we need it (currently all the -mode functions are presumed
> safe when called without argument, as in the "mode:" cookie).
> But if we do need it, it's better to reverse the meaning since we
> should usually aim to make ":prop nil" behave the same as when
> it's absent.

The only reason I could see needing :safe as an argument was if someone
had some hypothetical minor-mode that for some reason was not safe. In
which case, passing ":safe nil" seems like the simplest thing to me,
rather than eg ":safe 'no", or inverting the whole thing to use :risky
instead of :safe.

But, there's no option to do this with mode:, so this is not needed to
replace mode: for minor-modes.

bug#8613: "mode:" for minor-mode breaks set-visited-file-name

[Stefan Monnier]

>> (string-match "-mode\\'" (symbol-name (car form))))))
>                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Does not compute.

Not sure why it wouldn't compute, but I'll trust you on that one.

> Anyway, that would work, tho' perhaps it could just be hard-coded in
> hack-one-local-variable-eval-safep, since the safety of mode: is not
> customizable at present, and having hooks with non-empty defaults is
> sub-optimal.

Agreed.  As mentioned recently, I'm fine with a non-nil hook as long as
it's preloaded and not defcustom.  But I think safe-local-eval-functions
would be a likely defcustom, so it should be nil.

> The only reason I could see needing :safe as an argument was if someone
> had some hypothetical minor-mode that for some reason was not safe. In
> which case, passing ":safe nil" seems like the simplest thing to me,
> rather than eg ":safe 'no", or inverting the whole thing to use :risky
> instead of :safe.

I prefer :risky.  It's always handy to be able to do things like
:foo (plist-get <bar> :foo), so ":foo nil" should behave the same as
its absence.

> But, there's no option to do this with mode:, so this is not needed to
> replace mode: for minor-modes.

Agreed.  If/when we need it we can add a :risky.  In the mean time if
someone really needs it she can add a (put 'mode
'safe-local-eval-function nil) after the define-minor-mode.


        Stefan

bug#8613: "mode:" for minor-mode breaks set-visited-file-name

[Glenn Morris]

Do you have a preference for a solution?
Here's the hack-one-local-variable-eval-safep version:

*** lisp/files.el	2011-04-24 00:24:30 +0000
--- lisp/files.el	2011-05-09 23:22:07 +0000
***************
*** 3327,3332 ****
--- 3327,3336 ----
        ;; Certain functions can be allowed with safe arguments
        ;; or can specify verification functions to try.
        (and (symbolp (car exp))
+ 	   ;; Allow (minor)-modes calls with no arguments.
+ 	   ;; This obsoletes the use of "mode:" for such things.  (Bug#8613)
+ 	   (or (and (null (cdr exp))
+ 		    (string-match "-mode\\'" (symbol-name (car exp))))
  	       (let ((prop (get (car exp) 'safe-local-eval-function)))
  		 (cond ((eq prop t)
  			(let ((ok t))
***************
*** 3341,3347 ****
  		      (dolist (function prop)
  			(if (funcall function exp)
  			    (setq ok t)))
! 		      ok)))))))
  
  (defun hack-one-local-variable (var val)
    "Set local variable VAR with value VAL.
--- 3345,3351 ----
  			  (dolist (function prop)
  			    (if (funcall function exp)
  				(setq ok t)))
! 			  ok))))))))
  
  (defun hack-one-local-variable (var val)
    "Set local variable VAR with value VAL.

bug#8613: "mode:" for minor-mode breaks set-visited-file-name

[Stefan Monnier]

> Do you have a preference for a solution?

Either is fine.


        Stefan