defprotocol should throw error when signatures include variable number of parameters

Question

I tried to use & in the signature for a method in defprotocol. Apparently (see below), this is compiled so that & becomes a simple parameter name, and there is no special handling for variable number of parameters. I think the use of & in a protocol signature ought to be detected and immediately cause an exception (I also think this restriction on the signatures ought to be documented; I couldn't find it specified in the current documentation, though of course it is implied (as I later realized) by the fact that defprotocol creates a Java interface).

user=> (defprotocol Applier (app (link: this f & args)))
Applier
user=> (deftype A (link: ) Applier (app (link: _ f & args) (prn f & args) (apply f args)))
user.A
user=> (app (A.) + 1 2)

<core$PLUS clojure.core$PLUS@5d9d0d20> 1 2

IllegalArgumentException Don't know how to create ISeq from: java.lang.Long
clojure.lang.RT.seqFrom (RT.java:487)

Answer 1

Comment made by: coventry

Patch with test code attached. I have it throwing a CompilerException so that it shows source code location. Not sure whether this is kosher in clojure code, but I wish more macros provided this in their error handling.

Answer 2

Comment made by: tsdh

This issue has already been discussed in CLJ-1024. There I provided a patch that forbids varargs and destructuring forms at various places including defprotocol/definterface. My patch had been applied shortly before clojure 1.5 was released, but it had a bug (forbid too many uses), so it got reverted and the bug closed and declined.

I was told to bring up the issue again after 1.5 has been released.

So here is my patch again. This time it's much more relaxed and only forbids varargs in defprotocol/definterface method declarations, and in deftype/defrecord and reify method implementations.

Answer 3

Comment made by: coventry

Thanks, Tassilo. If there's anywhere in the JIRA system where I could check for prior work like that for other similar issues, I'd be grateful for a pointer.

Best regards,
Alex

Answer 4

Comment made by: tsdh

New version of my patch.

Now I use a CompilerException with proper file/line/column information like Alex did. I also added his test case (which passes).

Concerning your question, Alex: a search for "varargs" would have listed CLJ-1024, but probably you wouldn't have looked into it anyway, because it's a closed issue...

Answer 5

Comment made by: tsdh

Alex, if you don't object could we remove your patch in favor of mine which covers a bit more cases?

Answer 6

Comment made by: coventry

Yep. Just read through 1024 and the associated mailing list discussion. You should totally get the credit: Your patch is more comprehensive and you have been on this a long time. :-) Thanks for folding in the good parts of my patch.

Best regards,
Alex

Answer 7

Comment made by: tsdh

Ok, great. :-)

It seems I don't have the permissions to delete other peoples' attachments, so could you please delete your patch yourself?

Answer 8

Comment made by: coventry

Sure, Tassilo. It's done.

I think this also needs a regression test for the case hugod originally pointed out. I initially made the same mistake as you there, but amalloy pointed it out(link: 1) before I submitted the patch, so it is a natural mistake to make and should probably be documented in the source code.

Best regards,
Alex

(link: 1) http://logs.lazybot.org/irc.freenode.net/#clojure/2013-10-21.txt search for 14:48:34.

Answer 9

Comment made by: tsdh

Alex, I've added the regression test you suggested. Thanks for pointing that out.

Also, I added tests checking definterface method declarations, and tests checking inline method implementations made with defrecord, deftype, and reify.

However, there's a problem with the tests for deftype and reify I don't know how to fix. When I eval the macroexpand forms used in the tests in a REPL, I can see that the CompilerException is successfully thrown and printed. But it also seems to be caught somewhere in the middle, so that the macroexpand returns a form and the exception doesn't make it to the (is (thrown? ...)). Therefore, I've commented the these tests and added a big FIXME.

Answer 10

Comment made by: tsdh

New version of the patch with now all tests uncommented and passing. Andy Fingerhut made me aware that for the 4 deftype and reify tests, I need eval instead of just macroexpand.

Answer 11

Comment made by: jafingerhut

I have not investigated the reason yet, but patch 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch no longer applies cleanly after the latest commits to Clojure master on Oct 25 2013.

Answer 12

Comment made by: tsdh

I've rebased the patch onto the current master so that it applies cleanly again.

Answer 13

Comment made by: tsdh

Stu, I've assigned this issue to you because you've been assigned to CLJ-1165 which I have closed as duplicate of this issue.

One minor difference between my patch to this issue and CLJ-1165 is that here I use a CompilerException with file/line/column info whereas in CLJ-1165 I've used ex-info. I think the CE is more appropriate/informative, as the error is already triggered during macro expansion.

Answer 14

Comment made by: michaelblume

Rebased onto master

Answer 15

Comment made by: michaelblume

Be nice if we got this merged -- I just got a pull request using a varargs protocol that seemed to work by accident because the only time the varargs arity was called, it was called with two additional arguments, matching the '& and the 'args in the interface. Confused the hell out of me before I worked out what was going on.