Re: bug#347: C mode asks twice about local variables

Re: bug#347: C mode asks twice about local variables

[Stefan Monnier]

[ Moving this over to emacs-devel. ]

>> >> Can you try and call `c-file-style' with some extra "don't override"
>> >> (when called from that hack-local-variables-hook) argument so that any
>> >> variable that already has a buffer-local binding will not be
>> >> overridden?
>> Any comment about this suggestion?
> YUCK!!!  ;-)

> I don't know off-hand what the significance of buffer locality is in CC
> Mode styles.

Could you ask the CC mode maintainer, maybe?

> But trying to get a "don't override" parameter to work
> would be horrendous.  hack-local-variables is actually called after the
> language hook, isn't it?  By that time, the CC Mode style will already
> have been set, and the pertinent variables will already be buffer local.
> At least I think so.

I see, yes, it might be messy or even impossible.

>> Non-portability might indeed be a problem (tho, it's obviously "your"
>> problem rather than mine), but introducing a new hook in Emacs-23 won't
>> help you there, so it's not relevant to this discussion.

> What I had in mind was using an (if (boundp 'before-hack-local-variables)
> ...) to separate out new strategy from old.

I'm sure we can come up with a good test to decide whether :safe
will do the right thing.

> Oh, and the hook would need documenting in the Elisp manual, but I can
> manage that.

> Hey, this is so easy and obviously the right thing.  Let's do it!

It's going in the right direction but I'm still not completely satisfied.
How 'bout something like the following:

Some variables can be flagged as being special, in that when they apper
in the file-local list of settings, they "get set" by calling
a function.  E.g. `mode' is globally special and "setting it" will
actually call the corresponding mode.

Then `c-mode' can define `c-file-style' and `c-file-offsets' as being
special so that setting them actually calls a function of yours
(probably a function that could also be used for the :set in defcustom,
BTW).

Of course, the interesting bit is that hack-local-variables will be
careful to reorder the file-local settings such that special variables
are set first.


        Stefan

Re: bug#347: C mode asks twice about local variables

[Glenn Morris]

Stefan Monnier wrote:

> Some variables can be flagged as being special, in that when they apper
> in the file-local list of settings, they "get set" by calling
> a function.
[...]
> (probably a function that could also be used for the :set in defcustom,
[...]
> Of course, the interesting bit is that hack-local-variables will be
> careful to reorder the file-local settings such that special variables
> are set first.

Sounds like hack-local-variables should respect custom's :set-after
and :set parameters. Then what works for custom should Just Work for
hack-local-variables too.

Re: bug#347: C mode asks twice about local variables

[Stefan Monnier]

>> Some variables can be flagged as being special, in that when they apper
>> in the file-local list of settings, they "get set" by calling
>> a function.
> [...]
>> (probably a function that could also be used for the :set in defcustom,
> [...]
>> Of course, the interesting bit is that hack-local-variables will be
>> careful to reorder the file-local settings such that special variables
>> are set first.

> Sounds like hack-local-variables should respect custom's :set-after
> and :set parameters. Then what works for custom should Just Work for
> hack-local-variables too.

In theory, that's also what I thought at first.  But since defcustom
works well for global settings but poorly for buffer-local settings, you
can't assume that all :set and :set-after functions will work properly
when called for buffer-local settings.


        Stefan

Re: bug#347: C mode asks twice about local variables

[Alan Mackenzie]

'Evening, Stefan and Glenn!

On Wed, Jun 11, 2008 at 10:41:38AM -0400, Stefan Monnier wrote:
> > I don't know off-hand what the significance of buffer locality is in
> > CC Mode styles.

> Could you ask the CC mode maintainer, maybe?

He was as confused about it as I was.

> > What I had in mind was using an (if (boundp
> > 'before-hack-local-variables) ...) to separate out new strategy from
> > old.

[ .... ]

> > Oh, and the hook would need documenting in the Elisp manual, but I
> > can manage that.

> > Hey, this is so easy and obviously the right thing.  Let's do it!

> It's going in the right direction but I'm still not completely
> satisfied.  How 'bout something like the following:

> Some variables can be flagged as being special, in that when they apper
> in the file-local list of settings, they "get set" by calling
> a function.  E.g. `mode' is globally special and "setting it" will
> actually call the corresponding mode.

> Then `c-mode' can define `c-file-style' and `c-file-offsets' as being
> special so that setting them actually calls a function of yours
> (probably a function that could also be used for the :set in defcustom,
> BTW).

> Of course, the interesting bit is that hack-local-variables will be
> careful to reorder the file-local settings such that special variables
> are set first.

Why, in general, should "special" variables always be set first?  I
can't feel any enthusiasm for this approach.

To verify my feelings about the `before-hack-local-variables-hook'
approach, I have done a trial implementation of it.  It was a little
more complicated that I'd expected - I had to refactor a bit, replacing
`hack-local-variables-apply' with `hack-local-variables-filter' (which
removes unwanted variables from the hack-list rather than setting the
rest) and moving the setting of the hack-variables into h-l-v itself.

The change to cc-mode.el was straightforward.

See what you think.  Here is the trial patch to the code:



2008-06-15  Alan Mackenzie  <acm@muc.de>

	* progmodes/cc-mode.el (c-before-hack-hook): New function
	(Top Level): Install c-before-hack-hook on
	before-hack-local-variables-hook, rather than
	c-postprocess-file-styles on hack-local-variables-hook.

	* files.el (hack-local-variables-alist): New variable.
	(before-hack-local-variables-hook): New hook.
	(hack-local-variables-filter): Refactored version of
	Hack-local-variables-apply.
	(hack-local-variables): Call `before-hack-local-variables-hook'.



Index: cc-mode.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/progmodes/cc-mode.el,v
retrieving revision 1.76
diff -c -r1.76 cc-mode.el
*** cc-mode.el	26 May 2008 06:57:40 -0000	1.76
--- cc-mode.el	15 Jun 2008 21:06:32 -0000
***************
*** 656,661 ****
--- 656,681 ----
        (and (cdr rfn)
  	   (setq require-final-newline mode-require-final-newline)))))
  
+ (defun c-before-hack-hook ()
+   "Set the CC Mode style and \"offsets\" when in the buffer's local variables.
+ They are set only when, respectively, the pseudo variables
+ `c-file-style' and `c-file-offsets' are present in the list.
+ 
+ This function is called from the hook `before-hack-local-variables-hook'."
+   (when c-buffer-is-cc-mode
+     (let ((stile (cdr (assq 'c-file-style hack-local-variables-alist)))
+ 	  (offsets (cdr (assq 'c-file-offsets hack-local-variables-alist))))
+       (when stile
+ 	(or (stringp stile) (error "c-file-style is not a string"))
+ 	(c-set-style stile))
+       (when offsets
+ 	(mapc
+ 	 (lambda (langentry)
+ 	   (let ((langelem (car langentry))
+ 		 (offset (cdr langentry)))
+ 	     (c-set-offset langelem offset)))
+ 	 offsets)))))
+ 
  (defun c-remove-any-local-eval-or-mode-variables ()
    ;; If the buffer specifies `mode' or `eval' in its File Local Variable list
    ;; or on the first line, remove all occurrences.  See
***************
*** 747,753 ****
  	    (hack-local-variables))
  	  nil))))
  
! (add-hook 'hack-local-variables-hook 'c-postprocess-file-styles)
  
  (defmacro c-run-mode-hooks (&rest hooks)
    ;; Emacs 21.1 has introduced a system with delayed mode hooks that
--- 767,775 ----
  	    (hack-local-variables))
  	  nil))))
  
! (if (boundp 'before-hack-local-variables-hook)
!     (add-hook 'before-hack-local-variables-hook 'c-before-hack-hook)
!   (add-hook 'hack-local-variables-hook 'c-postprocess-file-styles))
  
  (defmacro c-run-mode-hooks (&rest hooks)
    ;; Emacs 21.1 has introduced a system with delayed mode hooks that




Index: files.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/files.el,v
retrieving revision 1.985
diff -c -r1.985 files.el
*** files.el	11 Jun 2008 01:47:47 -0000	1.985
--- files.el	15 Jun 2008 20:46:17 -0000
***************
*** 2514,2519 ****
--- 2514,2539 ----
    '(ignored-local-variables safe-local-variable-values)
    "Variables to be ignored in a file's local variable spec.")
  
+ (defvar hack-local-variables-alist nil
+   "Alist of (VAR . VALUE) pairs read from a buffer's local variables.
+ VAR, a symbol, is a variable to be set, and VALUE (unevaluated) is
+ the value it will get set to.
+ 
+ This alist contains the settings from both the \"-*-\" line at
+ the top of the buffer and the \"Local Variables\:\" section near
+ the bottom of the buffer.  The settings are in the same order as
+ in the buffer.")
+ 
+ (defvar before-hack-local-variables-hook nil
+   "Normal hook run before setting a file's local variables.
+ It is called after the checks for unsafe and risky variables are
+ done.  It is called only when there is at least one local
+ variable to set.
+ 
+ The details of the local variables are in the variable
+ `hack-local-variables-alist'; a hook function may change the
+ contents of this alist.")
+ 
  (defvar hack-local-variables-hook nil
    "Normal hook run after processing a file's local variables specs.
  Major modes can use this to examine user-specified local variables
***************
*** 2777,2798 ****
  	  mode-specified
  	result))))
  
! (defun hack-local-variables-apply (result project)
!   "Apply an alist of local variable settings.
! RESULT is the alist.
! Will query the user when necessary."
    (dolist (ignored ignored-local-variables)
!     (setq result (assq-delete-all ignored result)))
    (if (null enable-local-eval)
!       (setq result (assq-delete-all 'eval result)))
!   (when result
!     (setq result (nreverse result))
      ;; Find those variables that we may want to save to
      ;; `safe-local-variable-values'.
      (let (risky-vars unsafe-vars)
!       (dolist (elt result)
  	(let ((var (car elt))
  	      (val (cdr elt)))
  	  ;; Don't query about the fake variables.
  	  (or (memq var '(mode unibyte coding))
  	      (and (eq var 'eval)
--- 2797,2830 ----
  	  mode-specified
  	result))))
  
! (defun hack-local-variables-filter (variables project)
!   "Remove risky \(etc.) local variables from VARIABLES.
! These are determined from the options `enable-local-variables',
! `enable-local-eval', `ignored-local-variables' and possibly the
! result of querying the user.
! 
! VARIABLES is an alist, each element of which has the form (VAR
! . VALUE), VAR being a variable to set (a symbol), VALUE being its
! \(unevaluted) value.  This format is the same as
! `hack-local-variables-alist''s.  This function might modify
! VARIABLES's list structure.
! 
! PROJECT is .... ?????
! 
! The function's result is VARIABLES with all rejected variables
! removed.  This may well be nil."
    (dolist (ignored ignored-local-variables)
!     (setq variables (assq-delete-all ignored variables)))
    (if (null enable-local-eval)
!       (setq variables (assq-delete-all 'eval variables)))
!   (when variables
      ;; Find those variables that we may want to save to
      ;; `safe-local-variable-values'.
      (let (risky-vars unsafe-vars)
!       (dolist (elt variables)
  	(let ((var (car elt))
  	      (val (cdr elt)))
+ 	  ;; Scan the variables, collecting risky and unsafe ones.
  	  ;; Don't query about the fake variables.
  	  (or (memq var '(mode unibyte coding))
  	      (and (eq var 'eval)
***************
*** 2803,2815 ****
  	      (and (risky-local-variable-p var val)
  		   (push elt risky-vars))
  	      (push elt unsafe-vars))))
        (if (eq enable-local-variables :safe)
  	  ;; If caller wants only the safe variables,
! 	  ;; install only them.
! 	  (dolist (elt result)
! 	    (unless (or (member elt unsafe-vars)
! 			(member elt risky-vars))
! 	      (hack-one-local-variable (car elt) (cdr elt))))
  	;; Query, except in the case where all are known safe
  	;; if the user wants no query in that case.
  	(if (or (and (eq enable-local-variables t)
--- 2835,2848 ----
  	      (and (risky-local-variable-p var val)
  		   (push elt risky-vars))
  	      (push elt unsafe-vars))))
+ 
        (if (eq enable-local-variables :safe)
  	  ;; If caller wants only the safe variables,
! 	  ;; expunge the list of the rest.
! 	  (dolist (elt variables)
! 	    (if (or (member elt unsafe-vars)
! 		    (member elt risky-vars))
! 	      (setq variables (assq-delete-all (car elt) variables))))
  	;; Query, except in the case where all are known safe
  	;; if the user wants no query in that case.
  	(if (or (and (eq enable-local-variables t)
***************
*** 2817,2825 ****
  		     (null risky-vars))
  		(eq enable-local-variables :all)
  		(hack-local-variables-confirm
! 		 result unsafe-vars risky-vars project))
! 	    (dolist (elt result)
! 	      (hack-one-local-variable (car elt) (cdr elt))))))))
  
  (defun hack-local-variables (&optional mode-only)
    "Parse and put into effect this buffer's local variables spec.
--- 2850,2858 ----
  		     (null risky-vars))
  		(eq enable-local-variables :all)
  		(hack-local-variables-confirm
! 		 variables unsafe-vars risky-vars project))
! 	    variables
! 	  )))))
  
  (defun hack-local-variables (&optional mode-only)
    "Parse and put into effect this buffer's local variables spec.
***************
*** 2827,2835 ****
  is specified, returning t if it is specified."
    (let ((enable-local-variables
  	 (and local-enable-local-variables enable-local-variables))
! 	result)
      (when (or mode-only enable-local-variables)
!       (setq result (hack-local-variables-prop-line mode-only))
        ;; Look for "Local variables:" line in last page.
        (save-excursion
  	(goto-char (point-max))
--- 2860,2868 ----
  is specified, returning t if it is specified."
    (let ((enable-local-variables
  	 (and local-enable-local-variables enable-local-variables))
! 	result hack-local-variables-alist)
      (when (or mode-only enable-local-variables)
!       (setq hack-local-variables-alist (hack-local-variables-prop-line mode-only))
        ;; Look for "Local variables:" line in last page.
        (save-excursion
  	(goto-char (point-max))
***************
*** 2906,2921 ****
  			    (push (cons (if (eq var 'eval)
  					    'eval
  					  (indirect-variable var))
! 					val) result)
  			  (error nil)))))
  		  (forward-line 1)))))))
  
        ;; We've read all the local variables.  Now, return whether the
        ;; mode is specified (if MODE-ONLY is non-nil), or set the
        ;; variables (if MODE-ONLY is nil.)
        (if mode-only
  	  result
! 	(hack-local-variables-apply result nil)
  	(run-hooks 'hack-local-variables-hook)))))
  
  (defun safe-local-variable-p (sym val)
--- 2939,2959 ----
  			    (push (cons (if (eq var 'eval)
  					    'eval
  					  (indirect-variable var))
! 					val) hack-local-variables-alist)
  			  (error nil)))))
  		  (forward-line 1)))))))
+       (setq hack-local-variables-alist (nreverse hack-local-variables-alist))
  
        ;; We've read all the local variables.  Now, return whether the
        ;; mode is specified (if MODE-ONLY is non-nil), or set the
        ;; variables (if MODE-ONLY is nil.)
        (if mode-only
  	  result
! 	(setq hack-local-variables-alist
! 	      (hack-local-variables-filter hack-local-variables-alist nil))
! 	(run-hooks 'before-hack-local-variables-hook)
! 	(dolist (elt hack-local-variables-alist)
! 	  (hack-one-local-variable (car elt) (cdr elt)))
  	(run-hooks 'hack-local-variables-hook)))))
  
  (defun safe-local-variable-p (sym val)

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: bug#347: C mode asks twice about local variables

[Alan Mackenzie]

Hi, Stefan!

On Sun, Jun 15, 2008 at 10:04:12PM +0000, Alan Mackenzie wrote:
> 'Evening, Stefan and Glenn!

[ Followed by a patch to cc-mode.el and files.el which would introduce a
new hook, before-hack-local-variables-hook, with this ChangeLog entry.]

> 2008-06-15  Alan Mackenzie  <acm@muc.de>

> 	* progmodes/cc-mode.el (c-before-hack-hook): New function
> 	(Top Level): Install c-before-hack-hook on
> 	before-hack-local-variables-hook, rather than
> 	c-postprocess-file-styles on hack-local-variables-hook.

> 	* files.el (hack-local-variables-alist): New variable.
> 	(before-hack-local-variables-hook): New hook.
> 	(hack-local-variables-filter): Refactored version of
> 	Hack-local-variables-apply.
> 	(hack-local-variables): Call `before-hack-local-variables-hook'.

You haven't got round to replying yet!  We have a feature freeze in a
very few days.

This patch would solve the problem of the double question.  Shall I
install this (and write some Docu)?

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: bug#347: C mode asks twice about local variables

[Chong Yidong]

Alan Mackenzie <acm@muc.de> writes:

>> 2008-06-15  Alan Mackenzie  <acm@muc.de>
>
>> 	* progmodes/cc-mode.el (c-before-hack-hook): New function
>> 	(Top Level): Install c-before-hack-hook on
>> 	before-hack-local-variables-hook, rather than
>> 	c-postprocess-file-styles on hack-local-variables-hook.
>
>> 	* files.el (hack-local-variables-alist): New variable.
>> 	(before-hack-local-variables-hook): New hook.
>> 	(hack-local-variables-filter): Refactored version of
>> 	Hack-local-variables-apply.
>> 	(hack-local-variables): Call `before-hack-local-variables-hook'.

Looks OK to me, except for two things:

- hack-local-variables-alist, before-hack-local-variables-hook, and
  c-before-hack-hook need to be marked as risky local vars.

- how bout renaming hack-local-variables-alist to
  file-local-variables-alist (which is a better description), making it
  buffer-local, and setting its buffer value instead of let-binding it?
  Then it would provide a lasting record what vars were set in this
  buffer using the hack-local-variables mechanism, which might be a
  useful thing to have.

If no one has any other objections in a couple of days, I'd go ahead and
check it in.  Please add a note to NEWS as well.

Re: bug#347: C mode asks twice about local variables

[Chong Yidong]

Chong Yidong <cyd@stupidchicken.com> writes:

> - hack-local-variables-alist, before-hack-local-variables-hook, and
>   c-before-hack-hook need to be marked as risky local vars.

Come to think of it, maybe marking hack-local-variables-alist as risky
may not be enough.  It may be better to make the local vars code handle
it specially by ignoring it.

Re: bug#347: C mode asks twice about local variables

[Johan Bockgård]

Chong Yidong <cyd@stupidchicken.com> writes:

> - hack-local-variables-alist, before-hack-local-variables-hook, and
>   c-before-hack-hook need to be marked as risky local vars.

Names ending in -hook are automatically considered risky.

-- 
Johan Bockgård

Re: bug#347: C mode asks twice about local variables

[Chong Yidong]

Hi Alan,

>> 2008-06-15  Alan Mackenzie  <acm@muc.de>
>
>> 	* progmodes/cc-mode.el (c-before-hack-hook): New function
>> 	(Top Level): Install c-before-hack-hook on
>> 	before-hack-local-variables-hook, rather than
>> 	c-postprocess-file-styles on hack-local-variables-hook.
>
>> 	* files.el (hack-local-variables-alist): New variable.
>> 	(before-hack-local-variables-hook): New hook.
>> 	(hack-local-variables-filter): Refactored version of
>> 	Hack-local-variables-apply.
>> 	(hack-local-variables): Call `before-hack-local-variables-hook'.

I've checked in a modified version of this patch.  Could you test it out
and see if it DTRT?

Thanks!