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

Hide
Assembla Importer added a comment -

ataggart said: [file:b6gDSUZOur36b9eJe5cbCb]

Show
Assembla Importer added a comment - ataggart said: [file:b6gDSUZOur36b9eJe5cbCb]
Hide
Assembla Importer added a comment -

ataggart said: Also fixes #446.

Show
Assembla Importer added a comment - ataggart said: Also fixes #446.
Hide
Stuart Halloway added a comment -

The patch is causing a test failure

[java] Exception in thread "main" java.lang.IllegalArgumentException: 
     More than one matching method found: equiv, compiling:(clojure/pprint/cl_format.clj:428)

Can you take a look?

Show
Stuart Halloway added a comment - The patch is causing a test failure
[java] Exception in thread "main" java.lang.IllegalArgumentException: 
     More than one matching method found: equiv, compiling:(clojure/pprint/cl_format.clj:428)
Can you take a look?
Stuart Halloway made changes -
Field Original Value New Value
Reporter Alexander Taggart [ ataggart ]
Priority Major [ 3 ]
Approval Test Incomplete
Waiting On ataggart
Hide
Stuart Halloway added a comment -

The failing test happens when trying to find the correct equiv for signature (Number, long). Is the compiler wrong to propose this signature, or is the resolution method wrong in not having an answer? (It thinks two signatures are tied: (Object, long) and (Number, Number).)

Exception in thread "main" java.lang.IllegalArgumentException: More than one matching method found: equiv, compiling:(clojure/pprint/cl_format.clj:428)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6062)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6050)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.access$100(Compiler.java:35)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5492)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2372)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3277)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6057)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2385)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2385)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$FnMethod.parse(Compiler.java:4667)
	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3397)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6053)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.access$100(Compiler.java:35)
	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:480)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler.eval(Compiler.java:6114)
	at clojure.lang.Compiler.load(Compiler.java:6545)
	at clojure.lang.RT.loadResourceScript(RT.java:340)
	at clojure.lang.RT.loadResourceScript(RT.java:331)
	at clojure.lang.RT.load(RT.java:409)
	at clojure.lang.RT.load(RT.java:381)
	at clojure.core$load$fn__1427.invoke(core.clj:5308)
	at clojure.core$load.doInvoke(core.clj:5307)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.pprint$eval3969.invoke(pprint.clj:46)
	at clojure.lang.Compiler.eval(Compiler.java:6110)
	at clojure.lang.Compiler.load(Compiler.java:6545)
	at clojure.lang.RT.loadResourceScript(RT.java:340)
	at clojure.lang.RT.loadResourceScript(RT.java:331)
	at clojure.lang.RT.load(RT.java:409)
	at clojure.lang.RT.load(RT.java:381)
	at clojure.core$load$fn__1427.invoke(core.clj:5308)
	at clojure.core$load.doInvoke(core.clj:5307)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.core$load_one.invoke(core.clj:5132)
	at clojure.core$load_lib.doInvoke(core.clj:5169)
	at clojure.lang.RestFn.applyTo(RestFn.java:143)
	at sun.reflect.GeneratedMethodAccessor11.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:597)
	at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:77)
	at clojure.lang.Reflector.invokeInstanceMethod(Reflector.java:28)
	at clojure.core$apply.invoke(core.clj:602)
	at clojure.core$load_libs.doInvoke(core.clj:5203)
	at clojure.lang.RestFn.applyTo(RestFn.java:138)
	at sun.reflect.GeneratedMethodAccessor11.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:597)
	at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:77)
	at clojure.lang.Reflector.invokeInstanceMethod(Reflector.java:28)
	at clojure.core$apply.invoke(core.clj:604)
	at clojure.core$use.doInvoke(core.clj:5283)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.main$repl.doInvoke(main.clj:196)
	at clojure.lang.RestFn.invoke(RestFn.java:422)
	at clojure.main$repl_opt.invoke(main.clj:267)
	at clojure.main$main.doInvoke(main.clj:362)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.lang.Var.invoke(Var.java:401)
	at clojure.lang.AFn.applyToHelper(AFn.java:163)
	at clojure.lang.Var.applyTo(Var.java:518)
	at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: More than one matching method found: equiv
	at clojure.lang.Reflector.getMatchingParams(Reflector.java:639)
	at clojure.lang.Reflector.getMatchingParams(Reflector.java:578)
	at clojure.lang.Reflector.getMatchingMethod(Reflector.java:569)
	at clojure.lang.Compiler$StaticMethodExpr.<init>(Compiler.java:1439)
	at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:896)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	... 115 more
