Clojure

Method/Constructor resolution does not factor in widening conversion of primitive args

Details

  • Type: Enhancement Enhancement
  • Status: In Progress In Progress
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: Backlog
  • Component/s: None
  • Labels:
    None
  • Approval:
    Vetted

Description

Problem:
When making java calls (or inlined functions), if both args and param are primitive, no widening conversion is used to locate the proper overloaded method/constructor.

Examples:

user=> (Integer. (byte 0))
java.lang.IllegalArgumentException: No matching ctor found for class java.lang.Integer (NO_SOURCE_FILE:0)
</code></pre>
The above occurs because there is no Integer(byte) constructor, though it should match on Integer(int).
<pre><code>user=> (bit-shift-left (byte 1) 1)
Reflection warning, NO_SOURCE_PATH:3 - call to shiftLeft can't be resolved.
2

In the above, a call is made via reflection to Numbers.shiftLeft(Object, Object) and its associated auto-boxing, instead of directly to the perfectly adequate Numbers.shiftLeft(long, int).

Workarounds:
Explicitly casting to the formal type.

Ancillary benefits of fixing:
It would also reduce the amount of method overloading, e.g., RT.intCast(char), intCast(byte), intCast(short), could all be removed, since such calls would pass to RT.intCast(int).

  1. clj-445-prim-conversion-update-2-patch.txt
    20/Feb/12 2:04 PM
    36 kB
    Andy Fingerhut
  2. prim-conversion.patch
    29/Apr/11 6:43 AM
    36 kB
    Alexander Taggart
  3. prim-conversion-update-1.patch
    10/May/11 3:13 PM
    36 kB
    Alexander Taggart
  4. reorg-reflector.patch
    28/Apr/11 1:19 PM
    72 kB
    Alexander Taggart

Activity

