Improve error message when a protocol method is not found

Question

If you call a protocol function but pass the wrong arity (forget an argument for example), you currently a message that says "No single method ... of interface ... found for function ... of protocol ...". The code in question is getting matching methods from the Reflector and creates this message if the number of matches != 1.

There are really two cases there:
- matches == 0 - this happens frequently due to typos
- matches > 1 - this presumably happens infrequently

I propose that the == 0 case instead should have slightly different text at the beginning and a hint as to the intended arity within it:

"No method: ... of interface ... with arity ... found for function ... of protocol ...".

The >1 case should have similar changes: "Multiple methods: ... of interface ... with arity ... found for function ... of protocol ...".

Patch is attached. I used case which presumably should have better performance than a nested if/else. I was not sure whether the reported arity should match the actual Java method arity or Clojure protocol function arity (including the target). I did the former.

I did not add a test as I wasn't sure whether checking error messages in tests was appropriate or not. Happy to add that if requested.

Answer 1

Comment made by: cemerick

bq. I was not sure whether the reported arity should match the actual Java method arity or Clojure protocol function arity (including the target). I did the former.

I think it should be the latter. The message is emitted when the protocol methods are being invoked through the corresponding function, so it should be consistent with the errors emitted by regular functions.

+1 for some tests, too. There certainly are tests for reflection warnings and such.

FWIW, I'm happy to take this on if Alex is otherwise occupied.

Answer 2

Comment made by: alexmiller

Picking this up a zillion years later ... :)

1) I think I would now change the control flow slightly to make the normal path (methods.size() == 1) first and the error cases after that without that switch/case stuff:

if(methods.size() == 1) { this.onMethod = (java.lang.reflect.Method) methods.get(0); } else if(methods.size() == 0) { ... } else { ... }

2) The Compiler code should use tabs instead of spaces. :)

3) I would like tests added for the error messages in these two cases. Those can go in clojure.test-clojure.protocols.

Answer 3

Reference: https://clojure.atlassian.net/browse/CLJ-735 (reported by alexmiller)