Clojure

Fix confusing macroexpand1 ArityException handling

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: Release 1.6
  • Fix Version/s: None
  • Component/s: None
  • Patch:
    Code and Test

Description

macros can give very confusing error messages when they execute a form which generates an ArityException. clojure.lang.Compiler.macroexpand1 assumes that any ArityException comes from the call to the macro itself, which need not be the case. For instance:

user> (do (defmacro f [] (assoc)) (f))
ArityException Wrong number of args (-2) passed to: core$assoc clojure.lang.Compiler.macroexpand1 (Compiler.java:6488)
user> (use 'clojure.repl) (pst)
nil
ArityException Wrong number of args (-2) passed to: core$assoc
clojure.lang.Compiler.macroexpand1 (Compiler.java:6488)
clojure.lang.Compiler.macroexpand (Compiler.java:6544)
clojure.lang.Compiler.eval (Compiler.java:6618)
clojure.lang.Compiler.eval (Compiler.java:6624)
clojure.lang.Compiler.eval (Compiler.java:6597)
clojure.core/eval (core.clj:2864)
clojure.main/repl/read-eval-print-6596/fn-6599 (main.clj:260)
clojure.main/repl/read-eval-print--6596 (main.clj:260)
clojure.main/repl/fn--6605 (main.clj:278)
clojure.main/repl (main.clj:278)
clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn--1251 (interruptible_eval.clj:56)
clojure.core/apply (core.clj:617)
nil

Easy enough to see the source of the problem in this case, but because both the number of arguments actually passed is off by two, and the stacktrace element for the call to assoc has been dropped, this shortcut by macroexpand1 can get super confusing.

The attached patch corrects this behavior. E.g.

user=> (do (defmacro f [] (assoc)) (f))
ArityException Wrong number of args (0) passed to: core$assoc clojure.lang.AFn.throwArity (AFn.java:437)
user=> (use 'clojure.repl) (pst)
nil
ArityException Wrong number of args (0) passed to: core$assoc
user/f (NO_SOURCE_FILE:1)
clojure.lang.Var.invoke (Var.java:419)
clojure.lang.Var.applyTo (Var.java:532)
clojure.lang.Compiler.macroexpand1 (Compiler.java:6507)
clojure.lang.Compiler.macroexpand (Compiler.java:6580)
clojure.lang.Compiler.eval (Compiler.java:6654)
clojure.lang.Compiler.eval (Compiler.java:6660)
clojure.lang.Compiler.eval (Compiler.java:6633)
clojure.core/eval (core.clj:2864)
clojure.main/repl/read-eval-print-6594/fn-6597 (main.clj:260)
clojure.main/repl/read-eval-print--6594 (main.clj:260)
clojure.main/repl/fn--6603 (main.clj:278)
nil

Activity

Alex Miller made changes -
Field Original Value New Value
Labels Compiler bug enhancement errormsgs patch, Compiler errormsgs
Alex Coventry made changes -
Attachment 0003-Test-which-examines-stacktrace-for-desired-element.patch [ 12330 ]
Alex Coventry made changes -
Attachment 0001-Fix-macroexpand1-s-handling-of-ArityException.patch [ 12329 ]
Alex Coventry made changes -
Attachment 0003-Test-which-examines-stacktrace-for-desired-element.patch [ 12330 ]
Hide
Alex Coventry added a comment -

Patch with test

Show
Alex Coventry added a comment - Patch with test
Alex Coventry made changes -
Attachment 0001-Fix-macroexpand1-s-handling-of-ArityException.patch [ 12331 ]
Alex Coventry made changes -
Comment [ Adding a test which checks that the relevent stack trace elements are in the thrown exception. ]
Alex Coventry made changes -
Attachment 0001-Fix-macroexpand1-s-handling-of-ArityException.patch [ 12383 ]
Alex Coventry made changes -
Attachment 0001-Fix-macroexpand1-s-handling-of-ArityException.patch [ 12331 ]
Alex Coventry made changes -
Comment [ Amended patch to deal more gracefully with unexpected stack trace structure. ]
Hide
Alex Coventry added a comment -

Amended patch to deal more gracefully with unexpected stack trace structure.

Show
Alex Coventry added a comment - Amended patch to deal more gracefully with unexpected stack trace structure.
Alex Coventry made changes -
Alex Coventry made changes -
Attachment 0001-Fix-macroexpand1-s-handling-of-ArityException.patch [ 12383 ]
Hide
Alex Miller added a comment -

Also see CLJ-397 and CLJ-383.

Show
Alex Miller added a comment - Also see CLJ-397 and CLJ-383.
Hide
Alex Coventry added a comment - - edited

Thanks, Alex. It would be easy enough to move most of the logic into ArityException, which would be a compromise between Stu's[1] options 1 and 2. Is that worth doing?

Amending clojure.lang.AFn.throwArity to check whether "this" is a macro and adjust the arg count there accordingly might be the simplest way. I can see why Rich prefers all the logic to go into ArityException, but since ArityExceptions are used for things other than macros, I don't see a way to make an honest error message there without groveling the stack trace.

[1] http://dev.clojure.org/jira/browse/CLJ-397?focusedCommentId=24090&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-24090

Show
Alex Coventry added a comment - - edited Thanks, Alex. It would be easy enough to move most of the logic into ArityException, which would be a compromise between Stu's[1] options 1 and 2. Is that worth doing? Amending clojure.lang.AFn.throwArity to check whether "this" is a macro and adjust the arg count there accordingly might be the simplest way. I can see why Rich prefers all the logic to go into ArityException, but since ArityExceptions are used for things other than macros, I don't see a way to make an honest error message there without groveling the stack trace. [1] http://dev.clojure.org/jira/browse/CLJ-397?focusedCommentId=24090&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-24090
Hide
Alex Miller added a comment -

I would have to take more time than I have to make an informed opinion but I can say that from a general point of view inspecting StackTraceElements does not seem like the right solution to (almost) any problem.

Show
Alex Miller added a comment - I would have to take more time than I have to make an informed opinion but I can say that from a general point of view inspecting StackTraceElements does not seem like the right solution to (almost) any problem.
Hide
Alex Coventry added a comment - - edited

This patch causes Var.setMacro to set instance attribute AFn.macrop to true, so that AFn.throwArity can reduce the number of arguments reported.

I'm not used to negotiating java class hierarchies, so it's possible there's a cleaner way. Since Var.fn() returns an IFn, I added macrop handling methods IFn.setMacro and IFn.isMacro. These then needed to be implemented in Ref and Keyword, as well as AFn (where I wanted them) because they implement the IFn interface but don't inherit from AFn.

The real drawback I see with this approach is the duplicated state, though: ^{:macro true} vs AFn.macrop==true.

Show
Alex Coventry added a comment - - edited This patch causes Var.setMacro to set instance attribute AFn.macrop to true, so that AFn.throwArity can reduce the number of arguments reported. I'm not used to negotiating java class hierarchies, so it's possible there's a cleaner way. Since Var.fn() returns an IFn, I added macrop handling methods IFn.setMacro and IFn.isMacro. These then needed to be implemented in Ref and Keyword, as well as AFn (where I wanted them) because they implement the IFn interface but don't inherit from AFn. The real drawback I see with this approach is the duplicated state, though: ^{:macro true} vs AFn.macrop==true.
Alex Coventry made changes -
Hide
Andy Fingerhut added a comment -

I have not investigated the reason yet, but neither patch applies cleanly after the latest commits to Clojure master on Oct 25 2013. Given that what kinds of solution methods would be acceptable for this issue, it sounds like more thinking and code changes are probably needed anyway before worrying too much about that.

Show
Andy Fingerhut added a comment - I have not investigated the reason yet, but neither patch applies cleanly after the latest commits to Clojure master on Oct 25 2013. Given that what kinds of solution methods would be acceptable for this issue, it sounds like more thinking and code changes are probably needed anyway before worrying too much about that.
Alex Miller made changes -
Labels Compiler errormsgs Compiler errormsgs macro

People

Vote (3)
Watch (1)

Dates

  • Created:
    Updated: