[PATCH] ob-tangle.el: restore :tangle closure nil behavior

[PATCH] ob-tangle.el: restore :tangle closure nil behavior

[Tom Gillespie]

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

Hi,
   Here's a patch to fix the :tangle header behavior when it is
passed a closure that returns nil. Best,
Tom

[-- Attachment #2: 0001-ob-tangle.el-restore-tangle-closure-nil-behavior.patch --]
[-- Type: text/x-patch, Size: 2741 bytes --]

From f1e15e0634fffed4648aa11628a14e0a68c3b18d Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Tue, 15 Aug 2023 13:46:08 -0700
Subject: [PATCH] ob-tangle.el: restore :tangle closure nil behavior

* lisp/ob-tangle.el (org-babel-tangle-collect-blocks):
* lisp/ob-tangle.el (org-babel-tangle--unbracketed-link):
* lisp/ob-tangle.el (org-babel-tangle-single-block): src-tfile fix
case where (cdr (assq :tangle params)) could be nil

This patch restores the original behavior of the :tangle header
argument when a closure returned nil, e.g. #+header: :tangle (or),
by using (or (cdr (assq :tangle params)) "no"). Without this the
call to `file-name-directory' in `org-babel-tangle--unbracketed-link'
can fail with a wrong-type-argument stringp nil error.
---
 lisp/ob-tangle.el | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lisp/ob-tangle.el b/lisp/ob-tangle.el
index 4566e03ad..0df649d7e 100644
--- a/lisp/ob-tangle.el
+++ b/lisp/ob-tangle.el
@@ -473,14 +473,14 @@ code blocks by target file."
 		  (org-in-archived-heading-p))
 	(let* ((info (org-babel-get-src-block-info 'no-eval))
 	       (src-lang (nth 0 info))
-	       (src-tfile (cdr (assq :tangle (nth 2 info)))))
+	       (src-tfile (or (cdr (assq :tangle (nth 2 info))) "no")))
 	  (unless (or (string= src-tfile "no")
 		      (and tangle-file (not (equal tangle-file src-tfile)))
 		      (and lang-re (not (string-match-p lang-re src-lang))))
 	    ;; Add the spec for this block to blocks under its tangled
 	    ;; file name.
 	    (let* ((block (org-babel-tangle-single-block counter))
-                   (src-tfile (cdr (assq :tangle (nth 4 block))))
+                   (src-tfile (or (cdr (assq :tangle (nth 4 block))) "no"))
 		   (file-name (org-babel-effective-tangled-filename
                                buffer-fn src-lang src-tfile))
 		   (by-fn (assoc file-name blocks)))
@@ -510,7 +510,7 @@ The PARAMS are the 3rd element of the info for the same src block."
               (concat "file:"
                       (file-relative-name (substring bare (match-end 0))
                                           (file-name-directory
-                                           (cdr (assq :tangle params)))))
+                                           (or (cdr (assq :tangle params)) "no"))))
             bare))))))
 
 (defvar org-outline-regexp) ; defined in lisp/org.el
@@ -580,7 +580,7 @@ non-nil, return the full association list to be used by
 			 (match-end 0)
 		       (point-min))))
 	      (point)))))
