[CLJ-1024] Varargs protococol impls can be defined but not called Created: 10/Jul/12 Updated: 14/Feb/13 Resolved: 13/Feb/13 |
|
| Status: | Closed |
| Project: | Clojure |
| Component/s: | None |
| Affects Version/s: | Release 1.4 |
| Fix Version/s: | Release 1.5 |
| Type: | Enhancement | Priority: | Minor |
| Reporter: | Víctor M. Valenzuela | Assignee: | Stuart Halloway |
| Resolution: | Declined | Votes: | 1 |
| Labels: | patch | ||
| Attachments: |
|
| Patch: | Code |
| Approval: | Not Approved |
| Description |
|
The compiler accepts this: (deftype foo [] However calling ((foo.) :bar) will throw an AbstractMethodError. Wouldn't some checking be desirable? |
| Comments |
| Comment by Tassilo Horn [ 11/Jul/12 1:20 AM ] |
|
First of all, clojure.lang.IFn is no protocol but an interface. And it does not declare a public Object invoke(Object... obs) method. It has an `invoke` method with 20 Object parameters followed by an Object... parameter, but to give an implementation for that, you have to specify every parameter separately, and the last Object... arg is just a normal argument that must be an Object[]. That's because Java-varags Type... parameters are just Java syntax sugar, but in the byte-code its simply a Type-array. What your example does is provide an `invoke` implementation for the 2-args version, where the first parameter happens to be named `&`, which has no special meaning here. Take that example: (deftype MyFoo []
clojure.lang.IFn
(invoke [this & xs]
[& xs]))
((MyFoo.) 1 2)
=> [1 2]
But you are right in that `deftype`, `defrecord`, `defprotocol`, and `definferface` probably should error if user's seem to try to use varargs or destructuring. |
| Comment by Víctor M. Valenzuela [ 11/Jul/12 1:55 AM ] |
|
Cheers for a most clarifying response. The fact that the meaning of & gets 'subverted' makes the issue only (slightly) worse, in my opinion. Just for the record, destructuring seems to work, at least for interface impls. |
| Comment by Tassilo Horn [ 11/Jul/12 2:42 AM ] |
|
> The fact that the meaning of & gets 'subverted' makes the issue only (slightly) worse, in my opinion. I agree. I'll attach a patch which checks for those invalid usages soon. > Just for the record, destructuring seems to work, at least for interface impls. Could you please provide a complete example demonstrating your statement? I'm rather sure that varargs and destructuring don't work for any of defprotocol, definterface, deftype, defrecord, and reify. But you can use varargs and destructuring when providing dynamic implementations via `extend` (or `extend-protocol`, `extend-type`), because those impls are real functions in contrast to methods. |
| Comment by Tassilo Horn [ 11/Jul/12 2:43 AM ] |
|
I attached a patch. Here's the commit message: Check for invalid varags/destrucuring uses. Protocol, interface method declarations and implementations don't allow for (defprotocol FooBar compiles just fine, and & is interpreted as a usual argument that happens to be Similarly, providing method implementations via defrecord, deftype, and reify So this patch makes defprotocol, definterface, defrecord, deftype, and reify |
| Comment by Víctor M. Valenzuela [ 12/Jul/12 3:13 AM ] |
|
Glad you've considered my request. As for destructuring, I was speaking after this example, which may or may not work like it looks like - I don't know. (deftype foo [] ((foo.) [1 2] 3) |
| Comment by Tassilo Horn [ 12/Jul/12 8:22 AM ] |
|
Indeed, descructuring seems to work for method implementations. I'll adapt my patch... |
| Comment by Tassilo Horn [ 12/Jul/12 8:42 AM ] |
|
Revamped patch. Here's the commit message: Protocol, interface method declarations don't allow for varags and (defprotocol FooBar
(foo [this & more]))
compiles just fine, and & is interpreted as a usual argument that happens to be Similarly, providing method implementations via defrecord, deftype, and reify So this patch makes defprotocol and definterface throw an Similarly, defrecord, deftype, and reify throw an IllegalArgumentException if |
| Comment by Andy Fingerhut [ 12/Jul/12 3:43 PM ] |
|
Tassilo, with your patch 0001- [java] Testing clojure.test-clojure.metadata |
| Comment by Tassilo Horn [ 13/Jul/12 2:10 AM ] |
|
Hi Andy, this updated patch declares the two new checking fns private which makes the tests pass again. Stupid mistake by me: Of course, I've tested the last version, too, but afterwards I decided it wouldn't be bad to add some docstrings, and of course, adding docstrings cannot break anything. |
| Comment by Aaron Bedra [ 14/Aug/12 7:58 PM ] |
|
Patch applies cleanly against 4004d267e124f12b65b0d7fb6522f32a75e3c4fb. Submitter confirmed as a CA signer. |
| Comment by Aaron Bedra [ 14/Aug/12 8:02 PM ] |
|
This looks ok to me, but it seems like a fair amount of duplication to accomplish the task. It seems like we should just be able to ask if it is ok to proceed, instead of having to pick which function needs to be called in what case. |
| Comment by Tassilo Horn [ 15/Aug/12 1:23 AM ] |
|
Aaron, can you please elaborate? I don't get what you mean with duplication and asking if it is ok to proceed. |
| Comment by Tassilo Horn [ 31/Aug/12 1:40 AM ] |
|
Rebased to apply cleanly on master again. |
| Comment by Víctor M. Valenzuela [ 31/Aug/12 7:11 AM ] |
|
Pardon the silly contribution, but the added methods' usage of double-negations (when-not ...) seems unnecessary. |
| Comment by Tassilo Horn [ 31/Aug/12 8:03 AM ] |
|
Hi Victor, this revamped patch removes the double-negation and is more concise and clearer as a result. So your comment wasn't as silly as you thought. |
| Comment by Tassilo Horn [ 14/Feb/13 1:36 AM ] |
|
Hey Stu, do you mind to explain why you've declined the patch? |
| Comment by Marek Srank [ 14/Feb/13 8:52 AM ] |
|
@Tassilo: https://groups.google.com/forum/#!topic/clojure-dev/qjkW-cv8nog |
| Comment by Tassilo Horn [ 14/Feb/13 10:03 AM ] |
|
@Marek, Stu: Thanks, I've left a reply there: https://groups.google.com/d/msg/clojure-dev/qjkW-cv8nog/rMNFqbjNj-EJ |