bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Drew Adams]

WHY?

1. The Elisp manual says nothing about this.  Is this something new?
If there is really a problem or potential problem then the manual
should cover this in the section that describes this function.

2. What is wrong with code such as this at top level?  It results
in the warning.

(when (fboundp 'foo)
  (defvar toto 42 "...")
  (make-variable-buffer-local 'toto))

As it is, the warning makes no sense to me.  If there is some
*specific* problem that can sometimes arise then describe the
specific context where the problem might arise.

Either get rid of this warning or have it explicitly point to a
section of the Elisp manual that explains the problem - in precise
terms.


In GNU Emacs 24.3.50.1 (i686-pc-mingw32)
 of 2013-07-14 on ODIEONE
Bzr revision: 113423 lekktu@gmail.com-20130715004922-i67tg2ois14h3fpm
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/Devel/emacs/binary --enable-checking=yes,glyphs
 CFLAGS='-O0 -g3' CPPFLAGS='-Ic:/Devel/emacs/include'
 LDFLAGS='-Lc:/Devel/emacs/lib''

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Stefan Monnier]

> 1. The Elisp manual says nothing about this.  Is this something new?

No.

> 2. What is wrong with code such as this at top level?

Nothing.  But most of the cases I bumped into (all of which have been
fixed now, AFAIK) were cases where the author had confused
make-local-variable and make-variable-buffer-local.


        Stefan

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Drew Adams]

> > 2. What is wrong with code such as this at top level?
> 
> Nothing.  But most of the cases I bumped into (all of which have
> been fixed now, AFAIK)

I guess you mean problems in the distributed code (?).

> were cases where the author had confused make-local-variable
> and make-variable-buffer-local.

Fair enough.  That is worth trying to help users with.  And it
has long been something needing reminding.

For things like that, I don't tend to favor compiler warnings.
I tend to favor improving the doc or adding a wiki page or a
tutorial video or...

Anyway, is the current approach (warning about anything not at
top level) the best way?  I think it confuses more than it helps.
I understand that it is well-intentioned.  But I don't think the
resulting noise and confusion are worth it.  If the lesson is
not clear and accurate then the course isn't worth taking or
giving.

Can you characterize better the case where we want people to use
(or not to use) `make-variable-buffer-local'?  IOW, just saying
you should use this at top level is too gross (and not correct).

Can you improve the spec a bit and then the implementation so
the warning appears only when it should (or almost only then)?

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> > 1. The Elisp manual says nothing about this.  Is this something new?
>
> No.
>
> > 2. What is wrong with code such as this at top level?
>
> Nothing.  But most of the cases I bumped into (all of which have been
> fixed now, AFAIK) were cases where the author had confused
> make-local-variable and make-variable-buffer-local.

Maybe

  "Warning: `make-variable-buffer-local' not called at toplevel"

would have the same learning experience, without saying "should be...",
which is wrong and confusing to people who know what they're doing.


Michael.

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Stefan Monnier]

>   "Warning: `make-variable-buffer-local' not called at toplevel"

> would have the same learning experience, without saying "should be...",
> which is wrong and confusing to people who know what they're doing.

Sounds right.  Can someone make that change, plase?


        Stefan

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> >   "Warning: `make-variable-buffer-local' not called at toplevel"
>
> > would have the same learning experience, without saying "should be...",
> > which is wrong and confusing to people who know what they're doing.
>
> Sounds right.  Can someone make that change, plase?

Sorry, I can't (yet - I'm currently on the way to learn git).

Michael.

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Glenn Morris]

Stefan Monnier wrote:

> Sounds right.  Can someone make that change, plase?

Done. Of course, it won't remotely satisfy the OP.

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Drew Adams]

> > > "Warning: `make-variable-buffer-local' not called at toplevel"

> > > would have the same learning experience, without saying
> > > "should be...", which is wrong and confusing to people who
> > > know what they're doing.
> >
> > Sounds right.  Can someone make that change, plase?
> 
> Done. Of course, it won't remotely satisfy the OP.

Right.  It warns people not to use `make-variable-buffer-local'
except at top level.  Which is wrong.  Top level is irrelevant
to the proper use of `make-variable-buffer-local'.

By Stefan's admission, the problem this warning aims to solve is
a user who might be confusing `make-variable-buffer-local' with
`make-local-variable'.

His heuristic is presumably that IF `make-variable-buffer-local'
is used somewhere other than at top level THEN it is likely that
the user has confused the two functions, and `make-local-variable'
might be more appropriate.

It is the THEN part of that sentence, not the IF part, that is
the proper message for the user.

