Hi Ihor, Thanks once again for the detailed feedback. It all makes sense.
> > 2. *org-babel-expand-src-block* looks like an interactive command with > > unknown user intent. Do we need some mechanism to determine user > > intent? Or is it ok to assume =:eval=? > > Yes, assume :eval (arbitrarily - the situation si ambiguous). Simply to > follow the existing behavior. Done > > 3. *org-babel-lob-ingest* I'm not familiar with lob > > functionality. Are these blocks always ultimately intended for > > evaluation? Or could they also be used for export? > > They can indeed be used for export. > org-babel-lob-get-info uses the expanded body and is called by > org-babel-exp-process-buffer. > It looks like a bug. > We should probably avoid expanding noweb references in > org-babel-lob-ingest. The return value of org-babel-log-get-info is only > passed to org-babel-execute-src-block or to > org-babel-exp-do-export. Both these functions are handling noweb > expansion anyway. Makes sense. I've removed the expansion during the ingestion. > > > * lisp/ob-core.el (org-babel-expand-noweb-references): Add required > > CONTEXT parameter specifying the expansion context (:tangle, :export, > > or :eval). Use the provided context when recursively expanding nested > > noweb references. Update documentation to explain the context > > parameter and its role in recursive expansion. > > This will create a breaking change in a user-facing API function. > I'd prefer to avoid such breaking changes. > I suggest to add CONTEXT as optional parameter, after INFO and PARENT-BUFFER > By default, if CONTEXT Is not passed, assume :eval. > This will avoid breaking third-party code that is using > org-babel-expand-noweb-references. I've made the context parameter optional and moved it to the last position. > > > * lisp/ob-lob.el (org-babel-lob-ingest): Pass :eval context when > > ingesting named blocks into the Library of Babel. > > We should probably drop calling noweb expansion altogether here. > But we need to make sure that > https://lists.gnu.org/archive/html/emacs-orgmode/2017-09/msg00361.html > does not re-appear. Is there any additional due diligence I need to do regarding the issue reference here? In other news, I've submitted the assignment form to fsf and my understanding is that the process is almost complete. Thanks! Dominic
From c1c3328d7da31b0b19d41c3b14357e219c9600fe Mon Sep 17 00:00:00 2001 From: Dominic Meiser <[email protected]> Date: Tue, 30 Sep 2025 07:33:10 -0600 Subject: [PATCH] ob-core: Fix noweb tangle recursive expansion * lisp/ob-core.el (org-babel-expand-noweb-references): Add optional CONTEXT parameter (third parameter, after INFO and PARENT-BUFFER) specifying the expansion context (:tangle, :export, or :eval). Default to :eval when not provided for backward compatibility. Use the provided context when recursively expanding nested noweb references. Update documentation to explain the context parameter and its role in recursive expansion. (org-babel--expand-body): Add documentation explaining why :eval context is used for code evaluation, including during export and tangling when results need to be generated. (org-babel-check-confirm-evaluate): Pass :eval as CONTEXT. (org-babel-expand-src-block): Pass :eval as CONTEXT. * lisp/ob-tangle.el (org-babel-tangle-single-block): Pass :tangle as CONTEXT to org-babel-expand-noweb-references. * lisp/ob-exp.el (org-babel-exp-process-buffer): Pass :export as CONTEXT when expanding inline source blocks during export. (org-babel-exp-code): Pass :export as CONTEXT when expanding code blocks for export. (org-babel-exp-results): Pass :eval as CONTEXT when evaluating blocks during export to generate results. * lisp/ob-lob.el (org-babel-lob-ingest): Remove premature noweb expansion. Both org-babel-execute-src-block and org-babel-exp-do-export handle noweb expansion with appropriate context when the block is actually used, making expansion during ingest redundant and preventing context-specific behavior. * testing/lisp/test-ob-tangle.el (ob-tangle/expand-headers-as-noweb-references): Pass :tangle as CONTEXT to match tangle expansion behavior. (ob-tangle/noweb-tangle-recursive-expansion): New test verifying that :noweb tangle recursively expands nested noweb references during tangling. (ob-tangle/noweb-tangle-vs-export-contexts): New test verifying that noweb expansion respects different contexts. * testing/lisp/test-ob.el (test-ob/noweb-expansion): Pass :eval as CONTEXT in all test invocations to explicitly specify evaluation context. Previously, noweb expansion during tangling would fail to recursively expand nested references in blocks marked with :noweb tangle. The expansion context was hardcoded to :eval during recursive calls, meaning nested blocks would only expand if their :noweb setting permitted expansion in the :eval context. For example, when tangling a block containing <<ref>> where the referenced block has :noweb tangle and itself contains noweb references, those nested references would not expand because the recursive call used :eval context instead of :tangle. The context parameter determines which :noweb header argument values trigger expansion: - :tangle context: "tangle", "yes", "no-export", "strip-export", etc. - :export context: "yes", "strip-tangle" - :eval context: "eval", "yes", "no-export", "strip-export", etc. The CONTEXT parameter is optional and placed after INFO and PARENT-BUFFER to maintain backward compatibility with third-party code that may call org-babel-expand-noweb-references directly. Reported-by: Dominic Meiser <[email protected]> Link: https://list.orgmode.org/[email protected]/ --- lisp/ob-core.el | 42 +++++++++++++++---- lisp/ob-exp.el | 6 +-- lisp/ob-lob.el | 4 -- lisp/ob-tangle.el | 3 +- testing/lisp/test-ob-tangle.el | 77 +++++++++++++++++++++++++++++++++- testing/lisp/test-ob.el | 16 +++---- 6 files changed, 122 insertions(+), 26 deletions(-) diff --git a/lisp/ob-core.el b/lisp/ob-core.el index 1402827e4..0c0d88898 100644 --- a/lisp/ob-core.el +++ b/lisp/ob-core.el @@ -765,11 +765,19 @@ Otherwise, return a list with the following pattern: (defun org-babel--expand-body (info) "Expand noweb references in src block and remove any coderefs. The src block is defined by its INFO, as returned by -`org-babel-get-src-block-info'." +`org-babel-get-src-block-info'. + +This function uses the :eval context for noweb expansion because it is +called when code needs to be evaluated, either by `org-babel-execute-src-block' +or `org-babel-confirm-evaluate'. The :eval context is appropriate even during +export or tangling when the code block needs to be evaluated to generate results. +It is distinct from the :export and :tangle contexts which are used when +generating the source code body for display in exported documents or tangled +files." (let ((coderef (nth 6 info)) (expand (if (org-babel-noweb-p (nth 2 info) :eval) - (org-babel-expand-noweb-references info) + (org-babel-expand-noweb-references info nil :eval) (nth 1 info)))) (if (not coderef) expand (replace-regexp-in-string @@ -1003,7 +1011,7 @@ arguments and pop open the results in a preview buffer." (symbol-name (car el2))))))) (body (setf (nth 1 info) (if (org-babel-noweb-p params :eval) - (org-babel-expand-noweb-references info) (nth 1 info)))) + (org-babel-expand-noweb-references info nil :eval) (nth 1 info)))) (expand-cmd (intern (concat "org-babel-expand-body:" lang))) (assignments-cmd (intern (concat "org-babel-variable-assignments:" lang))) @@ -1143,7 +1151,7 @@ session." (user-error "No src code block at point") (setf (nth 1 info) (if (org-babel-noweb-p params :eval) - (org-babel-expand-noweb-references info) + (org-babel-expand-noweb-references info nil :eval) (nth 1 info))))) (session (cdr (assq :session params))) (dir (cdr (assq :dir params))) @@ -1513,7 +1521,7 @@ CONTEXT specifies the context of evaluation. It can be `:eval', (lang (nth 0 info)) (params (nth 2 info)) (body (if (org-babel-noweb-p params context) - (org-babel-expand-noweb-references info) + (org-babel-expand-noweb-references info nil context) (nth 1 info))) (expand-cmd (intern (concat "org-babel-expand-body:" lang))) (assignments-cmd (intern (concat "org-babel-variable-assignments:" @@ -3137,7 +3145,7 @@ CONTEXT may be one of :tangle, :export or :eval." (defvar org-babel-expand-noweb-references--cache-buffer nil "Cons (BUFFER . MODIFIED-TICK) for cached noweb references. See `org-babel-expand-noweb-references--cache'.") -(defun org-babel-expand-noweb-references (&optional info parent-buffer) +(defun org-babel-expand-noweb-references (&optional info parent-buffer context) "Expand Noweb references in the body of the current source code block. When optional argument INFO is non-nil, use the block defined by INFO @@ -3146,6 +3154,20 @@ instead. The block is assumed to be located in PARENT-BUFFER or current buffer \(when PARENT-BUFFER is nil). +CONTEXT specifies the context of expansion and can be one of :tangle, +:export, or :eval. When CONTEXT is nil, it defaults to :eval. + +The context determines which noweb header arguments are honored when +recursively expanding nested references: +- :tangle context: expands blocks with :noweb tangle, :noweb yes, etc. +- :export context: expands blocks with :noweb export, :noweb yes, etc. +- :eval context: expands blocks with :noweb eval, :noweb yes, etc. + +This is important for recursive expansion: when a block with :noweb tangle +references another block that also contains noweb references, those nested +references should only be expanded if the referenced block's :noweb setting +permits expansion in the tangle context. + For example the following reference would be replaced with the body of the source-code block named `example-block'. @@ -3173,7 +3195,8 @@ defined by `org-babel-lob'. For example would set the value of argument \"a\" equal to \"9\". Note that these arguments are not evaluated in the current source-code block but are passed literally to the \"example-block\"." - (let* ((parent-buffer (or parent-buffer (current-buffer))) + (let* ((context (or context :eval)) + (parent-buffer (or parent-buffer (current-buffer))) (info (or info (org-babel-get-src-block-info 'no-eval))) (lang (nth 0 info)) (body (nth 1 info)) @@ -3207,8 +3230,9 @@ block but are passed literally to the \"example-block\"." (expand-body (i) ;; Expand body of code represented by block info I. - `(let ((b (if (org-babel-noweb-p (nth 2 ,i) :eval) - (org-babel-expand-noweb-references ,i) + `(let ((b (if (org-babel-noweb-p (nth 2 ,i) context) + (org-babel-expand-noweb-references + ,i parent-buffer context) (nth 1 ,i)))) (if (not comment) b (let ((cs (org-babel-tangle-comment-links ,i))) diff --git a/lisp/ob-exp.el b/lisp/ob-exp.el index ab99541ab..f2c85ca43 100644 --- a/lisp/ob-exp.el +++ b/lisp/ob-exp.el @@ -215,7 +215,7 @@ this template." (string= "yes" (cdr (assq :noweb params)))) (org-babel-expand-noweb-references - info org-babel-exp-reference-buffer) + info org-babel-exp-reference-buffer :export) (nth 1 info))) (goto-char begin) (let ((replacement @@ -423,7 +423,7 @@ replaced with its value." (org-babel-noweb-wrap) "" (nth 1 info)) (if (org-babel-noweb-p (nth 2 info) :export) (org-babel-expand-noweb-references - info org-babel-exp-reference-buffer) + info org-babel-exp-reference-buffer :export) (nth 1 info)))) (org-fill-template (if (eq type 'inline) @@ -462,7 +462,7 @@ inhibit insertion of results into the buffer." (let ((lang (nth 0 info)) (body (if (org-babel-noweb-p (nth 2 info) :eval) (org-babel-expand-noweb-references - info org-babel-exp-reference-buffer) + info org-babel-exp-reference-buffer :eval) (nth 1 info))) (info (copy-sequence info)) (org-babel-current-src-block-location (point-marker))) diff --git a/lisp/ob-lob.el b/lisp/ob-lob.el index 472f9eda1..b0b2cb309 100644 --- a/lisp/ob-lob.el +++ b/lisp/ob-lob.el @@ -59,10 +59,6 @@ should not be inherited from a source block.") (let* ((info (org-babel-get-src-block-info 'no-eval)) (source-name (nth 4 info))) (when source-name - (setf (nth 1 info) - (if (org-babel-noweb-p (nth 2 info) :eval) - (org-babel-expand-noweb-references info) - (nth 1 info))) (let ((source (intern source-name))) (setq org-babel-library-of-babel (cons (cons source info) diff --git a/lisp/ob-tangle.el b/lisp/ob-tangle.el index 4c224743b..24c8f876b 100644 --- a/lisp/ob-tangle.el +++ b/lisp/ob-tangle.el @@ -581,7 +581,8 @@ non-nil, return the full association list to be used by (let ((body (if (org-babel-noweb-p params :tangle) (if (string= "strip-tangle" (cdr (assq :noweb (nth 2 info)))) (replace-regexp-in-string (org-babel-noweb-wrap) "" (nth 1 info)) - (org-babel-expand-noweb-references info)) + (org-babel-expand-noweb-references + info nil :tangle)) (nth 1 info)))) (with-temp-buffer (insert diff --git a/testing/lisp/test-ob-tangle.el b/testing/lisp/test-ob-tangle.el index cd6876370..e7b970c70 100644 --- a/testing/lisp/test-ob-tangle.el +++ b/testing/lisp/test-ob-tangle.el @@ -78,7 +78,7 @@ "Test that references to headers are expanded during noweb expansion." (org-test-at-id "2409e8ba-7b5f-4678-8888-e48aa02d8cb4" (org-babel-next-src-block 2) - (let ((expanded (org-babel-expand-noweb-references))) + (let ((expanded (org-babel-expand-noweb-references nil nil :tangle))) (should (string-match (regexp-quote "simple") expanded)) (should (string-match (regexp-quote "length 14") expanded))))) @@ -764,6 +764,81 @@ This is to ensure that we properly resolve the buffer name." ;; Clean up the tangled file with the filename from org-test-with-temp-text-in-file (delete-file tangle-filename))))) +(ert-deftest ob-tangle/noweb-tangle-recursive-expansion () + "Test that :noweb tangle recursively expands nested noweb references." + (let ((file (make-temp-file "org-tangle-"))) + (unwind-protect + (progn + (org-test-with-temp-text-in-file + (format " +#+begin_src c :tangle %s :noweb tangle +// some code +<<noweb-ref1>> +<<noweb-ref2>> +#+end_src + +#+begin_src c :noweb-ref noweb-ref1 +// code from source block A +#+end_src + +#+begin_src c :noweb-ref noweb-ref2 :noweb tangle +// code from source block B +<<noweb-ref3>> +#+end_src + +#+begin_src c :noweb-ref noweb-ref3 +// code from source block C +#+end_src +" file) + (let ((org-babel-noweb-error-all-langs nil) + (org-babel-noweb-error-langs nil)) + (org-babel-tangle))) + (let ((tangled-content (with-temp-buffer + (insert-file-contents file) + (buffer-string)))) + ;; The tangled output should contain the content from block C + ;; (not the unexpanded <<noweb-ref3>> reference) + (should (string-match-p "// code from source block C" tangled-content)) + ;; The tangled output should NOT contain the unexpanded reference + (should-not (string-match-p "<<noweb-ref3>>" tangled-content)))) + (delete-file file)))) + +(ert-deftest ob-tangle/noweb-tangle-vs-export-contexts () + "Test that noweb expansion respects different contexts during tangle vs export." + (let ((tangle-file (make-temp-file "org-tangle-"))) + (unwind-protect + (progn + (org-test-with-temp-text-in-file + (format " +#+begin_src c :tangle %s :noweb yes +// tangled code +<<tangle-only>> +<<no-export>> +#+end_src + +#+begin_src c :noweb-ref tangle-only :noweb tangle +// visible during tangle +#+end_src + +#+begin_src c :noweb-ref no-export :noweb no-export +// visible during eval but not export +#+end_src +" tangle-file) + ;; Test tangling + (let ((org-babel-noweb-error-all-langs nil) + (org-babel-noweb-error-langs nil)) + (org-babel-tangle))) + + ;; Check tangled content + (let ((tangled-content (with-temp-buffer + (insert-file-contents tangle-file) + (buffer-string)))) + ;; Should have tangle-only content + (should (string-match-p "// visible during tangle" tangled-content)) + ;; Should have no-export content since :noweb no-export allows tangle context + (should (string-match-p "// visible during eval but not export" tangled-content)))) + (delete-file tangle-file)))) + (provide 'test-ob-tangle) ;;; test-ob-tangle.el ends here diff --git a/testing/lisp/test-ob.el b/testing/lisp/test-ob.el index 870296acf..9fa8fa2c0 100644 --- a/testing/lisp/test-ob.el +++ b/testing/lisp/test-ob.el @@ -829,7 +829,7 @@ x #+begin_src sh bar #+end_src" - (org-babel-expand-noweb-references)))) + (org-babel-expand-noweb-references nil nil :eval)))) ;; Handle :noweb-sep. (should (string= @@ -845,7 +845,7 @@ x #+begin_src sh :noweb-ref foo :noweb-sep \"\" baz #+end_src" - (org-babel-expand-noweb-references)))) + (org-babel-expand-noweb-references nil nil :eval)))) ;; :noweb-ref is extracted from definition, not point of call. (should (string= @@ -869,7 +869,7 @@ x #+begin_src sh :noweb-sep \"\" (+ 1 1) #+end_src" - (org-babel-expand-noweb-references)))) + (org-babel-expand-noweb-references nil nil :eval)))) ;; Handle recursive expansion. (should (equal "baz" @@ -887,7 +887,7 @@ x #+begin_src emacs-lisp baz #+end_src" - (org-babel-expand-noweb-references)))) + (org-babel-expand-noweb-references nil nil :eval)))) ;; During recursive expansion, obey to `:noweb' property. (should (equal "<<bar>>" @@ -905,7 +905,7 @@ x #+begin_src emacs-lisp baz #+end_src" - (org-babel-expand-noweb-references)))) + (org-babel-expand-noweb-references nil nil :eval)))) ;; Respect COMMENT headlines (should (equal "C" @@ -929,7 +929,7 @@ x #+begin_src emacs-lisp :noweb-ref foo C #+end_src" - (org-babel-expand-noweb-references)))) + (org-babel-expand-noweb-references nil nil :eval)))) ;; Preserve case when replacing Noweb reference. (should (equal "(ignore)" @@ -941,7 +941,7 @@ x #+begin_src emacs-lisp :noweb yes<point> <<AA>> #+end_src" - (org-babel-expand-noweb-references)))) + (org-babel-expand-noweb-references nil nil :eval)))) ;; Test :noweb-ref expansion. (should (equal "(message \"!! %s\" \"Running confpkg-test-setup\") @@ -986,7 +986,7 @@ x " (goto-char (point-min)) (search-forward "begin_src") - (org-babel-expand-noweb-references))))) + (org-babel-expand-noweb-references nil nil :eval))))) (ert-deftest test-ob/splitting-variable-lists-in-references () (org-test-with-temp-text "" -- 2.51.0
