[PATCH] Make lexical eval default for elisp src blocks

[PATCH] Make lexical eval default for elisp src blocks

[John Kitchin]

Set default in `org-babel-default-header-args:emacs-lisp'. Add an
optional argument to the eval function.
---
 etc/ORG-NEWS          | 11 +++++++++++
 lisp/ob-emacs-lisp.el | 29 ++++++++++++++++++++---------
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 6b1d9d5..99241e2 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -467,6 +467,17 @@ docstring for more information.
 - ~org-latex-format-inlinetask-function~
 - ~org-link-search~
 ** New features
+*** Default lexical evaluation of emacs-lisp src blocks
+Emacs-lisp src blocks in babel are now evaluated using lexical scoping. There is a new header to control this behavior.
+
+The default results in an eval with lexical scoping.
+:lexical yes
+
+This turns lexical scoping off in the eval (the former behavior).
+:lexical no
+
+This uses the lexical environment with x=42 in the eval.
+:lexical '((x . 42))
 
 *** Behavior of ~org-return~ changed
 
diff --git a/lisp/ob-emacs-lisp.el b/lisp/ob-emacs-lisp.el
index 2eb2721..ee8c8dd 100644
--- a/lisp/ob-emacs-lisp.el
+++ b/lisp/ob-emacs-lisp.el
@@ -28,8 +28,14 @@
 ;;; Code:
 (require 'ob)
 
-(defvar org-babel-default-header-args:emacs-lisp nil
-  "Default arguments for evaluating an emacs-lisp source block.")
+(defvar org-babel-default-header-args:emacs-lisp
+  '((:lexical . "yes"))
+  "Default arguments for evaluating an emacs-lisp source block.
+
+:lexical is \"yes\" by default and causes src blocks to be eval'd
+using lexical scoping. It can also be an alist mapping symbols to
+their value. It is used as the optional LEXICAL argument to
+`eval'.")
 
 (defun org-babel-expand-body:emacs-lisp (body params)
   "Expand BODY according to PARAMS, return the expanded body."
@@ -51,13 +57,18 @@
 (defun org-babel-execute:emacs-lisp (body params)
   "Execute a block of emacs-lisp code with Babel."
   (save-window-excursion
-    (let ((result
-           (eval (read (format (if (member "output"
-                                           (cdr (assoc :result-params params)))
-                                   "(with-output-to-string %s)"
-                                 "(progn %s)")
-                               (org-babel-expand-body:emacs-lisp
-                                body params))))))
+    (let* ((lexical (cdr (assoc :lexical params)))
+	   (result
+	    (eval (read (format (if (member "output"
+					    (cdr (assoc :result-params params)))
+				    "(with-output-to-string %s)"
+				  "(progn %s)")
+				(org-babel-expand-body:emacs-lisp
+				 body params)))
+
+		  (if (listp lexical)
+		      lexical
+		    (string= "yes" lexical)))))
       (org-babel-result-cond (cdr (assoc :result-params params))
 	(let ((print-level nil)
               (print-length nil))
-- 
2.4.4

Re: [PATCH] Make lexical eval default for elisp src blocks

[Adam Porter]

John Kitchin <jkitchin@andrew.cmu.edu> writes:

Forgive my ignorance--I haven't really dug into lexical scoping yet--but
what is the basic effect will this change have on elisp code blocks?
Say I'm doing some sort-of literate development and I have some code
blocks that `setq' here and there, setting vars in the Emacs
environment.  Will the scope of these vars now be limited to their code
blocks?  Would I need to disable lexical scoping?  Thanks.

Re: [PATCH] Make lexical eval default for elisp src blocks

[John Kitchin]

I don't think so. I haven't seen this be the case. A simple example like
this works as expected I think.

#+BEGIN_SRC emacs-lisp
(setq x 4)
#+END_SRC

#+RESULTS:
: 4


#+BEGIN_SRC emacs-lisp
(+ x 9)
#+END_SRC

#+RESULTS:
: 13


So far, I have only seen where this makes some new things possible. e.g.

This will not work unless evaluated with lexical-binding
#+BEGIN_SRC emacs-lisp :results value :lexical yes
;; Graham's alambda
(defmacro alambda (parms &rest body)
  `(cl-labels ((self ,parms ,@body))
     #'self))

(setq
 N
 (alambda (n)
          (if (> n 0)
              (cons
               n
               (self (- n 1))))))

(princ (funcall N 3))
#+END_SRC

#+RESULTS:
| 3 | 2 | 1 |

This just provides a different approach to :var I think.

#+BEGIN_SRC emacs-lisp :lexical '((x . 23))
(print x)
#+END_SRC

#+RESULTS:
: 23

I would be interested to see any counter examples though, where behavior
changes, or stops working.



Adam Porter writes:

> John Kitchin <jkitchin@andrew.cmu.edu> writes:
>
> Forgive my ignorance--I haven't really dug into lexical scoping yet--but
> what is the basic effect will this change have on elisp code blocks?
> Say I'm doing some sort-of literate development and I have some code
> blocks that `setq' here and there, setting vars in the Emacs
> environment.  Will the scope of these vars now be limited to their code
> blocks?  Would I need to disable lexical scoping?  Thanks.


--
Professor John Kitchin
Doherty Hall A207F
Department of Chemical Engineering
Carnegie Mellon University
Pittsburgh, PA 15213
412-268-7803
@johnkitchin
http://kitchingroup.cheme.cmu.edu

Re: [PATCH] Make lexical eval default for elisp src blocks

[Nicolas Goaziou]

Hello,

John Kitchin <jkitchin@andrew.cmu.edu> writes:

> Set default in `org-babel-default-header-args:emacs-lisp'. Add an
> optional argument to the eval function.

Thanks for the patch. Some comments follow.

> +*** Default lexical evaluation of emacs-lisp src blocks
> +Emacs-lisp src blocks in babel are now evaluated using lexical scoping. There is a new header to control this behavior.
> +
> +The default results in an eval with lexical scoping.
> +:lexical yes
> +
> +This turns lexical scoping off in the eval (the former behavior).
> +:lexical no
> +
> +This uses the lexical environment with x=42 in the eval.
> +:lexical '((x . 42))

According to the Elisp manual:

     The value of LEXICAL can also be a non-empty alist specifying
     a particular "lexical environment" for lexical bindings; however,
     this feature is only useful for specialized purposes, such as in
     Emacs Lisp debuggers. *Note Lexical Binding::.

I'm not opposed to it, but is there any reason to include the
opportunity to specify an alist here?

> +(defvar org-babel-default-header-args:emacs-lisp
> +  '((:lexical . "yes"))
> +  "Default arguments for evaluating an emacs-lisp source block.
> +
> +:lexical is \"yes\" by default and causes src blocks to be eval'd
> +using lexical scoping. It can also be an alist mapping symbols to
> +their value. It is used as the optional LEXICAL argument to
> +`eval'.")

You need to separate sentences with two spaces.

You also need to add (defconst org-babel-header-args:emacs-lisp). See
for example `org-babel-header-args:R'.

>  (defun org-babel-expand-body:emacs-lisp (body params)
>    "Expand BODY according to PARAMS, return the expanded body."
> @@ -51,13 +57,18 @@
>  (defun org-babel-execute:emacs-lisp (body params)
>    "Execute a block of emacs-lisp code with Babel."
>    (save-window-excursion
> -    (let ((result
> -           (eval (read (format (if (member "output"
> -                                           (cdr (assoc :result-params params)))
> -                                   "(with-output-to-string %s)"
> -                                 "(progn %s)")
> -                               (org-babel-expand-body:emacs-lisp
> -                                body params))))))
> +    (let* ((lexical (cdr (assoc :lexical params)))

Nitpick: (cdr (assq :lexical params))

> +	   (result
> +	    (eval (read (format (if (member "output"
> +					    (cdr (assoc :result-params params)))

Nitpick, while you're at it: (cdr (assq :result-params params))

> +				    "(with-output-to-string %s)"
> +				  "(progn %s)")
> +				(org-babel-expand-body:emacs-lisp
> +				 body params)))
> +
> +		  (if (listp lexical)
> +		      lexical
> +		    (string= "yes" lexical)))))

There is no support for t. I think we should allow both :lexical
t and :lexical yes.


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Make lexical eval default for elisp src blocks

[John Kitchin]

[-- Attachment #1: Type: text/plain, Size: 3785 bytes --]

Thanks for the feedback. I have a few questions below.


On Mon, Apr 18, 2016 at 12:38 PM, Nicolas Goaziou <mail@nicolasgoaziou.fr>
wrote:

> Hello,
>
> John Kitchin <jkitchin@andrew.cmu.edu> writes:
>
> > Set default in `org-babel-default-header-args:emacs-lisp'. Add an
> > optional argument to the eval function.
>
> Thanks for the patch. Some comments follow.
>
> > +*** Default lexical evaluation of emacs-lisp src blocks
> > +Emacs-lisp src blocks in babel are now evaluated using lexical scoping.
> There is a new header to control this behavior.
> > +
> > +The default results in an eval with lexical scoping.
> > +:lexical yes
> > +
> > +This turns lexical scoping off in the eval (the former behavior).
> > +:lexical no
> > +
> > +This uses the lexical environment with x=42 in the eval.
> > +:lexical '((x . 42))
>
> According to the Elisp manual:
>
>      The value of LEXICAL can also be a non-empty alist specifying
>      a particular "lexical environment" for lexical bindings; however,
>      this feature is only useful for specialized purposes, such as in
>      Emacs Lisp debuggers. *Note Lexical Binding::.
>
> I'm not opposed to it, but is there any reason to include the
> opportunity to specify an alist here?
>

I just put it in because it is an option for the eval function, and it was
not difficult to implement. It might be useful for debugging.


>
> > +(defvar org-babel-default-header-args:emacs-lisp
> > +  '((:lexical . "yes"))
> > +  "Default arguments for evaluating an emacs-lisp source block.
> > +
> > +:lexical is \"yes\" by default and causes src blocks to be eval'd
> > +using lexical scoping. It can also be an alist mapping symbols to
> > +their value. It is used as the optional LEXICAL argument to
> > +`eval'.")
>
> You need to separate sentences with two spaces.
>

no problem.


>
> You also need to add (defconst org-babel-header-args:emacs-lisp). See
> for example `org-babel-header-args:R'.
>

Are you suggesting use defconst instead of defvar? Does it really need all
the things in org-babel-header-args:R? Or just the required default for
lexical?


>
> >  (defun org-babel-expand-body:emacs-lisp (body params)
> >    "Expand BODY according to PARAMS, return the expanded body."
> > @@ -51,13 +57,18 @@
> >  (defun org-babel-execute:emacs-lisp (body params)
> >    "Execute a block of emacs-lisp code with Babel."
> >    (save-window-excursion
> > -    (let ((result
> > -           (eval (read (format (if (member "output"
> > -                                           (cdr (assoc :result-params
> params)))
> > -                                   "(with-output-to-string %s)"
> > -                                 "(progn %s)")
> > -                               (org-babel-expand-body:emacs-lisp
> > -                                body params))))))
> > +    (let* ((lexical (cdr (assoc :lexical params)))
>
> Nitpick: (cdr (assq :lexical params))
>

no problem.


>
> > +        (result
> > +         (eval (read (format (if (member "output"
> > +                                         (cdr (assoc :result-params
> params)))
>
> Nitpick, while you're at it: (cdr (assq :result-params params))
>

no problem. Should all the assocs be replaced by assq, or just these ones?

>
> > +                                 "(with-output-to-string %s)"
> > +                               "(progn %s)")
> > +                             (org-babel-expand-body:emacs-lisp
> > +                              body params)))
> > +
> > +               (if (listp lexical)
> > +                   lexical
> > +                 (string= "yes" lexical)))))
>
> There is no support for t. I think we should allow both :lexical
> t and :lexical yes.
>

something like (member lexical '("yes" "t"))?


>
>
> Regards,
>
> --
> Nicolas Goaziou
>

[-- Attachment #2: Type: text/html, Size: 6559 bytes --]

Re: [PATCH] Make lexical eval default for elisp src blocks

[Nicolas Goaziou]

John Kitchin <jkitchin@andrew.cmu.edu> writes:

> I just put it in because it is an option for the eval function, and it was
> not difficult to implement. It might be useful for debugging.

Fair enough.

> Are you suggesting use defconst instead of defvar? Does it really need all
> the things in org-babel-header-args:R? Or just the required default for
> lexical?

Two variables are needed: `org-babel-default-header-args:emacs-lisp' and
`org-babel-header-args:emacs-lisp'. The latter is used, e.g., in linting
to know what keywords are allowed in an emacs-lisp block and, if
possible, the possible values for it.

> no problem. Should all the assocs be replaced by assq, or just these
> ones?

I usually replace (assoc/member KEYWORD ...) with (assq/memq KEYWORD
...) whenever I modify a S-exp around it. You don't need to change this
for parts you don't alter.

>> > +                                 "(with-output-to-string %s)"
>> > +                               "(progn %s)")
>> > +                             (org-babel-expand-body:emacs-lisp
>> > +                              body params)))
>> > +
>> > +               (if (listp lexical)
>> > +                   lexical
>> > +                 (string= "yes" lexical)))))
>>
>> There is no support for t. I think we should allow both :lexical
>> t and :lexical yes.
>>
>
> something like (member lexical '("yes" "t"))?

Honestly, I don't remember how Babel handles parameters. It could also
be '("yes" t) if the latter is recognized as a keyword. You get the
idea :)

Regards,

Re: [PATCH] Make lexical eval default for elisp src blocks

[John Kitchin]

[-- Attachment #1: Type: text/plain, Size: 2009 bytes --]

thanks. I think I have addressed these in a new patch I just submitted.

John

-----------------------------------
Professor John Kitchin
Doherty Hall A207F
Department of Chemical Engineering
Carnegie Mellon University
Pittsburgh, PA 15213
412-268-7803
@johnkitchin
http://kitchingroup.cheme.cmu.edu


On Mon, Apr 18, 2016 at 1:41 PM, Nicolas Goaziou <mail@nicolasgoaziou.fr>
wrote:

> John Kitchin <jkitchin@andrew.cmu.edu> writes:
>
> > I just put it in because it is an option for the eval function, and it
> was
> > not difficult to implement. It might be useful for debugging.
>
> Fair enough.
>
> > Are you suggesting use defconst instead of defvar? Does it really need
> all
> > the things in org-babel-header-args:R? Or just the required default for
> > lexical?
>
> Two variables are needed: `org-babel-default-header-args:emacs-lisp' and
> `org-babel-header-args:emacs-lisp'. The latter is used, e.g., in linting
> to know what keywords are allowed in an emacs-lisp block and, if
> possible, the possible values for it.
>
> > no problem. Should all the assocs be replaced by assq, or just these
> > ones?
>
> I usually replace (assoc/member KEYWORD ...) with (assq/memq KEYWORD
> ...) whenever I modify a S-exp around it. You don't need to change this
> for parts you don't alter.
>
> >> > +                                 "(with-output-to-string %s)"
> >> > +                               "(progn %s)")
> >> > +                             (org-babel-expand-body:emacs-lisp
> >> > +                              body params)))
> >> > +
> >> > +               (if (listp lexical)
> >> > +                   lexical
> >> > +                 (string= "yes" lexical)))))
> >>
> >> There is no support for t. I think we should allow both :lexical
> >> t and :lexical yes.
> >>
> >
> > something like (member lexical '("yes" "t"))?
>
> Honestly, I don't remember how Babel handles parameters. It could also
> be '("yes" t) if the latter is recognized as a keyword. You get the
> idea :)
>
> Regards,
>

[-- Attachment #2: Type: text/html, Size: 3113 bytes --]

[PATCH] Make lexical eval default for elisp src blocks

[John Kitchin]

Set default in `org-babel-default-header-args:emacs-lisp'. Add an
optional argument to the eval function.
---
 etc/ORG-NEWS          | 11 +++++++++++
 lisp/ob-emacs-lisp.el | 33 ++++++++++++++++++++++++---------
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 6b1d9d5..99241e2 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -467,6 +467,17 @@ docstring for more information.
 - ~org-latex-format-inlinetask-function~
 - ~org-link-search~
 ** New features
+*** Default lexical evaluation of emacs-lisp src blocks
+Emacs-lisp src blocks in babel are now evaluated using lexical scoping. There is a new header to control this behavior.
+
+The default results in an eval with lexical scoping.
+:lexical yes
+
+This turns lexical scoping off in the eval (the former behavior).
+:lexical no
+
+This uses the lexical environment with x=42 in the eval.
+:lexical '((x . 42))
 
 *** Behavior of ~org-return~ changed
 
diff --git a/lisp/ob-emacs-lisp.el b/lisp/ob-emacs-lisp.el
index 2eb2721..051d2bb 100644
--- a/lisp/ob-emacs-lisp.el
+++ b/lisp/ob-emacs-lisp.el
@@ -28,8 +28,18 @@
 ;;; Code:
 (require 'ob)
 
-(defvar org-babel-default-header-args:emacs-lisp nil
-  "Default arguments for evaluating an emacs-lisp source block.")
+(defconst org-babel-header-args:emacs-lisp
+  '((lexical . ((yes no t nil))))
+  "emacs-lisp specific header arguments.")
+
+(defvar org-babel-default-header-args:emacs-lisp
+  '((:lexical . "yes"))
+  "Default arguments for evaluating an emacs-lisp source block.
+
+:lexical is \"yes\" by default and causes src blocks to be eval'd
+using lexical scoping.  It can also be an alist mapping symbols to
+their value.  It is used as the optional LEXICAL argument to
+`eval'.")
 
 (defun org-babel-expand-body:emacs-lisp (body params)
   "Expand BODY according to PARAMS, return the expanded body."
@@ -51,13 +61,18 @@
 (defun org-babel-execute:emacs-lisp (body params)
   "Execute a block of emacs-lisp code with Babel."
   (save-window-excursion
-    (let ((result
-           (eval (read (format (if (member "output"
-                                           (cdr (assoc :result-params params)))
-                                   "(with-output-to-string %s)"
-                                 "(progn %s)")
-                               (org-babel-expand-body:emacs-lisp
-                                body params))))))
+    (let* ((lexical (cdr (assq :lexical params)))
+	   (result
+	    (eval (read (format (if (member "output"
+					    (cdr (assq :result-params params)))
+				    "(with-output-to-string %s)"
+				  "(progn %s)")
+				(org-babel-expand-body:emacs-lisp
+				 body params)))
+
+		  (if (listp lexical)
+		      lexical
+		    (member lexical '("yes" "t"))))))
       (org-babel-result-cond (cdr (assoc :result-params params))
 	(let ((print-level nil)
               (print-length nil))
-- 
2.4.4

Re: [PATCH] Make lexical eval default for elisp src blocks

[Nicolas Goaziou]

Hello,

John Kitchin <jkitchin@andrew.cmu.edu> writes:

> Set default in `org-babel-default-header-args:emacs-lisp'. Add an
> optional argument to the eval function.

Applied. Thank you.

However, it seems that some tests are now failing. I guess this is
related to Babel calls, which are eval'ed as emacs-lisp source blocks.

I have planned to change how Babel calls are evaluated during the next
days. This may make the failures disappear. Meanwhile, we could
force :lexical "no" in that case.

WDYT?


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Make lexical eval default for elisp src blocks

[John Kitchin]

I think it would be fine to make :lexical "no" be the default, since
that should preserve what we are used to. Users can alway set a
different default of their own, or make it "yes" when they know it is
needed.

Nicolas Goaziou writes:

> Hello,
>
> John Kitchin <jkitchin@andrew.cmu.edu> writes:
>
>> Set default in `org-babel-default-header-args:emacs-lisp'. Add an
>> optional argument to the eval function.
>
> Applied. Thank you.
>
> However, it seems that some tests are now failing. I guess this is
> related to Babel calls, which are eval'ed as emacs-lisp source blocks.
>
> I have planned to change how Babel calls are evaluated during the next
> days. This may make the failures disappear. Meanwhile, we could
> force :lexical "no" in that case.
>
> WDYT?
>
>
> Regards,


--
Professor John Kitchin
Doherty Hall A207F
Department of Chemical Engineering
Carnegie Mellon University
Pittsburgh, PA 15213
412-268-7803
@johnkitchin
http://kitchingroup.cheme.cmu.edu

Re: [PATCH] Make lexical eval default for elisp src blocks

[Nicolas Goaziou]

Hello,

John Kitchin <jkitchin@andrew.cmu.edu> writes:

> I think it would be fine to make :lexical "no" be the default, since
> that should preserve what we are used to. Users can alway set a
> different default of their own, or make it "yes" when they know it is
> needed.

Fair enough. Done. Thank you.

Regards,

-- 
Nicolas Goaziou