bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[Paul Pogonyshev]

[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]

I quite often get the following messages:

Warning: Unknown defun property ‘compiler-macro’ in ...

As far as I could trace it, the problem is indirectly caused by
`define-inline'. While definition of `defun-declarations-alist' does
contain `compiler-macro' in its init form, it can be removed later.
E.g. when I evaluated the variable, it was not there anymore, only
`gv-setter' was there.

It seems this is done unintentionally by `elisp-completion-at-point':

                       (`declare
                        (list t (mapcar (lambda (x) (symbol-name (car x)))
                                        (delete-dups
                                         ;; FIXME: We should include some
                                         ;; docstring with each entry.
                                         (append
                                          macro-declarations-alist
                                          defun-declarations-alist)))))

Here `delete-dups' destructively modifies a list that includes
`defun-declarations-alist' as its tail verbatim, not as a copy.
Attached patch should fix that.

Paul

* elisp-mode.el (elisp-completion-at-point): Fix to not alter
`defun-declarations-alist' by side effect.

[-- Attachment #2: xxx.diff --]
[-- Type: text/plain, Size: 1028 bytes --]

diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 2ad22dd..d8f3162 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -533,9 +533,9 @@ It can be quoted, or be inside a quoted form."
                                         (delete-dups
                                          ;; FIXME: We should include some
                                          ;; docstring with each entry.
-                                         (append
-                                          macro-declarations-alist
-                                          defun-declarations-alist)))))
+                                         (append macro-declarations-alist
+                                                 defun-declarations-alist
+                                                 nil)))))
                        ((and (or `condition-case `condition-case-unless-debug)
                              (guard (save-excursion
                                       (ignore-errors

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[npostavs]

Paul Pogonyshev <pogonyshev@gmail.com> writes:

> I quite often get the following messages:
>
> Warning: Unknown defun property ‘compiler-macro’ in ...
>
> As far as I could trace it, the problem is indirectly caused by
> `define-inline'. While definition of `defun-declarations-alist' does
> contain `compiler-macro' in its init form, it can be removed later.
> E.g. when I evaluated the variable, it was not there anymore, only
> `gv-setter' was there.
>
> It seems this is done unintentionally by `elisp-completion-at-point':
>
>                        (`declare
>                         (list t (mapcar (lambda (x) (symbol-name (car x)))
>                                         (delete-dups
>                                          ;; FIXME: We should include some
>                                          ;; docstring with each entry.
>                                          (append
>                                           macro-declarations-alist
>                                           defun-declarations-alist)))))
>
> Here `delete-dups' destructively modifies a list that includes
> `defun-declarations-alist' as its tail verbatim, not as a copy.
> Attached patch should fix that.

I agree with analysis and patch here.  Since this just appends another
nil, it should be safe for emacs-25, right?

>
> Paul
>
> * elisp-mode.el (elisp-completion-at-point): Fix to not alter
> `defun-declarations-alist' by side effect.

> -                                         (append
> -                                          macro-declarations-alist
> -                                          defun-declarations-alist)))))
> +                                         (append macro-declarations-alist
> +                                                 defun-declarations-alist
> +                                                 nil)))))

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Date: Sun, 17 Jul 2016 23:00:33 -0400
> Cc: 23648@debbugs.gnu.org
> 
> I agree with analysis and patch here.  Since this just appends another
> nil, it should be safe for emacs-25, right?

I'm not sure, actually.  How probable is the situation where this
problem pops up?  And when was the bug introduced?

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[Noam Postavsky]

On Mon, Jul 18, 2016 at 10:33 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: npostavs@users.sourceforge.net
>> Date: Sun, 17 Jul 2016 23:00:33 -0400
>> Cc: 23648@debbugs.gnu.org
>>
>> I agree with analysis and patch here.  Since this just appends another
>> nil, it should be safe for emacs-25, right?
>
> I'm not sure, actually.  How probable is the situation where this
> problem pops up?

It happens with 100% probability when performing completion inside a
(declare ...) form. Starting from emacs -Q, put into *scratch*

(defun foo ()
  (declare (indent 1))
  nil)

Macroexpanding this gives:

(prog1
    (defalias 'foo
      #'(lambda nil nil))
  (put 'foo 'lisp-indent-function '1))

Now move point to just after "indent" and type C-M-i (this gives
message "Sole completion"), macroexpanding now gives

(prog1
    (defalias 'foo
      #'(lambda nil nil))
  "Warning: Unknown defun property `indent' in foo")