-         (src-tfile (cdr (assq :tangle params)))
+         (src-tfile (or (cdr (assq :tangle params)) "no"))
 	 (result
 	  (list start-line
 		(if org-babel-tangle-use-relative-file-links
-- 
2.41.0

Re: [PATCH] ob-tangle.el: restore :tangle closure nil behavior

[Tom Gillespie]

After a bit more investigation don't apply this patch because the change
is insufficient to correct another issue.

Specifically org-babel-tangle-collect-blocks must check for and resolve
any closures that are passed to :tangle _before_ testing (string=
src-tfile "no").
As it stands blocks that are marked :tangle (and "no") with a closure
incorrectly make it to a call to (org-babel-get-src-block-info) which causes
a call to org-babel-process-params when :tangle would be "no".

Working on a proper fix now.

Re: [PATCH] ob-tangle.el: restore :tangle closure nil behavior

[Tom Gillespie]

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

Here is a corrected patch that fixes the fact that closures passed to the
:tangle header were not being evaluated. Details in the commit message.
Best,
Tom

On Tue, Aug 15, 2023 at 6:41 PM Tom Gillespie <tgbugs@gmail.com> wrote:
>
> After a bit more investigation don't apply this patch because the change
> is insufficient to correct another issue.
>
> Specifically org-babel-tangle-collect-blocks must check for and resolve
> any closures that are passed to :tangle _before_ testing (string=
> src-tfile "no").
> As it stands blocks that are marked :tangle (and "no") with a closure
> incorrectly make it to a call to (org-babel-get-src-block-info) which causes
> a call to org-babel-process-params when :tangle would be "no".
>
> Working on a proper fix now.

[-- Attachment #2: 0001-ob-tangle.el-restore-tangle-closure-evaluation-befor.patch --]
[-- Type: text/x-patch, Size: 6587 bytes --]

From ce56fe5c1a24ba37c5c7c2f541c48684b0cb1e0b Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Tue, 15 Aug 2023 13:46:08 -0700
Subject: [PATCH] ob-tangle.el: restore :tangle closure evaluation before eval
 info

* lisp/ob-tangle.el (org-babel-tangle): check that :tangle is not no
before attempting to tangle a single block
(org-babel-tangle--unbracketed-link): new optional argument
info-was-evaled to control whether (cdr (assq :tangle params)) is
expanded via `org-babel-read'
(org-babel-tangle-collect-blocks): expand closures passed to :tangle
with `org-babel-read' so that blocks with :tangle closure that eval to
"no" will not incorrectly eval other parameters
(org-babel-tangle-single-block): src-tfile handle cases where output
of :tangle closure could be nil and conver to "no"

This patch fixes a bug where header arguments like :tangle (or "no")
were treated as if they were tangling to a file named "(or \"no\")".
As a result, org-bable would call org-babel-get-src-block-info with
'no-eval set to nil, causing parameters to be evaluated despite the
fact that when :tangle no or equivalent is set, the other parameters
should never be evaluated.

This patch also restores the original behavior of the :tangle header
argument when a closure returned nil, e.g. #+header: :tangle (or), by
using (or (cdr (assq :tangle params)) "no"). Without this the call to
`file-name-directory' in `org-babel-tangle--unbracketed-link' can fail
with a wrong-type-argument stringp nil error.
---
 lisp/ob-tangle.el | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/lisp/ob-tangle.el b/lisp/ob-tangle.el
index 4566e03ad..a9fb0e286 100644
--- a/lisp/ob-tangle.el
+++ b/lisp/ob-tangle.el
@@ -316,9 +316,13 @@ matching a regular expression."
 		     (write-region nil nil file-name)
 		     (mapc (lambda (mode) (set-file-modes file-name mode)) modes))
                    (push file-name path-collector))))))
-	 (if (equal arg '(4))
-	     (org-babel-tangle-single-block 1 t)
-	   (org-babel-tangle-collect-blocks lang-re tangle-file)))
+         (if (equal arg '(4))
+             (let* ((info (org-babel-get-src-block-info 'no-eval))
+                    (params (nth 2 info))
+                    (src-tfile (or (org-babel-read (cdr (assq :tangle params))) "no")))
+               (unless (string= src-tfile "no")
+                 (org-babel-tangle-single-block 1 t)))
+           (org-babel-tangle-collect-blocks lang-re tangle-file)))
 	(message "Tangled %d code block%s from %s" block-counter
 		 (if (= block-counter 1) "" "s")
 		 (file-name-nondirectory
@@ -473,14 +477,14 @@ code blocks by target file."
 		  (org-in-archived-heading-p))
 	(let* ((info (org-babel-get-src-block-info 'no-eval))
 	       (src-lang (nth 0 info))
-	       (src-tfile (cdr (assq :tangle (nth 2 info)))))
+	       (src-tfile (or (org-babel-read (cdr (assq :tangle (nth 2 info)))) "no")))
 	  (unless (or (string= src-tfile "no")
 		      (and tangle-file (not (equal tangle-file src-tfile)))
 		      (and lang-re (not (string-match-p lang-re src-lang))))
 	    ;; Add the spec for this block to blocks under its tangled
 	    ;; file name.
 	    (let* ((block (org-babel-tangle-single-block counter))
-                   (src-tfile (cdr (assq :tangle (nth 4 block))))
+                   (src-tfile (or (cdr (assq :tangle (nth 4 block))) "no"))
 		   (file-name (org-babel-effective-tangled-filename
                                buffer-fn src-lang src-tfile))
 		   (by-fn (assoc file-name blocks)))
@@ -490,7 +494,7 @@ code blocks by target file."
     (mapcar (lambda (b) (cons (car b) (nreverse (cdr b))))
 	    (nreverse blocks))))
 
-(defun org-babel-tangle--unbracketed-link (params)
+(defun org-babel-tangle--unbracketed-link (params &optional info-was-evaled)
   "Get a raw link to the src block at point, without brackets.
 
 The PARAMS are the 3rd element of the info for the same src block."
@@ -510,7 +514,10 @@ The PARAMS are the 3rd element of the info for the same src block."
               (concat "file:"
                       (file-relative-name (substring bare (match-end 0))
                                           (file-name-directory
-                                           (cdr (assq :tangle params)))))
+                                           (or (if info-was-evaled
+                                                   (cdr (assq :tangle params))
+                                                 (org-babel-read (cdr (assq :tangle params))))
+                                               "no"))))
             bare))))))
 
 (defvar org-outline-regexp) ; defined in lisp/org.el
