bug#34757: Invalid bytecode from byte compiler

[Stefan Monnier <monnier@IRO.UMontreal.CA>]

> Just to be sure I understand correctly, you would like to remove the
> decompilation of trivial function calls entirely?

Yes, tho the main motivation was to try and figure out what the
decompilation is useful for.

This only affects "open code" (i.e. not the content of functions, which
are already never decompiled), so the impact should be minor and it
removes a rather complicated and brittle chunk of code whose purpose is
not clear.  It was originally introduced when we didn't have
byte-compiled function objects, so its main purpose was one of
performance, to avoid pessimizing the code by replacing trivial function
calls with more costly (byte-code "...") expressions but nowadays such
(byte-code "...") expressions only occur basically at the top-level of
.elc files where such pessimization would be unnoticeable in terms
of performance.

It does impact the readability of .elc files, OTOH, so I'm not
completely happy with the result when considering the few cases where
I was happy to be able to make sense of a .elc file to better understand
the source of a problem (after all, that's why I wrote the
elisp-byte-code-mode).

> It seems the special case is necessary to avoid compilation errors,

I haven't found it to be really necessary, no.

> and that it's used for (byte-compile 3), so I think the comment could
> be improved a little.

(byte-compile 3) seems to work correctly here even without the
special case.  It returns (byte-code "\300\207" [3] 1) which is indeed
a correct expression that evaluates to 3 (just like the argument to
`byte-compile` was an expression whose evaluation returns 3).

Let's not forget that what `byte-compile` tries to do is to preserve the
invariant that

    (eval EXP) ≃ (eval (byte-compile EXP))

This said, if you remove the special case, you will bump into
a corner-case bug in `byte-compile` which happens because that function
incorrectly considers that `byte-compile-top-level` returns a value
whereas in reality it returns an expression (just like `byte-compile`):
the decompilation happens to turn expressions that return constant
values (like byte-compiled functions) into their value (as an
optimization which relies on the fact that those objects are
self-evaluating), so if you remove it, you then bump into this bug of
byte-compile.  The patch below would fix this bug.

But indeed the decompilation of constants is handy since that's what
people expect from `byte-compile`.  When I (byte-compile '(lambda (x)
(foo))) I expect to receive a byte-compiled function, and not
a byte-code expression which when evaluated will return that
byte-compiled function.

> I'm not sure this case can actually happen without doing something
> silly, but (byte-compile '(internal-get-closed-var 0)) throws an error
> with Stefan's patch, because the byte code is (byte-constant . 0)
> (byte-return).

This source code is arguably invalid, so it's not a real problem, but
indeed we should burp in that case.  I can't remember enough of how
internal-get-closed-var is handled to know where'd the bug be, offhand.


        Stefan


diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index f46cab2c17..ae17553d0c 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -2674,7 +2674,11 @@ byte-compile
          (setq fun (byte-compile-top-level fun nil 'eval)))
         (if macro (push 'macro fun))
         (if (symbolp form)
-            (fset form fun)
+            ;; byte-compile returns an *expression* equivalent to the
+            ;; `fun' expression, so we need to evaluate it, tho normally
+            ;; this is not needed because the expression is just a constant
+            ;; byte-code object, which is self-evaluating.
+            (fset form (eval fun t))
           fun)))))))
 
 (defun byte-compile-sexp (sexp)