Clojure

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

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: Release 1.9
  • Fix Version/s: Release 1.11
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Screened

Description

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

Attachments

  1. Text File
    keyword-arity-exception.patch
    03/May/18 7:51 AM
    8 kB
    Marc O'Morain
  2. Text File
    keyword-arity-exception-01.patch
    04/May/18 6:34 AM
    8 kB
    Marc O'Morain
  3. Text File
    keyword-arity-exception-02.patch
    07/May/18 3:25 PM
    8 kB
    Marc O'Morain
  4. Text File
    keyword-arity-exception-03.patch
    09/Jul/18 5:40 PM
    7 kB
    Marc O'Morain

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Marc O'Morain added a comment -

Fix typo: later => latter.

Show
Marc O'Morain added a comment - Fix typo: later => latter.
Hide
Permalink
Alexander Taggart added a comment - - edited

+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"

Show
Alexander Taggart added a comment - - edited +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"
Hide
Permalink
Alex Miller added a comment -

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

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

Show
Alex Miller added a comment - Also, actually there is a typo artity-exceptions in the test. And I would agree with Alexander's comment, should leave existing throwArity().
Hide
Permalink
Marc O'Morain added a comment -

Attach updated patch.

Show
Marc O'Morain added a comment - Attach updated patch.
Hide
Permalink
Marc O'Morain added a comment -

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.

Show
Marc O'Morain added a comment - 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.
Hide
Permalink
Alex Miller added a comment -

yes and yes

Show
Alex Miller added a comment - yes and yes
Hide
Permalink
Marc O'Morain added a comment -

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

Show
Marc O'Morain added a comment - Added patch making throwArity(int) package private, and adding deprecation docstring.
Hide
Permalink
Alex Miller added a comment -

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.

Show
Alex Miller added a comment - 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.
Hide
Permalink
Marc O'Morain added a comment -

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

Show
Marc O'Morain added a comment - Added patch that improves the message for the 21+ arg case.
Hide
Permalink
Marc O'Morain added a comment - - edited

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?

Show
Marc O'Morain added a comment - - edited 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?
Hide
Permalink
Alex Miller added a comment -

Sure, separate ticket though.

Show
Alex Miller added a comment - Sure, separate ticket though.
Hide
Permalink
Marc O'Morain added a comment -

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.

Show
Marc O'Morain added a comment - 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.
Hide
Permalink
Alex Miller added a comment -

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

Show
Alex Miller added a comment - I’ll get to it next time I cycle through.
Hide
Permalink
Marc O'Morain added a comment -

Hi Alex,

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

Marc

Show
Marc O'Morain added a comment - Hi Alex, Any update on this ticket? I’m anxious to avoid having to rebase the patch against master again. Marc
Hide
Permalink
Alex Miller added a comment -

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

Show
Alex Miller added a comment - I've screened, but I'm not sure we'll look at it for 1.10. I will rebase if needed, no big deal.
Hide
Permalink
Marc O'Morain added a comment -

Thanks Alex

Show
Marc O'Morain added a comment - Thanks Alex

People

Vote (1)
Watch (1)

Dates

  • Created:
    Updated:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.