Possible regression in 1.5 (map destructuring in protocol function arguments)?

243 views
Skip to first unread message

Hugo Duncan

unread,
Feb 13, 2013, 6:03:13 PM2/13/13
to cloju...@googlegroups.com

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

unread,
Feb 13, 2013, 10:27:53 PM2/13/13
to cloju...@googlegroups.com
Hi Hugo,

I have reverted that commit. Thanks for the catch!

Stu



Hugo

--
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to clojure-dev...@googlegroups.com.
To post to this group, send email to cloju...@googlegroups.com.
Visit this group at http://groups.google.com/group/clojure-dev?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.



Tassilo Horn

unread,
Feb 14, 2013, 10:48:22 AM2/14/13
to Stuart Halloway, cloju...@googlegroups.com
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

Baishampayan Ghose

unread,
Feb 14, 2013, 11:03:13 AM2/14/13
to cloju...@googlegroups.com

Tassilo,

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

~BG

Sent from phone. Please excuse brevity.

Hugo Duncan

unread,
Feb 14, 2013, 11:07:43 AM2/14/13
to cloju...@googlegroups.com
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

Tassilo Horn

unread,
Feb 14, 2013, 11:20:20 AM2/14/13
to Baishampayan Ghose, cloju...@googlegroups.com
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

Tassilo Horn

unread,
Feb 14, 2013, 11:24:27 AM2/14/13
to Hugo Duncan, cloju...@googlegroups.com
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
0001-Protocol-interface-method-declarations-don-t-allow-f.patch

Rich Hickey

unread,
Feb 14, 2013, 1:51:15 PM2/14/13
to cloju...@googlegroups.com, Stuart Halloway


On Thursday, February 14, 2013 10:48:22 AM UTC-5, Tassilo Horn wrote:
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.

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.

Rich
 

Tassilo Horn

unread,
Feb 14, 2013, 3:25:19 PM2/14/13
to Rich Hickey, cloju...@googlegroups.com, Stuart Halloway
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
Reply all
Reply to author
Forward
0 new messages