bug#34757: Invalid bytecode from byte compiler

[Pip Cet]

This bug still appears to be present. Maybe it's time to apply
Stefan's patch and see whether anything breaks?

On Sat, Mar 16, 2019 at 4:51 PM Pip Cet <pipcet@gmail.com> wrote:
>
> On Fri, Mar 15, 2019 at 8:30 PM Stefan Monnier <monnier@iro.umontreal.ca> 
> wrote:
> >
> > > 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.
>
> Thanks for explaining!
>
> > 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.
>
> I agree completely, for what it's worth.
>
> > 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).
>
> I can speak only for myself, but I think the byte-compiler "magically"
> deciding to turn (rare) top-level non-defuns into plain code rather
> than byte code is confusing. It's much better with your patches.
>
> > > It seems the special case is necessary to avoid compilation errors,
> >
> > I haven't found it to be really necessary, no.
>
> Well, you fixed it with the second patch.
>
> > > 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).
>
> No argument here. The case branch affects what happens when
> (byte-compile 3) is called, but isn't necessary for the result to be
> correct, and the comment can be misread to imply it's irrelevant in
> this case.
>
> > Let's not forget that what `byte-compile` tries to do is to preserve the
> > invariant that
> >
> >     (eval EXP) ≃ (eval (byte-compile EXP))
>
> I think byte-compile does different things for different arguments:
> the behavior for symbols and other expressions is simply different.
>
> > 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.
>
> I don't think that was a bug, but it was an unfortunate wrinkle in the
> (undocumented) API of byte-compile-top-level.
>
> > 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 think it's more than handy: it's how I'd read the current
> documentation, and how I'd expect a function called byte-compile to
> behave.
>
> > > 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
>
> The source code is invalid, but the LAP code is valid-looking, and I
> can't conclude it cannot be generated by valid source code being
> passed to `byte-compile' somehow.
>
> > 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
>
> I think this would be clearer if it read "byte-compile-top-level
> returns an *expression*..."