[CLJ-1248] Show type information in reflection warning messages when available Created: 24/Aug/13 Updated: 14/Feb/14 Resolved: 14/Feb/14
|Fix Version/s:||Release 1.6|
|Patch:||Code and Test|
The reflection warning messages currently don't show any type information. I think adding this would make the messages more helpful by making it more obvious what the problem is. I suggest these changes:
|Comment by Alex Miller [ 25/Aug/13 8:31 AM ]|
I think these are a good idea. I think it would be better to separate the reflected class from the field though since we are referring to fields that don't exist.
Your 3rd example actually highlights something more interesting though. In this case the problem is not actually that Integer/valueOf does not exist but rather that it is being called with the wrong types. In these cases, what I want to know is: what is the type of the parameters being passed.
In #2 there are actually two possible sources of error - an unexpected type for x or an unexpected type or arity for the parameters. It would be useful to check whether the method of this name exists and at what arity to determine which of these cases exists and give a more precise error.
In any case, the implementation needs to be supplied as a patch, not a link. See: http://dev.clojure.org/display/community/Developing+Patches
|Comment by Christoffer Sawicki [ 25/Aug/13 3:25 PM ]|
+1 on all points. I'll begin work on an updated patch.
Yes, there is a deeper more interesting problem lurking here.
One question is what should result in reflection warnings and what in compile-time errors. (I think some types of reflection warnings should be promoted to errors.)
Here are some examples of current behavior with comments:
^ Error because the compiler can statically see this is never going to work. Fine.
^ This is never going to work either, but only gives a warning. A bit surprising; I'd prefer an error.
^ This could work. Fine.
^ This is never going to work if x is a String but x can be of any type at run-time. Personally, I think this should be an error...
|Comment by Alex Miller [ 25/Aug/13 4:36 PM ]|
You should take any warning/error differences to one (or more) new tickets where they can be evaluated individually. The more you put in one ticket, the more likely it is to get bogged down and/or rejected. My gut feeling is that there would not be a lot of support for the warning->error changes you suggest.
|Comment by Christoffer Sawicki [ 28/Aug/13 3:05 PM ]|
Here's an updated patch that changes the messages like this:
(I wish I could edit the issue description.)
|Comment by Alex Miller [ 28/Aug/13 3:18 PM ]|
Updated description per latest patch.
|Comment by Alex Miller [ 12/Feb/14 1:12 AM ]|
I used this in a local build to resolve an issue tonight and found it very helpful. One comment I have is that in the message part "(argument types: [java.util.regex.Pattern])", I would like to remove the outer [ ] around the argument types. In the case I was working on the first type was actually a long which has class name "[J" so I found the outer s distracting.
|Comment by Christoffer Sawicki [ 12/Feb/14 5:41 AM ]|
Thanks for the success story!
I think I choose to use a vector to disambiguate the case with one argument that is unknown, i.e. "(argument types: [unknown])". Without the vector, it's not obvious if there's one argument of unknown type or if all of multiple arguments are of unknown type.
Given your input and some more thought, it's probably not worth making every other message worse for this single case. I'll update the patch tonight.
|Comment by Alex Miller [ 12/Feb/14 9:49 AM ]|
I went ahead and updated the patch. I also fixed a couple whitespace issues and changed the word "of" to "on" before the type as I think it reads better.
|Comment by Christoffer Sawicki [ 12/Feb/14 1:57 PM ]|