Warnings about keymaps

Warnings about keymaps

[Eli Zaretskii]

Compilation of Lisp files yields warnings such as these two:

    Compiling emacs/lisp/cvs-status.el

    In toplevel form:
    emacs/lisp/cvs-status.el:93:22:Warning: variable assignment to constant
	`cvs-status-mode-map'
    Compiling emacs/lisp/diff-mode.el

    In toplevel form:
    emacs/lisp/diff-mode.el:1273:70:Warning: variable assignment to
	constant `diff-mode-map'

Sounds like a bug to me.

Re: Warnings about keymaps

[Glenn Morris]

Eli Zaretskii wrote:

>     emacs/lisp/cvs-status.el:93:22:Warning: variable assignment to constant
> 	`cvs-status-mode-map'

I think this is because easy-mmode-defmap defines cvs-status-mode-map
as a constant. Then, later on, the define-derived-mode macro tries to
do a defvar on cvs-status-mode-map. The simple change is to make
easy-mmode-defmap use defvar instead of defconst; the slightly more
correct thing (that I could not get to work...) is to make
define-derived-mode only defvar the mode map if it is not already
defined.

Re: Warnings about keymaps

[Stefan Monnier]

> Compilation of Lisp files yields warnings such as these two:
>     Compiling emacs/lisp/cvs-status.el

>     In toplevel form:
>     emacs/lisp/cvs-status.el:93:22:Warning: variable assignment to constant
> 	`cvs-status-mode-map'
>     Compiling emacs/lisp/diff-mode.el

>     In toplevel form:
>     emacs/lisp/diff-mode.el:1273:70:Warning: variable assignment to
> 	constant `diff-mode-map'

> Sounds like a bug to me.

Yes, there's some kind of bug in there.  The bug can be seen more
directly by byte-compiling the file below:

   (defconst foo nil)
   (defvar foo nil)

Looks like the "assignment-to-constant" warning should be more careful
to distinguish defvar from setq.


        Stefan

Re: Warnings about keymaps

[Glenn Morris]

Stefan Monnier wrote:

> The bug can be seen more directly by byte-compiling the file below:
>
>    (defconst foo nil)
>    (defvar foo nil)
>
> Looks like the "assignment-to-constant" warning should be more
> careful to distinguish defvar from setq.

This seems a fairly nonsensical thing to do, so why shouldn't the
compiler warn about it? The defvar is a best a no-op, at worst a bug
(if you were expecting it to have an effect).

Re: Warnings about keymaps

[Stefan Monnier]

>> The bug can be seen more directly by byte-compiling the file below:
>> 
>> (defconst foo nil)
>> (defvar foo nil)
>> 
>> Looks like the "assignment-to-constant" warning should be more
>> careful to distinguish defvar from setq.

> This seems a fairly nonsensical thing to do, so why shouldn't the
> compiler warn about it?

It could, but it shouldn't complain about assignment to a constant,
since the defvar will not perform any assignment.  I.e. maybe a warning
is OK, but not the warning we currently get.

> The defvar is a best a no-op, at worst a bug
> (if you were expecting it to have an effect).

In the OP's case the defvar was expected to have as effect to "provide
a default value to the var in case it's not defined elsewhere".  So it
seems to behave as expected (it's a no-op since the var is defined
elsewhere).


        Stefan

Re: Warnings about keymaps

[Glenn Morris]

Stefan Monnier wrote:

> It could, but it shouldn't complain about assignment to a constant,
> since the defvar will not perform any assignment.  I.e. maybe a warning
> is OK, but not the warning we currently get.

How about this? Or perhaps the defvar should just be silently
optimized away, like the default-boundp test seems to be trying to do.

*** bytecomp.el 10 Sep 2009 06:22:30 -0000 2.258
--- bytecomp.el 14 Sep 2009 23:30:56 -0000
***************
*** 4025,4032 ****
  		(let ((tmp (make-symbol "defconst-tmp-var")))
  		  `(funcall '(lambda (,tmp) (defconst ,var ,tmp))
  			    ,value))
  	      ;; `defvar' sets `var' only when unbound.