Show
Stuart Halloway added a comment - The failing test happens when trying to find the correct equiv for signature (Number, long). Is the compiler wrong to propose this signature, or is the resolution method wrong in not having an answer? (It thinks two signatures are tied: (Object, long) and (Number, Number).)
Exception in thread "main" java.lang.IllegalArgumentException: More than one matching method found: equiv, compiling:(clojure/pprint/cl_format.clj:428)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6062)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6050)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.access$100(Compiler.java:35)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5492)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2372)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3277)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6057)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2385)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2385)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$FnMethod.parse(Compiler.java:4667)
	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3397)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6053)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.access$100(Compiler.java:35)
	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:480)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler.eval(Compiler.java:6114)
	at clojure.lang.Compiler.load(Compiler.java:6545)
	at clojure.lang.RT.loadResourceScript(RT.java:340)
	at clojure.lang.RT.loadResourceScript(RT.java:331)
	at clojure.lang.RT.load(RT.java:409)
	at clojure.lang.RT.load(RT.java:381)
	at clojure.core$load$fn__1427.invoke(core.clj:5308)
	at clojure.core$load.doInvoke(core.clj:5307)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.pprint$eval3969.invoke(pprint.clj:46)
	at clojure.lang.Compiler.eval(Compiler.java:6110)
	at clojure.lang.Compiler.load(Compiler.java:6545)
	at clojure.lang.RT.loadResourceScript(RT.java:340)
	at clojure.lang.RT.loadResourceScript(RT.java:331)
	at clojure.lang.RT.load(RT.java:409)
	at clojure.lang.RT.load(RT.java:381)
	at clojure.core$load$fn__1427.invoke(core.clj:5308)
	at clojure.core$load.doInvoke(core.clj:5307)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.core$load_one.invoke(core.clj:5132)
	at clojure.core$load_lib.doInvoke(core.clj:5169)
	at clojure.lang.RestFn.applyTo(RestFn.java:143)
	at sun.reflect.GeneratedMethodAccessor11.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:597)
	at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:77)
	at clojure.lang.Reflector.invokeInstanceMethod(Reflector.java:28)
	at clojure.core$apply.invoke(core.clj:602)
	at clojure.core$load_libs.doInvoke(core.clj:5203)
	at clojure.lang.RestFn.applyTo(RestFn.java:138)
	at sun.reflect.GeneratedMethodAccessor11.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:597)
	at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:77)
	at clojure.lang.Reflector.invokeInstanceMethod(Reflector.java:28)
	at clojure.core$apply.invoke(core.clj:604)
	at clojure.core$use.doInvoke(core.clj:5283)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.main$repl.doInvoke(main.clj:196)
	at clojure.lang.RestFn.invoke(RestFn.java:422)
	at clojure.main$repl_opt.invoke(main.clj:267)
	at clojure.main$main.doInvoke(main.clj:362)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.lang.Var.invoke(Var.java:401)
	at clojure.lang.AFn.applyToHelper(AFn.java:163)
	at clojure.lang.Var.applyTo(Var.java:518)
	at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: More than one matching method found: equiv
	at clojure.lang.Reflector.getMatchingParams(Reflector.java:639)
	at clojure.lang.Reflector.getMatchingParams(Reflector.java:578)
	at clojure.lang.Reflector.getMatchingMethod(Reflector.java:569)
	at clojure.lang.Compiler$StaticMethodExpr.<init>(Compiler.java:1439)
	at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:896)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	... 115 more
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 ]
Hide
Alexander Taggart added a comment - - edited

In working on implementing support for vararg methods, I found a number of flaws with the previous solutions. Please disregard them.

I've attached a single patch (reflector-compiler-numbers.diff) which is a rather substantial overhaul of the Reflector code, with some enhancements to the Compiler and Numbers code.

The patch notes:

  • Moved reflection functionality from Compiler to Reflector.
  • Reflector supports finding overloaded methods by widening conversion, boxing conversion, and casting.
  • During compilation Reflector attempts to find best wildcard match.
  • Reflector refers to *unchecked-math* when reflectively invoking methods and constructors.
  • Both Reflector and Compiler support variable arity java methods and constructor; backwards compatible with passing an array or nil in the vararg slot.
  • Added more informative error messages to Reflector.
  • Added tests to clojure.test-clojure.reflector.
  • Altered overloaded functions of clojure.lang.Numbers to service Object/double/long params; fixes some ambiguity issues and avoids unnecessary boxing in some cases.
  • Patch closes issues 380, 440, 445, 666, and possibly 259 (not enough detail provided).
