I just tried running RC15 on pallet and came up against this
(simplified):

  (defprotocol P (f [_ {:keys [k]}] "using map destructuring"))

The protocol can be implemented and functions correctly with 1.4, but
raises a compile error in 1.5.

  "No varargs nor destructuring support for definterface and defprotocol
  method sigs."

The relevant code in 1.5 was introduced in CLJ-1024, which seemed to
consider vector destructuring, but not map destructuring.

Looking at the implementation of protocols, I can't see any issue with
supporting destructuring, and it is useful in documenting the expected
argument usage.

Is this a regression?

Hugo

Stuart Halloway <stuart....@gmail.com> writes:

Hi Hugo & Stu,

> I have reverted that commit. Thanks for the catch!

IMO, the commit has still its purpose, else it wouldn't have been
accepted in the first place.  See below.

>>
>> I just tried running RC15 on pallet and came up against this
>> (simplified):
>>
>>   (defprotocol P (f [_ {:keys [k]}] "using map destructuring"))
>>
>> The protocol can be implemented and functions correctly with 1.4, but
>> raises a compile error in 1.5.
>>
>>   "No varargs nor destructuring support for definterface and defprotocol
>>   method sigs."

The rational for my patch that produces this error is that destructuring
makes no sense at protocol and interface method declarations.

  (defprotocol P (f [_ {:keys [k]}]))

is identical to

  (defprotocol P (f [_ some-map]))

in that it creates an interface with f-method with one argument.  Since
its just a declaration of a method and not a function definition,
there's no such thing as destructuring.

The problem becomes more important with varargs.

  (defprotocol P (f [this & that]))

creates a P interface with a method f with 2 arguments.  The first
argument is & and the second is that.  The &-sign has not its usual
clojure function varargs meaning here.  It's a Java method declaration,
not a function definition.  Thus the & suggest I could call

  (f my-p-obj 1 2 3 4 5 6 7)

but that will produce and ArityException.

My commit message summarized the issue as follows:

--8<---------------cut here---------------start------------->8---
Protocol, interface method declarations don't allow for varags and
destructuring support. Currently, for example

  (defprotocol FooBar
    (foo [this & more]))

compiles just fine, and & is interpreted as a usual argument that
happens to be named & without special meaning. But clearly, the user
wanted to specify a varags parameter here. The same applies to
definterface.

Similarly, providing method implementations via defrecord, deftype, and
reify don't allow for varags (but dynamic extenions via extend do).

So this patch makes defprotocol and definterface throw an
IllegalArgumentException if a user tries to use varargs and
destructuring in method signatures.

Similarly, defrecord, deftype, and reify throw an
IllegalArgumentException if any method implementation arglist contains a
varargs argument.
--8<---------------cut here---------------end--------------->8---

I have to admit that forbidding destructuring in definterface/
defprotocol method declarations might have been to much.  Although it
has no semantics here, it might serve as documentation (e.g., the map
given as argument must have a :k key in Hugo's example).  And it's not
wrong, cause all destructuring forms are one single form, thus they
stand for one parameter.

But varargs in defprotocol/definterface method declarations and method
impls defined with deftype, defrecord, and reify are certainly wrong.
Those are Java methods that don't know anything about the Clojure
varargs semantics of the &-sign.

Stu, would you accept a modified patch that only deals with the varags
issue?

Bye,
Tassilo

Tassilo,

What do you think about emitting an warning instead of throwing an exception?

~BG

Sent from phone. Please excuse brevity.

Tassilo Horn <ts...@gnu.org> writes:

> IMO, the commit has still its purpose, else it wouldn't have been
> accepted in the first place.  See below.

I agree w.r.t varargs.

> The rational for my patch that produces this error is that destructuring
> makes no sense at protocol and interface method declarations.
>
>   (defprotocol P (f [_ {:keys [k]}]))
>
> is identical to
>
>   (defprotocol P (f [_ some-map]))
>
> in that it creates an interface with f-method with one argument.  Since
> its just a declaration of a method and not a function definition,
> there's no such thing as destructuring.

I find that it still serves to document the expected keys passed to an argument.

Hugo

Baishampayan Ghose <b.g...@gmail.com> writes:

Hi!

> What do you think about emitting an warning instead of throwing an
> exception?

I could live with that, tho I think suggesting varargs support where it
doesn't work is producing so much confusion among users of the protocol
or interface that an exception is justified.

Bye,
Tassilo

Hugo Duncan <dunca...@gmail.com> writes:

Hi Hugo,

>> IMO, the commit has still its purpose, else it wouldn't have been
>> accepted in the first place.  See below.
>
> I agree w.r.t varargs.

Great.  Attached is a new version of the patch that drops the
destructuring checks completely and only deals with invalid varargs
uses.

>> The rational for my patch that produces this error is that
>> destructuring makes no sense at protocol and interface method
>> declarations.
>>
>>   (defprotocol P (f [_ {:keys [k]}]))
>>
>> is identical to
>>
>>   (defprotocol P (f [_ some-map]))
>>
>> in that it creates an interface with f-method with one argument.
>> Since its just a declaration of a method and not a function
>> definition, there's no such thing as destructuring.
>
> I find that it still serves to document the expected keys passed to an
> argument.

Yes, I agree.  That's why I've dropped that part of the patch.

Bye,
Tassilo

Rich Hickey <richh...@gmail.com> writes:

>> > I have reverted that commit. Thanks for the catch!
>>
>> IMO, the commit has still its purpose, else it wouldn't have been
>> accepted in the first place.  See below.
>>
>> Stu, would you accept a modified patch that only deals with the varags
>> issue?
>
> Not for 1.5. It will have to wait at this point.

Ok, no problem.  Since the original JIRA issue has been changed to
closed & declined, should I open a new issue or simply bring up the
topic again on this list once 1.5 is out?

Bye,
Tassilo