Clojure

Show type information in reflection warning messages when available

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

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:

(set! *warn-on-reflection* true)

(defn foo [^String x] (.blah x))
Before: reference to field blah can't be resolved.
After:  reference to field blah on java.lang.String can't be resolved.

(defn foo [^String x] (.zap x 1))
Before: call to zap can't be resolved.
After:  call to method zap on java.lang.String can't be resolved (no such method).

(defn foo [] (Integer/valueOf #"boom"))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf on java.lang.Integer can't be resolved (argument types: java.util.regex.Pattern).

(defn foo [x] (Integer/valueOf x))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf on java.lang.Integer can't be resolved (argument types: unknown).

Patch: clj-1248-2.patch

Activity

Hide
Alex Miller added a comment -

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.

For example:
1) field: "reference to field blah in java.lang.String can't be resolved."
2) method: "call to method zap in java.lang.String can't be resolved."
3) static method: "call to method valueOf in java.lang.Integer can't be resolved."

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

Show
Alex Miller added a comment - 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. For example: 1) field: "reference to field blah in java.lang.String can't be resolved." 2) method: "call to method zap in java.lang.String can't be resolved." 3) static method: "call to method valueOf in java.lang.Integer can't be resolved." 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
Hide
Christoffer Sawicki added a comment - - edited

+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:

(fn [] (Integer/valueOff :foo))
CompilerException java.lang.IllegalArgumentException: No matching method: valueOff, compiling:(NO_SOURCE_PATH:1:8) 

^ Error because the compiler can statically see this is never going to work. Fine.

(fn [] (Integer/valueOf :foo))
Reflection warning, NO_SOURCE_PATH:1:8 - call to valueOf can't be resolved.

^ This is never going to work either, but only gives a warning. A bit surprising; I'd prefer an error.

(fn [x] (.foo x))
Reflection warning, NO_SOURCE_PATH:1:9 - reference to field foo can't be resolved.

^ This could work. Fine.

(fn [^String x] (.foo x))
Reflection warning, NO_SOURCE_PATH:1:17 - reference to field foo can't be resolved.

^ 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...

Show
Christoffer Sawicki added a comment - - edited +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:
(fn [] (Integer/valueOff :foo))
CompilerException java.lang.IllegalArgumentException: No matching method: valueOff, compiling:(NO_SOURCE_PATH:1:8) 
^ Error because the compiler can statically see this is never going to work. Fine.
(fn [] (Integer/valueOf :foo))
Reflection warning, NO_SOURCE_PATH:1:8 - call to valueOf can't be resolved.
^ This is never going to work either, but only gives a warning. A bit surprising; I'd prefer an error.
(fn [x] (.foo x))
Reflection warning, NO_SOURCE_PATH:1:9 - reference to field foo can't be resolved.
^ This could work. Fine.
(fn [^String x] (.foo x))
Reflection warning, NO_SOURCE_PATH:1:17 - reference to field foo can't be resolved.
^ 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...
Hide
Alex Miller added a comment -

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.

Show
Alex Miller added a comment - 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.
Hide
Christoffer Sawicki added a comment -

Here's an updated patch that changes the messages like this:

(defn foo [^String x] (.blah x))
Before: reference to field blah can't be resolved.
After:  reference to field blah of java.lang.String can't be resolved.

(defn foo [^String x] (.zap x 1))
Before: call to zap can't be resolved.
After:  call to method zap of java.lang.String can't be resolved (no such method).

(defn foo [] (Integer/valueOf #"boom"))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf of java.lang.Integer can't be resolved (argument types: [java.util.regex.Pattern]).

(defn foo [x] (Integer/valueOf x))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf of java.lang.Integer can't be resolved (argument types: [unknown]).

(I wish I could edit the issue description.)

Show
Christoffer Sawicki added a comment - Here's an updated patch that changes the messages like this:
(defn foo [^String x] (.blah x))
Before: reference to field blah can't be resolved.
After:  reference to field blah of java.lang.String can't be resolved.

(defn foo [^String x] (.zap x 1))
Before: call to zap can't be resolved.
After:  call to method zap of java.lang.String can't be resolved (no such method).

(defn foo [] (Integer/valueOf #"boom"))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf of java.lang.Integer can't be resolved (argument types: [java.util.regex.Pattern]).

(defn foo [x] (Integer/valueOf x))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf of java.lang.Integer can't be resolved (argument types: [unknown]).
(I wish I could edit the issue description.)
Hide
Alex Miller added a comment -

Updated description per latest patch.

Show
Alex Miller added a comment - Updated description per latest patch.
Hide
Alex Miller added a comment - - edited

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.

Show
Alex Miller added a comment - - edited 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.
Hide
Christoffer Sawicki added a comment -

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.

Show
Christoffer Sawicki added a comment - 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.
Hide
Alex Miller added a comment -

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.

Show
Alex Miller added a comment - 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.
Hide
Christoffer Sawicki added a comment -

OK, thanks!

Show
Christoffer Sawicki added a comment - OK, thanks!

People

Vote (12)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: