Bug: buffer local variables handled wrong [9.0.5 (release_9.0.5-497-g5bc540 @ /home/hs/.emacs.d/lib/org-mode/lisp/)]

Bug: buffer local variables handled wrong [9.0.5 (release_9.0.5-497-g5bc540 @ /home/hs/.emacs.d/lib/org-mode/lisp/)]

[Stefan-W. Hahn]

Good day,

Emacs  : GNU Emacs 26.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.18.9)
 of 2017-05-20
Package: Org mode version 9.0.5 (release_9.0.5-497-g5bc540 @ /home/hs/.emacs.d/lib/org-mode/lisp/)

I use a minor mode (moccur-edit-mode, seems a little bit old) which
initializes one variable in this way:

,----
| (defvar moccur-edit-old-content)
| (make-local-variable 'moccur-edit-old-content)
`----

This leads to following result in (buffer-local-variables):

,----
| ... (moccur-edit-file-overlays) moccur-edit-old-content (company-prefix) ...
`----

I think this is correct and happens not only by the used minor-mode.

When doing org-capture now I got a lisp error:

Debugger entered--Lisp error: listp moccur-edit-old-content

This error comes from org-clone-local-variables, because there the
prediction for local variables is always to be a list.

I traced all other code points where (buffer-local-variables) is used:

,----
| grep --color -nH -e buffer-local-var *.el
| 1. org-agenda.el:2158:	 (let ((save (buffer-local-variables)))
| 2. org.el:9401:	   (buffer-local-variables)))))
| 3. org.el:9406:  (dolist (pair (buffer-local-variables from-buffer))
| 4. org-element.el:4091:   (t (let ((local-variables (buffer-local-variables)))
| 5. ox.el:2646:	     (dolist (entry (buffer-local-variables (buffer-base-buffer)) vars)
`----

The code 2., 4. and 5. are correct, they use (consp v) or (symbolp v) to
decide what to do.

The code 1. and 3. are wrong. They both work directly with (car v) or
(cdr v).

For 1. and 3. I would like to suggest the following corrections:

modified   lisp/org-agenda.el
@@ -2159,11 +2159,12 @@ org-agenda-mode
 	   (kill-all-local-variables)
 	   (mapc 'make-local-variable org-agenda-local-vars)
 	   (dolist (elem save)
-	     (let ((var (car elem))
-		   (val (cdr elem)))
-	       (when (and val
-			  (member var org-agenda-local-vars))
-		 (set var val)))))
+	     (if (consp elem)
+		 (let ((var (car elem))
+		       (val (cdr elem)))
+		   (when (and val
+			      (member var org-agenda-local-vars))
+		     (set var val))))))
 	 (setq-local org-agenda-this-buffer-is-sticky t))
 	(org-agenda-sticky
 	 ;; Creating a sticky Agenda buffer for the first time
modified   lisp/org.el
@@ -9404,11 +9404,12 @@ org-clone-local-variables
   "Clone local variables from FROM-BUFFER.
 Optional argument REGEXP selects variables to clone."
   (dolist (pair (buffer-local-variables from-buffer))
-    (let ((name (car pair)))
-      (when (and (symbolp name)
-		 (not (memq name org-unique-local-variables))
-		 (or (null regexp) (string-match regexp (symbol-name name))))
-	(set (make-local-variable name) (cdr pair))))))
+    (if (consp pair)
+	(let ((name (car pair)))
+	  (when (and (symbolp name)
+		     (not (memq name org-unique-local-variables))
+		     (or (null regexp) (string-match regexp (symbol-name name))))
+	    (set (make-local-variable name) (cdr pair)))))))
 
 ;;;###autoload
 (defun org-run-like-in-org-mode (cmd)


With kind regards,
Stefan

-- 
Stefan-W. Hahn                          It is easy to make things.
                                        It is hard to make things simple.

Re: Bug: buffer local variables handled wrong [9.0.5 (release_9.0.5-497-g5bc540 @ /home/hs/.emacs.d/lib/org-mode/lisp/)]

[Nicolas Goaziou]

Hello,

"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:

> I use a minor mode (moccur-edit-mode, seems a little bit old) which
> initializes one variable in this way:
>
> ,----
> | (defvar moccur-edit-old-content)
> | (make-local-variable 'moccur-edit-old-content)
> `----
>
> This leads to following result in (buffer-local-variables):
>
> ,----
> | ... (moccur-edit-file-overlays) moccur-edit-old-content (company-prefix) ...
> `----
>
> I think this is correct and happens not only by the used minor-mode.
>
> When doing org-capture now I got a lisp error:
>
> Debugger entered--Lisp error: listp moccur-edit-old-content
>
> This error comes from org-clone-local-variables, because there the
> prediction for local variables is always to be a list.

Fixed. Thank you for the report and the analysis.

Regards,

-- 
Nicolas Goaziou

Re: Bug: buffer local variables handled wrong [9.0.5 (release_9.0.5-497-g5bc540 @ /home/hs/.emacs.d/lib/org-mode/lisp/)]

[Stefan-W. Hahn]

Mail von Nicolas Goaziou, Sat, 03 Jun 2017 at 22:46:01 +0200:

Good morning,

> >
> > This error comes from org-clone-local-variables, because there the
> > prediction for local variables is always to be a list.
> 
> Fixed. Thank you for the report and the analysis.

Thanks.

Because for me pcase is magic, I tried to understand your corretion of the
code.

To understand it I expanded the pcase form with (macrostep-expand), e.g. in
org-agenda-mode:

,----
| (defun org-agenda-mode ()
| ...
| 	   (mapc #'make-local-variable org-agenda-local-vars)
| 	   (dolist (elem save)
| 	     (if
| 		 (consp elem)
| 		 (let*
| 		     ((x
| 		       (car elem))
| 		      (x
| 		       (cdr elem)))
| 		   (if
| 		       (consp x)
| 		       (let*
| 			   ((x
| 			     (car x))
| 			    (x
| 			     (cdr x)))
| 			 (if
| 			     (null x)
| 			     (let
| 				 ((val x)
| 				  (var x))
| 			       (when
| 				   (and val
| 					(memq var org-agenda-local-vars))
| 				 (set var val)))
| 			   nil))
| 		     nil))
| 	       nil)))
| 	 (setq-local org-agenda-this-buffer-is-sticky t))
| ...
`----

for me it looks not correct, because var and val get expanded to (cdr
(cdr elem)).

Also the second one

,----
| (defun org-clone-local-variables (from-buffer &optional regexp)
| ...
|   (dolist (pair (buffer-local-variables from-buffer))
|     (if
| 	(consp pair)
| 	(let*
| 	    ((x
| 	      (car pair))
| 	     (x
| 	      (cdr pair)))
| 	  (if
| 	      (consp x)
| 	      (let*
| 		  ((x
| 		    (cdr x)))
| 		(if
| 		    (null x)
| 		    (let
| 			((name x))
| 		      (when
| 			  (and
| 			   (not
| 			    (memq name org-unique-local-variables))
| 			   (or
| 			    (null regexp)
| 			    (string-match-p regexp
| 					    (symbol-name name))))
| 			(set
| 			 (make-local-variable name)
| 			 (cdr pair))))
| 		  nil))
| 	    nil))
|       nil)))
`----

here name get expanded to (cdr (cdr pair)).

For me both cases don't look correct or do I missinterpret something?

With kind regards,
Stefan

-- 
Stefan-W. Hahn                          It is easy to make things.
                                        It is hard to make things simple.

Re: Bug: buffer local variables handled wrong [9.0.5 (release_9.0.5-497-g5bc540 @ /home/hs/.emacs.d/lib/org-mode/lisp/)]

[Nicolas Goaziou]

Hello,

"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:

> For me both cases don't look correct or do I missinterpret something?

No, you're right. I fixed it. Thank you.

Regards,

-- 
Nicolas Goaziou

Re: Bug: buffer local variables handled wrong [9.0.5 (release_9.0.5-497-g5bc540 @ /home/hs/.emacs.d/lib/org-mode/lisp/)]

[Stefan-W. Hahn]

Mail von Nicolas Goaziou, Sun, 04 Jun 2017 at 09:19:09 +0200:

Good morning again,

> Hello,
> 
> "Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:
> 
> > For me both cases don't look correct or do I missinterpret something?
> 
> No, you're right. I fixed it. Thank you.

I looked at it, but sorry, I think this also is not right, it expands to:

,----
| (defun org-agenda-mode ()
| ...
| 	   (mapc #'make-local-variable org-agenda-local-vars)
| 	   (dolist (elem save)
| 	     (if
| 		 (consp elem)
| 		 (let*
| 		     ((x
| 		       (car elem))
| 		      (x
| 		       (cdr elem)))
| 		   (let
| 		       ((val x)
| 			(var x))
| 		     (when
| 			 (and val
| 			      (memq var org-agenda-local-vars))
| 		       (set var val))))
| 	       nil)))
| 	 (setq-local org-agenda-this-buffer-is-sticky t))
| 	(org-agenda-sticky
`----

and the second one:

,----
| (defun org-clone-local-variables (from-buffer &optional regexp)
| ...
|   (dolist (pair (buffer-local-variables from-buffer))
|     (if
| 	(consp pair)
| 	(let*
| 	    ((x
| 	      (car pair))
| 	     (x
| 	      (cdr pair)))
| 	  (if
| 	      (consp x)
| 	      (let*
| 		  ((x
| 		    (cdr x)))
| 		(if
| 		    (null x)
| 		    (let
| 			((name x))
| 		      (when
| 			  (and
| 			   (not
| 			    (memq name org-unique-local-variables))
| 			   (or
| 			    (null regexp)
| 			    (string-match-p regexp
| 					    (symbol-name name))))
| 			(set
| 			 (make-local-variable name)
| 			 (cdr pair))))
| 		  nil))
| 	    nil))
|       nil)))
`----

Both looking wrong for me. Sorry.

With kind regards,
Stefan

-- 
Stefan-W. Hahn                          It is easy to make things.
                                        It is hard to make things simple.

Re: Bug: buffer local variables handled wrong [9.0.5 (release_9.0.5-497-g5bc540 @ /home/hs/.emacs.d/lib/org-mode/lisp/)]

[Nicolas Goaziou]

"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:

> I looked at it, but sorry, I think this also is not right, it expands to:
>
> ,----
> | (defun org-agenda-mode ()
> | ...
> | 	   (mapc #'make-local-variable org-agenda-local-vars)
> | 	   (dolist (elem save)
> | 	     (if
> | 		 (consp elem)
> | 		 (let*
> | 		     ((x
> | 		       (car elem))
> | 		      (x
> | 		       (cdr elem)))
> | 		   (let
> | 		       ((val x)
> | 			(var x))
> | 		     (when
> | 			 (and val
> | 			      (memq var org-agenda-local-vars))
> | 		       (set var val))))
> | 	       nil)))
> | 	 (setq-local org-agenda-this-buffer-is-sticky t))
> | 	(org-agenda-sticky
> `----
>
> and the second one:
>
> ,----
> | (defun org-clone-local-variables (from-buffer &optional regexp)
> | ...
> |   (dolist (pair (buffer-local-variables from-buffer))
> |     (if
> | 	(consp pair)
> | 	(let*
> | 	    ((x
> | 	      (car pair))
> | 	     (x
> | 	      (cdr pair)))
> | 	  (if
> | 	      (consp x)
> | 	      (let*
> | 		  ((x
> | 		    (cdr x)))
> | 		(if
> | 		    (null x)
> | 		    (let
> | 			((name x))
> | 		      (when
> | 			  (and
> | 			   (not
> | 			    (memq name org-unique-local-variables))
> | 			   (or
> | 			    (null regexp)
> | 			    (string-match-p regexp
> | 					    (symbol-name name))))
> | 			(set
> | 			 (make-local-variable name)
> | 			 (cdr pair))))
> | 		  nil))
> | 	    nil))
> |       nil)))
> `----
>
> Both looking wrong for me. Sorry.

What do you think is wrong?

In particular

  (let (res)
    (dolist (pair (buffer-local-variables))
      (pcase pair
        (`(,var . ,val)
         (push (list 'set var val) res))))
    res)

expands to

  ((set flyspell-word-cache-result _)
   (set flyspell-word-cache-end -1)
   (set undo-auto--last-boundary-cause (2 #<buffer *scratch*>))
   (set syntax-ppss-last (1 0 nil nil nil nil nil 0 nil nil nil))
   (set syntax-propertize--done 139)
   (set flyspell-changes nil)
   (set deactivate-mark nil)
   (set flyspell-pre-point 139)
   (set auto-revert-notify-modified-p nil)
   ...
   )

which looks correct.

Regards,

Re: Bug: buffer local variables handled wrong [9.0.5 (release_9.0.5-497-g5bc540 @ /home/hs/.emacs.d/lib/org-mode/lisp/)]

[Stefan-W. Hahn]

Mail von Nicolas Goaziou, Sun, 04 Jun 2017 at 10:24:57 +0200:

Hello,

> "Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:
> 
> > I looked at it, but sorry, I think this also is not right, it expands to:
> >
> > Both looking wrong for me. Sorry.
> 
> What do you think is wrong?
> 
> In particular
> 
>   (let (res)
>     (dolist (pair (buffer-local-variables))
>       (pcase pair
>         (`(,var . ,val)
>          (push (list 'set var val) res))))
>     res)
> 
> expands to
> 
>   ((set flyspell-word-cache-result _)
>    (set flyspell-word-cache-end -1)
>    (set undo-auto--last-boundary-cause (2 #<buffer *scratch*>))
>    (set syntax-ppss-last (1 0 nil nil nil nil nil 0 nil nil nil))
>    (set syntax-propertize--done 139)
>    (set flyspell-changes nil)
>    (set deactivate-mark nil)
>    (set flyspell-pre-point 139)
>    (set auto-revert-notify-modified-p nil)
>    ...
>    )
> 
> which looks correct.

Obviously you are right, I get the ame result when evaluating it.

What I don't understand is, if I expand the pcase with (macrostep-expand) I
get the following:

,----
|     (let (res)
|     (dolist (pair (buffer-local-variables))
|       (if
|           (consp pair)
|           (let*
|               ((x
|                 (car pair))
|                (x
|                 (cdr pair)))
|             (let
|                 ((val x)
|                  (var x))
|               (push
|                (list 'set var val)
|                res)))
|         nil))
|     res)
`----

And this is obviously wrong.

With kind regards,
Stefan

-- 
Stefan-W. Hahn                          It is easy to make things.
                                        It is hard to make things simple.

Re: Bug: buffer local variables handled wrong [9.0.5 (release_9.0.5-497-g5bc540 @ /home/hs/.emacs.d/lib/org-mode/lisp/)]

[Nicolas Goaziou]

"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:

> What I don't understand is, if I expand the pcase with (macrostep-expand) I
> get the following:
>
> ,----
> |     (let (res)
> |     (dolist (pair (buffer-local-variables))
> |       (if
> |           (consp pair)
> |           (let*
> |               ((x
> |                 (car pair))
> |                (x
> |                 (cdr pair)))
> |             (let
> |                 ((val x)
> |                  (var x))
> |               (push
> |                (list 'set var val)
> |                res)))
> |         nil))
> |     res)
> `----
>
> And this is obviously wrong.

This is not obviously wrong. You may be thinking that both `x' symbols
are the same, but they are not. E.g.,

  (let ((s1 (make-symbol "x"))
        (s2 (make-symbol "x")))
    (list (list s1 s2) (eq s1 s2)))

  =>

  ((x x) nil)

Regards,

Re: Bug: buffer local variables handled wrong [9.0.5 (release_9.0.5-497-g5bc540 @ /home/hs/.emacs.d/lib/org-mode/lisp/)]

[Stefan-W. Hahn]

Mail von Nicolas Goaziou, Sun, 04 Jun 2017 at 11:35:34 +0200:

Hello,

> "Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:
> 
> > What I don't understand is, if I expand the pcase with (macrostep-expand) I
> > get the following:
> >
> > ,----
> > |     (let (res)
> > |     (dolist (pair (buffer-local-variables))
> > |       (if
> > |           (consp pair)
> > |           (let*
> > |               ((x
> > |                 (car pair))
> > |                (x
> > |                 (cdr pair)))
> > |             (let
> > |                 ((val x)
> > |                  (var x))
> > |               (push
> > |                (list 'set var val)
> > |                res)))
> > |         nil))
> > |     res)
> > `----
> >
> > And this is obviously wrong.
> 
> This is not obviously wrong. You may be thinking that both `x' symbols
> are the same, but they are not. E.g.,
> 
>   (let ((s1 (make-symbol "x"))
>         (s2 (make-symbol "x")))
>     (list (list s1 s2) (eq s1 s2)))
> 
>   =>
> 
>   ((x x) nil)

This I understand, but cannot see it in the macrostep-expanded code.
I will try learn more to understand this. 

So it is obvoius now that you are a lisp magician of 8th grade. :-)

Thanks and kind regards,
Stefan

-- 
Stefan-W. Hahn                          It is easy to make things.
                                        It is hard to make things simple.