bug#53526: 29.0.50; macroexp-warn-and-return API change

bug#53526: 29.0.50; macroexp-warn-and-return API change

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Package: Emacs
Version: 29.0.50


The following change in `macroexp.el` on `master` is not backward
compatible with the Emacs-28 API:

-(defun macroexp-warn-and-return (msg form &optional category compile-only)
+(defun macroexp-warn-and-return (arg msg form &optional category compile-only)

I suspect that the `arg` should be added at the end instead.  While I'm
here I also noticed that `byte-compile-form-stack` is a poor name for
a variable declared in `macroexp.el`.  It should either be renamed to
use the `macroexp-` prefix, or moved to `bytecomp.el` (and it probably
should have a double-hyphen, since I think it's not meant to be used by
anyone but us).


        Stefan

bug#53526: 29.0.50; macroexp-warn-and-return API change

[Alan Mackenzie]

Hello, Stefan.

On Tue, Jan 25, 2022 at 11:56:22 -0500, Stefan Monnier wrote:
> Package: Emacs
> Version: 29.0.50


> The following change in `macroexp.el` on `master` is not backward
> compatible with the Emacs-28 API:

> -(defun macroexp-warn-and-return (msg form &optional category compile-only)
> +(defun macroexp-warn-and-return (arg msg form &optional category compile-only)

No, it isn't.  All the uses of the function are in lisp/emacs-lisp, and
I understood the function to be an internal one.

> I suspect that the `arg` should be added at the end instead.

The other functions (like byte-compile-warn-x) which have acquired this
extra argument need to have it at the start, since there are an
indeterminate number of &rest args going into a `format'.  So it seemed
better just to do the same with this function, to preserve a sort of
compatibility.

> While I'm here I also noticed that `byte-compile-form-stack` is a poor
> name for a variable declared in `macroexp.el`.

It's an integral part of bytecomp.el.  It got moved to macroexp.el
because it is used (twice) there, and that file is loaded into bootstrap
emacs before bytecomp.el.

There is precedent for this "mis"naming, namely
byte-compile-bound-variables.

> It should either be renamed to use the `macroexp-` prefix, or moved to
> `bytecomp.el` (and it probably should have a double-hyphen, since I
> think it's not meant to be used by anyone but us).

It started off life with a double hyphen in bytecomp.el.  But when it
started getting used in macroexp.el (during the expansion of a macro) it
lost the extra hyphen and got moved there.  It's no big deal, I think,
just that there's no completely neat way of doing this, so the
compromise actually used is pretty arbitrary.  If the variable were in
bytecomp.el, we'd probably need a boundp call in the two places we use
it in macroexp.el.

Whilst on the topic of macroexp-warn-and-return (and
macroexp--wrap-warn), I have to admit having difficulty understanding
these functions, both how they work and what they're for.

My impression up till a couple of days ago was that they were ways of
coping with the old warning position mechanism, and were intended to
compensate for its deficiencies.

Now, I'm much less sure.  Was I indeed mistaken?  If I was, what then is
the purpose of these functions, which defer warning messages in some
fashion?  If I was right, it would be a good thing to dismantle them,
since they are complicated and difficult, and no longer needed.  As I
said, I don't really understand them.

Thanks!

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#53526: 29.0.50; macroexp-warn-and-return API change

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> -(defun macroexp-warn-and-return (msg form &optional category compile-only)
>> +(defun macroexp-warn-and-return (arg msg form &optional category compile-only)
> No, it isn't.  All the uses of the function are in lisp/emacs-lisp, and
> I understood the function to be an internal one.

No, its name was changed from "macroexp--" to "macroexp-" in Emacs-28,
specifically to make available for third party packages.  It was
announced in etc/NEWS, for example.

While `bindat.el` lives in `lisp/emacs-lisp`, it's an example of
a non-core package that benefits from it.

>> I suspect that the `arg` should be added at the end instead.
> The other functions (like byte-compile-warn-x) which have acquired this
> extra argument need to have it at the start, since there are an
> indeterminate number of &rest args going into a `format'.  So it seemed
> better just to do the same with this function, to preserve a sort of
> compatibility.

While I can see the value of this aesthetic argument, I think breaking
backward compatibility was a published API is a more serious problem.

On the upside, moving it to the end will make it optional, which is good
since in many cases we can use the `form` argument instead (which
`byte-compile-warn-x` doesn't have).

>> While I'm here I also noticed that `byte-compile-form-stack` is a poor
>> name for a variable declared in `macroexp.el`.
>
> It's an integral part of bytecomp.el.  It got moved to macroexp.el
> because it is used (twice) there, and that file is loaded into bootstrap
> emacs before bytecomp.el.
>
> There is precedent for this "mis"naming, namely
> byte-compile-bound-variables.

`byte-compile-bound-variables` is defined in `bytecomp.el`, not in `macroexp.el`.
And indeed, `byte-compile-bound-variables` is only set/modified by the
byte compiler, so it really belongs there.

I can see that just moving the definition to bytecomp.el and using
(defvar byte-compile-form-stack) in macroexp.el won't work because the
`push` requires the var to have a value.

Still, the current setup is really ugly: that var belongs in
`bytecomp.el`.

> It started off life with a double hyphen in bytecomp.el.  But when it
> started getting used in macroexp.el (during the expansion of a macro) it
> lost the extra hyphen and got moved there.

I'd put a double hyphen there simply because it's not something that we
want to expose as an official API.  Just because the bytecompiler's
macroexpansion phase is implemented in a separate file doesn't justify
making the var public.

> It's no big deal, I think, just that there's no completely neat way of
> doing this, so the compromise actually used is pretty arbitrary.
> If the variable were in bytecomp.el, we'd probably need a boundp call
> in the two places we use it in macroexp.el.

It at least deserves a prominent comment explaining why it's there.

> Whilst on the topic of macroexp-warn-and-return (and
> macroexp--wrap-warn), I have to admit having difficulty understanding
> these functions, both how they work and what they're for.
>
> My impression up till a couple of days ago was that they were ways of
> coping with the old warning position mechanism, and were intended to
> compensate for its deficiencies.

The original motivation was indeed to improve the error messages by
including more relevant line information.  This part was made largely
irrelevant with your patch.

But it's still relevant because macros can use it without being tied to
the byte-compiler.  Also a nice side-effect is that the warnings are
emitted (mostly) in the order they appear in the code, whereas otherwise
we'd first have the warnings emitted during macroexpansion, then
warnings emitted during the compilation.

> Now, I'm much less sure.  Was I indeed mistaken?  If I was, what then is
> the purpose of these functions, which defer warning messages in some
> fashion?  If I was right, it would be a good thing to dismantle them,
> since they are complicated and difficult, and no longer needed.  As I
> said, I don't really understand them.

I don't see what's difficult about it: it lets you attach a warning to
a piece of code.


        Stefan

bug#53526: 29.0.50; macroexp-warn-and-return API change

[Alan Mackenzie]

Hello, Stefan.

On Tue, Jan 25, 2022 at 14:10:12 -0500, Stefan Monnier wrote:
> >> -(defun macroexp-warn-and-return (msg form &optional category compile-only)
> >> +(defun macroexp-warn-and-return (arg msg form &optional category compile-only)
> > No, it isn't.  All the uses of the function are in lisp/emacs-lisp, and
> > I understood the function to be an internal one.

> No, its name was changed from "macroexp--" to "macroexp-" in Emacs-28,
> specifically to make available for third party packages.  It was
> announced in etc/NEWS, for example.

Are you aware of it being used anywhere else but lisp/emacs-lisp?

> While `bindat.el` lives in `lisp/emacs-lisp`, it's an example of
> a non-core package that benefits from it.

> >> I suspect that the `arg` should be added at the end instead.
> > The other functions (like byte-compile-warn-x) which have acquired this
> > extra argument need to have it at the start, since there are an
> > indeterminate number of &rest args going into a `format'.  So it seemed
> > better just to do the same with this function, to preserve a sort of
> > compatibility.

> While I can see the value of this aesthetic argument, I think breaking
> backward compatibility was a published API is a more serious problem.

Again, does anything else use it?

> On the upside, moving it to the end will make it optional, which is good
> since in many cases we can use the `form` argument instead (which
> `byte-compile-warn-x` doesn't have).

> >> While I'm here I also noticed that `byte-compile-form-stack` is a poor
> >> name for a variable declared in `macroexp.el`.

> > It's an integral part of bytecomp.el.  It got moved to macroexp.el
> > because it is used (twice) there, and that file is loaded into bootstrap
> > emacs before bytecomp.el.

> > There is precedent for this "mis"naming, namely
> > byte-compile-bound-variables.

> `byte-compile-bound-variables` is defined in `bytecomp.el`, not in `macroexp.el`.
> And indeed, `byte-compile-bound-variables` is only set/modified by the
> byte compiler, so it really belongs there.

> I can see that just moving the definition to bytecomp.el and using
> (defvar byte-compile-form-stack) in macroexp.el won't work because the
> `push` requires the var to have a value.

> Still, the current setup is really ugly: that var belongs in
> `bytecomp.el`.

Well, I suppose it could be defined in bytecomp.el and just declared in
macroexp.el.  It's not going to get used before it's been initialised in
bytecomp.el.

> > It started off life with a double hyphen in bytecomp.el.  But when it
> > started getting used in macroexp.el (during the expansion of a macro) it
> > lost the extra hyphen and got moved there.

> I'd put a double hyphen there simply because it's not something that we
> want to expose as an official API.  Just because the bytecompiler's
> macroexpansion phase is implemented in a separate file doesn't justify
> making the var public.

OK, we can mange that.

> > It's no big deal, I think, just that there's no completely neat way of
> > doing this, so the compromise actually used is pretty arbitrary.
> > If the variable were in bytecomp.el, we'd probably need a boundp call
> > in the two places we use it in macroexp.el.

> It at least deserves a prominent comment explaining why it's there.

> > Whilst on the topic of macroexp-warn-and-return (and
> > macroexp--wrap-warn), I have to admit having difficulty understanding
> > these functions, both how they work and what they're for.

> > My impression up till a couple of days ago was that they were ways of
> > coping with the old warning position mechanism, and were intended to
> > compensate for its deficiencies.

> The original motivation was indeed to improve the error messages by
> including more relevant line information.  This part was made largely
> irrelevant with your patch.

> But it's still relevant because macros can use it without being tied to
> the byte-compiler.  Also a nice side-effect is that the warnings are
> emitted (mostly) in the order they appear in the code, whereas otherwise
> we'd first have the warnings emitted during macroexpansion, then
> warnings emitted during the compilation.

OK, thanks.

> > Now, I'm much less sure.  Was I indeed mistaken?  If I was, what then is
> > the purpose of these functions, which defer warning messages in some
> > fashion?  If I was right, it would be a good thing to dismantle them,
> > since they are complicated and difficult, and no longer needed.  As I
> > said, I don't really understand them.

> I don't see what's difficult about it: it lets you attach a warning to
> a piece of code.

There's a lot difficult about it.  The doc string says, vaguely, "Return
code equivalent to FORM labeled with warning MSG.".  "Labeled" is not
used like this anywhere else in Emacs.  Nothing says what the nature of
this "labelling" is, or what needs to be done to the resulting code to
make it executable, when the "labelling" gets undone, or when the warning
gets emitted.  Nothing says what "equivalent" means here, either.  In
what sense is the new code "equivalent", and in what respects is it
different?

The source code is difficult to read, too.  At least, I found it so,
having spent several hours on it.  macroexp--warn-wrap, an essential part
of the mechanism, is entirely lacking any doc string or comment.  It
performs actions when it is called, and returns code which is going to
get executed at some later stage.  When?  A comment explaining this would
be exceptionally helpful.

Nothing in the source code says what macroexp-warn-and-return is _for_.

I suspect the difficulty in understanding this facility will have
strongly dissuaded any external hackers from attempting to use it.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#53526: 29.0.50; macroexp-warn-and-return API change

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> No, its name was changed from "macroexp--" to "macroexp-" in Emacs-28,
>> specifically to make available for third party packages.  It was
>> announced in etc/NEWS, for example.
> Are you aware of it being used anywhere else but lisp/emacs-lisp?

Yes and no: there's a use of `macroexp--warn-and-return` in `peg.el`
(in GNU ELPA).  This should be updated to use `macroexp-warn-and-return`
when Emacs-28 is released.

But changing the API this way will discourage its use outside of Emacs
since it's be a pain to write code that deals with such changes (short
of imposing Emacs-29 as the minimum supported version).

>> Still, the current setup is really ugly: that var belongs in
>> `bytecomp.el`.
> Well, I suppose it could be defined in bytecomp.el and just declared in
> macroexp.el.

That's all we need.

> It's not going to get used before it's been initialised in
> bytecomp.el.

If that's the case, it's even better.

>> I'd put a double hyphen there simply because it's not something that we
>> want to expose as an official API.  Just because the bytecompiler's
>> macroexpansion phase is implemented in a separate file doesn't justify
>> making the var public.
> OK, we can mange that.

Thanks.

> I suspect the difficulty in understanding this facility will have
> strongly dissuaded any external hackers from attempting to use it.

Could be.
I suspect it's more a lack of exposure and the fact that most macros are
quick hacks that don't bother to perform much checking.

But it's definitely a facility that's useful for libraries that mostly
define a DSL via macros, like `peg.el` and `bindat.el`.


        Stefan