Clojure

Varargs protococol impls can be defined but not called

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Declined
  • Affects Version/s: Release 1.4
  • Fix Version/s: Release 1.5
  • Component/s: None
  • Labels:
  • 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?

Activity

Hide
Tassilo Horn added a comment -

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.

Show
Tassilo Horn added a comment - 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.
Hide
Víctor M. Valenzuela added a comment -

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.

Show
Víctor M. Valenzuela added a comment - 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.
Hide
Tassilo Horn added a comment -

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

Show
Tassilo Horn added a comment - > 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.
Hide
Tassilo Horn added a comment - - edited

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.

Show
Tassilo Horn added a comment - - edited 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.
Tassilo Horn made changes -
Field Original Value New Value
Attachment 0001-CLJ-1024-Check-for-invalid-varags-destrucuring-uses.patch [ 11376 ]
Tassilo Horn made changes -
Labels patch
Hide
Víctor M. Valenzuela added a comment -

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)

Show
Víctor M. Valenzuela added a comment - 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)
Hide
Tassilo Horn added a comment -

Indeed, descructuring seems to work for method implementations. I'll adapt my patch...

Show
Tassilo Horn added a comment - Indeed, descructuring seems to work for method implementations. I'll adapt my patch...
Hide
Tassilo Horn added a comment -

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.

Show
Tassilo Horn added a comment - 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.
Tassilo Horn made changes -
Attachment 0001-CLJ-1024-Check-for-invalid-varags-destrucuring-uses.patch [ 11377 ]
Tassilo Horn made changes -
Attachment 0001-CLJ-1024-Check-for-invalid-varags-destrucuring-uses.patch [ 11376 ]
Hide
Andy Fingerhut added a comment -

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)))

Show
Andy Fingerhut added a comment - 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)))
Hide
Tassilo Horn added a comment -

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.

Show
Tassilo Horn added a comment - 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.
Tassilo Horn made changes -
Attachment 0001-CLJ-1024-Check-for-invalid-varags-destrucuring-uses.patch [ 11380 ]
Tassilo Horn made changes -
Attachment 0001-CLJ-1024-Check-for-invalid-varags-destrucuring-uses.patch [ 11377 ]
Rich Hickey made changes -
Patch Code [ 10001 ]
Approval Vetted [ 10003 ]
Fix Version/s Release 1.5 [ 10150 ]
Hide
Aaron Bedra added a comment -

Patch applies cleanly against 4004d267e124f12b65b0d7fb6522f32a75e3c4fb. Submitter confirmed as a CA signer.

Show
Aaron Bedra added a comment - Patch applies cleanly against 4004d267e124f12b65b0d7fb6522f32a75e3c4fb. Submitter confirmed as a CA signer.
Hide
Aaron Bedra added a comment -

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.

Show
Aaron Bedra added a comment - 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.
Aaron Bedra made changes -
Approval Vetted [ 10003 ] Incomplete [ 10006 ]
Hide
Tassilo Horn added a comment -

Aaron, can you please elaborate? I don't get what you mean with duplication and asking if it is ok to proceed.

Show
Tassilo Horn added a comment - Aaron, can you please elaborate? I don't get what you mean with duplication and asking if it is ok to proceed.
Hide
Tassilo Horn added a comment -

Rebased to apply cleanly on master again.

Show
Tassilo Horn added a comment - Rebased to apply cleanly on master again.
Tassilo Horn made changes -
Attachment 0001-CLJ-1024-Check-for-invalid-varags-destrucuring-uses.patch [ 11462 ]
Tassilo Horn made changes -
Attachment 0001-CLJ-1024-Check-for-invalid-varags-destrucuring-uses.patch [ 11380 ]
Hide
Víctor M. Valenzuela added a comment -

Pardon the silly contribution, but the added methods' usage of double-negations (when-not ...) seems unnecessary.

Show
Víctor M. Valenzuela added a comment - Pardon the silly contribution, but the added methods' usage of double-negations (when-not ...) seems unnecessary.
Hide
Tassilo Horn added a comment -

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.

Show
Tassilo Horn added a comment - 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.
Tassilo Horn made changes -
Tassilo Horn made changes -
Attachment 0001-CLJ-1024-Check-for-invalid-varags-destrucuring-uses.patch [ 11462 ]
Stuart Halloway made changes -
Approval Incomplete [ 10006 ] Screened [ 10004 ]
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Stuart Halloway made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Closed [ 6 ]
Stuart Halloway made changes -
Assignee Stuart Halloway [ stu ]
Status Closed [ 6 ] Reopened [ 4 ]
Resolution Completed [ 1 ]
Stuart Halloway made changes -
Approval Ok [ 10007 ] Not Approved [ 10008 ]
Issue Type Defect [ 1 ] Enhancement [ 4 ]
Stuart Halloway made changes -
Resolution Declined [ 2 ]
Status Reopened [ 4 ] Closed [ 6 ]
Hide
Tassilo Horn added a comment -

Hey Stu, do you mind to explain why you've declined the patch?

Show
Tassilo Horn added a comment - Hey Stu, do you mind to explain why you've declined the patch?
Hide
Tassilo Horn added a comment -

@Marek, Stu: Thanks, I've left a reply there: https://groups.google.com/d/msg/clojure-dev/qjkW-cv8nog/rMNFqbjNj-EJ

Show
Tassilo Horn added a comment - @Marek, Stu: Thanks, I've left a reply there: https://groups.google.com/d/msg/clojure-dev/qjkW-cv8nog/rMNFqbjNj-EJ

People

Vote (1)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: