bug#70368: [PATCH] Use a dedicated type to represent interpreted-function values

[Eli Zaretskii]

> Date: Sat, 13 Apr 2024 15:56:34 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Change `function` so that when evaluating #'(lambda ...)
> we return an object of type `interpreted-function` rather than
> a list starting with one of `lambda` or `closure`.
> The new type reuses the existing PVEC_CLOSURE (nee PVEC_COMPILED)
> tag used for byte-code function and tries to align the corresponding
> elements:
> 
> - the arglist, the docstring, and the interactive-form go in the
>   same slots as for byte-code functions.
> - the body of the function goes in the slot used for the bytecode string.
> - the lexical context goes in the slot used for the constants of
>   bytecoded functions.

I don't think I understand the implications of this on compatibility
of byte-code.  Will the byte-code produced by Emacs 30 after these
changes be compatible or incompatible with previous versions of Emacs?
And what about the compatibility in the other direction?

> The first point above means that `help-function-arglist`,
> `documentation`, and `interactive-form`s don't need to
> distinguish interpreted and bytecode functions any more.

Why is such a distinction needed?

> Main benefits of the change:
> 
> - We can now reliably distinguish a list from a function value.
>   This removes some ambiguity in cases where the data can be made of
>   a list or a function, such as `run-hooks` or completion tables.
> - `cl-defmethod` can dispatch on `interactive-function`.
>   Dispatch on `function` also works now for interpreted functions (but still
>   won't work for functions represented as lists or as symbols, of course).
> - Function values are now self-evaluating.  That was already the case
>   when byte-compiled, but not when interpreted since
>   (eval '(closure ...)) signals a void-function error.
>   That also avoids false-positive warnings about "don't quote your lambdas"
>   when doing things like `(mapcar ',func ...)`.

Are there no downsides, none whatever?  It would sound too good to be
true...

> -enum Lisp_Compiled
> +enum Lisp_Closure
>    {
> -    COMPILED_ARGLIST = 0,
> -    COMPILED_BYTECODE = 1,
> -    COMPILED_CONSTANTS = 2,
> -    COMPILED_STACK_DEPTH = 3,
> -    COMPILED_DOC_STRING = 4,
> -    COMPILED_INTERACTIVE = 5
> +    CLOSURE_ARGLIST = 0,
> +    CLOSURE_CODE = 1,
> +    CLOSURE_CONSTANTS = 2,
> +    CLOSURE_STACK_DEPTH = 3,
> +    CLOSURE_DOC_STRING = 4,
> +    CLOSURE_INTERACTIVE = 5
>    };
>  
>  /* Flag bits in a character.  These also get used in termhooks.h.
> @@ -3307,9 +3307,9 @@ WINDOW_CONFIGURATIONP (Lisp_Object a)
>  }
>  
>  INLINE bool
> -COMPILEDP (Lisp_Object a)
> +CLOSUREP (Lisp_Object a)
>  {
> -  return PSEUDOVECTORP (a, PVEC_COMPILED);
> +  return PSEUDOVECTORP (a, PVEC_CLOSURE);
>  }

Do the above changes require changes to .gdbinit?  E.g., I see that we
use PVEC_COMPILED there.

> --- a/doc/lispref/control.texi
> +++ b/doc/lispref/control.texi
> @@ -2411,7 +2411,7 @@ Handling Errors
>  @group
>  Debugger entered--Lisp error: (error "Oops")
>    signal(error ("Oops"))
> -  (closure (t) (err) (signal 'error (cdr err)))((user-error "Oops"))
> +  #f(lambda (err) [t] (signal 'error (cdr err)))((user-error "Oops"))
>    user-error("Oops")

This new #f syntax is not documented anywhere, AFAICT.  If this is the
new printed representation (and maybe also read syntax?) of functions,
it should be documented, like we do with other printed representations.

> --- a/lisp/emacs-lisp/byte-opt.el
> +++ b/lisp/emacs-lisp/byte-opt.el
> @@ -164,7 +164,7 @@ byte-compile-inline-expand
>         ;; The byte-code will be really inlined in byte-compile-unfold-bcf.
>         (byte-compile--check-arity-bytecode form fn)
>         `(,fn ,@(cdr form)))
> -      ((or `(lambda . ,_) `(closure . ,_))
> +      ((pred interpreted-function-p)

What does this change mean for backward compatibility?

> diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
> index fb3278c08ab..c82f8ba6ae2 100644
> --- a/lisp/emacs-lisp/bytecomp.el
> +++ b/lisp/emacs-lisp/bytecomp.el
> @@ -2900,9 +2900,14 @@ byte-compile-output-as-comment
>  (defun byte-compile--reify-function (fun)
>    "Return an expression which will evaluate to a function value FUN.
>  FUN should be an interpreted closure."
> -  (pcase-let* ((`(closure ,env ,args . ,body) fun)
> -               (`(,preamble . ,body) (macroexp-parse-body body))
> -               (renv ()))
> +  (let* ((args (aref fun 0))
> +         (body (aref fun 1))
> +         (env (aref fun 2))
> +         (docstring (function-documentation fun))
> +         (iform (interactive-form fun))
> +         (preamble `(,@(if docstring (list docstring))
> +                     ,@(if iform (list iform))))
> +         (renv ()))

And this?

> @@ -5571,7 +5575,7 @@ display-call-tree
>                    " <compiled macro>"
>                  " <macro>"))
>               ((eq 'lambda (car f))
> -              "<function>")
> +              "<function-like list>")
                  ^^^^^^^^^^^^^^^^^^^^
Should this be documented somewhere?

> +DEFUN ("closurep", Fclosurep, Sclosurep,
> +       1, 1, 0,
> +       doc: /* Return t if OBJECT is a function object.  */)

If the doc string is correct, then why is the function called
'closurep'?  It's against mnemonic memory.

> +  (Lisp_Object object)
> +{
> +  if (CLOSUREP (object))
> +    return Qt;
> +  return Qnil;
> +}
> +
>  DEFUN ("byte-code-function-p", Fbyte_code_function_p, Sbyte_code_function_p,
>         1, 1, 0,
>         doc: /* Return t if OBJECT is a byte-compiled function object.  */)
>    (Lisp_Object object)
>  {
> -  if (CLOSUREP (object))
> +  if (CLOSUREP (object) && STRINGP (AREF (object, CLOSURE_CODE)))
> +    return Qt;
> +  return Qnil;
> +}
> +
> +DEFUN ("interpreted-function-p", Finterpreted_function_p,
> +       Sinterpreted_function_p, 1, 1, 0,
> +       doc: /* Return t if OBJECT is an interpreted function value.  */)
                                                       ^^^^^^^^^^^^^^
"function value"? what's that?

> +  (Lisp_Object object)
> +{
> +  if (CLOSUREP (object) && CONSP (AREF (object, CLOSURE_CODE)))
>      return Qt;
>    return Qnil;

These new primitives should be documented in the ELisp manual, and
perhaps also mentioned in NEWS.

> +DEFUN ("make-interpreted-closure", Fmake_interpreted_closure,
> +       Smake_interpreted_closure, 3, 5, 0,
> +       doc: /* Make an interpreted closure.

Please try to mention the arguments in the first sentence of doc
string.

And this primitive should also be documented in the ELisp manual.

> +ARGS should be the list of formal arguments.
> +BODY should be a non-empty list of forms.
> +ENV should be a lexical environment, like the second argument of `eval'.
> +IFORM if non-nil should be of the form (interactive ...).  */)
> +  (Lisp_Object args, Lisp_Object body, Lisp_Object env,
> +   Lisp_Object docstring, Lisp_Object iform)
> +{
> +  CHECK_CONS (body);          /* Make sure it's not confused with byte-code! 
> */
> +  if (!NILP (iform))
> +    {
> +      iform = Fcdr (iform);
> +      return CALLN (Fmake_byte_code,
> +                    args, body, env, Qnil, docstring,
> +                    NILP (Fcdr (iform))
> +                    ? Fcar (iform)
> +                    : CALLN (Fvector, XCAR (iform), XCDR (iform)));
> +    }
> +  else if (!NILP (docstring))
> +    return CALLN (Fmake_byte_code, args, body, env, Qnil, docstring);
> +  else
> +    return CALLN (Fmake_byte_code, args, body, env);

I'm probably missing something, but if the doc string says "make an
_interpreted_ closure", why does the implementation call
make-byte-code?  Isn't byte-code a kind-of antithesis of
"interpreted"?

Finally, please quote 'like this' or `like this' in the commit log
messages, not `like this`.  Example:

> In preparation for the use of `PVEC_COMPILED` objects for
> interpreted functions, rename them to use a more neutral name.
> 
> * src/lisp.h (enum pvec_type): Rename `PVEC_COMPILED` to `PVEC_CLOSURE`.
> (enum Lisp_Compiled): Use `CLOSURE_` prefix i.s.o `COMPILED_`.
> Also use `CODE` rather than `BYTECODE`.
> (CLOSUREP): Rename from `COMPILEDP`.
> (enum Lisp_Closure): Rename from `Lisp_Compiled`.

The line that begins with "* src/lisp.h" is too long, btw, it should
be at most 70 columns, preferably 65.  And there are a few others that
are likewise too long.

Thanks.