@@ -520,6 +527,8 @@ Return the list of block attributes needed by
 `org-babel-tangle-collect-blocks'.  When ONLY-THIS-BLOCK is
 non-nil, return the full association list to be used by
 `org-babel-tangle' directly."
+  ;; this should only ever be called when :tangle has already been determined
+  ;; to be non-nil and not string= "no"
   (let* ((info (org-babel-get-src-block-info))
 	 (start-line
 	  (save-restriction (widen)
@@ -530,7 +539,7 @@ non-nil, return the full association list to be used by
 	 (extra (nth 3 info))
          (coderef (nth 6 info))
 	 (cref-regexp (org-src-coderef-regexp coderef))
-	 (link (org-babel-tangle--unbracketed-link params))
+	 (link (org-babel-tangle--unbracketed-link params 'info-was-evaled))
 	 (source-name
 	  (or (nth 4 info)
 	      (format "%s:%d"
@@ -580,7 +589,7 @@ non-nil, return the full association list to be used by
 			 (match-end 0)
 		       (point-min))))
 	      (point)))))
-         (src-tfile (cdr (assq :tangle params)))
+         (src-tfile (or (cdr (assq :tangle params)) "no"))
 	 (result
 	  (list start-line
 		(if org-babel-tangle-use-relative-file-links
@@ -602,6 +611,11 @@ non-nil, return the full association list to be used by
   "Return a list of begin and end link comments for the code block at point.
 INFO, when non nil, is the source block information, as returned
 by `org-babel-get-src-block-info'."
+  ;; currently all call sites for this function pass info that came from a
+  ;; 'no-eval call to `org-babel-get-src-block-info', specifically they come
+  ;; info populated into `org-babel-expand-noweb-references--cache', as such
+  ;; we DO NOT pass the indicator that 'info-was-evaled to
+  ;; `org-babel-tangle--unbracketed-link' below
   (let ((link-data (pcase (or info (org-babel-get-src-block-info 'no-eval))
 		     (`(,_ ,_ ,params ,_ ,name ,start ,_)
 		      `(("start-line" . ,(org-with-point-at start
-- 
2.41.0

Re: [PATCH] ob-tangle.el: restore :tangle closure nil behavior

[Ihor Radchenko]

Tom Gillespie <tgbugs@gmail.com> writes:

> Subject: [PATCH] ob-tangle.el: restore :tangle closure evaluation before eval
>  info
> This patch fixes a bug where header arguments like :tangle (or "no")
> were treated as if they were tangling to a file named "(or \"no\")".
> As a result, org-bable would call org-babel-get-src-block-info with
> 'no-eval set to nil, causing parameters to be evaluated despite the
> fact that when :tangle no or equivalent is set, the other parameters
> should never be evaluated.

What do you mean by "restore"? Were it evaluated in the past?
May you please provide a reproducer?

> -(defun org-babel-tangle--unbracketed-link (params)
> +(defun org-babel-tangle--unbracketed-link (params &optional info-was-evaled)

This is not acceptable. Taking care about evaluating INFO should be done
in a single place instead of adding checks across the babel code. If we
go the proposed way, I expect a number of bugs appearing when somebody
forgets to change the eval check in some place.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] ob-tangle.el: restore :tangle closure nil behavior

[Tom Gillespie]

On Wed, Aug 16, 2023 at 2:09 AM Ihor Radchenko <yantar92@posteo.net> wrote:
>
> Tom Gillespie <tgbugs@gmail.com> writes:
>
> > Subject: [PATCH] ob-tangle.el: restore :tangle closure evaluation before eval
> >  info
> > This patch fixes a bug where header arguments like :tangle (or "no")
> > were treated as if they were tangling to a file named "(or \"no\")".
> > As a result, org-bable would call org-babel-get-src-block-info with
> > 'no-eval set to nil, causing parameters to be evaluated despite the
> > fact that when :tangle no or equivalent is set, the other parameters
> > should never be evaluated.
>
> What do you mean by "restore"? Were it evaluated in the past?
> May you please provide a reproducer?

Hrm. I think I may have mixed two commit lines. It is the case that
:tangle closures used to work, but you are right, the historical behavior
when tangling closures meant that all parameters were evaluated (tested
with the block below in 27 and 28).

#+begin_src elisp :var value=(error "oops") :tangle (or "no")
value
#+end_src

My use case is that I have blocks that I want to tangle that set :var
from e.g. the library of babel, which isn't always loaded, but which also
is not required if :tangle is no.

> > -(defun org-babel-tangle--unbracketed-link (params)
> > +(defun org-babel-tangle--unbracketed-link (params &optional info-was-evaled)
>
> This is not acceptable. Taking care about evaluating INFO should be done
> in a single place instead of adding checks across the babel code. If we
> go the proposed way, I expect a number of bugs appearing when somebody
> forgets to change the eval check in some place.

I don't like the solution either. I see two potential alternatives.
1. change the structure of the info list to indicate whether it has
already been evaluated
2. always call org-babel-read on (cdr (assq :tangle params)) even
if it may already have been evaluated which can lead to some unexpected
and potentially nasty results.

I don't think we can consolidate evaluating parameters
into one place in the general case because there are
order dependencies where a setting in one param header
should mask others (as is the case here). In principle we
could consolidate them, but I think that would add significant
complexity because we would have to push all the logic for
handling whether a given ordering restriction applies inside
that location. e.g. if I have a block set :eval (if ev "yes" "no")
it would be bad form to evaluate the parameters before determining
whether the :eval closure evaluates to "yes" or "no". Should that
go inside org-process-params, or should it be handled locally
by e.g. org-babel-tangle and org-babel-execute-src-block separately?

Thoughts?

Re: [PATCH] ob-tangle.el: restore :tangle closure nil behavior

[Ihor Radchenko]

Tom Gillespie <tgbugs@gmail.com> writes:

>> What do you mean by "restore"? Were it evaluated in the past?
>> May you please provide a reproducer?
>
> Hrm. I think I may have mixed two commit lines. It is the case that
> :tangle closures used to work, but you are right, the historical behavior
> when tangling closures meant that all parameters were evaluated (tested
> with the block below in 27 and 28).
>
> #+begin_src elisp :var value=(error "oops") :tangle (or "no")
> value
> #+end_src

This example clearly shows that evaluating everything is a bad idea.
:var has no effect during tangling anyway.

> My use case is that I have blocks that I want to tangle that set :var
> from e.g. the library of babel, which isn't always loaded, but which also
> is not required if :tangle is no.

My confusion about you patch comes from the fact that

#+begin_src emacs-lisp :tangle (if (= 1 1) "yes")
2
#+end_src

works just fine on main.

I admit that I don't fully understand your use case.

>> > -(defun org-babel-tangle--unbracketed-link (params)
>> > +(defun org-babel-tangle--unbracketed-link (params &optional info-was-evaled)
>>
>> This is not acceptable. Taking care about evaluating INFO should be done
>> in a single place instead of adding checks across the babel code. If we
>> go the proposed way, I expect a number of bugs appearing when somebody
>> forgets to change the eval check in some place.
>
> I don't like the solution either. I see two potential alternatives.
> 1. change the structure of the info list to indicate whether it has
> already been evaluated
> 2. always call org-babel-read on (cdr (assq :tangle params)) even
> if it may already have been evaluated which can lead to some unexpected
> and potentially nasty results.

I think that instead of repeating (cdr (assq :tangle params)) we need a
common API that will abstract away evaluation and modify PARAMS by side
effect if necessary.

Something like (org-babel-get-heading-arg :tangle info/params)

> I don't think we can consolidate evaluating parameters
> into one place in the general case because there are
> order dependencies where a setting in one param header
> should mask others (as is the case here).

May you please elaborate?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] ob-tangle.el: restore :tangle closure nil behavior

[Tom Gillespie]

> My confusion about you patch comes from the fact that
>
> #+begin_src emacs-lisp :tangle (if (= 1 1) "yes")
> 2
> #+end_src
>
> works just fine on main.

It appears to work fine on main, but that is because
what is actually happening behind the scenes is that in the test
(unless (or (string= src-tfile "no") ...) ...) the actual comparison is
(string= "(if (= 1 1) \"yes\")" "no") which appears to work, but is
not comparing the result of the closure, only its string value.

> I admit that I don't fully understand your use case.

I want to use a closure to conditionally control whether a block will tangle.
If I hardcode :tangle no, then :var x=(error "oops") will not evaluate. The
(error "oops") is a placeholder for a number of things that will result in
an error if the condition for :tangle (when condition "file-name") is not
satisfied.

> Something like (org-babel-get-heading-arg :tangle info/params)

I need to go to bed, because I definitely started on an implementation
of that I forgot about it as a potential solution. Yes, this seems like
a better and clearer way to go about it.

> May you please elaborate?

Disregard, your suggestion clarified what you meant, and in
that case, yes we can consolidate.

Re: [PATCH] ob-tangle.el: restore :tangle closure nil behavior

[Ihor Radchenko]

Tom Gillespie <tgbugs@gmail.com> writes:

>> My confusion about you patch comes from the fact that
>>
>> #+begin_src emacs-lisp :tangle (if (= 1 1) "yes")
>> 2
>> #+end_src
>>
>> works just fine on main.
>
> It appears to work fine on main, but that is because
> what is actually happening behind the scenes is that in the test
> (unless (or (string= src-tfile "no") ...) ...) the actual comparison is
> (string= "(if (= 1 1) \"yes\")" "no") which appears to work, but is
> not comparing the result of the closure, only its string value.

What I see when tangling a file with the above code block is

Debugger entered--returning value: ("emacs-lisp" "2" ((:colname-names)
         (:rowname-names) (:result-params "replace")
         (:result-type . value) (:results . "replace")
         (:exports . "code")
         (:tangle . "yes") <---------- evaluated
         (:lexical . "no")
         (:eval . "never-export") (:comments . "link")
         (:hlines . "no") (:noweb . "yes") (:cache . "no")
         (:session . "none")) "" nil 2 "(ref:%s)")
  (org-babel-get-src-block-info)
* (#<subr org-babel-tangle-single-block> 1)
* (apply #<subr org-babel-tangle-single-block> 1)
* (org-babel-tangle-single-block 1)
  (org-babel-tangle-collect-blocks nil nil)
  (org-babel-tangle nil)
  (funcall-interactively org-babel-tangle nil)

>> I admit that I don't fully understand your use case.
>
> I want to use a closure to conditionally control whether a block will tangle.
> If I hardcode :tangle no, then :var x=(error "oops") will not evaluate. The
> (error "oops") is a placeholder for a number of things that will result in
> an error if the condition for :tangle (when condition "file-name") is not
> satisfied.

Do you mean something like

#+PROPERTY: headline-args :var x=1

#+begin_src elisp :tangle (if (= x 1) "yes" "no")
...
#+end_src
?

>> Something like (org-babel-get-heading-arg :tangle info/params)
>
> I need to go to bed, because I definitely started on an implementation
> of that I forgot about it as a potential solution. Yes, this seems like
> a better and clearer way to go about it.

Let me know if you need any help.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>