Show
Alexander Taggart added a comment - - edited In working on implementing support for vararg methods, I found a number of flaws with the previous solutions. Please disregard them. I've attached a single patch (reflector-compiler-numbers.diff) which is a rather substantial overhaul of the Reflector code, with some enhancements to the Compiler and Numbers code. The patch notes:
  • Moved reflection functionality from Compiler to Reflector.
  • Reflector supports finding overloaded methods by widening conversion, boxing conversion, and casting.
  • During compilation Reflector attempts to find best wildcard match.
  • Reflector refers to *unchecked-math* when reflectively invoking methods and constructors.
  • Both Reflector and Compiler support variable arity java methods and constructor; backwards compatible with passing an array or nil in the vararg slot.
  • Added more informative error messages to Reflector.
  • Added tests to clojure.test-clojure.reflector.
  • Altered overloaded functions of clojure.lang.Numbers to service Object/double/long params; fixes some ambiguity issues and avoids unnecessary boxing in some cases.
  • Patch closes issues 380, 440, 445, 666, and possibly 259 (not enough detail provided).
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 ]
Hide
Alexander Taggart added a comment -

Updated patch to fix a bug where a concrete class with multiple identical Methods (e.g., one from an interface, another from an abstract class) would result in ambiguity. Now resolved by arbitrary selection (this is what the original code did as well albeit not explicitly).

Show
Alexander Taggart added a comment - Updated patch to fix a bug where a concrete class with multiple identical Methods (e.g., one from an interface, another from an abstract class) would result in ambiguity. Now resolved by arbitrary selection (this is what the original code did as well albeit not explicitly).
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.diff [ 10110 ]
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.diff [ 10110 ]
Hide
Alexander Taggart added a comment -

Updated patch to work with latest master branch.

Show
Alexander Taggart added a comment - Updated patch to work with latest master branch.
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.diff [ 10123 ]
Hide
Stuart Halloway added a comment -

patch appears to be missing test file clojure/test_clojure/reflector.clj.

Show
Stuart Halloway added a comment - patch appears to be missing test file clojure/test_clojure/reflector.clj.
Stuart Halloway made changes -
Waiting On richhickey ataggart
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.diff [ 10123 ]
Hide
Alexander Taggart added a comment -

Bit by git.

Patch corrected to contain clojure.test-clojure.reflector.

Show
Alexander Taggart added a comment - Bit by git. Patch corrected to contain clojure.test-clojure.reflector.
Alexander Taggart made changes -
Attachment reflector-compiler-numbers.diff [ 10140 ]
Hide
Stuart Halloway added a comment -

Rich: I verified that the patch applied but reviewed only briefly, knowing you will want to go over this one closely.

Show
Stuart Halloway added a comment - Rich: I verified that the patch applied but reviewed only briefly, knowing you will want to go over this one closely.
Stuart Halloway made changes -
Approval Incomplete Screened
Waiting On ataggart
Hide
Stuart Halloway added a comment -

After applying this patch, I am getting method missing errors:

java.lang.NoSuchMethodError: clojure.lang.Numbers.lt(JLjava/lang/Object;)

but only when using compiled code, e.g. the same code works in the REPL and then fails after compilation. Haven't been able to isolate an example that I can share here yet, but hoping this will cause someone to have an "a, hah" moment...

Show
Stuart Halloway added a comment - After applying this patch, I am getting method missing errors:
java.lang.NoSuchMethodError: clojure.lang.Numbers.lt(JLjava/lang/Object;)
but only when using compiled code, e.g. the same code works in the REPL and then fails after compilation. Haven't been able to isolate an example that I can share here yet, but hoping this will cause someone to have an "a, hah" moment...
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 ]
Hide
Alexander Taggart added a comment -

The patch now contains only the minimal changes needed to support widening conversion. Cleanup of Numbers overloads, etc., can wait until this patch gets applied. The vararg support is in a separate patch on CLJ-440.

Show
Alexander Taggart added a comment - The patch now contains only the minimal changes needed to support widening conversion. Cleanup of Numbers overloads, etc., can wait until this patch gets applied. The vararg support is in a separate patch on CLJ-440.
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 ]
Hide
Christopher Redinger added a comment -

Please test patch

Show
Christopher Redinger added a comment - Please test patch
Christopher Redinger made changes -
Approval Incomplete Test
Waiting On ataggart stu
Hide
Christopher Redinger added a comment -

FYI: Patch applies cleanly on master and all tests pass as of 4/21 (2011)

Show
Christopher Redinger added a comment - FYI: Patch applies cleanly on master and all tests pass as of 4/21 (2011)
Hide
Christopher Redinger added a comment -

This work is too big to take into the 1.3 beta right now. We'll revisit for a future release.

Show
Christopher Redinger added a comment - This work is too big to take into the 1.3 beta right now. We'll revisit for a future release.
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 ]
Hide
Alexander Taggart added a comment - - edited

To better facilitate understanding of the changes, I've broken them up into two patches, each with a number of isolable, incremental commits:

reorg-reflector.patch: Moves the reflection/invocation code from Compiler to Reflector, and eliminates redundant code. The result is a single code base for resolving methods/constructors, which will allow for altering that mechanism without excess external coordination. This contains no behaviour changes.

prim-conversion.patch: Depends on the above. Alters the method/constructor resolution process:

  • more consistent with java resolution, especially when calling pre-1.5 APIs
  • adds support for widening conversion of primitive numerics of the same category (this is more strict than java, and more clojuresque)
  • adds support for wildcard matches at compile-time (i.e., you don't need to type-hint every arg to avoid reflection).

This also provides a base to add further features, e.g., CLJ-666.

Show
Alexander Taggart added a comment - - edited To better facilitate understanding of the changes, I've broken them up into two patches, each with a number of isolable, incremental commits: reorg-reflector.patch: Moves the reflection/invocation code from Compiler to Reflector, and eliminates redundant code. The result is a single code base for resolving methods/constructors, which will allow for altering that mechanism without excess external coordination. This contains no behaviour changes. prim-conversion.patch: Depends on the above. Alters the method/constructor resolution process:
  • more consistent with java resolution, especially when calling pre-1.5 APIs
  • adds support for widening conversion of primitive numerics of the same category (this is more strict than java, and more clojuresque)
  • adds support for wildcard matches at compile-time (i.e., you don't need to type-hint every arg to avoid reflection).
This also provides a base to add further features, e.g., CLJ-666.
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 ]
Hide
Alexander Taggart added a comment -

It's documented in situ, but here are the conversion rules that the reflector uses to find methods:

  1. By Type:
    • object to ancestor type
    • primitive to a wider primitive of the same numeric category (new)
  2. Boxing:
    • boxed number to its primitive
    • boxed number to a wider primitive of the same numeric category (new for Short and Byte args)
    • primitive to its boxed value
    • primitive to Number or Object (new)
  3. Casting:
    • long to int
    • double to float
Show
Alexander Taggart added a comment - It's documented in situ, but here are the conversion rules that the reflector uses to find methods:
  1. By Type:
    • object to ancestor type
    • primitive to a wider primitive of the same numeric category (new)
  2. Boxing:
    • boxed number to its primitive
    • boxed number to a wider primitive of the same numeric category (new for Short and Byte args)
    • primitive to its boxed value
    • primitive to Number or Object (new)
  3. Casting:
    • long to int
    • double to float
Hide
Alexander Taggart added a comment -

prim-conversion-update-1.patch applies to current master.

Show
Alexander Taggart added a comment - prim-conversion-update-1.patch applies to current master.
Alexander Taggart made changes -
Attachment prim-conversion-update-1.patch [ 10225 ]
Hide
Alexander Taggart added a comment -

Created CLJ-792 for the reflector reorg.

Show
Alexander Taggart added a comment - Created CLJ-792 for the reflector reorg.
Stuart Halloway made changes -
Waiting On stu
Stuart Halloway made changes -
Fix Version/s Approved Backlog [ 10034 ]
Fix Version/s Reviewed Backlog [ 10053 ]
Hide
Stuart Sierra added a comment -

prim-conversion-update-1.patch does not apply as of f5bcf64.

Is CLJ-792 now a prerequisite of this ticket?

Show
Stuart Sierra added a comment - prim-conversion-update-1.patch does not apply as of f5bcf64. Is CLJ-792 now a prerequisite of this ticket?
Stuart Sierra made changes -
Approval Test [ 10013 ] Incomplete [ 10006 ]
Hide
Alexander Taggart added a comment -

Yes, after the original patch was deemed "too big".

After this much time with no action from TPTB, feel free to kill both tickets.

Show
Alexander Taggart added a comment - Yes, after the original patch was deemed "too big". After this much time with no action from TPTB, feel free to kill both tickets.
Hide
Andy Fingerhut added a comment -

Again, not sure if this is any help, but I've tested starting from Clojure head as of Feb 20, 2012, applying clj-792-reorg-reflector-patch2.txt attached to CLJ-792, and then applying clj-445-prim-conversion-update-2-patch.txt attached to this ticket, and the result compiles and passes all but 2 tests. I don't know whether those failures are easy to fix or not, or whether issues may have been introduced with these patches.

Show
Andy Fingerhut added a comment - Again, not sure if this is any help, but I've tested starting from Clojure head as of Feb 20, 2012, applying clj-792-reorg-reflector-patch2.txt attached to CLJ-792, and then applying clj-445-prim-conversion-update-2-patch.txt attached to this ticket, and the result compiles and passes all but 2 tests. I don't know whether those failures are easy to fix or not, or whether issues may have been introduced with these patches.
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: