Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)]

Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)]

[Štěpán Němec]

Recipe:
-------

emacs -Q
M-x load-library RET ob-python RET
M-x org-mode RET

#+begin_src python :var text="a\nb\nc"
return text
#+end_src

#+RESULTS:
: a
: 	b
: 	c


Commentary:
-----------

ob-python seems to prepend a TAB character to every line except for the
first one.

Emacs  : GNU Emacs 28.0.50 (build 11, x86_64-pc-linux-gnu, GTK+ Version 3.24.14, cairo version 1.17.3)
 of 2020-03-11
Package: Org mode version 9.3.6 (release_9.3.6-397-ga08960 @ /home/stepnem/.emacs.d/lib/org/lisp/)

Re: Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)]

[Jack Kamm]

Hello,

Thanks for reporting. I've just fixed this issue in master (commit
6149b6cb6).

The problem was that ob-python adds tab indentation to the code body
before putting it inside a main() function, which adds spurious
indentation to multiline strings passed through :var.

I fixed the issue by moving variable assignment from the code body to
the code preamble, which is executed outside the main() function.

Best,
Jack

Štěpán Němec <stepnem@gmail.com> writes:

> Recipe:
> -------
>
> emacs -Q
> M-x load-library RET ob-python RET
> M-x org-mode RET
>
> #+begin_src python :var text="a\nb\nc"
> return text
> #+end_src
>
> #+RESULTS:
> : a
> : 	b
> : 	c
>
>
> Commentary:
> -----------
>
> ob-python seems to prepend a TAB character to every line except for the
> first one.
>
> Emacs  : GNU Emacs 28.0.50 (build 11, x86_64-pc-linux-gnu, GTK+ Version 3.24.14, cairo version 1.17.3)
>  of 2020-03-11
> Package: Org mode version 9.3.6 (release_9.3.6-397-ga08960 @ /home/stepnem/.emacs.d/lib/org/lisp/)

Re: Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)]

[Matthew Lundin]

Jack Kamm <jackkamm@gmail.com> writes:

> Hello,
>
> Thanks for reporting. I've just fixed this issue in master (commit
> 6149b6cb6).
>
> The problem was that ob-python adds tab indentation to the code body
> before putting it inside a main() function, which adds spurious
> indentation to multiline strings passed through :var.
>
> I fixed the issue by moving variable assignment from the code body to
> the code preamble, which is executed outside the main() function.

A heads up... I believe this changes the scope of the :var variables,
since they previously were local to the main() function and now they are
declared globally. After this change, some of my existing python source
blocks (i.e., ones in which I attempt to assign a new value to a
variable defined by :var) now generate the following error:

Traceback (most recent call last):
  File "<stdin>", line 223, in <module>
  File "<stdin>", line 214, in main
UnboundLocalError: local variable 'members' referenced before assignment

I hesitate to call this a bug, since it would be fine to think of
everything within the source block as local and the header :var
declarations as global. But if this is the case, then I think it should
be documented somewhere, especially since this change may break people's
existing source blocks. And I suppose it would be worthwhile to ask: Is
this change consistent with other org-babel modules? Is there a
canonical way that org-babel handles scope?

Best,

Matt

Re: Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)]

[Jack Kamm]

Hi Matt --

> A heads up... I believe this changes the scope of the :var variables,
> since they previously were local to the main() function and now they are
> declared globally. After this change, some of my existing python source
> blocks (i.e., ones in which I attempt to assign a new value to a
> variable defined by :var) now generate the following error:
>
> Traceback (most recent call last):
>   File "<stdin>", line 223, in <module>
>   File "<stdin>", line 214, in main
> UnboundLocalError: local variable 'members' referenced before assignment

Thanks for noticing this and pointing it out. This was an oversight on
my end (I don't really use ":var" or non-session blocks).

Unfortunately, the fix for the original bug will have to be a bit more
complicated to avoid this error. I'll put it on my todo list, but if
anyone wants to have a crack at it, that would be very welcome.

We should also add a unit test for this regression so it doesn't happen
again.

> I hesitate to call this a bug, since it would be fine to think of
> everything within the source block as local and the header :var
> declarations as global.

I think it's fair to call this a bug. I think it would be inconvenient
to be unable to assign to these variables. Also, I did not intend to
break any existing code with this.

> And I suppose it would be worthwhile to ask: Is this change consistent
> with other org-babel modules? Is there a canonical way that org-babel
> handles scope?

IMO everything ought to be in the same scope, and the user shouldn't
have to think about the scope of the main() function that ob-python
wraps the body in. Especially since non-session blocks are evaluated
independently of other blocks, we really shouldn't have to think about
their scope.

I'm not sure if the Org manual provides any guidance here, but from a
quick check I didn't see anything about it.

Ideally I think everything should just be in global scope; however
ob-python needs to use a wrapper function for the "return" keyword.

Re: Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)]

[Jack Kamm]

An update on this -- I decided to revert my fix for multiline Python
variables. I left the unit test I added for this, but marked it as
expected to fail.

I'll try to submit a proper fix for this soon. However I've been swamped
at work so I can't promise when (hopefully a couple weeks). In the
meantime, I think it better to leave the original bug in place, rather
than break any existing ob-python use cases.

When I do get to this, I'll submit it as a patch to the list first, to
make sure I don't accidentally cause a new bug.

Best,
Jack

Jack Kamm <jackkamm@gmail.com> writes:

> Hi Matt --
>
>> A heads up... I believe this changes the scope of the :var variables,
>> since they previously were local to the main() function and now they are
>> declared globally. After this change, some of my existing python source
>> blocks (i.e., ones in which I attempt to assign a new value to a
>> variable defined by :var) now generate the following error:
>>
>> Traceback (most recent call last):
>>   File "<stdin>", line 223, in <module>
>>   File "<stdin>", line 214, in main
>> UnboundLocalError: local variable 'members' referenced before assignment
>
> Thanks for noticing this and pointing it out. This was an oversight on
> my end (I don't really use ":var" or non-session blocks).
>
> Unfortunately, the fix for the original bug will have to be a bit more
> complicated to avoid this error. I'll put it on my todo list, but if
> anyone wants to have a crack at it, that would be very welcome.
>
> We should also add a unit test for this regression so it doesn't happen
> again.
>
>> I hesitate to call this a bug, since it would be fine to think of
>> everything within the source block as local and the header :var
>> declarations as global.
>
> I think it's fair to call this a bug. I think it would be inconvenient
> to be unable to assign to these variables. Also, I did not intend to
> break any existing code with this.
>
>> And I suppose it would be worthwhile to ask: Is this change consistent
>> with other org-babel modules? Is there a canonical way that org-babel
>> handles scope?
>
> IMO everything ought to be in the same scope, and the user shouldn't
> have to think about the scope of the main() function that ob-python
> wraps the body in. Especially since non-session blocks are evaluated
> independently of other blocks, we really shouldn't have to think about
> their scope.
>
> I'm not sure if the Org manual provides any guidance here, but from a
> quick check I didn't see anything about it.
>
> Ideally I think everything should just be in global scope; however
> ob-python needs to use a wrapper function for the "return" keyword.

Re: Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)]

[Jack Kamm]

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

Hi,

Here's my second attempt to fix the issue. I'll wait a few days for
comments before merging, to try and avoid causing a bug like last time.

It turns out the problem was deeper than just the :var argument,
multiline strings in the body were also getting mangled. For example,
this block had the same problem as the original report:

#+begin_src python
  text = """a
  b
  c
  """
  return text
#+end_src

#+RESULTS:
: a
: 	b
: 	c

My fix this time was to use functions from python.el to indent, and to
detect whether we are in a string and shouldn't be indented.

I also added a couple more unit tests, one for multiline strings, and
one for the variable scope/assignment issue that Matt reported.

Cheers,
Jack


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ob-python.el-Fix-multiline-strings-in-non-session-re.patch --]
[-- Type: text/x-patch, Size: 2904 bytes --]

From 179178d39f6216172e1a070f570cf941f99b1a89 Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Sat, 6 Jun 2020 10:59:23 -0700
Subject: [PATCH] ob-python.el: Fix multiline strings in non-session :results
 value

* lisp/ob-python.el (org-babel-python-evaluate-external-process): Use
functions from python.el to indent lines, avoiding multiline strings.
* testing/lisp/test-ob-python.el (test-ob-python/multiline-var): Set
test as expected to succeed.
(test-ob-python/multiline-str): Add test for multiline string in body.
(test-ob-python/header-var-assignment): Test that :var is in correct
scope and can be assigned to.

cf. https://orgmode.org/list/87tv009l9a.fsf@gmail.com/#t
---
 lisp/ob-python.el              | 14 +++++++++-----
 testing/lisp/test-ob-python.el | 20 +++++++++++++++++++-
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index dbcfac08d..622f69ce3 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -296,11 +296,15 @@ (defun org-babel-python-evaluate-external-process
 			(if (member "pp" result-params)
 			    org-babel-python-pp-wrapper-method
 			  org-babel-python-wrapper-method)
-			(mapconcat
-			 (lambda (line) (format "\t%s" line))
-			 (split-string (org-remove-indentation (org-trim body))
-				       "[\r\n]")
-			 "\n")
+			(with-temp-buffer
+			 (insert body)
+			 (goto-char (point-min))
+			 (while (< (point) (point-max))
+			   (unless (python-syntax-context 'string)
+			     (python-indent-shift-right (line-beginning-position)
+							(line-end-position)))
+			   (forward-line 1))
+			 (buffer-string))
 			(org-babel-process-file-name tmp-file 'noquote))))
 		     (org-babel-eval-read-file tmp-file))))))
     (org-babel-result-cond result-params
diff --git a/testing/lisp/test-ob-python.el b/testing/lisp/test-ob-python.el
index 763942b16..e3f0571a1 100644
--- a/testing/lisp/test-ob-python.el
+++ b/testing/lisp/test-ob-python.el
@@ -174,7 +174,6 @@ (ert-deftest test-ob-python/assign-underscore ()
 	    (org-babel-execute-src-block)))))
 
 (ert-deftest test-ob-python/multiline-var ()
-  :expected-result :failed
   (should
    (equal "a\nb\nc"
 	  (org-test-with-temp-text "#+begin_src python :var text=\"a\\nb\\nc\"
@@ -182,6 +181,25 @@ (ert-deftest test-ob-python/multiline-var ()
 #+end_src"
 	    (org-babel-execute-src-block)))))
 
+(ert-deftest test-ob-python/multiline-str ()
+  (should
+   (equal "a\nb\nc"
+	  (org-test-with-temp-text "#+begin_src python
+text=\"a\\nb\\nc\"
+return text
+#+end_src"
+	    (org-babel-execute-src-block)))))
+
+(ert-deftest test-ob-python/header-var-assignment ()
+  (should
+   (equal "success"
+	  (org-test-with-temp-text "#+begin_src python :var text=\"failure\"
+text
+text=\"success\"
+return text
+#+end_src"
+	    (org-babel-execute-src-block)))))
+
 (provide 'test-ob-python)
 
 ;;; test-ob-python.el ends here
-- 
2.27.0

Re: Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)]

[Kyle Meyer]

Jack Kamm writes:

> My fix this time was to use functions from python.el to indent, and to
> detect whether we are in a string and shouldn't be indented.
>
> I also added a couple more unit tests, one for multiline strings, and
> one for the variable scope/assignment issue that Matt reported.

Thanks for working on this.

> diff --git a/lisp/ob-python.el b/lisp/ob-python.el
> index dbcfac08d..622f69ce3 100644
> --- a/lisp/ob-python.el
> +++ b/lisp/ob-python.el
> @@ -296,11 +296,15 @@ (defun org-babel-python-evaluate-external-process
>  			(if (member "pp" result-params)
>  			    org-babel-python-pp-wrapper-method
>  			  org-babel-python-wrapper-method)
> -			(mapconcat
> -			 (lambda (line) (format "\t%s" line))
> -			 (split-string (org-remove-indentation (org-trim body))
> -				       "[\r\n]")
> -			 "\n")
> +			(with-temp-buffer
> +			 (insert body)
> +			 (goto-char (point-min))
> +			 (while (< (point) (point-max))

nit-pick: In this code base, (not (eobp)) is the more common way to
spell this.

    % git grep "(< (point) (point-max))" master | wc -l
    1
    % git grep "(not (eobp))" master | wc -l
    44

> +			   (unless (python-syntax-context 'string)
> +			     (python-indent-shift-right (line-beginning-position)
> +							(line-end-position)))

These lead to byte-compiler warnings:

    Compiling /home/kyle/src/emacs/org-mode-devel/lisp/ob-python.el...
    
    In end of data:
    ob-python.el:391:1:Warning: the following functions are not known to be
        defined: python-syntax-context, python-indent-shift-right

At the moment, python.el is only loaded for sessions (in
org-babel-python-initiate-session-by-key), assuming
org-babel-python-mode is set to `python'.  With the above change, you're
now using python.el functions in a non-session code path, so you'll need
a (require 'python) somewhere.

Also, just a note: I wondered whether python-syntax-context and
python-indent-shift-right are available in Emacs 24.3, the minimum Emacs
version we support.  They are.

Re: Bug: ob-python mangles multiline :var values [9.3.6 (release_9.3.6-397-ga089600)]

[Jack Kamm]

Hi Kyle,

Thanks for the code review which was very helpful as always. I've fixed
the style and compile errors that you noticed, and pushed the commit to
master.

Cheers,
Jack