! 	      `(if (not (default-boundp ',var)) (setq-default ,var ,value))))
  	(when (eq fun 'defconst)
  	  ;; This will signal an appropriate error at runtime.
  	  `(eval ',form)))
--- 4036,4046 ----
  		(let ((tmp (make-symbol "defconst-tmp-var")))
  		  `(funcall '(lambda (,tmp) (defconst ,var ,tmp))
  			    ,value))
+ 	      (if (memq var byte-compile-const-variables)
+ 		  (byte-compile-warn "attempt to defvar constant `%s' \
+ has no effect" var)
  		;; `defvar' sets `var' only when unbound.
! 		`(if (not (default-boundp ',var)) (setq-default ,var ,value)))))
  	(when (eq fun 'defconst)
  	  ;; This will signal an appropriate error at runtime.
  	  `(eval ',form)))

Re: Warnings about keymaps

[Stefan Monnier]

>> It could, but it shouldn't complain about assignment to a constant,
>> since the defvar will not perform any assignment.  I.e. maybe a warning
>> is OK, but not the warning we currently get.

> How about this? Or perhaps the defvar should just be silently
> optimized away, like the default-boundp test seems to be trying to do.

This warning might be OK, but I have some issues with it:
- most importantly: it doesn't solve the problem at hand which is how
  to get rid of the other warning.
- what's its use?  It's not like we've seen lots of bugs where people
  use both defconst and defvar for a variable.
- Finally, if the defconst+defvar is really what you want to do (as in
  the present case where the defconst and the defvar both come from
  macros that we may not want to change), how do you silence
  the warning?


        Stefan


> *** bytecomp.el 10 Sep 2009 06:22:30 -0000 2.258
> --- bytecomp.el 14 Sep 2009 23:30:56 -0000
> ***************
> *** 4025,4032 ****
>   		(let ((tmp (make-symbol "defconst-tmp-var")))
>   		  `(funcall '(lambda (,tmp) (defconst ,var ,tmp))
>   			    ,value))
>   	      ;; `defvar' sets `var' only when unbound.
> ! 	      `(if (not (default-boundp ',var)) (setq-default ,var ,value))))
>   	(when (eq fun 'defconst)
>   	  ;; This will signal an appropriate error at runtime.
>   	  `(eval ',form)))
> --- 4036,4046 ----
>   		(let ((tmp (make-symbol "defconst-tmp-var")))
>   		  `(funcall '(lambda (,tmp) (defconst ,var ,tmp))
>   			    ,value))
> + 	      (if (memq var byte-compile-const-variables)
> + 		  (byte-compile-warn "attempt to defvar constant `%s' \
> + has no effect" var)
>   		;; `defvar' sets `var' only when unbound.
> ! 		`(if (not (default-boundp ',var)) (setq-default ,var ,value)))))
>   	(when (eq fun 'defconst)
>   	  ;; This will signal an appropriate error at runtime.
>   	  `(eval ',form)))

Re: Warnings about keymaps

[Glenn Morris]

Stefan Monnier wrote:

> - most importantly: it doesn't solve the problem at hand which is how
>   to get rid of the other warning.

The other warning?

> - what's its use?  It's not like we've seen lots of bugs where people
>   use both defconst and defvar for a variable.

It simply replaces the current "variable assignment to a constant"
warning with a slightly more informative one, as you suggested.

> - Finally, if the defconst+defvar is really what you want to do (as in
>   the present case where the defconst and the defvar both come from
>   macros that we may not want to change), how do you silence
>   the warning?

The same way you silence the current one about variable assignment to
a constant. :)

I don't think is a particularly useful change. I think it would be
better to change define-derived-mode to not defvar the map if it is
already defined; or to change easy-mode-defmap to not use defconst
(are you saying users should not add key bindings to
log-edit-mode-map, for example?).

Re: Warnings about keymaps

[Stefan Monnier]

>> - most importantly: it doesn't solve the problem at hand which is how
>> to get rid of the other warning.
> The other warning?
>> - what's its use?  It's not like we've seen lots of bugs where people
>> use both defconst and defvar for a variable.
> It simply replaces the current "variable assignment to a constant"
> warning with a slightly more informative one, as you suggested.

Sorry, I misread your patch.  So you actually completely remove the
defvar if var was already defconst'd.  That's an attractive approach,
tho it's a bit risky, because byte-compile-const-variables only contains
variables that "may have been defconst'd", not those that "we know for
sure have been defconst'd".  E.g. try it with code such as

  (when toto
    (defconst foo bar))
  (defvar foo nil)

with your patch, you'd end up with `foo' undefined in the case where
toto is nil.  It may seem like a braindead case, but when comparing
"broken code in corner cases" vs "incorrect warning in some cases",
I prefer the incorrect warning.

>> - Finally, if the defconst+defvar is really what you want to do (as in
>> the present case where the defconst and the defvar both come from
>> macros that we may not want to change), how do you silence
>> the warning?

> The same way you silence the current one about variable assignment to
> a constant. :)

The difference is that the current one is incorrect, so the act of
silencing it needs to be done in bytecomp.el.  Whereas yours is meant to
be correct, in which case silencing it would need to be done by tweaking
the code and/or using with-no-warnings (which I profoundly disklike).

> I think it would be better to change define-derived-mode to not defvar
> the map if it is already defined;

I don't know how to do that.

> or to change easy-mode-defmap to not use defconst

That might be an acceptable workaround.  But the underlying problem will
still be there.

> (are you saying users should not add key bindings to
> log-edit-mode-map, for example?).

That's unrelated: defconst only says that the variable's binding won't
change, not that the object it is bound to is immutable.  My locally
hacked Emacs has "strong defconst" which really makes the variable
immutable, so (setq log-edit-mode-map <blabla>) signals an error, but
(define-key log-edit-mode-map [foo] 'bar) still works just fine.


        Stefan

Re: Warnings about keymaps

[Glenn Morris]

Stefan Monnier wrote:

> It may seem like a braindead case, but when comparing "broken code
> in corner cases" vs "incorrect warning in some cases", I prefer the
> incorrect warning.

Definitely.

>> I think it would be better to change define-derived-mode to not defvar
>> the map if it is already defined;
>
> I don't know how to do that.

Oh well, that makes two of us.

>> or to change easy-mode-defmap to not use defconst
>
> That might be an acceptable workaround.  But the underlying problem will
> still be there.

But under a carpet. :)

>> (are you saying users should not add key bindings to
>> log-edit-mode-map, for example?).
>
> That's unrelated: defconst only says that the variable's binding won't
> change, not that the object it is bound to is immutable.

I don't understand that distinction. I just read the doc of defconst:

    The intent is that neither programs nor users should ever change
    this value.

which seems clear to me.

Re: Warnings about keymaps

[Stephen J. Turnbull]

Glenn Morris writes:
 > Stefan Monnier wrote:

 > > That's unrelated: defconst only says that the variable's binding won't
 > > change, not that the object it is bound to is immutable.
 > 
 > I don't understand that distinction. I just read the doc of defconst:
 > 
 >     The intent is that neither programs nor users should ever change
 >     this value.

It's the same distinction as between "const *" and "* const" in C.
The value of the variable is a constant object, but that object may be
mutable if it is a container.  Under this definition, if you were
implementing Emacs Lisp in Emacs Lisp, it would make sense to
implement the obarray as a defconst.  (You probably already knew that,
but your question literally says "that distinction", not "why you make
that distinction".)

I would like to know "why?" though.  XEmacs's docstring says "The
intent is that programs do not change this value, but users may."  The
awkward expression of the Emacs docstring suggests that the XEmacs
version is indeed the original.  Can somebody explain why it was
changed?  Is Emacs moving toward true constants?  (Eg, in r/o memory?)

Re: Warnings about keymaps

[Stefan Monnier]

>>> (are you saying users should not add key bindings to
>>> log-edit-mode-map, for example?).
>> That's unrelated: defconst only says that the variable's binding won't
>> change, not that the object it is bound to is immutable.
> I don't understand that distinction.

It's the same distinction that makes it possible for a function to
modify an array passed as argument in C, even though arguments are
passed "by value" (i.e. it receives a copy of the pointer to the array,
so it can't change which array is pointed, but it can modify the
array's contents).


        Stefan

Re: Warnings about keymaps

[Richard Stallman]

Should there be a way to undo the "constant" marking that defconst makes?