Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Stefan Monnier]

>     Move the ‘declare’ form before the interactive spec in 10 functions.

BTW, I think we should do one of two things:
- Make sure all orderings work.
- Detect the wrong order and emit a warning when compiling the code.


        Stefan

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Juri Linkov]

>>     Move the ‘declare’ form before the interactive spec in 10 functions.
>
> BTW, I think we should do one of two things:
> - Make sure all orderings work.
> - Detect the wrong order and emit a warning when compiling the code.

The second option looks safer because false positives would have
less drastic effect.

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Lars Ingebrigtsen]

This leads to:

  ELC      mh-e/mh-speed.elc
Warning: Unknown defun property `ignore' in mh-speed-toggle
Warning: Unknown defun property `ignore' in mh-speed-view

There's apparently (declare (ignore args)) stuff there, which isn't
really valid, and is being picked up now that the declare is being
parsed.

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

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Juri Linkov]

> This leads to:
>
>   ELC      mh-e/mh-speed.elc
> Warning: Unknown defun property `ignore' in mh-speed-toggle
> Warning: Unknown defun property `ignore' in mh-speed-view
>
> There's apparently (declare (ignore args)) stuff there, which isn't
> really valid, and is being picked up now that the declare is being
> parsed.

This is very strange - I see no warnings.

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Juri Linkov]

>> This leads to:
>>
>>   ELC      mh-e/mh-speed.elc
>> Warning: Unknown defun property `ignore' in mh-speed-toggle
>> Warning: Unknown defun property `ignore' in mh-speed-view
>>
>> There's apparently (declare (ignore args)) stuff there, which isn't
>> really valid, and is being picked up now that the declare is being
>> parsed.
>
> This is very strange - I see no warnings.

Sorry, I didn't notice warnings because warnings are highlighted
with blue color, not red or orange as one would expect.

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Dmitry Gutov]

On 20.01.2021 23:50, Juri Linkov wrote:
> Sorry, I didn't notice warnings because warnings are highlighted
> with blue color, not red or orange as one would expect.

Sounds like https://debbugs.gnu.org/29192.

Thanks for the change, by the way. I tried to get this right, but 
apparently looked at the wrong examples.

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Juri Linkov]

>> Sorry, I didn't notice warnings because warnings are highlighted
>> with blue color, not red or orange as one would expect.
>
> Sounds like https://debbugs.gnu.org/29192.

It seems the right fix is to change the format of the
"Unknown defun property" warning to the same format
whose highlighting is already supported, i.e. with

  filename:line-number:column-number: Warning: Unknown defun property

> Thanks for the change, by the way. I tried to get this right, but
> apparently looked at the wrong examples.

Indeed, there were many misleading examples.

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Juri Linkov]

> It seems the right fix is to change the format of the
> "Unknown defun property" warning to the same format
> whose highlighting is already supported, i.e. with
>
>   filename:line-number:column-number: Warning: Unknown defun property

But the problem is that filename is unknown here.
Also to support all existing warnings without filename
maybe a new rule should be added:

diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index a92592bd1e..f342c77a3d 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -404,6 +404,9 @@ compilation-error-regexp-alist-alist
           (regexp "[0-9][0-9][0-9]")))
      1 (2 . 4) (3 . 5) (6 . 7))
 
+    (gnu-emacs
+     "^\\(Warning\\):" 1 nil nil 1)
+
     (cucumber
      ,(rx (| (: bol
                 (| (: "cucumber" (? " -p " (+ (not space))))

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Stefan Monnier]

>> It seems the right fix is to change the format of the
>> "Unknown defun property" warning to the same format
>> whose highlighting is already supported, i.e. with
>>
>>   filename:line-number:column-number: Warning: Unknown defun property
>
> But the problem is that filename is unknown here.

Traditionally, you can work around this by using "<unknown>" or
"<stdin>" or some such pseudo-filename.

> +    (gnu-emacs
> +     "^\\(Warning\\):" 1 nil nil 1)

I'd much prefer if Emacs followed the GNU Coding Standard format for
error messages.  Dog food and all that.


        Stefan

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Juri Linkov]

>>> It seems the right fix is to change the format of the
>>> "Unknown defun property" warning to the same format
>>> whose highlighting is already supported, i.e. with
>>>
>>>   filename:line-number:column-number: Warning: Unknown defun property
>>
>> But the problem is that filename is unknown here.
>
> Traditionally, you can work around this by using "<unknown>" or
> "<stdin>" or some such pseudo-filename.

Then maybe to use a function name as a file name?

diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index 0f8dd5a284..619bcf9278 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -301,8 +301,9 @@ defun
                                 (cdr body)
                               body)))
                     nil)
-                   (t (message "Warning: Unknown defun property `%S' in %S"
-                               (car x) name)))))
+                   (t (message "%S:%d:Warning: Unknown defun property `%S'"
+                               name (count-lines (point-min) (point))
+                               (car x))))))
                    decls))
           (def (list 'defalias
                      (list 'quote name)

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Stefan Monnier]

> -                   (t (message "Warning: Unknown defun property `%S' in %S"
> -                               (car x) name)))))
> +                   (t (message "%S:%d:Warning: Unknown defun property `%S'"
> +                               name (count-lines (point-min) (point))
> +                               (car x))))))

I think `count-lines` is a problem because we don't know in which buffer
we are when this is executed.

Another approach could be to use something like
`macroexp--warn-and-return`.


        Stefan

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Juri Linkov]

>> -                   (t (message "Warning: Unknown defun property `%S' in %S"
>> -                               (car x) name)))))
>> +                   (t (message "%S:%d:Warning: Unknown defun property `%S'"
>> +                               name (count-lines (point-min) (point))
>> +                               (car x))))))
>
> I think `count-lines` is a problem because we don't know in which buffer
> we are when this is executed.

The GNU Coding Standard format requires a line number between colons,
so maybe count-lines would give some plausible number in most cases.

> Another approach could be to use something like
> `macroexp--warn-and-return`.

`macroexp--warn-and-return` uses `load-file-name` that is nil here.
Or do you mean the docstring of `macroexp--funcall-if-compiled`:

  The purpose is to delay warnings to bytecomp.el, so they can use things
  like `byte-compile-warn' to get better file-and-line-number data
  and also to avoid outputting the warning during normal execution.

I tried it, but it has no effect.

  (let ((warnfun (lambda ()
                   (message "Warning: Unknown defun property `%S' in %S"
                            (car x) name))))
    `(progn
       (macroexp--funcall-if-compiled ',warnfun)))

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Stefan Monnier]

>>> -                   (t (message "Warning: Unknown defun property `%S' in %S"
>>> -                               (car x) name)))))
>>> +                   (t (message "%S:%d:Warning: Unknown defun property `%S'"
>>> +                               name (count-lines (point-min) (point))
>>> +                               (car x))))))
>> I think `count-lines` is a problem because we don't know in which buffer
>> we are when this is executed.
> The GNU Coding Standard format requires a line number between colons,
> so maybe count-lines would give some plausible number in most cases.

But 0 would work as well for that purpose.

>> Another approach could be to use something like
>> `macroexp--warn-and-return`.
> `macroexp--warn-and-return` uses `load-file-name` that is nil here.

Not necessarily, no: when compiling, it will let bytecomp.el emit the
warning which gives proper file and line numbers.

Do we care about the format used when we're not compiling?
[ This is not a rethorical question.  ]