>  And when was the bug introduced?

Code seems to have been that way since it was introduced in 24.4:
dd8791e9 "* lisp/emacs-lisp/lisp.el (lisp-completion-at-point):
Provide specialized
completion tables when completing error conditions and
`declare' arguments...."

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Mon, 18 Jul 2016 11:53:42 -0400
> Cc: Paul Pogonyshev <pogonyshev@gmail.com>, 23648@debbugs.gnu.org
> 
> > I'm not sure, actually.  How probable is the situation where this
> > problem pops up?
> 
> It happens with 100% probability when performing completion inside a
> (declare ...) form. Starting from emacs -Q, put into *scratch*
> 
> (defun foo ()
>   (declare (indent 1))
>   nil)
> 
> Macroexpanding this gives:
> 
> (prog1
>     (defalias 'foo
>       #'(lambda nil nil))
>   (put 'foo 'lisp-indent-function '1))
> 
> Now move point to just after "indent" and type C-M-i (this gives
> message "Sole completion"), macroexpanding now gives
> 
> (prog1
>     (defalias 'foo
>       #'(lambda nil nil))
>   "Warning: Unknown defun property `indent' in foo")

Sorry, I'm not following: what do you mean by "macroexpanding" in this
context?  When you wrote "when performing completion", I expected to
see some simple completion gesture that leads to an error, but it
sounds like I'm missing something.

> >  And when was the bug introduced?
> 
> Code seems to have been that way since it was introduced in 24.4:
> dd8791e9 "* lisp/emacs-lisp/lisp.el (lisp-completion-at-point):
> Provide specialized
> completion tables when completing error conditions and
> `declare' arguments...."

Thanks.

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[Noam Postavsky]

On Mon, Jul 18, 2016 at 2:16 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Noam Postavsky <npostavs@users.sourceforge.net>
>> Date: Mon, 18 Jul 2016 11:53:42 -0400
>> Cc: Paul Pogonyshev <pogonyshev@gmail.com>, 23648@debbugs.gnu.org
>>
>> > I'm not sure, actually.  How probable is the situation where this
>> > problem pops up?
>>
>> It happens with 100% probability when performing completion inside a
>> (declare ...) form. Starting from emacs -Q, put into *scratch*
>>
>> (defun foo ()
>>   (declare (indent 1))
>>   nil)
>>
>> Macroexpanding this gives:
>>
>> (prog1
>>     (defalias 'foo
>>       #'(lambda nil nil))
>>   (put 'foo 'lisp-indent-function '1))
>>
>> Now move point to just after "indent" and type C-M-i (this gives
>> message "Sole completion"), macroexpanding now gives
>>
>> (prog1
>>     (defalias 'foo
>>       #'(lambda nil nil))
>>   "Warning: Unknown defun property `indent' in foo")
>
> Sorry, I'm not following: what do you mean by "macroexpanding" in this
> context?

