GNU bug report logs - #39373
27.0.50; [PATCH] mode-local-print-bindings broken with lexical-binding

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 39373 in the body.
You can then email your comments to 39373 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50; [PATCH] mode-local-print-bindings broken with lexical-binding
Date: Fri, 31 Jan 2020 18:08:01 +0100

[Message part 1 (text/plain, inline)]

'mode-local-print-bindings' in mode-local.el seems to be broken since the file was converted to lexical binding.
Straightforward patch attached -- acceptable for emacs-27? (It's a regression from Emacs 26.)

[0001-Fix-describe-mode-local-bindings-lexical-binding-dam.patch (application/octet-stream, attachment)]

Message #8 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50;
 [PATCH] mode-local-print-bindings broken with lexical-binding
Date: Fri, 31 Jan 2020 21:27:48 +0200

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Fri, 31 Jan 2020 18:08:01 +0100
> 
> Straightforward patch attached -- acceptable for emacs-27?

If you find an easier way of fixing it, yes.

Message #11 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with
 lexical-binding
Date: Fri, 31 Jan 2020 20:38:08 +0100

31 jan. 2020 kl. 20.27 skrev Eli Zaretskii <eliz <at> gnu.org>:

> If you find an easier way of fixing it, yes.

Sorry if I'm misunderstanding you, but the patch just replaces 'add-to-list' with 'push' -- is that not easy enough?

Message #14 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with
 lexical-binding
Date: Fri, 31 Jan 2020 22:25:06 +0200

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Fri, 31 Jan 2020 20:38:08 +0100
> Cc: 39373 <at> debbugs.gnu.org
> 
> 31 jan. 2020 kl. 20.27 skrev Eli Zaretskii <eliz <at> gnu.org>:
> 
> > If you find an easier way of fixing it, yes.
> 
> Sorry if I'm misunderstanding you, but the patch just replaces 'add-to-list' with 'push' -- is that not easy enough?

That's not the "usual" way of fixing problems with lexical-binding,
and it isn't immediately clear to me why that fixes the problem.

Message #17 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with
 lexical-binding
Date: Fri, 31 Jan 2020 21:51:48 +0100

31 jan. 2020 kl. 21.25 skrev Eli Zaretskii <eliz <at> gnu.org>:

> That's not the "usual" way of fixing problems with lexical-binding,
> and it isn't immediately clear to me why that fixes the problem.

'add-to-list' doesn't work on lexical variables because it's a plain function taking the variable symbol as argument. 'push' works on lexical variables since it is a macro that expands to direct variable reference and setq. I should perhaps have included this in the commit message.

Happily, Emacs provides generalised variable support for 'if' and 'cons', so that they can be used as target for 'push'.

We could sink the pushes to the leaves of the condition tree, if you prefer:

(cond ((get s 'mode-variable-flag)
       (if (get s 'constant-flag) (push s mc) (push s mv)))
      ((get s 'override-flag)
       (if (get s 'constant-flag) (push s fo) (push s ov)))
      (t (push s us)))

Not as elegant or concise, but does not rely on if and cond as GVs in push.

Message #20 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with
 lexical-binding
Date: Sat, 01 Feb 2020 09:48:19 +0200

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Fri, 31 Jan 2020 21:51:48 +0100
> Cc: 39373 <at> debbugs.gnu.org
> 
> 'add-to-list' doesn't work on lexical variables because it's a plain
> function taking the variable symbol as argument. 'push' works on
> lexical variables since it is a macro that expands to direct
> variable reference and setq.

If that's the case, then why don't we say that loud and clear in the
ELisp manual?  (The doc string of add-to-list has some vague
recommendation, not sure if it really talks about this aspect, but
that's definitely not enough for such a serious issue.)

And doesn't it mean we should audit all the gazillion uses of
add-to-list in our sources, and do that urgently?

> I should perhaps have included this in the commit message.

That would have definitely helped, thanks.

> We could sink the pushes to the leaves of the condition tree, if you prefer:

No, your original patch is fine, but please let's update the docs
ASAP.

Thanks.

Message #23 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Mattias Engdegård
 <mattiase <at> acm.org>
Cc: 39373 <at> debbugs.gnu.org
Subject: RE: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with
 lexical-binding
Date: Sat, 1 Feb 2020 07:53:18 -0800 (PST)

> > 'add-to-list' doesn't work on lexical variables because it's a plain
> > function taking the variable symbol as argument. 'push' works on
> > lexical variables since it is a macro that expands to direct
> > variable reference and setq.
> 
> If that's the case, then why don't we say that loud and clear in the
> ELisp manual?  (The doc string of add-to-list has some vague
> recommendation, not sure if it really talks about this aspect, but
> that's definitely not enough for such a serious issue.)
> 
> And doesn't it mean we should audit all the gazillion uses of
> add-to-list in our sources, and do that urgently?

+1.  `add-to-list' uses `symbol-value'.  Everything that
uses `symbol-value' is in the same boat.

Message #26 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with
 lexical-binding
Date: Sat, 1 Feb 2020 20:24:56 +0100

[Message part 1 (text/plain, inline)]

1 feb. 2020 kl. 08.48 skrev Eli Zaretskii <eliz <at> gnu.org>:

> If that's the case, then why don't we say that loud and clear in the
> ELisp manual?  (The doc string of add-to-list has some vague
> recommendation, not sure if it really talks about this aspect, but
> that's definitely not enough for such a serious issue.)
> 
> And doesn't it mean we should audit all the gazillion uses of
> add-to-list in our sources, and do that urgently?

Yes, I agree, but it's not quite as simple as I thought at first. A mechanised scan of the source for uses of add-to-list on lexical variables revealed a handful:

lisp/cedet/mode-local.el:823:23: add-to-list to lexical variable mc
lisp/net/tramp-cache.el:376:45: add-to-list to lexical variable properties
lisp/net/tramp-cache.el:430:25: add-to-list to lexical variable result
lisp/net/zeroconf.el:259:40: add-to-list to lexical variable result
lisp/net/zeroconf.el:267:40: add-to-list to lexical variable result
lisp/net/zeroconf.el:281:23: add-to-list to lexical variable result
lisp/org/org.el:18685:49: add-to-list to lexical variable load-uncore
lisp/autoinsert.el:174:47: add-to-list to lexical variable modes
lisp/whitespace.el:1687:33: add-to-list to lexical variable style
test/lisp/emacs-lisp/map-tests.el:230:30: add-to-list to lexical variable result

However, add-to-list has a compiler macro which tries to make it work even for lexical variables, and it mostly does -- specifically, when the LIST-VAR parameter is on the form (quote VARIABLE), which is the case in all the above cases except the one i mode-local.el.

The macro (which apparently works in non-compiled code as well) attempts to warn about lexical variables but somehow this warning doesn't always trigger. I haven't researched this further.

It all seems rather fragile to me, and I'd rather add a note about not using add-to-list for lexical variables to its doc string and the manual, and fix all the calls listed above. However, this last point is not strictly necessary for correctness.

Two patches attached: a doc update, and a replacement of add-to-list in the cases listed above.

[0001-Clarify-add-to-list-documentation-bug-39373.patch (application/octet-stream, attachment)]

[0002-Replace-add-to-list-to-lexical-variable-with-push-bu.patch (application/octet-stream, attachment)]

Message #29 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with
 lexical-binding
Date: Sat, 01 Feb 2020 21:28:14 +0200

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Sat, 1 Feb 2020 20:24:56 +0100
> Cc: 39373 <at> debbugs.gnu.org
> 
> It all seems rather fragile to me, and I'd rather add a note about not using add-to-list for lexical variables to its doc string and the manual, and fix all the calls listed above.

I tend to agree.  Stefan, any comments?

> Two patches attached: a doc update, and a replacement of add-to-list in the cases listed above.

LGTM, on both counts, but let's hear from Stefan (and others, if
someone has an opinion).

Thanks.

Message #32 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken
 with lexical-binding
Date: Sat, 01 Feb 2020 15:15:08 -0500

>> If that's the case, then why don't we say that loud and clear in the ELisp manual?
>> (The doc string of add-to-list has some vague
>> recommendation, not sure if it really talks about this aspect, but
>> that's definitely not enough for such a serious issue.)

Good idea.

>> And doesn't it mean we should audit all the gazillion uses of
>> add-to-list in our sources, and do that urgently?

In dynamically-scoped files, it's OK.  But yes, we should audit them and
change those that need changing for lexical scoping.

I added a compiler macro as a crutch to handle the most command
problems, but it's just an ugly hack which can make things worse by
hiding the problem.

> The compiler-macro (which apparently works in non-compiled code as well)

Compiler macros work when the code passes through `macroexpand-all`, so
it works when the code is compiled as well as when it's `load`ed (thanks
to "eager" macroexpansion), but not when it's passed directly to `eval`.

> attempts to warn about lexical variables but somehow this warning
> doesn't always trigger.  I haven't researched this further.

The message is supposed not to trigger when it's applied to dynamically
scoped var, but it's probably not 100% reliable.

> It all seems rather fragile to me, and I'd rather add a note about not using
> add-to-list for lexical variables to its doc string and the manual, and fix
> all the calls listed above.

Yes, this compiler-macro isn't supposed to replace educating the
programmers (in the manual) and fixing the actual problems.

BTW, this problem doesn't affect only `add-to-list`.  Other culprits
include `add-hook`, `run-hooks`, `set`, and `symbol-value`.

> Two patches attached: a doc update, and a replacement of add-to-list in the cases listed above.

LGTM,


        Stefan

Message #35 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with
 lexical-binding
Date: Sat, 1 Feb 2020 22:40:16 +0100

1 feb. 2020 kl. 21.15 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:

> Compiler macros work when the code passes through `macroexpand-all`, so
> it works when the code is compiled as well as when it's `load`ed (thanks
> to "eager" macroexpansion), but not when it's passed directly to `eval`.

Understood, thank you. (Obviously they aren't expanded when the function is called indirectly, but I didn't find anyone doing that with add-to-list.)

> The message is supposed not to trigger when it's applied to dynamically
> scoped var, but it's probably not 100% reliable.

It appears that the warning triggers (as a hard error, actually) when add-to-list is called directly from a function, but not if it only occurs inside a lambda. All the cases found were inside lambdas (or they would have been fixed long ago).

> BTW, this problem doesn't affect only `add-to-list`.  Other culprits
> include `add-hook`, `run-hooks`, `set`, and `symbol-value`.

Right. I see that some of them are detected by the compiler (in byte-compile-form), but add-to-list is commented out.

The set of functions is a bit open-ended; there is also add-to-ordered-list, add-to-history, etc. Not sure how much text needs to be added for all these. There is a general note about lexical variables and symbol values in the manual, in the section about lexical binding.

> LGTM,

Thanks, pushed to emacs-27.

Message #38 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: monnier <at> iro.umontreal.ca, 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with
 lexical-binding
Date: Sun, 02 Feb 2020 05:31:25 +0200

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Sat, 1 Feb 2020 22:40:16 +0100
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 39373 <at> debbugs.gnu.org
> 
> > BTW, this problem doesn't affect only `add-to-list`.  Other culprits
> > include `add-hook`, `run-hooks`, `set`, and `symbol-value`.
> 
> Right. I see that some of them are detected by the compiler (in byte-compile-form), but add-to-list is commented out.
> 
> The set of functions is a bit open-ended; there is also add-to-ordered-list, add-to-history, etc. Not sure how much text needs to be added for all these. There is a general note about lexical variables and symbol values in the manual, in the section about lexical binding.

I'd prefer to have a short note in the description of each one of
them.

Thanks.

Message #41 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: monnier <at> iro.umontreal.ca, 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with
 lexical-binding
Date: Sun, 2 Feb 2020 12:58:40 +0100

2 feb. 2020 kl. 04.31 skrev Eli Zaretskii <eliz <at> gnu.org>:

> I'd prefer to have a short note in the description of each one of
> them.

Right -- I added a note to 'add-to-ordered-list' and 'add-to-history', since their arguments were described as variables.

For 'set' and 'symbol-value', the argument is described as a symbol, which I'd say makes a slight difference here, since lexical variables are mentioned in the description of the relationship between variables and symbols, and also explicitly in the documentation of 'set' in the manual.

The hook functions mostly talk about symbols and hooks, not variables, and I think the risk of them being used on lexvars is slight.

Good enough?

Message #44 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken
 with lexical-binding
Date: Sun, 02 Feb 2020 08:55:24 -0500

> The hook functions mostly talk about symbols and hooks, not variables, and
> I think the risk of them being used on lexvars is slight.

I also expected so, but I've already changed a handful of
`(run-hooks 'foo)` to `(mapcar #'funcall foo)`.  The problem is a bit
different, tho: it's *also* wrong to use it on dynamically-scoped let-bound
vars (tho it mostly works), so the doc should not focus on lexical
vars but on the difference between vars and hooks.


        Stefan

Message #47 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with
 lexical-binding
Date: Sun, 2 Feb 2020 15:14:35 +0100

2 feb. 2020 kl. 14.55 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:

> I also expected so, but I've already changed a handful of
> `(run-hooks 'foo)` to `(mapcar #'funcall foo)`.  The problem is a bit
> different, tho: it's *also* wrong to use it on dynamically-scoped let-bound
> vars (tho it mostly works), so the doc should not focus on lexical
> vars but on the difference between vars and hooks.

Very well, in that case you are probably better placed to provide the nuanced modifications necessary, if any.

Message #52 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken
 with lexical-binding
Date: Sun, 09 Aug 2020 13:39:10 +0200

Mattias Engdegård <mattiase <at> acm.org> writes:

> 2 feb. 2020 kl. 14.55 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:
>
>> I also expected so, but I've already changed a handful of
>> `(run-hooks 'foo)` to `(mapcar #'funcall foo)`.  The problem is a bit
>> different, tho: it's *also* wrong to use it on dynamically-scoped let-bound
>> vars (tho it mostly works), so the doc should not focus on lexical
>> vars but on the difference between vars and hooks.
>
> Very well, in that case you are probably better placed to provide the
> nuanced modifications necessary, if any.

If I'm reading this thread right, the specific problem in this bug
report was fixed (and the documentation of add-to-list updated), but the
more general problem of going through all the instances of add-to-list
(and friends) in lexical files hasn't been done?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #55 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken with
 lexical-binding
Date: Sun, 9 Aug 2020 15:28:11 +0200

9 aug. 2020 kl. 13.39 skrev Lars Ingebrigtsen <larsi <at> gnus.org>:

> If I'm reading this thread right, the specific problem in this bug
> report was fixed (and the documentation of add-to-list updated), but the
> more general problem of going through all the instances of add-to-list
> (and friends) in lexical files hasn't been done?

No, I believe add-to-list calls have been updated and I would be happy to mark the bug as done.
Stefan alluded to some improvement possibilities in the hook function documentation (re the difference between variables and hooks).

Message #58 received at 39373 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 39373 <at> debbugs.gnu.org
Subject: Re: bug#39373: 27.0.50; [PATCH] mode-local-print-bindings broken
 with lexical-binding
Date: Sun, 09 Aug 2020 21:42:33 +0200

Mattias Engdegård <mattiase <at> acm.org> writes:

> 9 aug. 2020 kl. 13.39 skrev Lars Ingebrigtsen <larsi <at> gnus.org>:
>
>> If I'm reading this thread right, the specific problem in this bug
>> report was fixed (and the documentation of add-to-list updated), but the
>> more general problem of going through all the instances of add-to-list
>> (and friends) in lexical files hasn't been done?
>
> No, I believe add-to-list calls have been updated and I would be happy
> to mark the bug as done.
> Stefan alluded to some improvement possibilities in the hook function
> documentation (re the difference between variables and hooks).

OK; then I'll just close this bug report, and if a new bug report about
those improvements can be opened (if that helps to track any work to be
done in this area).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.