Stuart Halloway made changes -
Field Original Value New Value
Reporter Alexander Taggart [ ataggart ]
Priority Major [ 3 ]
Approval Test Incomplete
Waiting On ataggart
Stuart Halloway made changes -
Fix Version/s Release.Next [ 10038 ]
Fix Version/s Backlog [ 10035 ]
Waiting On ataggart richhickey
Alexander Taggart made changes -
Attachment fixbug445-update.diff [ 10101 ]
Alexander Taggart made changes -
Attachment disambig.diff [ 10102 ]
Attachment jls.diff [ 10103 ]
Attachment numbers.diff [ 10104 ]
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.diff [ 10109 ]
Alexander Taggart made changes -
Attachment disambig.diff [ 10102 ]
Alexander Taggart made changes -
Attachment fixbug445-update.diff [ 10101 ]
Alexander Taggart made changes -
Attachment jls.diff [ 10103 ]
Alexander Taggart made changes -
Attachment numbers.diff [ 10104 ]
Alexander Taggart made changes -
Comment [ Just wanted to add that I'm also working on getting vararg method resolution to work, so it might be worthwhile to only apply the update and numbers patches. ]
Alexander Taggart made changes -
Comment [ disambig.diff: Ambiguity between methods of equal narrowness is further resolved by choosing the one with the fewest boxing conversions.

jls.diff: Method resolution now first attempts to select a method without boxing in order to be more in keeping with JLS 15.12.2.

numbers.diff: Altered public functions of clojure.lang.Numbers to service Object/double/long params, including removing/adding overloads as needed.

These patches can be applied in any order, but fixbug445-update.diff must be applied first. ]
Alexander Taggart made changes -
Comment [ fixbug445-update.diff:

- Updates test to pull in new location of clojure.test-helper

- Adds improved ambiguous method error message

- Does not fix the method resolution problem raised by the compilation failure of pprint. ]
Alexander Taggart made changes -
Comment [ It depends on whose version of "correct" we use.

According to the JLS ([sec 15.12.2|http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#15.12.2]) the method resolution code is incorrect as it does not first attempt to resolve methods without using boxing conversions. The JLS requirement ensures old java calls wouldn't suddenly become ambiguous with the introduction of boxing. This is why a java call using {{(Number, long)}} doesn't result in an ambiguity error; the backwards compatibility requirement is hiding it.

I have a working patch that implements this no-boxing first pass.

----

That said, in a world with boxing there are still plenty of chances to run into ambiguity. For example:
{noformat}
void foo(Object a, long b){};
void foo(Object a, Object b){};
{noformat}
Calling {{foo}} with {{(long, long)}} will be ambiguous. The first non-boxing pass doesn't match either, and the second boxing pass matches both since the JLS only allows for subtype conversion to choose the best of the acceptable methods.

I have a working patch that tries to further disambiguate based on which method requires fewer boxing/unboxing conversions.

----

A third option (not exclusive with the above) would be to decide if this is a problem for clojure-java interop, and/or if the overloading being used in classes such as {{clojure.lang.Numbers}} needs to be reviewed. As an example of the latter, simply removing {{equiv(Number, Number)}}, and leaving the extant {{Object}}/{{double}}/{{long}} overloads solves the build-breaking ambiguity.

Aside: there is already some deviation from the JLS method resolution by the fact that {{char}} doesn't resolve methods with numeric parameters, and that {{long}} and {{doubles}} are cast to {{int}} and {{float}} as needed. ]
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.diff [ 10109 ]
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.diff [ 10110 ]
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.diff [ 10110 ]
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.diff [ 10123 ]
Stuart Halloway made changes -
Waiting On richhickey ataggart
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.diff [ 10123 ]
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.diff [ 10140 ]
Stuart Halloway made changes -
Approval Incomplete Screened
Waiting On ataggart
Stuart Halloway made changes -
Approval Screened Incomplete
Waiting On ataggart
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.diff [ 10140 ]
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.patch [ 10154 ]
Alexander Taggart made changes -
Attachment refcompnum-update-1.patch [ 10160 ]
Alexander Taggart made changes -
Comment [ refcompnum-update-1.patch: Fixes "does not apply" to current master due to purging "throws Exception" from clojure.lang. ]
Alexander Taggart made changes -
Comment [ Updated patch:

- Fixes "does not apply" to current master due to moving UNCHECKED_MATH from Compiler to RT).
- Fixes a bug where {{(= 1 1.0)}} was returning true; numbers that are not in the same category should return false. For Objects this was working correctly, but the new widening conversion meant calling with {{(long, double)}} args resolved to {{Util.equiv(double, double)}}, which uses primitive equality.
-- Added two special overloads for {{(long, double)}} and {{(double, long)}}, both always return false.
-- Added tests for the proper categorical behavior. ]
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.patch [ 10154 ]
Alexander Taggart made changes -
Attachment refcompnum-update-1.patch [ 10160 ]
Alexander Taggart made changes -
Attachment clj445.patch [ 10170 ]
Alexander Taggart made changes -
Attachment clj445.patch [ 10170 ]
Alexander Taggart made changes -
Attachment clj445.patch [ 10172 ]
Alexander Taggart made changes -
Attachment clj445.patch [ 10172 ]
Alexander Taggart made changes -
Attachment 445.patch [ 10182 ]
Christopher Redinger made changes -
Assignee Christopher Redinger [ redinger ]
Christopher Redinger made changes -
Approval Incomplete Test
Waiting On ataggart stu
Christopher Redinger made changes -
Assignee Christopher Redinger [ redinger ] Alexander Taggart [ ataggart ]
Fix Version/s Approved Backlog [ 10034 ]
Fix Version/s Release.Next [ 10038 ]
Waiting On stu
Alexander Taggart made changes -
Attachment 445.patch [ 10182 ]
Alexander Taggart made changes -
Assignee Alexander Taggart [ ataggart ]
Alexander Taggart made changes -
Assignee Alexander Taggart [ ataggart ]
Alexander Taggart made changes -
Attachment reorg-reflector.patch [ 10203 ]
Attachment prim-conversion.patch [ 10204 ]
Alexander Taggart made changes -
Waiting On stu
Alexander Taggart made changes -
Attachment prim-conversion.patch [ 10204 ]
Alexander Taggart made changes -
Attachment prim-conversion.patch [ 10205 ]
Alexander Taggart made changes -
Attachment prim-conversion-update-1.patch [ 10225 ]
Stuart Halloway made changes -
Waiting On stu
Stuart Halloway made changes -
Fix Version/s Approved Backlog [ 10034 ]
Fix Version/s Reviewed Backlog [ 10053 ]
Stuart Sierra made changes -
Approval Test [ 10013 ] Incomplete [ 10006 ]
Andy Fingerhut made changes -
Alex Miller made changes -
Fix Version/s Reviewed Backlog [ 10053 ]
Fix Version/s Backlog [ 10035 ]
Alex Miller made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]

People

Vote (3)
Watch (4)

Dates

  • Created:
    Updated: