"Wrong number of args" ArityExceptions are misleading

[camsaul]

If you call a defmethod with the wrong number of args you'll get a misleading arity exception that reports you've passed one more are than you actually have:

(m/defmulti my-multimethod
  (fn [x y]
    [(keyword x) (keyword y)]))

(m/defmethod my-multimethod [:x :y]
  [_])

(my-multimethod :x :y)
;; ->
;; Execution error (ArityException) 
;; Wrong number of args (3) passed to: my-multimethod-primary-method-x-y

This is of course because the defmethod actually macroexpands to

(do
  (defn my-multimethod-primary-method-x-y [next-method _])
  (methodical.util/add-primary-method!
   #'my-multimethod
   [:x :y]
   my-multimethod-primary-method-x-y))

and gets the extra next-method parameter...

instead of macroexpanding to

(defn my-multimethod-primary-method-x-y
  [next-method _]
  ...)

we could do something like

(defn my-multimethod-primary-method-x-y
  [next-method a#]
  ((fn [_]
     ...) a#))

so we get correct error messages. I don't think this would make a noticeable performance difference but probably worth benching just to be sure.

Alternatively just write reify a version of IFn that gives meaningful error messages. Maybe it could even include stuff like the dispatch value

[camsaul]

Failing test (methodical.macros-test):

(m/defmulti mf5
  {:arglists '([x y])}
  (fn [x & _]
    (keyword x)))

(m/defmethod mf5 :default
  [x y]
  {x y})

(m/defmethod mf5 :after :default
  [x m]
  (assoc m :after x))

(t/deftest arity-exception-test
  (t/testing "'Wrong number of args' errors should not include `next-method` in the number of args passed (#54)"
    (t/testing "Sanity check"
      (t/is (= {:x 1, :after :x}
               (mf5 :x 1))))
    (t/is (thrown-with-msg?
           clojure.lang.ArityException
           #"Wrong number of args \(1\) passed to: methodical\.macros-test/mf5-primary-method-default"
           (mf5 :x)))
    (t/is (thrown-with-msg?
           clojure.lang.ArityException
           #"Wrong number of args \(1\) passed to: methodical\.macros-test/mf5-after-method-bad-after"
           (mf5 ::bad-after 1)))))

I poked around at fixing this a bit but I think what we need to is either:

• implement a custom function type that throws the correct error when the wrong arity is invoked

• pass next-method on its own and return a function e.g. instead of generating functions like

  (defn mf5-primary-method-default
    [next-method x y]
    (+ x y))

  we could generate them like

     (defn mf5-primary-method-default* [next-method]
    (fn [x y]
      (+ x y)))

  This seems a little annoying and we'd have to tweak all the method combinations

[camsaul]

Messages are also pretty bad if you add methods programmatically e.g. with add-primary-method. Maybe add-primary-method can put metadata on the primary method function (if applicable) that is used in the error

[camsaul]

Dispatch function arity errors aren't nice either =(

(m/defmulti x
  (fn [x y]
    [(keyword x) (keyword y)]))

(m/defmethod x [:x :y]
  [x y]
  {:x x, :y y})

(x :a)
;; => Wrong number of args (1) passed to: methodical.impl.combo-test/eval34222/fn--34224

We could wrap the dispatch function in a try-catch and throw a better exception but I'm not sure what the performance impact of that would be.

[camsaul]

I think the only reasonable way of implementing this would be to automatically wrap the primary method function in something that does a try-catch and catches clojure.lang.ArityException and throws a better one instead... not sure what performance impact that would have tho.

If the choice is between better errors or slightly better performance, I think we should choose better errors

[camsaul]

Wrapping things doesn't actually have a dramatic effect. A third of a nanosecond on my computer

[camsaul]

Here's an example of how we might accomplish wrapping things

(defn wrap-dispatch-fn [f ^String fn-description ^Integer num-implicit-args]
  (letfn [(rethrow-arity-exception [^clojure.lang.ArityException e]
            (throw (doto (clojure.lang.ArityException.
                          (- (.actual e) num-implicit-args)
                          fn-description
                          (.getCause e))
                     (.setStackTrace (.getStackTrace e)))))]
    (fn
      ([]
       (try
         (f)
         (catch clojure.lang.ArityException e
           (rethrow-arity-exception e))))

      ([a]
       (try
         (f a)
         (catch clojure.lang.ArityException e
           (rethrow-arity-exception e))))

      ([a b]
       (try
         (f a b)
         (catch clojure.lang.ArityException e
           (rethrow-arity-exception e))))

      ([a b c]
       (try
         (f a b c)
         (catch clojure.lang.ArityException e
           (rethrow-arity-exception e))))

      ([a b c d]
       (try
         (f a b c d)
         (catch clojure.lang.ArityException e
           (rethrow-arity-exception e))))

      ([a b c d e]
       (try
         (f a b c d e)
         (catch clojure.lang.ArityException e
           (rethrow-arity-exception e))))

      ([a b c d e & more]
       (try
         (apply f a b c d e more)
         (catch clojure.lang.ArityException e
           (rethrow-arity-exception e)))))))

(ns-unmap *ns* 'x)
(m/defmulti x
  (let [dispatch-fn (fn [x y]
                      [(keyword x) (keyword y)])]
    (wrap-dispatch-fn dispatch-fn
                      (format "Methodical multimethod %s dispatch function %s"
                              (pr-str `x)
                              (some-> dispatch-fn class .getCanonicalName))
                      0)))

(m/defmethod x [:x :y]
  [x y]
  {:x x, :y y})

(x :a)
;; => Wrong number of args (1) passed to: Methodical multimethod methodical.impl.combo-test/x dispatch function methodical.impl.combo-test/eval36766/dispatch-fn--36768