Refactor org-babel-shell-initialize? (was Re: ob-shell intentions and paperwork (was Bash results broken?))

[Matt <matt@excalamus.com>]

 ---- On Sat, 31 Dec 2022 07:56:10 -0500  Ihor Radchenko  wrote --- 
 > As for being a macro, there will be not much gain - the convention is
 > mostly designed for things like `cl-defun' aimed to be used in the code.
 > `org-babel-shell-initialize' is only used by `org-babel-shell-names'.

I'm not sure what you mean by "to be used in code".  Do you mean called within the global scope?

 > I do not have objections if it were a macro though. (But I do not see
 > how it would help debugging).
 >
 > > Because `org-babel-shell-initialize' is a function factory, you can't easily examine or modify their definitions.  `C-h f org-babel-execute:sh' jumps to the top of lisp/ob-shell.el.  Changing the definition requires reevaluating the definition for all the execute functions (or first changing `org-babel-shell-names').
 > 
 > This is indeed a downside. Any better ideas?
 > ob-core dictates that we must have org-babel-execute:lang functions to
 > make things work.

My apologies, I feel there are too many separate issues I've brought up.  Since I've already brought them up, let me try to be more clear about them.  

I observe four issues with the current form of `org-babel-shell-initialize':

1. The source is not explicit for a given `org-babel-execute:some-shell', making it difficult to debug
2. Source navigation gets confused when looking up help for a generated function.  For example, `C-h f org-babel-execute:sh' goes to the top of ob-shell.el
3. It does not adhere to the coding convention
4. Except for using the customize interface, changing `org-babel-shell-names' requires reevaluating the function generator, currently `org-babel-shell-initialize'

Here's my perspective on each point.

1. The source is not explicit for a given `org-babel-execute:some-shell', making it difficult to debug

The benefit of using a macro is clarity of the defined functions.  Here's the core `org-babel-shell-initialize' functionality as a macro.  Note that it doesn't loop through `org-babel-shell-names'.

(defmacro define-babel-shell-1 (shell-name)
  (declare (indent nil) (debug t))
  (let ((function-name (intern (concat "my-org-babel-execute:" shell-name))))
    `(progn
       (defun ,function-name (body params)
         ,(format "Execute a block of %s commands with Babel." shell-name)
         (let ((shell-file-name ,shell-name)
               (org-babel-prompt-command
                (or (alist-get ,shell-name org-babel-shell-set-prompt-commands)
                    (alist-get t org-babel-shell-set-prompt-commands))))
           (org-babel-execute:shell body params)))
       (defalias
         ',(intern (concat "org-babel-variable-assignments:" shell-name))
         'org-babel-variable-assignments:shell
         ,(format "Return list of %s statements assigning to the block's variables." shell-name))
       (defvar ,(intern (concat "org-babel-default-header-args:" shell-name)) nil))))

You can expand to see the definitions:

(pp-macroexpand-expression '(define-babel-shell-1 "csh"))

Is there a way to see the definition of`org-babel-execute:csh' using the current `org-babel-shell-initialize', that is, when generated by a function?

Looking at the expansion, I see what appears to be an error:

(alist-get "csh" org-babel-shell-set-prompt-commands)

`org-babel-shell-set-prompt-commands' is an alist keyed by string shell names and whose values are shell commands to set the prompt.  `alist-get' uses `eq' by default which always returns nil for string comparisons.  That is, (alist-get "csh" org-babel-shell-set-prompt-commands) returns nil, not because the corresponding alist value is nil.  Rather, because the (eq "csh" "csh") comparison evaluates as nil.  The TESTFN probably should be `equal' or `string=':

(alist-get "csh" org-babel-shell-set-prompt-commands nil nil 'equal)

Then, it will return the prompt associated with "csh".

All this is visible from the function version of `org-babel-shell-initialize', of course.  However, it requires mentally tracking that ,name resolves to a string.  Using a macro along with `pp-macroexpand-expression' makes the defined function explicit.  The forms generated by the macro expansion are readily available to eval whereas the function version makes the definitions inaccessible (AFAICT).  

2. Source navigation gets confused when looking up help for a generated function.  For example, `C-h f org-babel-execute:sh' goes to the top of ob-shell.el

Generating the execute functions with a macro also has this problem.  I'm not sure there's a way around it other than defining each function with `defun'.  Doing that would be a bad idea because of the massive code duplication/maintenance it would create.

3. It does not adhere to the coding convention.

I'll requote the convention here for convenience.

> (elisp) Coding Conventions says,
>
> "• Constructs that define a function or variable should be macros, not
> functions, and their names should start with ‘define-’. The macro
> should receive the name to be defined as the first argument. That
> will help various tools find the definition automatically. Avoid
> constructing the names in the macro itself, since that would
> confuse these tools."

It's not clear to me why the convention exists, beyond consistency (a valid reason).  I suspected it was to make the code clearer (points 1) and to "help various tools find the definition automatically" (point 2).  

I'm biased by my experience into thinking a macro actually addresses point 1.  I could be wrong about it, though. It could just have been luck and personal preference, and I may be overlooking some hidden complexity, a common problem with macros.  Is there anything you see that I might be overlooking?

AFAICT, a macro doesn't help with finding the definition of the generated function.  Any idea what tools it's talking about?

Also, the way I defined `define-babel-shell-1' doesn't fit the convention.  Something like this would:

(defmacro define-babel-execute-shell-2 (name)
  "Define functions and variables needed by Org Babel to execute a shell.

NAME is a symbol of the form `org-babel-execute:my-shell'."
  (declare (indent nil) (debug t))
  (let ((shell-program (cadr (split-string (symbol-name name) ":"))))
    `(progn
       (defun ,name (body params)
         ,(format "Execute a block of %s commands with Babel." shell-program)
         (let ((shell-file-name ,shell-program)
               (org-babel-prompt-command
                (or (alist-get ,shell-program org-babel-shell-set-prompt-commands)
                    (alist-get t org-babel-shell-set-prompt-commands))))
           (org-babel-execute:shell body params)))
       (defalias
         ',(intern (concat "org-babel-variable-assignments:" shell-program))
         'org-babel-variable-assignments:shell
         ,(format "Return list of %s statements assigning to the block's variables." shell-program))
       (defvar ,(intern (concat "org-babel-default-header-args:" shell-program)) nil))))

AFAICT, adhering strictly to the convention would make things needlessly complicated.  The execute function's symbols would need to be interned beforehand, creating an extra step between the `org-babel-shell-names' and the execute functions, only for them to be converted and parsed out in the macro:

(intern "org-babel-execute:test")
(symbolp 'org-babel-execute:test)
(pp-macroexpand-expression '(define-babel-execute-shell-2 org-babel-execute:test))

4. Except for using the customize interface, changing `org-babel-shell-names' requires reevaluating the function generator (`org-babel-shell-initialize' or some variant of `define-babel-shell-1' )

A macro would not solve the need to re-evaluate the function generation when a change is made to `org-babel-shell-names'.  Either way, we need to loop/map over the list of shells.

I'm curious your thoughts about removing the `:set' function from `org-babel-shell-names' and using `add-variable-watcher' instead.  In my explorations, the watcher gets called when using customize, as well as when a new shell is added to `org-babel-shell-names' using `add-to-list'.