Improve error message when calling a keyword with the wrong number of arguments

Question

When you call a {{Keyword}} with the wrong number of arguments, the error message does not report how many arguments were passed:

(:kw "one" "two" "three") => java.lang.IllegalArgumentException: Wrong number of args passed to keyword: :kw

compare to calling an {{IFn}}, which does show the number of arguments passed:

(name "one" "two" "three") => clojure.lang.ArityException: Wrong number of args (3) passed to: core/name

The latter error message is more clear and makes it easier to debug.

The attached patch re-uses the {{ArityException}} class used elsewhere to generate error messages in the latter form when calling a keyword with the wrong number of arguments.

Addresses CLJ-1067

Patch: keyword-arity-exception-03.patch

Screened by: Alex Miller

Answer 1

Comment made by: marc

Fix typo: later => latter.

Answer 2

Comment made by: ataggart

+100 for improving error messages.

Albeit unlikely, since throwArity() is a public method, removing it risks breaking external code.

Less importantly, the resulting sentence looks a bit awkward with the three colons:
"Wrong number of args (3) passed to: keyword: :my.ns/foo"

Alternatively:
"Wrong number of args (3) passed to: keyword :my.ns/foo"
"Wrong number of args (3) passed to: :my.ns/foo"

The latter parallels the result when calling a non-keyword function:
"Wrong number of args (3) passed to: my.ns/bar"

Answer 3

Comment made by: alexmiller

Also, actually there is a typo artity-exceptions in the test.

And I would agree with Alexander's comment, should leave existing throwArity().

Answer 4

Comment made by: marc

Attach updated patch.

Answer 5

Comment made by: marc

I've attached an updated patch that fixes the type in the {{deftest}} declaration and formats the exception message in the way Alexander suggested.

Alex - should I add a Java comment to explain why the 0-arity version of Keyword.throwArtity() exists? Something like:

/** * @deprecated CLJ-2350 This function is no longer called, but has not been removed to maintain the public interface. */

Reflecting on the public interface, perhaps we could make the new function, {{throwArity(int n)}}, package-private. This would avoid it becoming part of the public API of {{clojure.lang}}.

Answer 6

Comment made by: alexmiller

yes and yes

Answer 7

Comment made by: marc

Added patch making {{throwArity(int)}} package private, and adding deprecation docstring.

Answer 8

Comment made by: alexmiller

The final invoke takes a variadic as the final arg so it's actually 21+, not 21. Could count the args array. The patch has also drifted from master and needs to be rebased.

Answer 9

Comment made by: marc

Added patch that improves the message for the 21+ arg case.

Answer 10

Comment made by: marc

There are two more places in the code that have the same bug in the 21+ arg case:

https://github.com/clojure/clojure/blob/71511b7800e18c83377a322f43585a853b303698/src/jvm/clojure/lang/RestFn.java#L4078
https://github.com/clojure/clojure/blob/71511b7800e18c83377a322f43585a853b303698/src/jvm/clojure/lang/AFn.java#L140

Would you like an additional patch to cover those cases?

Answer 11

Comment made by: alexmiller

Sure, separate ticket though.

Answer 12

Comment made by: marc

Hi Alex, any update on this ticket? I've updated the patch to the latest master as requested, so it should be good to merge now.

Answer 13

_Comment made by: alexmiller_

I’ll get to it next time I cycle through.

Answer 14

_Comment made by: marc_

Hi Alex,

Any update on this ticket? I’m anxious to avoid having to rebase the patch against master again.

Marc

Answer 15

Comment made by: alexmiller

I've screened, but I'm not sure we'll look at it for 1.10. I will rebase if needed, no big deal.