> I tried it, but it has no effect.
>
>   (let ((warnfun (lambda ()
>                    (message "Warning: Unknown defun property `%S' in %S"
>                             (car x) name))))
>     `(progn
>        (macroexp--funcall-if-compiled ',warnfun)))

How did you try it?


        Stefan

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Juri Linkov]

>> The GNU Coding Standard format requires a line number between colons,
>> so maybe count-lines would give some plausible number in most cases.
>
> But 0 would work as well for that purpose.

I don't know if this is the best that could be done:

diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index 0f8dd5a284..a2cbb85412 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -301,7 +301,7 @@ defun
                                 (cdr body)
                               body)))
                     nil)
-                   (t (message "Warning: Unknown defun property `%S' in %S"
+                   (t (message "unknown:0:Warning: Unknown defun property `%S' in %S"
                                (car x) name)))))
                    decls))
           (def (list 'defalias

>>> Another approach could be to use something like
>>> `macroexp--warn-and-return`.
>> `macroexp--warn-and-return` uses `load-file-name` that is nil here.
>
> Not necessarily, no: when compiling, it will let bytecomp.el emit the
> warning which gives proper file and line numbers.
>
> Do we care about the format used when we're not compiling?
> [ This is not a rethorical question.  ]

Indeed, this format is useful only when compiling.

>> I tried it, but it has no effect.
>>
>>   (let ((warnfun (lambda ()
>>                    (message "Warning: Unknown defun property `%S' in %S"
>>                             (car x) name))))
>>     `(progn
>>        (macroexp--funcall-if-compiled ',warnfun)))
>
> How did you try it?

Compiled this, then tried to byte-compile a file with wrong `declare`,
still same warning without line numbers.

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Stefan Monnier]

>>>   (let ((warnfun (lambda ()
>>>                    (message "Warning: Unknown defun property `%S' in %S"
>>>                             (car x) name))))
>>>     `(progn
>>>        (macroexp--funcall-if-compiled ',warnfun)))
>>
>> How did you try it?
>
> Compiled this, then tried to byte-compile a file with wrong `declare`,
> still same warning without line numbers.

Ah, well, you used `message` so you got what you deserved ;-)
If you look at `macroexp--warn-and-return` it uses `byte-compile-warn`.
BTW, did you you use `macroexp--funcall-if-compiled` instead of
`macroexp--warn-and-return` because of bootstrapping concerns?


        Stefan

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Juri Linkov]

>>>>   (let ((warnfun (lambda ()
>>>>                    (message "Warning: Unknown defun property `%S' in %S"
>>>>                             (car x) name))))
>>>>     `(progn
>>>>        (macroexp--funcall-if-compiled ',warnfun)))
>>>
>>> How did you try it?
>>
>> Compiled this, then tried to byte-compile a file with wrong `declare`,
>> still same warning without line numbers.
>
> Ah, well, you used `message` so you got what you deserved ;-)
> If you look at `macroexp--warn-and-return` it uses `byte-compile-warn`.
> BTW, did you you use `macroexp--funcall-if-compiled` instead of
> `macroexp--warn-and-return` because of bootstrapping concerns?

Because I don't know what to give to the second arg FORM of
'macroexp--warn-and-return'.  I tried to use 'name' for FORM,
but then it prints some irrelevant warning:

  mh-e/mh-speed.el:161:25: Warning: reference to free variable `mh-speed-toggle'

But with this patch that uses `byte-compile-warn`:

diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index 0f8dd5a284..bf9f3f3fbc 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -301,8 +301,12 @@ defun
                                 (cdr body)
                               body)))
                     nil)
-                   (t (message "Warning: Unknown defun property `%S' in %S"
-                               (car x) name)))))
+                   (t (let ((warnfun (lambda ()
+                                       (byte-compile-warn
+                                        "Unknown defun property `%S' in %S"
+                                        (car x) name))))
+                        `(progn
+                           (macroexp--funcall-if-compiled ',warnfun)))))))
                    decls))
           (def (list 'defalias
                      (list 'quote name)

these warnings finally look right:

    ELC      mh-e/mh-speed.elc
  In toplevel form:
  mh-e/mh-speed.el:161:25: Warning: Unknown defun property `ignore' in
      mh-speed-toggle
  mh-e/mh-speed.el:179:9: Warning: Unknown defun property `ignore' in
      mh-speed-view

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Stefan Monnier]

> Because I don't know what to give to the second arg FORM of
> 'macroexp--warn-and-return'.

I think `nil` is the right answer: the (unknown) declaration expands to `nil`.


        Stefan

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Juri Linkov]

>> Because I don't know what to give to the second arg FORM of
>> 'macroexp--warn-and-return'.
>
> I think `nil` is the right answer: the (unknown) declaration expands to `nil`.

`nil` doesn't output line numbers:

  (macroexp--warn-and-return
   (format-message "Warning: Unknown defun property `%S' in %S"
                   (car x) name)
   nil)

