Clojure

better error message when calling macros with arity

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Blocker Blocker
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.3
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

The two magic arguments to macros lead to weird error messages:

(defmacro foo [a b] a)
(foo)
java.lang.IllegalArgumentException: Wrong number of args (2) passed to: user$foo (NO_SOURCE_FILE:)0

Activity

Hide
Assembla Importer added a comment -

stu said: Updating tickets (#427, #426, #421, #420, #397)

Show
Assembla Importer added a comment - stu said: Updating tickets (#427, #426, #421, #420, #397)
Hide
Assembla Importer added a comment -

stu said: The choke point here is line 5286 of Compiler.java. At this point, we know we are in macro, but we don't know we if we have an arity problem. When the arity problem is throw, it is a plain old IllegalArgumentException. Some choices:

(1) Hack: parse the exception, pull out the number, subtract 2. Yuck.

(2) Make the exception smarter (subclass the exception as ArityException or some such, catch that here, subtract 2.

(3) Make AFunction smarter, so that it knows if it is a macro or not (also requires change to defmacro). Then throwArity can do the right thing at the point of the problem.

Option #2 feels smaller than #3, but maybe knowing when an AFunction is AMacro will be useful elsewhere.

Show
Assembla Importer added a comment - stu said: The choke point here is line 5286 of Compiler.java. At this point, we know we are in macro, but we don't know we if we have an arity problem. When the arity problem is throw, it is a plain old IllegalArgumentException. Some choices: (1) Hack: parse the exception, pull out the number, subtract 2. Yuck. (2) Make the exception smarter (subclass the exception as ArityException or some such, catch that here, subtract 2. (3) Make AFunction smarter, so that it knows if it is a macro or not (also requires change to defmacro). Then throwArity can do the right thing at the point of the problem. Option #2 feels smaller than #3, but maybe knowing when an AFunction is AMacro will be useful elsewhere.
Hide
Assembla Importer added a comment -

richhickey said: #2 please

Show
Assembla Importer added a comment - richhickey said: #2 please
Hide
Assembla Importer added a comment -

fogus said: I agree with #2 also, but in general I think in creating a (reasonable) set of exception subclasses we could start down the path of making error reporting and handling more friendly.

Show
Assembla Importer added a comment - fogus said: I agree with #2 also, but in general I think in creating a (reasonable) set of exception subclasses we could start down the path of making error reporting and handling more friendly.
Hide
Assembla Importer added a comment -

stu said: Updating tickets (#429, #437, #397, #420)

Show
Assembla Importer added a comment - stu said: Updating tickets (#429, #437, #397, #420)
Hide
Assembla Importer added a comment -

mikehinchey said: [file:atduak20Cr35rOeJe5cbLA]: fixes using Stuart's #2 choice

Show
Assembla Importer added a comment - mikehinchey said: [file:atduak20Cr35rOeJe5cbLA]: fixes using Stuart's #2 choice
Hide
Stuart Halloway added a comment -

Second patch (arity-unwrapped) subsumes first, and does not return a wrapped exception. Wrapped exception would defeat the purpose of the patch, by giving REPL root-cause consumers an incorrect message.

Show
Stuart Halloway added a comment - Second patch (arity-unwrapped) subsumes first, and does not return a wrapped exception. Wrapped exception would defeat the purpose of the patch, by giving REPL root-cause consumers an incorrect message.
Stuart Halloway made changes -
Field Original Value New Value
Assignee Rich Hickey [ richhickey ]
Attachment 0397-arity-unwrapped.patch [ 10005 ]
Reporter Assembla Importer [ importer ]
Priority Blocker [ 1 ]
Approval Test Screened
Patch Code and Test
Rich Hickey made changes -
Approval Screened Ok
Stuart Halloway made changes -
Status In Progress [ 3 ] Closed [ 6 ]
Resolution Completed [ 1 ]

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: