bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

[Eric Abrahamsen]

If you write a generic method and try to specialize on an &optional
argument, like so:

--8<---------------cut here---------------start------------->8---
(cl-defgeneric testy (arg1 &optional arg2))

(cl-defmethod testy ((arg1 string) &optional (arg2 integer))
  (message "Don't do this"))
--8<---------------cut here---------------end--------------->8---

It will behave weirdly when you try to call the function with a second
argument, but won't bark at you when you evaluate this form, and during
compilation will give you the error:

Warning: reference to free variable 'integer'

I was doing this with a specialization on an EIEIO class in EBDB, and
the warning was even weirder, but never mind that.

A few things could be done here:

- the manual section on "Generic Functions" could say explicitly that
  you can't specialize on optional arguments
- `eval'ling the `defmethod' form above could raise an error directly
- the compiler could say more explicitly what the problem is

I'd favor all three of these changes! Happy to implement what I can
(maybe not the compiler part).

Eric

bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> - the manual section on "Generic Functions" could say explicitly that
>   you can't specialize on optional arguments
> - `eval'ling the `defmethod' form above could raise an error directly
> - the compiler could say more explicitly what the problem is
>
> I'd favor all three of these changes!

Me, too.

> Happy to implement what I can (maybe not the compiler part).

I've added Stefan to the CCs; perhaps he has some comments.

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

bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

[Eric Abrahamsen]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> - the manual section on "Generic Functions" could say explicitly that
>>   you can't specialize on optional arguments
>> - `eval'ling the `defmethod' form above could raise an error directly
>> - the compiler could say more explicitly what the problem is
>>
>> I'd favor all three of these changes!
>
> Me, too.

Here's a very simply manual patch.

I squinted long and hard at cl-generic.el, and I'm not confident about where
we'd put the error. In `cl--generic-lambda'?

>> Happy to implement what I can (maybe not the compiler part).
>
> I've added Stefan to the CCs; perhaps he has some comments.

He has already (kindly) made fun of me for my dumb code, so he ought to
be aware. :)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dont-specialize-optional-args.diff --]
[-- Type: text/x-patch, Size: 554 bytes --]

diff --git a/doc/lispref/functions.texi b/doc/lispref/functions.texi
index 77d1465c87..6d4a93b08a 100644
--- a/doc/lispref/functions.texi
+++ b/doc/lispref/functions.texi
@@ -1208,6 +1208,8 @@ Generic Functions
 Extensions for GNU Emacs Lisp}), or of one of its child classes.
 @end table
 
+Arguments marked as &optional can't be specialized on.
+
 Method definitions can make use of a new argument-list keyword,
 @code{&context}, which introduces extra specializers that test the
 environment at the time the method is run.  This keyword should appear

bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eric Abrahamsen [2021-08-26 18:31:44] wrote:
> (cl-defmethod testy ((arg1 string) &optional (arg2 integer))
>   (message "Don't do this"))
[...]
> Warning: reference to free variable 'integer'

Hmm....

The same thing happens in Common-Lisp, tho.  What's going on here is
that the arglist of `cl-defmethod` allows Common-Lisp style arguments
(e.g. `&key`) so your `(arg2 integer)` means to use `integer` as the
default value for `arg2`, just as in

    (cl-defun testy (arg1 &optional (arg2 integer)) ...)


-- Stefan

bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

[Lars Ingebrigtsen]

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

> Eric Abrahamsen [2021-08-26 18:31:44] wrote:
>> (cl-defmethod testy ((arg1 string) &optional (arg2 integer))
>>   (message "Don't do this"))
> [...]
>> Warning: reference to free variable 'integer'
>
> Hmm....
>
> The same thing happens in Common-Lisp, tho.  What's going on here is
> that the arglist of `cl-defmethod` allows Common-Lisp style arguments
> (e.g. `&key`) so your `(arg2 integer)` means to use `integer` as the
> default value for `arg2`, just as in
>
>     (cl-defun testy (arg1 &optional (arg2 integer)) ...)

