Clojure

defprotocol should throw error when signatures include variable number of parameters

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: Release 1.3
  • Fix Version/s: None
  • Component/s: None
  • Patch:
    Code and Test
  • Approval:
    Triaged

Description

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 [this f & args]))
Applier
user=> (deftype A [] Applier (app [_ 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)

Activity

Alex Miller made changes -
Field Original Value New Value
Labels protocols
Alex Miller made changes -
Approval Triaged [ 10120 ]
Issue Type Enhancement [ 4 ] Defect [ 1 ]
Alex Miller made changes -
Labels protocols errormsgs protocols
Alex Miller made changes -
Priority Minor [ 4 ] Major [ 3 ]
Hide
Alex Coventry added a comment - - edited

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.

Show
Alex Coventry added a comment - - edited 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.
Alex Coventry made changes -
Attachment 0002-Error-msg-for-variadic-syntax-in-defprotocol-sigs.patch [ 12352 ]
Hide
Tassilo Horn added a comment -

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.

Show
Tassilo Horn added a comment - 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.
Tassilo Horn made changes -
Attachment 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch [ 12355 ]
Hide
Alex Coventry added a comment -

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

Show
Alex Coventry added a comment - 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
Hide
Tassilo Horn added a comment -

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

Show
Tassilo Horn added a comment - 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...
Tassilo Horn made changes -
Attachment 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch [ 12356 ]
Tassilo Horn made changes -
Attachment 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch [ 12355 ]
Hide
Tassilo Horn added a comment -

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

Show
Tassilo Horn added a comment - Alex, if you don't object could we remove your patch in favor of mine which covers a bit more cases?
Tassilo Horn made changes -
Patch Code and Test [ 10002 ]
Hide
Alex Coventry added a comment -

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

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

Ok, great.

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

Show
Tassilo Horn added a comment - - edited Ok, great. It seems I don't have the permissions to delete other peoples' attachments, so could you please delete your patch yourself?
Alex Coventry made changes -
Attachment 0002-Error-msg-for-variadic-syntax-in-defprotocol-sigs.patch [ 12352 ]
Hide
Alex Coventry added a comment -

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

[1] http://logs.lazybot.org/irc.freenode.net/%23clojure/2013-10-21.txt search for 14:48:34.

Show
Alex Coventry added a comment - 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[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 [1] http://logs.lazybot.org/irc.freenode.net/%23clojure/2013-10-21.txt search for 14:48:34.
Hide
Tassilo Horn added a comment - - edited

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.

Show
Tassilo Horn added a comment - - edited 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.
Tassilo Horn made changes -
Attachment 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch [ 12385 ]
Tassilo Horn made changes -
Attachment 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch [ 12356 ]
Hide
Tassilo Horn added a comment -

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.

Show
Tassilo Horn added a comment - 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.
Tassilo Horn made changes -
Attachment 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch [ 12386 ]
Tassilo Horn made changes -
Attachment 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch [ 12385 ]
Hide
Andy Fingerhut added a comment -

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.

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

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

Show
Tassilo Horn added a comment - I've rebased the patch onto the current master so that it applies cleanly again.
Tassilo Horn made changes -
Tassilo Horn made changes -
Attachment 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch [ 12386 ]
Tassilo Horn made changes -
Assignee Tassilo Horn [ tsdh ]
Hide
Tassilo Horn added a comment -

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.

Show
Tassilo Horn added a comment - 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.
Tassilo Horn made changes -
Assignee Tassilo Horn [ tsdh ] Stuart Halloway [ stu ]
Alex Miller made changes -
Issue Type Defect [ 1 ] Enhancement [ 4 ]

People

Vote (2)
Watch (4)

Dates

  • Created:
    Updated: