<< Back to previous view

[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: Text File 0001-CLJ-1024-Check-for-invalid-varags-destrucuring-uses.patch    
Patch: Code

 Description   

The compiler accepts this:

(deftype foo []
clojure.lang.IFn
(invoke [this & xs]))

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
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 destructuring and varags (but dynamic extenions via extend do).

So this patch makes defprotocol, definterface, defrecord, deftype, and reify
throw an IllegalArgumentException if any argument vector contains a
destructuring form or varargs argument.

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 []
clojure.lang.IFn
(invoke [this [a b] c] (println a b c)))

((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
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.

Comment by Andy Fingerhut [ 12/Jul/12 3:43 PM ]

Tassilo, with your patch 0001-CLJ-1024-Check-for-invalid-varags-destrucuring-uses.patch dated July 12, 2012, I get the following error message while testing, apparently because some metadata is missing on the new functions your patch adds to core:

[java] Testing clojure.test-clojure.metadata
[java]
[java] FAIL in (public-vars-with-docstrings-have-added) (metadata.clj:45)
[java] expected: (= [] (remove (comp :added meta) public-vars-with-docstrin
gs-not-generated))
[java] actual: (not (= [] (#'clojure.core/throw-on-varargs-and-destr #'cl
ojure.core/throw-on-varargs)))

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

Generated at Sat Nov 22 17:57:36 CST 2014 using JIRA 4.4#649-r158309.