Hm...  So the warning here is correct, I guess.  But perhaps it could
also have said something about it being ambiguous syntax (since is also
knows the declared parameter list from `cl-defgeneric')...

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

bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Hm...  So the warning here is correct, I guess.  But perhaps it could
> also have said something about it being ambiguous syntax (since is also
> knows the declared parameter list from `cl-defgeneric')...

[ I don't see how the arglist of `cl-defgeneric` would have helped here
  discover the confusion.  ]

Or we could deprecate the use of Common-Lisp style arglists in
cl-defmethod.  I don't know how often it's used, tho.


        Stefan

bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

[Lars Ingebrigtsen]

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

>> Hm...  So the warning here is correct, I guess.  But perhaps it could
>> also have said something about it being ambiguous syntax (since is also
>> knows the declared parameter list from `cl-defgeneric')...
>
> [ I don't see how the arglist of `cl-defgeneric` would have helped here
>   discover the confusion.  ]

I may be misremembering the semantics of defgeneric, but I thought it
was fine to say:

(cl-defgeneric zot (a &optional b)
  )

But this means that you can only specialise on a -- b is an optional
argument.  So

(cl-defmethod zot ((a integer) &optional (b "foo"))
  (list a b))

is fine and valid -- it's a default value for b.

(cl-defmethod foo ((a integer) &optional (b string))
  ...)

on the other hand, looks like the person who wrote it wanted to
specialise on b, so if the default is something that is a type
specifier, then we could output an additional warning about that.

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

bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> (cl-defmethod foo ((a integer) &optional (b string))
>   ...)
>
> on the other hand, looks like the person who wrote it wanted to
> specialise on b, so if the default is something that is a type
> specifier, then we could output an additional warning about that.

The default value part of args in Common lisp arglists is an
*expression*, so (b string) just means that `b` should take the value of
the `string` variable.

The best I can see (if we keep the CL arglist feature) is to try and see
if that expression looks like a valid CLOS specializer and if so emit
a warning about possible confusion.

I wonder what Common Lisp compilers do.


        Stefan

bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

[Lars Ingebrigtsen]

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

> The default value part of args in Common lisp arglists is an
> *expression*, so (b string) just means that `b` should take the value of
> the `string` variable.
>
> The best I can see (if we keep the CL arglist feature) is to try and see
> if that expression looks like a valid CLOS specializer and if so emit
> a warning about possible confusion.

Yes, that's what I tried to say.  :-)

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

bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

[Eric Abrahamsen]

On 08/28/21 13:41 PM, Stefan Monnier wrote:
>> (cl-defmethod foo ((a integer) &optional (b string))
>>   ...)
>>
>> on the other hand, looks like the person who wrote it wanted to
>> specialise on b, so if the default is something that is a type
>> specifier, then we could output an additional warning about that.
>
> The default value part of args in Common lisp arglists is an
> *expression*, so (b string) just means that `b` should take the value of
> the `string` variable.
>
> The best I can see (if we keep the CL arglist feature) is to try and see
> if that expression looks like a valid CLOS specializer and if so emit
> a warning about possible confusion.

That's a pretty byzantine set of rules! Anyway, I think "reference to
free variable string" is probably close enough to clue the developer in
to what is happening. In the case of an EIEIO class name, though, the
error was:

"`ebdb-record' is an obsolete variable, use `ebdb-record' instead"

which is just...

So maybe something special for class and struct types would be sufficient?

bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

[Michael Heerdegen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> "`ebdb-record' is an obsolete variable, use `ebdb-record' instead"

This sounds like Bug#39169.  Did I forget to commit the fix I had
posted there?

Michael.

bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

[Eric Abrahamsen]

On 08/29/21 13:10 PM, Michael Heerdegen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> "`ebdb-record' is an obsolete variable, use `ebdb-record' instead"
>
> This sounds like Bug#39169.  Did I forget to commit the fix I had
> posted there?

I thought I'd seen discussion about this before! The code you described
in that bug report did get committed, so I can only assume this is a
separate but look-alike situation.

bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

[Michael Heerdegen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> > This sounds like Bug#39169.  Did I forget to commit the fix I had
> > posted there?
>
> I thought I'd seen discussion about this before! The code you described
> in that bug report did get committed, so I can only assume this is a
> separate but look-alike situation.

Please search for the string "use \\='%s" in eieio-core.el.  We made one
of three occurrences less confusing, seems you hit some other (2/3?).  I
guess we want to change this one as well.

AFAIR you can also just disable `eieio-backward-compatibility' (file or
directory locally).

Michael.

bug#50219: 28.0.50; Provide better errors when trying to specialize on optional args in generic methods

[Eric Abrahamsen]

On 08/29/21 21:16 PM, Michael Heerdegen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> > This sounds like Bug#39169.  Did I forget to commit the fix I had
>> > posted there?
>>
>> I thought I'd seen discussion about this before! The code you described
>> in that bug report did get committed, so I can only assume this is a
>> separate but look-alike situation.
>
> Please search for the string "use \\='%s" in eieio-core.el.  We made one
> of three occurrences less confusing, seems you hit some other (2/3?).  I
> guess we want to change this one as well.

Both the line 343 and line 423 sites do the same thing: define the class
symbol as an obsolete variable, and recommend using the quoted symbol
instead.

This is applicable to most situations where people are specifically
trying to do something with the class, but in this case (the &optional
args) the method is supposed to be doing something with the symbol-value
of a (here unquoted) symbol (I was using it wrong). I can't think of a
situation where it would make sense to be using the class symbol in this
way, so I think the proper place to raise an error is in cl-generic.el,
not in the class definitions here.

> AFAIR you can also just disable `eieio-backward-compatibility' (file or
> directory locally).

But I can't do that for everyone who's compiling EBDB locally (on
package update, etc).