=>

    ELC      mh-e/mh-speed.elc
  Warning: Unknown defun property `ignore' in mh-speed-toggle
  Warning: Unknown defun property `ignore' in mh-speed-view

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Stefan Monnier]

>> I think `nil` is the right answer: the (unknown) declaration expands to `nil`.
>
> `nil` doesn't output line numbers:
>
>   (macroexp--warn-and-return
>    (format-message "Warning: Unknown defun property `%S' in %S"
>                    (car x) name)
>    nil)
>
> =>
>
>     ELC      mh-e/mh-speed.elc
>   Warning: Unknown defun property `ignore' in mh-speed-toggle
>   Warning: Unknown defun property `ignore' in mh-speed-view

Hmm... that suggests the (macroexp--compiling-p) test returns nil for
some reason, which points to an error in `macroexp.el`.
[ I see an error in the code when form is nil, but that's unrelated to
  what you're seeing.  ]


        Stefan

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Stefan Monnier]

>     ELC      mh-e/mh-speed.elc
>   Warning: Unknown defun property `ignore' in mh-speed-toggle
>   Warning: Unknown defun property `ignore' in mh-speed-view

OK, I found the culprit (`macroexp-macroexpand` had forgotten to bind
`macroexpand-all-environment`).  I pushed a patch that fixes it (for
`defmacro` as well).


        Stefan

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Juri Linkov]

> This leads to:
>
>   ELC      mh-e/mh-speed.elc
> Warning: Unknown defun property `ignore' in mh-speed-toggle
> Warning: Unknown defun property `ignore' in mh-speed-view
>
> There's apparently (declare (ignore args)) stuff there, which isn't
> really valid, and is being picked up now that the declare is being
> parsed.

I wonder what the intention was to do here, what is the closest
valid thing?  Maybe

  (declare (advertised-calling-convention (&rest ignored) "28.1"))

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> I wonder what the intention was to do here, what is the closest
> valid thing?  Maybe
>
>   (declare (advertised-calling-convention (&rest ignored) "28.1"))

I think just

(defun mh-speed-toggle (&rest _ignored)

would be fine?

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

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Mike Kupfer]

Lars Ingebrigtsen wrote:

> Juri Linkov <juri@linkov.net> writes:
> 
> > I wonder what the intention was to do here

I tried to track this down.  (declare (ignore VAR)) is a Common Lisp
thing, to tell the compiler that VAR is unused in the function (so don't
warn about it being unused).  My CL reference doesn't mention "args" as
a pseudo-variable that could be used with ignore.  But it's an ancient
reference; I don't know what the current standard allows.

Based on iterating with "git blame", it looks like the (declare
(ignore...)) stuff was introduced with MH-E 7.0, which I think was the
first MH-E release that had speedbar support.  I don't know why a CL
construct was used here, or why the arguments are ignored.  Maybe Bill
Wohler remembers.

> > , what is the closest
> > valid thing?  Maybe
> >
> >   (declare (advertised-calling-convention (&rest ignored) "28.1"))
> 
> I think just
> 
> (defun mh-speed-toggle (&rest _ignored)
> 
> would be fine?

Seems reasonable to me.

cheers,
mike

Re: master 5065698: Move the ‘declare’ form before the interactive spec in 10 functions.

[Lars Ingebrigtsen]

Mike Kupfer <mkupfer@alum.berkeley.edu> writes:

>> I think just
>> 
>> (defun mh-speed-toggle (&rest _ignored)
>> 
>> would be fine?
>
> Seems reasonable to me.

OK; done on the trunk now.

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