Calling macroexpand on the defun form. The most convenient method for
testing is to put point at the closing bracket of the defun, and then
`M-x pp-macroexpand-last-sexp'.

> When you wrote "when performing completion", I expected to
> see some simple completion gesture that leads to an error, but it
> sounds like I'm missing something.

The simple completion gesture would be the C-M-i I mentioned, which
(silently) destroys the value of defun-declarations-alist. The
symptoms of the wrong value can be seen when defun is macroexpanded.

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Mon, 18 Jul 2016 14:58:20 -0400
> Cc: Paul Pogonyshev <pogonyshev@gmail.com>, 23648@debbugs.gnu.org
> 
> >> Now move point to just after "indent" and type C-M-i (this gives
> >> message "Sole completion"), macroexpanding now gives
> >>
> >> (prog1
> >>     (defalias 'foo
> >>       #'(lambda nil nil))
> >>   "Warning: Unknown defun property `indent' in foo")
> >
> > Sorry, I'm not following: what do you mean by "macroexpanding" in this
> > context?
> 
> Calling macroexpand on the defun form. The most convenient method for
> testing is to put point at the closing bracket of the defun, and then
> `M-x pp-macroexpand-last-sexp'.
> 
> > When you wrote "when performing completion", I expected to
> > see some simple completion gesture that leads to an error, but it
> > sounds like I'm missing something.
> 
> The simple completion gesture would be the C-M-i I mentioned, which
> (silently) destroys the value of defun-declarations-alist. The
> symptoms of the wrong value can be seen when defun is macroexpanded.

Sorry, I guess I'm too stupid to understand this advanced stuff.  Or
maybe it's too late.

Thanks.

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Sorry, I guess I'm too stupid to understand this advanced stuff.  Or
> maybe it's too late.

I think the example uses macroexpand only to demonstrate what (obviously
ill) code you will get when you compile the mentioned form after
performing the completion as mentioned.

Michael.

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Noam Postavsky <npostavs@users.sourceforge.net>,  23648@debbugs.gnu.org,  pogonyshev@gmail.com
> Date: Mon, 18 Jul 2016 23:28:55 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Sorry, I guess I'm too stupid to understand this advanced stuff.  Or
> > maybe it's too late.
> 
> I think the example uses macroexpand only to demonstrate what (obviously
> ill) code you will get when you compile the mentioned form after
> performing the completion as mentioned.

Thanks, but I'm still none the wiser about the questions I asked.

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[npostavs]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Michael Heerdegen <michael_heerdegen@web.de>
>> Cc: Noam Postavsky <npostavs@users.sourceforge.net>,  23648@debbugs.gnu.org,  pogonyshev@gmail.com
>> Date: Mon, 18 Jul 2016 23:28:55 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Sorry, I guess I'm too stupid to understand this advanced stuff.  Or
>> > maybe it's too late.
>> 
>> I think the example uses macroexpand only to demonstrate what (obviously
>> ill) code you will get when you compile the mentioned form after
>> performing the completion as mentioned.
>
> Thanks, but I'm still none the wiser about the questions I asked.

Hmm, maybe it will be clearer like this:

Evaluate:

(macroexpand '(defun foo ()
                (declare (indent 1))
                nil)) ;=> (prog1 (defalias (quote foo) (function (lambda nil nil))) (function-put (quote foo) (quote lisp-indent-function) (quote 1)))

This gives the correct result (the (function-put...) part comes from the
(declare (indent 1))).

Now perform a completion on a declare clause, e.g., put cursor after
"ind" and hit C-M-i: (declare (ind)) completes to (declare (indent)).

Now evaluate the same expression as before:

(macroexpand '(defun foo ()
                (declare (indent 1))
                nil)) ;=> (prog1 (defalias (quote foo) (function (lambda nil nil))) "Warning: Unknown defun property ‘indent’ in foo")

This give the wrong result, the (declare (indent 1)) is giving the
"Warning:...".  Emacs has unlearned the indent declaration.  In fact it
unlearned all the declarations for defun except for gv-setter, you can
see this by looking at defun-declarations-alist's value.

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: Michael Heerdegen <michael_heerdegen@web.de>,  23648@debbugs.gnu.org,  pogonyshev@gmail.com
> Date: Wed, 20 Jul 2016 21:09:42 -0400
> 
> > Thanks, but I'm still none the wiser about the questions I asked.
> 
> Hmm, maybe it will be clearer like this:
> 
> Evaluate:
> 
> (macroexpand '(defun foo ()
>                 (declare (indent 1))
>                 nil)) ;=> (prog1 (defalias (quote foo) (function (lambda nil 
> nil))) (function-put (quote foo) (quote lisp-indent-function) (quote 1)))
> 
> This gives the correct result (the (function-put...) part comes from the
> (declare (indent 1))).
> 
> Now perform a completion on a declare clause, e.g., put cursor after
> "ind" and hit C-M-i: (declare (ind)) completes to (declare (indent)).
> 
> Now evaluate the same expression as before:
> 
> (macroexpand '(defun foo ()
>                 (declare (indent 1))
>                 nil)) ;=> (prog1 (defalias (quote foo) (function (lambda nil 
> nil))) "Warning: Unknown defun property ‘indent’ in foo")
> 
> This give the wrong result, the (declare (indent 1)) is giving the
> "Warning:...".  Emacs has unlearned the indent declaration.  In fact it
> unlearned all the declarations for defun except for gv-setter, you can
> see this by looking at defun-declarations-alist's value.

Thanks, but I think we are mis-communicating.  What I need is not a
demonstration of the bug in action; I already got that.  What I asked
for is different:

> How probable is the situation where this problem pops up?  And when
> was the bug introduced?

You already answered the second question.  For the first, I expected
to see something done frequently by either users or Lisp programs,
which bumps into this bug.  Evaluating macroexpand, twice, with
completion in-between, doesn't qualify in my book as a frequent user
action, I hope you will agree (even if you personally happen to use it
quite a lot).

So I'm still looking for the answer to the "how probable" question.  I
need that to make up my mind about the urgency of the fix.

Thanks.

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[Noam Postavsky]

On Thu, Jul 21, 2016 at 10:22 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> Thanks, but I think we are mis-communicating.  What I need is not a
> demonstration of the bug in action; I already got that.  What I asked
> for is different:
>
>> How probable is the situation where this problem pops up?  And when
>> was the bug introduced?
>
> You already answered the second question.  For the first, I expected
> to see something done frequently by either users or Lisp programs,
> which bumps into this bug.  Evaluating macroexpand, twice, with
> completion in-between, doesn't qualify in my book as a frequent user
> action, I hope you will agree (even if you personally happen to use it
> quite a lot).
>
> So I'm still looking for the answer to the "how probable" question.  I
> need that to make up my mind about the urgency of the fix.

Ah, okay. The probability of a user hitting this depends on how likely
they are to perform completion inside a declare form. This depends on
the kind of code the user writes, so it's hard to put a number on it.
Also, if the user has a package like company enabled that performs
completion during idle time, then just moving point through a declare
form should be enough to trigger it, so in this case it might depend
also on the kind of code the user reads. If the user never opens an
elisp file with declare forms, then they certainly won't hit this.

Once the user has triggered the problem via completion, all
compilation of defuns (e.g. during package installation/upgrade) with
declare forms will be broken (as well as loading uncompiled defuns
with declare forms). Admittedly only gv-expander (along with
gv-setter, but that one doesn't get unlearned) is vital to correct
compilation, the rest (advertised-calling-convention obsolete
interactive-only pure side-effect-free compiler-macro doc-string
indent) are only optimizations or advisory in nature.

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Thu, 21 Jul 2016 17:27:49 -0400
> Cc: Michael Heerdegen <michael_heerdegen@web.de>, 23648@debbugs.gnu.org, 
> 	Paul Pogonyshev <pogonyshev@gmail.com>
> 
> > So I'm still looking for the answer to the "how probable" question.  I
> > need that to make up my mind about the urgency of the fix.
> 
> Ah, okay. The probability of a user hitting this depends on how likely
> they are to perform completion inside a declare form. This depends on
> the kind of code the user writes, so it's hard to put a number on it.
> Also, if the user has a package like company enabled that performs
> completion during idle time, then just moving point through a declare
> form should be enough to trigger it, so in this case it might depend
> also on the kind of code the user reads. If the user never opens an
> elisp file with declare forms, then they certainly won't hit this.
> 
> Once the user has triggered the problem via completion, all
> compilation of defuns (e.g. during package installation/upgrade) with
> declare forms will be broken (as well as loading uncompiled defuns
> with declare forms). Admittedly only gv-expander (along with
> gv-setter, but that one doesn't get unlearned) is vital to correct
> compilation, the rest (advertised-calling-convention obsolete
> interactive-only pure side-effect-free compiler-macro doc-string
> indent) are only optimizations or advisory in nature.

OK, let's push to emacs-25.  Thanks.

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[npostavs]

tags 23648 fixed
close 23648 25.1
quit

Eli Zaretskii <eliz@gnu.org> writes:

> OK, let's push to emacs-25.  Thanks.

Push as bc4c07fc.

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[Paul Pogonyshev]

Shouldn't it be also merged to master?

Paul

On 23 July 2016 at 16:18,  <npostavs@users.sourceforge.net> wrote:
> tags 23648 fixed
> close 23648 25.1
> quit
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
>> OK, let's push to emacs-25.  Thanks.
>
> Push as bc4c07fc.

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[Eli Zaretskii]

> From: Paul Pogonyshev <pogonyshev@gmail.com>
> Date: Sun, 24 Jul 2016 19:07:59 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, 23648@debbugs.gnu.org, michael_heerdegen@web.de
> 
> Shouldn't it be also merged to master?

It will be, when the next merge takes place.

bug#23648: [PATCH] `defun-declarations-alist' can be unintentionally modified

[Dmitry Gutov]

On 07/18/2016 06:00 AM, npostavs@users.sourceforge.net wrote:

>> Here `delete-dups' destructively modifies a list that includes
>> `defun-declarations-alist' as its tail verbatim, not as a copy.
>> Attached patch should fix that.
>
> I agree with analysis and patch here.

I like the patch as well. Maybe it would be better to use 
cl-remove-duplicates with nconc instead (the use of nil is a little 
non-obvious), but that would require having cl-lib loaded at runtime, 
and elisp-mode depending on it.

In any case, the proposed patch looks safe.