[CLJ-1248] Show type information in reflection warning messages when available Created: 24/Aug/13 Updated: 28/Sep/13
|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.