[PATCH] ob-haskell: Line Continuations Mangle Block Output

[PATCH] ob-haskell: Line Continuations Mangle Block Output

[Nick Daly]

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

Hi Org Maintainers,

Please see the attached patch to remove "Prelude> " and "Prelude| " line
continuations from the block result output when parsing blocks that contain
multi-line function declarations.

This likely requires yesterday's patch to return value-type results from
Haskell blocks.  This patch applies cleanly against the org-mode master.

#+name: chain-ecm
#+BEGIN_SRC haskell
  :{
  chain :: (Integral a) => a -> [a]
  chain 1 = [1]
  chain n
      | even n = n:chain (n `div` 2)
      | odd n  = n:chain (n*3 + 1)
  :}
  chain 10
#+END_SRC

Results without patch:
: Prelude| Prelude| Prelude| Prelude| Prelude| Prelude| Prelude>
[10,5,16,8,4,2,1]

Results with patch:
: | 10 | 5 | 16 | 8 | 4 | 2 | 1 |

Thank you for your time,
Nick

[-- Attachment #2: ob-haskell-trim-prelude.diff --]
[-- Type: text/x-patch, Size: 783 bytes --]

--- lisp/ob-haskell.el
+++ lisp/ob-haskell.el
@@ -84,9 +84,11 @@
                                        (reverse (mapcar #'org-trim raw)))))))
     (org-babel-reassemble-table
      (let ((result
-            (pcase result-type
-              (`output (mapconcat #'identity (reverse results) "\n"))
-              (`value (car results)))))
+	    (replace-regexp-in-string
+	     "Prelude[|>] " ""
+             (pcase result-type
+               (`output (mapconcat #'identity (reverse results) "\n"))
+               (`value (car results))))))
        (org-babel-result-cond (cdr (assq :result-params params))
 	 result (if (stringp result) (org-babel-script-escape result))))
      (org-babel-pick-name (cdr (assq :colname-names params))

Diff finished.  Sun May 17 14:26:21 2020

Re: [PATCH] ob-haskell: Line Continuations Mangle Block Output

[Nick Daly]

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

Hi Org Maintainers,

Attached is an updated patch that makes output trimming work with
blocks that do and don't produce results.  The old patch creates a
=let: Wrong type argument: arrayp, nil= error when evaluating blocks
that don't produce output.  This necessarily incorporates yesterday's
patch.

Thanks for your time,
Nick


Multi-line function declarations with output still work fine.

#+BEGIN_SRC haskell
  :{
  chain :: (Integral a) => a -> [a]
  chain 1 = [1]
  chain n
      | even n = n:chain (n `div` 2)
      | odd n  = n:chain (n*3 + 1)
  :}
  chain 10
#+END_SRC

#+RESULTS:
| 10 | 5 | 16 | 8 | 4 | 2 | 1 |


Silent declaration-only blocks correctly evaluate silently.

#+BEGIN_SRC haskell :results silent
  :{
  flip' :: (a -> b -> c) -> (b -> a -> c)
  flip' f = \x y -> f y x
  :}
#+END_SRC


Single-line function calls also return the expected results.

#+name: flip'-hello
#+BEGIN_SRC haskell
  flip' zip [1,2,3,4,5,6] "hello"
#+END_SRC

#+RESULTS: flip'-hello
| h | 1 |
| e | 2 |
| l | 3 |
| l | 4 |
| o | 5 |

[-- Attachment #2: ob-haskell-trim-prelude-2.diff --]
[-- Type: text/x-patch, Size: 1031 bytes --]

diff --git a/lisp/ob-haskell.el b/lisp/ob-haskell.el
index bea162528..cb581fe3b 100644
--- a/lisp/ob-haskell.el
+++ b/lisp/ob-haskell.el
@@ -83,12 +83,16 @@
 			  (cdr (member org-babel-haskell-eoe
                                        (reverse (mapcar #'org-trim raw)))))))
     (org-babel-reassemble-table
-     (let ((result
+     (let* ((result
             (pcase result-type
               (`output (mapconcat #'identity (reverse results) "\n"))
-              (`value (car results)))))
+              (`value (car results))))
+            (result
+             (if (stringp result)
+                 (replace-regexp-in-string "Prelude[|>] " "" result)
+               result)))
        (org-babel-result-cond (cdr (assq :result-params params))
-	 result (org-babel-script-escape result)))
+	 result (if (stringp result) (org-babel-script-escape result))))
      (org-babel-pick-name (cdr (assq :colname-names params))
 			  (cdr (assq :colname-names params)))
      (org-babel-pick-name (cdr (assq :rowname-names params))

Re: [PATCH] ob-haskell: Line Continuations Mangle Block Output

[Kyle Meyer]

Nick Daly writes:

> Attached is an updated patch that makes output trimming work with
> blocks that do and don't produce results.  The old patch creates a
> =let: Wrong type argument: arrayp, nil= error when evaluating blocks
> that don't produce output.  This necessarily incorporates yesterday's
> patch.

Thanks for the patch.

> diff --git a/lisp/ob-haskell.el b/lisp/ob-haskell.el
> index bea162528..cb581fe3b 100644
> --- a/lisp/ob-haskell.el
> +++ b/lisp/ob-haskell.el
> @@ -83,12 +83,16 @@
>  			  (cdr (member org-babel-haskell-eoe
>                                         (reverse (mapcar #'org-trim raw)))))))
>      (org-babel-reassemble-table
> -     (let ((result
> +     (let* ((result
>              (pcase result-type
>                (`output (mapconcat #'identity (reverse results) "\n"))
> -              (`value (car results)))))
> +              (`value (car results))))
> +            (result
> +             (if (stringp result)
> +                 (replace-regexp-in-string "Prelude[|>] " "" result)
> +               result)))

Oy, it's pretty nasty that those leak through.  I know ob-python (and
probably other languages) also suffers from similar brittleness.  It'd
be nice of course to figure out how to prevent the prompts leakage in
the first place, but, short of that, I think we should at least make the
regexp stricter so that it matches just the start of the string.

And that (stringp result) check is for the same reason as the one from
your first patch from yesterday which is now included ...

>         (org-babel-result-cond (cdr (assq :result-params params))
> -	 result (org-babel-script-escape result)))
> +	 result (if (stringp result) (org-babel-script-escape result))))

... here.  I believe result is nil in the problematic case, so this
could be

    (and result (org-babel-script-escape result))

However, based on stepping through the example in your patch from
yesterday, I think these two issues might be more closely related than
you realize.  In the

    (cdr (member org-babel-haskell-eoe
                 (reverse (mapcar #'org-trim raw))))

bit visible as a context line above, this is what I see for raw when I
step through org-babel-execute:haskell:

    ("Prelude| Prelude| Prelude| Prelude> \"org-babel-haskell-eoe\"" "")

So it looks like the member call above is returning nil because the
prompt markers are corrupting the element.  If that's the case, it seems
like the output cleansing should happen upstream of that call.

What do you think?

Re: [PATCH] ob-haskell: Line Continuations Mangle Block Output

[Nick Daly]

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

Hi Kyle, thanks for the thoughtful analysis.

On Wed, May 20, 2020 at 12:51 AM Kyle Meyer <kyle@kyleam.com> wrote:
> So it looks like the member call above is returning nil because the
> prompt markers are corrupting the element.  If that's the case, it seems
> like the output cleansing should happen upstream of that call.
>
> What do you think?

After a bit of tinkering, I realized there are two things going on
here, only one of which I fully understand:

1. My core functional issue is that =comint-prompt-regexp= isn't set
   up to handle the "Prelude| " entries or the repeated prompts.  The
   other patches I submitted were unnecessary.

2. The =comint-prompt-regexp= gets default values from somewhere I
   don't understand and can't find with a quick source grep.

In ob-haskell, we set =comint-prompt-regexp= to the (undefined)
haskell-prompt plus "or optional-lambda":

  (defvar haskell-prompt-regexp)

  (defun org-babel-execute:haskell (body params) ...
  (setq-local comint-prompt-regexp
    (concat haskell-prompt-regexp "\\|^λ?> "))))

That causes an evaluation error that prevents the first source block
evaluation but, strangely, that also results in this mess in the
*haskell* buffer on subsequent evaluations:

: "^\\*?[[:upper:]][\\._[:alnum:]]*\\(?:
\\*?[[:upper:]][\\._[:alnum:]]*\\)*\\( λ\\)?> "

=comint-prompt-regexp='s variable documentation calls out much simpler
regexps that do basically the same thing as the one above and handles
the repeated "Prelude| " entries.  This one is based off the Canonical
Lisp example:

: "^[^>\n]+\\(> \\)?"

I've attached a patch against git master that results in fewer
undefined variable errors and depends less default-variable magic.
Now, =haskell-prompt-regexp= and =comint-prompt-regexp= are explicitly
set using defaults that can be M-x customized, and the default value
handles the repeated "Prelude| " entries without breaking the original
"λ> " prompt handling.

Thanks,
Nick

[-- Attachment #2: ob-haskell-trim-prelude-3.diff --]
[-- Type: text/x-patch, Size: 988 bytes --]

diff --git a/lisp/ob-haskell.el b/lisp/ob-haskell.el
index bea162528..893e4220c 100644
--- a/lisp/ob-haskell.el
+++ b/lisp/ob-haskell.el
@@ -56,15 +56,25 @@
 
 (defvar org-babel-haskell-eoe "\"org-babel-haskell-eoe\"")
 
-(defvar haskell-prompt-regexp)
+(defvar haskell-prompt-regexp "^[^>\n]*\\(> \\)?"
+  "Filter out prompts from Haskell interpreters:
+
+GHC:
+
+- '^Prelude> '
+- '^Prelude| Prelude| Prelude> '
+
+Unknown Interpreter:
+
+- '^> '
+- '^λ> '")
 
 (defun org-babel-execute:haskell (body params)
   "Execute a block of Haskell code."
   (require 'inf-haskell)
   (add-hook 'inferior-haskell-hook
             (lambda ()
-              (setq-local comint-prompt-regexp
-                          (concat haskell-prompt-regexp "\\|^λ?> "))))
+              (setq-local comint-prompt-regexp haskell-prompt-regexp)))
   (let* ((session (cdr (assq :session params)))
          (result-type (cdr (assq :result-type params)))
          (full-body (org-babel-expand-body:generic

Re: [PATCH] ob-haskell: Line Continuations Mangle Block Output

[Nick Daly]

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

Apologies, one last patch.

On Sat, May 23, 2020 at 7:02 PM Nick Daly <nick.m.daly@gmail.com> wrote:
> : "^\\*?[[:upper:]][\\._[:alnum:]]*\\(?:
> \\*?[[:upper:]][\\._[:alnum:]]*\\)*\\( λ\\)?> "
>
> =comint-prompt-regexp='s variable documentation calls out much simpler
> regexps
>
> : "^[^>]+\\(> \\)?"

This simplified patch breaks one case that I'd forgotten about: the
true one-liner, where the output displays before the "Prelude> "
prompt even appears.

#+BEGIN_SRC haskell
scanl (+) 0 [1,2,3,4]
#+END_SRC

#+BEGIN_EXAMPLE
Prelude> scanl (+) 0 [1,2,3,4]
"org-babel-haskell-eoe"
[0,1,3,6,10]
Prelude> "org-babel-haskell-eoe"
Prelude>
#+END_EXAMPLE

This latest patch updates the original (more complicated) regexp that
works with this out-of-order output.  This should display the expected
result in all known cases:

One liners:

#+BEGIN_SRC haskell
scanl (+) 0 [1,2,3,4]
#+END_SRC

#+RESULTS:
| 0 | 1 | 3 | 6 | 10 |

Silent multi-line blocks:

#+BEGIN_SRC haskell :results silent
:{
flip' :: (a -> b -> c) -> (b -> a -> c)
flip' f = \x y -> f y x
:}
#+END_SRC

Multi-line blocks with value results:

#+BEGIN_SRC haskell
:{
sum' :: (Num a) => [a] -> a
sum' xs = foldl (\ acc x -> acc + x) 0 xs
:}
sum' [1,2,3,4] == 10
#+END_SRC

#+RESULTS:
: True

Multi-line blocks with output results:

#+BEGIN_SRC haskell :results output
:{
sum' :: (Num a) => [a] -> a
sum' xs = foldl (\ acc x -> acc + x) 0 xs
:}
print "hi"
#+END_SRC

#+RESULTS:
:
: hi

Thanks again for your time,
Nick

[-- Attachment #2: ob-haskell-trim-prelude-4.diff --]
[-- Type: text/x-patch, Size: 1116 bytes --]

diff --git a/lisp/ob-haskell.el b/lisp/ob-haskell.el
index bea162528..6ac34f2f5 100644
--- a/lisp/ob-haskell.el
+++ b/lisp/ob-haskell.el
@@ -56,15 +56,27 @@
 
 (defvar org-babel-haskell-eoe "\"org-babel-haskell-eoe\"")
 
-(defvar haskell-prompt-regexp)
+(defvar haskell-prompt-regexp "^\\(\\*?[[:upper:]][\\._[:alnum:]]*\\(?: \\*?[[:upper:]][\\._[:alnum:]]*\\)*\\( λ\\)?[|>] \\)*"
+  "Filter out prompts from Haskell interpreters:
+
+GHC:
+
+- 'output
+   ^Prelude> EOE'
+- '^Prelude> output EOE'
+- '^Prelude| Prelude| Prelude> output EOE'
+
+Unknown Interpreter:
+
+- '^> '
+- '^λ> '")
 
 (defun org-babel-execute:haskell (body params)
   "Execute a block of Haskell code."
   (require 'inf-haskell)
   (add-hook 'inferior-haskell-hook
             (lambda ()
-              (setq-local comint-prompt-regexp
-                          (concat haskell-prompt-regexp "\\|^λ?> "))))
+              (setq-local comint-prompt-regexp haskell-prompt-regexp)))
   (let* ((session (cdr (assq :session params)))
          (result-type (cdr (assq :result-type params)))
          (full-body (org-babel-expand-body:generic

Re: [PATCH] ob-haskell: Line Continuations Mangle Block Output

[Kyle Meyer]

Nick Daly writes:

> On Sat, May 23, 2020 at 7:02 PM Nick Daly <nick.m.daly@gmail.com> wrote:
>> : "^\\*?[[:upper:]][\\._[:alnum:]]*\\(?:
>> \\*?[[:upper:]][\\._[:alnum:]]*\\)*\\( λ\\)?> "
>>
>> =comint-prompt-regexp='s variable documentation calls out much simpler
>> regexps
>>
>> : "^[^>]+\\(> \\)?"
>
> This simplified patch breaks one case that I'd forgotten about: the
> true one-liner, where the output displays before the "Prelude> "
> prompt even appears.
>
> #+BEGIN_SRC haskell
> [... several examples ... ]

Thanks for the nice examples.  It'd be great to eventually include at
least some of them as tests.

> diff --git a/lisp/ob-haskell.el b/lisp/ob-haskell.el
> index bea162528..6ac34f2f5 100644
> --- a/lisp/ob-haskell.el
> +++ b/lisp/ob-haskell.el
> @@ -56,15 +56,27 @@

For the next iteration, could you send a patch generated by
git-format-patch?  See <https://orgmode.org/worg/org-contribute.html>.

Please also consider signing copyright papers, but I suspect the
ob-haskell.el changes will end up being few enough lines that they can
be accepted as a TINYCHANGE.

>  (defvar org-babel-haskell-eoe "\"org-babel-haskell-eoe\"")
>  
> -(defvar haskell-prompt-regexp)
> +(defvar haskell-prompt-regexp "^\\(\\*?[[:upper:]][\\._[:alnum:]]*\\(?: \\*?[[:upper:]][\\._[:alnum:]]*\\)*\\( λ\\)?[|>] \\)*"
> +  "Filter out prompts from Haskell interpreters:
> +
> +GHC:
> +
> +- 'output
> +   ^Prelude> EOE'
> +- '^Prelude> output EOE'
> +- '^Prelude| Prelude| Prelude> output EOE'
> +
> +Unknown Interpreter:
> +
> +- '^> '
> +- '^λ> '")

This is inf-haskell's variable.  ob-haskell shouldn't set it.  The
original (defvar haskell-prompt-regexp) just silenced the bytecompiler.
If ob-haskell needs to tweak comint-prompt-regexp to deal with the
"^Prelude| Prelude| Prelude" above, it should done without overwriting
an inf-haskell value.

As touched on in my other email, there's also the issue of compatibility
with versions of haskell mode before v17.1 that lack a
haskell-prompt-regexp.  I believe this is why you're seeing the
undefined errors.

So, I dunno.  I'm not a ob-haskell user or a really a Babel user.  But
in my view it'd be cleaner to just leave inferior-haskell-mode's
comint-prompt-regexp alone and strip the "Prelude| Prelude| ..."  once
the output comes out of org-babel-comint-with-output.  It's hacky and
error-prone, but I don't think it's so different than what
org-babel-comint-with-output already does.

Re: [PATCH] ob-haskell: Line Continuations Mangle Block Output

[Nick Daly]

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

Hi Kyle, thank you again for your help and analysis.  In summary,
org-mode needs no patches, but inf-haskell's comint-prompt-regexp
needs updates.  I'll follow up with the inf-haskell maintainers.

----

> inf-haskell used to set comint-prompt-regexp in the body of
> inferior-haskell-mode...  This is inf-haskell's variable.
> ob-haskell shouldn't set it.

Thank you for your help in all this.  I couldn't find where the regex
was set in the org-mode source because it wasn't coming from org-mode
at all.  Putting this all together suggests that the correct fix is to
update the inf-haskell regex to handle "Prelude| " correctly in the
first place.

I believe it's possible to compress the current inf-haskell regexp
from this:

    (setq-local comint-prompt-regexp
                ;; Why the backslash in [\\._[:alnum:]]?
                "^\\*?[[:upper:]][\\._[:alnum:]]*\\(?:
\\*?[[:upper:]][\\._[:alnum:]]*\\)*\\( λ\\)?> \\|^λ?> $")

Down into this, without any significant loss of fidelity.  We really
don't care about any characters at the start of the line before the
final "> ".

    (setq-local comint-prompt-regexp "^[[:alnum:].*_() |λ]*> ")

This seems useful because, as I discovered this morning, importing
modules mangles the prompt further:

    Prelude> import Data.Time
    Prelude Data.Time> :m + Data.Time.Clock
    Prelude Data.Time Data.Time.Clock>

The testing data that this configuration correctly parsed is attached
for reference.

I'll figure out where to submit that patch to, and send it off.  It's
a single line change that shouldn't require any assignment papers.

Thanks again,
Nick

[-- Attachment #2: ghc-prelude-prompt-testing.txt --]
[-- Type: text/plain, Size: 1257 bytes --]

Prelude> import Data.Time
Prelude Data.Time> :m + Data.Time.Clock
Prelude Data.Time Data.Time.Clock> scanl (+) 0 [1,2,3,4]
"org-babel-haskell-eoe"
[0,1,3,6,10]
Prelude Data.Time Data.Time.Clock> "org-babel-haskell-eoe"
Prelude Data.Time Data.Time.Clock> :{
flip' :: (a -> b -> c) -> (b -> a -> c)
flip' f = \x y -> f y x
:}
"org-babel-haskell-eoe"
Prelude Data.Time Data.Time.Clock| Prelude Data.Time Data.Time.Clock| Prelude Data.Time Data.Time.Clock| Prelude Data.Time Data.Time.Clock> "org-babel-haskell-eoe"
Prelude Data.Time Data.Time.Clock> :{
sum' :: (Num a) => [a] -> a
sum' xs = foldl (\ acc x -> acc + x) 0 xs
:}
sum' [1,2,3,4] == 10
"org-babel-haskell-eoe"
Prelude Data.Time Data.Time.Clock| Prelude Data.Time Data.Time.Clock| Prelude Data.Time Data.Time.Clock| Prelude Data.Time Data.Time.Clock> True
Prelude Data.Time Data.Time.Clock> "org-babel-haskell-eoe"
Prelude Data.Time Data.Time.Clock> :{
sum' :: (Num a) => [a] -> a
sum' xs = foldl (\ acc x -> acc + x) 0 xs
:}
print "hi"
"org-babel-haskell-eoe"
Prelude Data.Time Data.Time.Clock| Prelude Data.Time Data.Time.Clock| Prelude Data.Time Data.Time.Clock| Prelude Data.Time Data.Time.Clock> "hi"
Prelude Data.Time Data.Time.Clock> "org-babel-haskell-eoe"
Prelude Data.Time Data.Time.Clock>

Re: [PATCH] ob-haskell: Line Continuations Mangle Block Output

[Kyle Meyer]

Nick Daly writes:

> After a bit of tinkering, I realized there are two things going on
> here, only one of which I fully understand:
>
> 1. My core functional issue is that =comint-prompt-regexp= isn't set
>    up to handle the "Prelude| " entries or the repeated prompts.  The
>    other patches I submitted were unnecessary.
>
> 2. The =comint-prompt-regexp= gets default values from somewhere I
>    don't understand and can't find with a quick source grep.

Here's what I can gather.  inf-haskell used to set comint-prompt-regexp
in the body of inferior-haskell-mode.  Here's an example from 11d6abf
(2017-08-24):

  (setq-local comint-prompt-regexp
                ;; Why the backslash in [\\._[:alnum:]]?
                "^\\*?[[:upper:]][\\._[:alnum:]]*\\(?: \\*?[[:upper:]][\\._[:alnum:]]*\\)*\\( λ\\)?> \\|^λ?> $")

In ca94d81 (revamped inf-haskell, 2017-08-26), which was included in the
v17.1 release, haskell-prompt-regexp was introduced and the line above
is now

  (setq-local comint-prompt-regexp haskell-prompt-regexp)

> In ob-haskell, we set =comint-prompt-regexp= to the (undefined)
> haskell-prompt plus "or optional-lambda":

With a haskell-mode after the commit I point to above, it shouldn't be
undefined at the time we set comint-prompt-regexp because
org-babel-interpret-haskell loads inf-haskell before that.

However, I'm confused why Org's b46787743 (Fix ob-haskell.el to work
with custom ghci prompts, 2017-12-02) added the λ bit (author of that
patch cc'd).  As far as I can tell, that is a part of
inferior-haskell-mode's default comint-prompt-regexp and has been since
28997b2 (Add support for popular "λ> " prompt to inf-haskell,
2013-07-04).

You've sent an updated patch in a follow-up message, so I'll continue
there...