Assuming that the heuristic is good, the "warning" should really
draw the user's attention, not to "top level", but to
`make-local-variable' (which it does not mention).

Non top-levelness is only the **means by which the compiler**
detects a situation where the user might need help.  The warning
as proposed does not provide the help.  At all.

Think of what the user needs, not what the heuristic does.

You think you've found a good way to detect a case where the user
needs to be informed about `make-variable-buffer-local' vs
`make-local-variable'.  Great.  So tell the user about that, not
about how you figured out that s?he might be confused (= not at
top level).

If you want to issue a warning based on this heuristic, the 
message to the user should be to suggest that s?he might want
to use `make-local-variable' instead.  Don't even mention "top
level".  Just tell the user to consider using `make-local-variable'.

A useful message would even point to a doc section clarifying
the difference between the two functions.

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Juanma Barranquero]

On Fri, Aug 16, 2013 at 8:54 AM, Glenn Morris <rgm@gnu.org> wrote:

> Done. Of course, it won't remotely satisfy the OP.

FWIW, looking at the change I've had the same reaction as Drew.

The message still suggests that using make-variable-buffer-local other
than at toplevel is erroneous.

It should say something like

  Use of make-variable-buffer-local other than at top-level could be
an error; perhaps you wanted to use make-local-variable?

only shortish and better redacted.

   J

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Michael Heerdegen]

Juanma Barranquero <lekktu@gmail.com> writes:

> FWIW, looking at the change I've had the same reaction as Drew.

Well, it was the best short formulation that came to my mind.

> The message still suggests that using make-variable-buffer-local other
> than at toplevel is erroneous.
>
> It should say something like
>
>   Use of make-variable-buffer-local other than at top-level could be
> an error; perhaps you wanted to use make-local-variable?
>
> only shortish and better redacted.

The "shortish" part is the problem.  Is it even allowed to use more than
one line?  Presumably this kind of warning would not appear to often
when you do things right, so using a longer formulation could be
justified.


Michael.

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Drew Adams]

> > FWIW, looking at the change I've had the same reaction as Drew.
> 
> Well, it was the best short formulation that came to my mind.
...
> The "shortish" part is the problem.

Is this short enough and clear enough?

 "Consider using `make-local-variable' instead"

The context (line #) should make it clear what the "instead" refers to.

This way we're just drawing the user's attention to the possibility
that it might be more appropriate to use `make-local-variable' in that
context.  It's up to the user to check what that means, by checking the
doc.

If we could include a reference to the manual that would be even better.
The node is (elisp) `Creating Buffer-Local'.  If a link is not possible
then perhaps just add text.  Is this too long (dunno)?

 "Consider using `make-local-variable' instead [(elisp) Creating Buffer-Local]

(Yes, links or textual references from product to doc can mean more
maintenance, unless they can be updated automatically.  But they can
help users.)

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Stefan Monnier]

> The message still suggests that using make-variable-buffer-local other
> than at toplevel is erroneous.

Notice that the message is only emitted when the argument to the"
function is a constant.  I know of only one case where it is "not
erroneous" to have such a call not at top-level (from message.el):

   ;; FIXME: On XEmacs this causes problems since let-binding like:
   ;; (let ((message-options message-options)) ...)
   ;; as in `message-send' and `mml-preview' loses to buffer-local
   ;; variable initialization.
   (unless (featurep 'xemacs)
     (make-variable-buffer-local 'message-options))

So yes, I think in 99% of the cases, it's erroneous.


        Stefan

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Juanma Barranquero]

On Fri, Aug 16, 2013 at 6:25 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:

> Notice that the message is only emitted when the argument to the"
> function is a constant.   [...]
> So yes, I think in 99% of the cases, it's erroneous.

OK, but even so, the message should say why is erroneous, or suggest a
better way. Saying that is erroneous, and nothing more, will still
leave 1% of programmers quite puzzled ;-)

   J

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Drew Adams]

> > The message still suggests that using make-variable-buffer-local
> > other than at toplevel is erroneous.
> 
> Notice that the message is only emitted when the argument to the"
> function is a constant.  I know of only one case where it is "not
> erroneous" to have such a call not at top-level (from message.el):
> 
>    ;; FIXME: On XEmacs this causes problems since let-binding like:
>    ;; (let ((message-options message-options)) ...)
>    ;; as in `message-send' and `mml-preview' loses to buffer-local
>    ;; variable initialization.
>    (unless (featurep 'xemacs)
>      (make-variable-buffer-local 'message-options))
> 
> So yes, I think in 99% of the cases, it's erroneous.

Hm.  A certain Stefan said this previously: ;-)

 >> > 2. What is wrong with code such as this at top level?
 >> 
 >> Nothing.  But most of the cases I bumped into... were cases
 >> where the author had confused make-local-variable and
 >> make-variable-buffer-local.

Putting the two statements together, I guess you're saying:

1. There is nothing *inherently* wrong with
`make-variable-buffer-local' at other than top level, at least in
cases like the example cited in the bug report:

(when (fboundp 'foo)
  (defvar toto 42 "...")
  (make-variable-buffer-local 'toto))

2. But though there is nothing inherently wrong with that, in 99%
of the cases it is erroneous to use `make-variable-buffer-local',
except at top level.

That would indeed be a good case for a warning.  (And perhaps a
note in the doc.)  But see my previous mail about the content of
the warning message: it should point to `make-local-variable' and
it should not mention "top level" at all.

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Glenn Morris]

Please make the warning message a complete copy of this entire discussion.

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Michael Heerdegen]

Drew Adams <drew.adams@oracle.com> writes:

> Is this short enough and clear enough?
>
>  "Consider using `make-local-variable' instead"
>
> The context (line #) should make it clear what the "instead" refers to.

That's still confusing.  Why should I consider that?  A user already
knowing the difference may think "why is there such a (new!) warning?
Did they change the implementation?  Is there something important I have
forgotten, or something new I don't know about?

I would prefer a sentence like

"Did you mean `make-local-variable'?"

which is short and just says: I, Emacs, think you confused those two,
but it's ok if you know what you're doing.


Michael.

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Michael Heerdegen]

Glenn Morris <rgm@gnu.org> writes:

> Please make the warning message a complete copy of this entire
> discussion.

If the sum of times we spent discussing is less then the sum of times
users don't waste because they are confused by the warning message, it
was worth the time.  But I doubt we reach that goal with your proposal.

Michael.

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Glenn Morris]

This feature is 8 years old, BTW.

So "Is this something new?" in Jul 2013 tells you something about how
much of an issue this has been in practice.

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Drew Adams]

> > Is this short enough and clear enough?
> >
> >  "Consider using `make-local-variable' instead"
> >
> > The context (line #) should make it clear what the "instead" refers to.
> 
> That's still confusing.  Why should I consider that?

The message just suggests that you might want to use `m-l-v' instead.

Follow the link to the doc, or use `C-h f make-local-variable' and
`C-h f make-variable-buffer-local' to find out why.  Yes, that's why
it helps to add a manual reference to the msg.

You should not expect a warning message to give you a tutorial.  But
sometimes it can point you to further info that will enlighten you.

> A user already knowing the difference

will either:

a) realize whether or not she made a mistake, by taking 2 sec to
   "consider" which is appropriate here

or:

b) ignore the message, being sure of what s?he is doing.

In both cases the message will have served its purpose (the only
reasonable purpose a message here can have).

> may think "why is there such a (new!) warning?

Now that's a different topic.  ;-)

Emacs is adding warnings all the time.  A user may wonder, indeed.
But that is not particular to this or any that new message.

There is no getting around that, in this case anyway, because there is
nothing new here, besides the fact of Emacs Dev wanting to draw attention
to this possible confusion (which is good to do, especially in the doc).

> Did they change the implementation?  Is there something important I have
> forgotten, or something new I don't know about?

Yes, one wonders.

> I would prefer a sentence like
> 
> "Did you mean `make-local-variable'?"
> 
> which is short and just says: I, Emacs, think you confused those two,
> but it's ok if you know what you're doing.

I disagree that that is as clear, but it's OK by me.  Certainly a lot
better than pointing the user to top-level vs not top-level, which is
a red herring.

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Michael Heerdegen]

Glenn Morris <rgm@gnu.org> writes:

> This feature is 8 years old, BTW.
>
> So "Is this something new?" in Jul 2013 tells you something about how
> much of an issue this has been in practice.

I'll tell you something funny.  When Stefan said that 99% of
non-top-level uses of `make-variable-buffer-local' were wrong, I
searched my large .emacs for counter-examples.  I found 4 such places,
and for every place, it was by mistake!  Of course I knew the difference
when I wrote that code, but these function names sound so similar...

All these occurrences were wrapped inside something like

  (add-hook 'hook (lambda () ... (make-variable-buffer-local ...)))

or inside `with-eval-after-load' (that also wraps the body inside a
lambda) and no warning had been printed all the time.

It would be good to warn also for calls inside lambdas.


Michael.

bug#14926: 24.3.50; Warning "`make-variable-buffer-local' should be called at toplevel"

[Lars Ingebrigtsen]

Juanma Barranquero <lekktu@gmail.com> writes:

> On Fri, Aug 16, 2013 at 8:54 AM, Glenn Morris <rgm@gnu.org> wrote:
>
>> Done. Of course, it won't remotely satisfy the OP.
>
> FWIW, looking at the change I've had the same reaction as Drew.
>
> The message still suggests that using make-variable-buffer-local other
> than at toplevel is erroneous.
>
> It should say something like
>
>   Use of make-variable-buffer-local other than at top-level could be
> an error; perhaps you wanted to use make-local-variable?
>
> only shortish and better redacted.

The doc string of `make-variable-buffer-local' mentions the later
function, so I think that's sufficient for people who wants to know more
about the warning.  Closing.

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